From 1d1a8f508802b6f742cb8317d2da8738f08d385b Mon Sep 17 00:00:00 2001 From: David Date: Mon, 20 Jul 2026 18:53:08 -0700 Subject: [PATCH] fix(agent): repair text-only explicit-mutation stalls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ordinary "fix …" turns set expected_mutation but skipped the text-only no-change cascade, so a prose diagnosis ended as incomplete · stalled with verification skipped. Run NoChanges repair for finished text-only expected mutation turns; keep scaffold/validation implementation-only; ignore bare "to finish" infinitives; update scripted fixtures for the extra rounds. --- crates/hi-agent/src/agent/turn/model_round.rs | 2 + .../src/agent/turn/steer/impl_cascade.rs | 80 +++++++++++- .../src/agent/turn/steer/implementation.rs | 3 +- .../hi-agent/src/agent/turn/steer/review.rs | 10 ++ crates/hi-agent/src/task_contract.rs | 20 ++- crates/hi-agent/src/tests/outcome.rs | 115 ++++++++++++++++-- crates/hi-agent/src/tests/retry.rs | 14 ++- crates/hi-agent/src/tests/truncation.rs | 9 +- 8 files changed, 233 insertions(+), 20 deletions(-) diff --git a/crates/hi-agent/src/agent/turn/model_round.rs b/crates/hi-agent/src/agent/turn/model_round.rs index 41cc25b..d477681 100644 --- a/crates/hi-agent/src/agent/turn/model_round.rs +++ b/crates/hi-agent/src/agent/turn/model_round.rs @@ -1156,6 +1156,8 @@ If the task is already complete, stop and give your final recap." &mut completion.content, read_only_intent, implementation_intent, + expected_mutation, + made_tool_call, &mut implementation_tracker, &mut evidence, &mut review_repair, diff --git a/crates/hi-agent/src/agent/turn/steer/impl_cascade.rs b/crates/hi-agent/src/agent/turn/steer/impl_cascade.rs index 6113497..248614b 100644 --- a/crates/hi-agent/src/agent/turn/steer/impl_cascade.rs +++ b/crates/hi-agent/src/agent/turn/steer/impl_cascade.rs @@ -59,14 +59,34 @@ pub(super) enum ImplementationCascadeAction { } /// First matching implementation completeness gate, or `None` if satisfied. +/// +/// Structured implementation tasks (`/build`, keep-building, …) run the full +/// cascade (no-change → scaffold-only → missing-validation). +/// +/// Ordinary explicit mutation turns (`expected_mutation`, e.g. "fix the parser +/// bug") only get the no-change gate when the text answer looks finished *and* +/// the turn never used tools (`text_only_turn`). That is the live failure: +/// pure diagnosis text → "verification skipped — no files changed" → +/// `incomplete · stalled` with no edit attempt. Unfinished narration, incomplete +/// plans, and turns that already used tools (plan/read/wait) keep their existing +/// silent-continue / plan-continue paths. Scaffold/validation remain +/// implementation-only so a normal fix that lands an edit is not forced into a +/// validation loop. pub(super) fn select_implementation_completeness( implementation_intent: Option, + expected_mutation: bool, + finished_text_answer: bool, + text_only_turn: bool, tracker: &ImplementationTracker, ) -> Option { - if implementation_intent.is_none() { + let gates: &[ImplementationGate] = if implementation_intent.is_some() { + IMPLEMENTATION_COMPLETENESS_CASCADE + } else if expected_mutation && finished_text_answer && text_only_turn { + &[ImplementationGate::NoChanges] + } else { return None; - } - for &gate in IMPLEMENTATION_COMPLETENESS_CASCADE { + }; + for &gate in gates { if let Some(action) = evaluate_gate(gate, tracker) { return Some(action); } @@ -165,6 +185,9 @@ mod tests { let tracker = ImplementationTracker::default(); let action = select_implementation_completeness( Some(ImplementationIntent { tui: false }), + false, + true, + false, &tracker, ); assert!(matches!( @@ -175,4 +198,55 @@ mod tests { }) )); } + + #[test] + fn explicit_mutation_without_implementation_intent_still_selects_no_changes() { + let tracker = ImplementationTracker::default(); + let action = select_implementation_completeness(None, true, true, true, &tracker); + assert!(matches!( + action, + Some(ImplementationCascadeAction::Repair { + gate: ImplementationGate::NoChanges, + .. + }) + )); + } + + #[test] + fn unfinished_expected_mutation_defers_to_silent_continue() { + let tracker = ImplementationTracker::default(); + assert!( + select_implementation_completeness(None, true, false, true, &tracker).is_none(), + "plan/unfinished narration must not be hijacked into no-change repair" + ); + } + + #[test] + fn tool_using_expected_mutation_skips_text_only_no_change_gate() { + let tracker = ImplementationTracker::default(); + assert!( + select_implementation_completeness(None, true, true, false, &tracker).is_none(), + "plan/read/wait turns already used tools; do not force edit via text cascade" + ); + } + + #[test] + fn explicit_mutation_after_edit_skips_scaffold_and_validation_gates() { + let tracker = ImplementationTracker { + mutation_seen: true, + substantive_edit_seen: true, + validation_after_last_mutation: false, + ..Default::default() + }; + assert!( + select_implementation_completeness(None, true, true, true, &tracker).is_none(), + "ordinary fix turns must not demand post-edit validation repair" + ); + } + + #[test] + fn plain_non_mutation_turn_skips_cascade() { + let tracker = ImplementationTracker::default(); + assert!(select_implementation_completeness(None, false, true, true, &tracker).is_none()); + } } diff --git a/crates/hi-agent/src/agent/turn/steer/implementation.rs b/crates/hi-agent/src/agent/turn/steer/implementation.rs index d921b00..6300108 100644 --- a/crates/hi-agent/src/agent/turn/steer/implementation.rs +++ b/crates/hi-agent/src/agent/turn/steer/implementation.rs @@ -126,7 +126,8 @@ if repeated_result_no_progress { ui.status(INCOMPLETE_STATUS); return RoundControl::BreakInner(false); } - if implementation_intent.is_some() && !implementation_tracker.mutation_seen + if (implementation_intent.is_some() || expected_mutation) + && !implementation_tracker.mutation_seen { if implementation_tracker.no_change_nudges < 2 { implementation_tracker.no_change_nudges += 1; diff --git a/crates/hi-agent/src/agent/turn/steer/review.rs b/crates/hi-agent/src/agent/turn/steer/review.rs index 9f05ba6..3bc5d89 100644 --- a/crates/hi-agent/src/agent/turn/steer/review.rs +++ b/crates/hi-agent/src/agent/turn/steer/review.rs @@ -25,6 +25,8 @@ impl crate::Agent { completion_content: &mut Vec, read_only_intent: Option, implementation_intent: Option, + expected_mutation: bool, + made_tool_call: bool, implementation_tracker: &mut ImplementationTracker, evidence: &mut EvidenceTracker, review_repair: &mut ReviewRepairState, @@ -103,8 +105,15 @@ if let Some(intent) = read_only_intent } } // Table-driven implementation completeness (order = IMPLEMENTATION_COMPLETENESS_CASCADE). +// For ordinary expected_mutation turns: finished text + never used tools only. +// Unfinished narration, incomplete plans, and tool-using turns take the paths below. +let finished_text_answer = !looks_unfinished && !plan_incomplete; +let text_only_turn = !made_tool_call; match super::impl_cascade::select_implementation_completeness( implementation_intent, + expected_mutation, + finished_text_answer, + text_only_turn, implementation_tracker, ) { Some(super::impl_cascade::ImplementationCascadeAction::Repair { @@ -116,6 +125,7 @@ match super::impl_cascade::select_implementation_completeness( }) => { super::impl_cascade::spend_implementation_gate(gate, implementation_tracker); evidence.quality_repair_nudges = evidence.quality_repair_nudges.saturating_add(1); + *continue_total_nudges = continue_total_nudges.saturating_add(1); *force_tools_next = force_tools; *text_tool_fallback_next = text_tool_fallback; ui.nudge(status); diff --git a/crates/hi-agent/src/task_contract.rs b/crates/hi-agent/src/task_contract.rs index f7f2516..97edec4 100644 --- a/crates/hi-agent/src/task_contract.rs +++ b/crates/hi-agent/src/task_contract.rs @@ -247,8 +247,10 @@ fn clause_requests_mutation(clause: &str, question: bool) -> bool { return matches!(previous, Some("you" | "please")); } // Outside questions, skip tool/artifact-noun usages ("cargo build", - // "apply the patch") and auxiliary/interrogative frames ("does it - // build", "will this delete data") that split across a filename dot. + // "apply the patch"), bare infinitives that are not the clause's + // imperative ("wait for the download to finish"), and auxiliary/ + // interrogative frames ("does it build", "will this delete data") + // that split across a filename dot. !matches!( previous, Some( @@ -264,6 +266,7 @@ fn clause_requests_mutation(clause: &str, question: bool) -> bool { | "cmake" | "go" | "rustc" + | "to" | "the" | "a" | "an" @@ -614,6 +617,19 @@ mod tests { } } + #[test] + fn bare_infinitive_finish_does_not_expect_mutation() { + // "wait … to finish" is not an imperative mutation request; the + // mutation verb is only the bare infinitive after "to". Branding it + // expected_mutation stalled wait-poll turns after two edit nudges. + let contract = TaskContract::derive( + "wait for the download to finish", + VerificationMode::Auto, + ); + assert_eq!(contract.intent, TaskIntent::Mutation); + assert!(!contract.explicit_mutation); + } + #[test] fn read_only_prefix_never_expects_mutation() { let contract = TaskContract::derive("read-only fix report", VerificationMode::Auto); diff --git a/crates/hi-agent/src/tests/outcome.rs b/crates/hi-agent/src/tests/outcome.rs index ba3c460..d8133e4 100644 --- a/crates/hi-agent/src/tests/outcome.rs +++ b/crates/hi-agent/src/tests/outcome.rs @@ -1,6 +1,6 @@ use super::common::{ - Canned, IsolatedWorkspace, NullUi, ProviderStep, ScriptedProvider, agent, completion, config, - scripted_agent, write_completion, + Canned, IsolatedWorkspace, NullUi, ProviderStep, RecordingUi, ScriptedProvider, agent, + completion, config, scripted_agent, write_completion, }; use super::*; use hi_ai::{ChatRequest, StreamEvent}; @@ -194,25 +194,119 @@ async fn ambiguous_question_answered_in_text_completes() { #[tokio::test] async fn explicit_mutation_request_without_changes_is_stalled() { + // Explicit mutation turns now share the implementation no-change cascade + // (two edit nudges) before branding incomplete · stalled. A single + // text-only diagnosis used to skip repair entirely and stall only at + // classify — with zero mid-loop incomplete status. let workspace = IsolatedWorkspace::new("outcome-explicit-no-changes"); let mut agent = agent( - vec![completion( - vec![Content::Text( - "The bug is in parser.rs line 42; an edit there would resolve it.".into(), - )], - 1, - 1, - )], + vec![ + completion( + vec![Content::Text( + "The bug is in parser.rs line 42; an edit there would resolve it.".into(), + )], + 1, + 1, + ), + completion( + vec![Content::Text( + "Still diagnosing; the edit belongs in parser.rs.".into(), + )], + 1, + 1, + ), + completion( + vec![Content::Text( + "I would edit parser.rs but am not calling tools.".into(), + )], + 1, + 1, + ), + ], workspace.config(), ); + let mut ui = RecordingUi::default(); let outcome = agent - .run_turn("fix the parser bug", &mut NullUi) + .run_turn("fix the parser bug", &mut ui) .await .unwrap(); assert_eq!(outcome.status, TurnStatus::Incomplete); assert_eq!(outcome.stop_reason, TurnStopReason::Stalled); + assert!( + agent.last_turn_telemetry().stalled_unfinished, + "cascade exhaustion must set stalled_unfinished mid-loop" + ); + assert_eq!( + agent.last_turn_telemetry().continue_nudges, + 2, + "two no-change repair continues before stall" + ); + assert!( + ui.statuses + .iter() + .any(|s| s.contains("no file changes") || s.contains("turn stopped incomplete")), + "expected no-change repair / incomplete status, got: {:?}", + ui.statuses + ); +} + +#[tokio::test] +async fn explicit_mutation_text_only_gets_edit_repair_then_lands() { + // Live fingerprint: "fix …" + text diagnosis used to print + // "verification skipped — no files changed" then incomplete · stalled + // without ever forcing an edit. The cascade must nudge, then accept a + // write and complete. + let workspace = IsolatedWorkspace::new("outcome-explicit-repair-lands"); + std::fs::create_dir_all(workspace.path("src")).unwrap(); + std::fs::write(workspace.path("src/parser.rs"), "fn parse() {}\n").unwrap(); + let mut cfg = workspace.config(); + cfg.gates.verification = crate::VerificationMode::Disabled; + let mut agent = agent( + vec![ + completion( + vec![Content::Text( + "The bug is in parser.rs line 1; an edit there would resolve it.".into(), + )], + 1, + 1, + ), + // After the no-change repair nudge the model edits and finishes. + write_completion("src/parser.rs"), + completion( + vec![Content::Text("Fixed the parser bug.".into())], + 1, + 1, + ), + ], + cfg, + ); + let mut ui = RecordingUi::default(); + + let outcome = agent + .run_turn("fix the parser bug", &mut ui) + .await + .unwrap(); + + assert_eq!(outcome.status, TurnStatus::Completed); + assert_eq!(outcome.stop_reason, TurnStopReason::NoApplicableVerification); + assert!( + !agent.last_turn_telemetry().stalled_unfinished, + "successful write must not leave the turn unfinished" + ); + assert!( + ui.statuses + .iter() + .any(|s| s.contains("no file changes")), + "expected no-change repair nudge before the write, got: {:?}", + ui.statuses + ); + assert!( + !ui.statuses.iter().any(|s| s.contains("turn stopped incomplete")), + "must not hard-stop incomplete after the edit lands: {:?}", + ui.statuses + ); } #[tokio::test] @@ -796,3 +890,4 @@ fn top_level_error_kind_classifies_usage_vs_infra() { assert_eq!(TopLevelErrorKind::Usage.exit_code(), 2); assert_eq!(TopLevelErrorKind::Infra.exit_code(), 3); } + diff --git a/crates/hi-agent/src/tests/retry.rs b/crates/hi-agent/src/tests/retry.rs index b2db9b6..7812e89 100644 --- a/crates/hi-agent/src/tests/retry.rs +++ b/crates/hi-agent/src/tests/retry.rs @@ -174,8 +174,11 @@ async fn request_too_large_drops_prior_context_and_retries_latest_prompt() { .push(Message::tool_result("read-1", huge_old_output.clone())); let mut ui = RecordingUi::default(); + // Question form stays mutation-capable for tools but is not + // expected_mutation, so the successful retry is not hijacked into the + // text-only no-change edit cascade (which would request more rounds). agent - .run_turn("fix the current bug", &mut ui) + .run_turn("what is the current bug status?", &mut ui) .await .unwrap(); @@ -203,7 +206,7 @@ async fn request_too_large_drops_prior_context_and_retries_latest_prompt() { assert!( requests[1] .iter() - .any(|m| m.text().contains("fix the current bug")), + .any(|m| m.text().contains("what is the current bug status?")), "latest user request is preserved" ); assert!( @@ -236,8 +239,13 @@ async fn request_too_large_context_drop_records_durable_boundary() { records: records.clone(), })); + // See request_too_large_drops_prior_context_and_retries_latest_prompt: + // avoid expected_mutation so the post-recovery text answer is final. agent - .run_turn("fix the current bug", &mut RecordingUi::default()) + .run_turn( + "what is the current bug status?", + &mut RecordingUi::default(), + ) .await .unwrap(); diff --git a/crates/hi-agent/src/tests/truncation.rs b/crates/hi-agent/src/tests/truncation.rs index 81edc3e..dc055c3 100644 --- a/crates/hi-agent/src/tests/truncation.rs +++ b/crates/hi-agent/src/tests/truncation.rs @@ -417,7 +417,10 @@ async fn truncation_with_partial_tool_call_does_not_orphan() { }, stop_reason: Some("length".into()), }, - // Second response: the model continues and finishes cleanly. + // Second response: truncation recovery continues; "write …" is + // expected_mutation, so a finished text-only answer would enter the + // no-change edit cascade. Land the write instead, then recap. + write_completion("main.rs"), completion(vec![Content::Text("Done writing the file.".into())], 10, 50), ]; let mut agent = agent(responses, cfg); @@ -477,6 +480,10 @@ async fn truncation_with_partial_text_tool_call_strips_raw_protocol() { }, stop_reason: Some("max_tokens".into()), }, + // Same as truncation_with_partial_tool_call_does_not_orphan: after + // truncation recovery, land the write rather than a text-only finish + // that would re-enter the expected_mutation no-change cascade. + write_completion("main.py"), completion(vec![Content::Text("Done after retry.".into())], 10, 50), ]; let mut agent = agent(responses, cfg);