Skip to content

Commit 5b8a152

Browse files
author
Yogthos
committed
fix: address 8 review issues from merge audit
HIGH: - heal: re-apply heal_loaded_messages() call in spawn_loop_runner (lost during revert of unrelated retry.rs breakage) - compact_model: add deferred-wiring doc note MEDIUM: - inflight: move sequential add into Prepared arm to match parallel path — no more spurious add/delete for Immediate outcomes - context_manager: guard ctx_max == 0 returning None instead of computing inf ratio LOW: - storm: remove no-op Value round-trip in args normalisation (serde_json::Map is already BTreeMap, to_string is canonical) - storm: document BUILTIN_TOOL_NAMES sync and feature-gated tools - schema_flatten: add debug_assert for type=object precondition - scavenge: skip past unmatched brace to avoid O(n²) rescan on pathological JSON inputs
1 parent ae0be38 commit 5b8a152

7 files changed

Lines changed: 53 additions & 11 deletions

File tree

src/agent/agent_loop/context_manager.rs

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,16 @@ pub fn decide_after_usage(
133133
aggressive: false,
134134
};
135135
};
136+
if ctx_max == 0 {
137+
return PostUsageDecision {
138+
kind: PostUsageDecisionKind::None,
139+
prompt_tokens,
140+
ctx_max,
141+
ratio: 0.0,
142+
tail_budget: None,
143+
aggressive: false,
144+
};
145+
}
136146
let ratio = prompt_tokens as f64 / ctx_max as f64;
137147

138148
if ratio > FORCE_SUMMARY_THRESHOLD {
@@ -270,10 +280,10 @@ mod tests {
270280

271281
#[test]
272282
fn zero_ctx_max_handled_gracefully() {
273-
// ratio would be infinity, but comparison still works
283+
// ctx_max == 0 is degenerate (unknown model, config error).
284+
// Guard returns None rather than computing inf/NaN ratio.
274285
let d = decide_after_usage(Some(1000), 0, false);
275-
// ratio > FORCE_SUMMARY_THRESHOLD → ExitWithSummary
276-
assert_eq!(d.kind, PostUsageDecisionKind::ExitWithSummary);
286+
assert_eq!(d.kind, PostUsageDecisionKind::None);
277287
}
278288

279289
// ============================================================

src/agent/agent_loop/integration.rs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ use tokio::task::JoinHandle;
6767
use crate::event::AgentEvent;
6868

6969
use super::bridge::EventBridge;
70+
use super::heal;
7071
use super::message::{LoopMessage, UserMessage};
7172
use super::run::run_agent_loop;
7273
use super::steering::steering_from_queue;
@@ -435,7 +436,7 @@ pub fn spawn_loop_runner(cfg: LoopSpawnConfig) -> LoopRunner {
435436
}
436437
}
437438

438-
let context = Context {
439+
let mut context = Context {
439440
system_prompt: cfg.system_prompt,
440441
messages: cfg.history.iter().map(loop_message_to_value).collect(),
441442
tools: cfg.tools,
@@ -454,6 +455,22 @@ pub fn spawn_loop_runner(cfg: LoopSpawnConfig) -> LoopRunner {
454455
let event_tx_inner = event_tx.clone();
455456
let signal_inner = signal_for_task.clone();
456457

458+
// Heal messages loaded from disk before the first LLM call.
459+
// Shrinks oversized tool results and drops unpaired tool
460+
// calls that would otherwise 400 the next API request.
461+
let heal_result =
462+
heal::heal_loaded_messages(&context.messages, heal::DEFAULT_MAX_RESULT_CHARS);
463+
if heal_result.healed_count > 0 {
464+
tracing::info!(
465+
target: "dirge::agent_loop",
466+
healed = %heal_result.healed_count,
467+
chars_saved = %heal_result.chars_saved,
468+
"healed {} message(s) after session restore",
469+
heal_result.healed_count,
470+
);
471+
context.messages = heal_result.messages;
472+
}
473+
457474
// Code-review bug #4 fix: run the loop AND the
458475
// translation pump in the SAME outer task via
459476
// `tokio::join!`. The earlier version spawned the loop

src/agent/agent_loop/scavenge.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,7 @@ fn iterate_json_objects(text: &str) -> Vec<String> {
234234
}
235235
}
236236
}
237+
// Unmatched brace — skip past it to avoid O(n²) rescan.
237238
i += 1;
238239
}
239240
out

src/agent/agent_loop/schema_flatten.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,10 @@ pub fn analyze_schema(schema: &Value) -> FlattenDecision {
4040
///
4141
/// Returns the input unchanged if it's not a deep/wide schema.
4242
pub fn flatten_schema(schema: &Value) -> Value {
43+
debug_assert!(
44+
schema.get("type").and_then(|v| v.as_str()) == Some("object"),
45+
"flatten_schema precondition: root schema must have type=object"
46+
);
4347
let mut flat_props = serde_json::Map::new();
4448
let mut required: Vec<String> = Vec::new();
4549
collect("", schema, &mut flat_props, &mut required, true);

src/agent/agent_loop/storm.rs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,11 @@ impl StormBreaker {
109109
return StormVerdict::pass();
110110
}
111111
}
112-
let args_str = serde_json::to_string(&call.arguments).unwrap_or_default();
113-
let args = serde_json::from_str::<serde_json::Value>(&args_str)
114-
.and_then(|v| serde_json::to_string(&v))
115-
.unwrap_or(args_str);
112+
// serde_json::Map is a BTreeMap — key order is already
113+
// canonical. to_string produces compact form so integer/
114+
// float differences (1 vs 1.0) are handled by serde's
115+
// number serialisation.
116+
let args = serde_json::to_string(&call.arguments).unwrap_or_default();
116117

117118
let mutating = self.is_mutating.as_ref().map(|f| f(call)).unwrap_or(false);
118119
let read_only = !mutating;
@@ -194,6 +195,7 @@ impl StormBreaker {
194195
}
195196

196197
/// Built-in mutating tools: calls that change filesystem state.
198+
/// Kept in sync with `crate::agent::tools::BUILTIN_TOOL_NAMES`.
197199
fn default_mutating(call: &ToolCall) -> bool {
198200
matches!(
199201
call.name.as_str(),
@@ -203,6 +205,10 @@ fn default_mutating(call: &ToolCall) -> bool {
203205

204206
/// Built-in storm-exempt tools: cheap inspectors that should never
205207
/// trip the repeat-loop guard regardless of repetition count.
208+
/// Kept in sync with `crate::agent::tools::BUILTIN_TOOL_NAMES`.
209+
/// `find_callers` / `find_callees` are behind `#[cfg(feature = "semantic")]`
210+
/// but listing them here is harmless — the match simply won't fire
211+
/// when the feature is off.
206212
fn default_exempt(call: &ToolCall) -> bool {
207213
matches!(
208214
call.name.as_str(),

src/agent/agent_loop/tools.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,6 @@ pub async fn execute_tool_calls_sequential(
113113
})
114114
.await;
115115

116-
// Inflight: add at dispatch entry. Idempotent.
117-
inflight.add(&tool_call.id);
118-
119116
// 2. prepare
120117
let prepared =
121118
prepare_tool_call(context, assistant_message, tool_call, config, signal).await;
@@ -128,6 +125,8 @@ pub async fn execute_tool_calls_sequential(
128125
is_error,
129126
},
130127
PrepareOutcome::Prepared { tool, args } => {
128+
// Inflight: add now that we know the tool will actually run.
129+
inflight.add(&tool_call.id);
131130
let executed =
132131
execute_prepared_tool_call(&tool, tool_call, &args, signal, emit).await;
133132
finalize_executed_tool_call(

src/agent/agent_loop/types.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,11 @@ pub struct LoopConfig {
281281
/// truncation). When `Some`, summarisation and related tasks
282282
/// use this model instead of the session model. Reasonix uses
283283
/// `deepseek-v4-flash` for all auxiliary work.
284+
///
285+
/// **Status**: deferred. Wiring requires a second `StreamFn`
286+
/// constructed from a separate model + provider, which needs
287+
/// `LoopSpawnConfig` / `provider.rs` plumbing. Until then this
288+
/// field is accepted but not acted on.
284289
pub compact_model: Option<String>,
285290
}
286291

0 commit comments

Comments
 (0)