From ea694a33b125d0127e4610eee62971a1b1b3ecfc Mon Sep 17 00:00:00 2001 From: calvinsturm Date: Fri, 3 Jul 2026 18:52:23 -0700 Subject: [PATCH] Split Rust validator outcomes --- src/agent.rs | 74 ++++++- src/agent/completion_policy.rs | 2 + src/agent/phase_transitions.rs | 61 +++++- src/agent/run_finalize.rs | 21 +- src/agent/runtime_effects.rs | 13 +- src/agent/validators.rs | 277 ++++++++++++++++++++++++++- src/agent_runtime/checkpoint.rs | 3 + src/agent_runtime/state.rs | 2 + src/agent_tests.rs | 328 +++++++++++++++++++++++++++++++- src/cli_dispatch_eval_replay.rs | 1 + 10 files changed, 749 insertions(+), 33 deletions(-) diff --git a/src/agent.rs b/src/agent.rs index cd906f6..f12f4ec 100644 --- a/src/agent.rs +++ b/src/agent.rs @@ -297,6 +297,7 @@ impl Agent

{ .required_validation_command(user_prompt) .map(ToOwned::to_owned), inferred_validator: None, + inferred_validator_outcomes: Vec::new(), satisfied: false, repair_mode: false, exact_final_answer_required: self.exact_final_answer_required(user_prompt), @@ -2908,6 +2909,15 @@ impl Agent

{ "next_phase": crate::agent::interrupts::run_phase_name(&effect.transition.to_phase) }), ); + if let Some(developer_message) = effect.developer_message { + messages.push(Message { + role: Role::Developer, + content: Some(developer_message), + tool_call_id: None, + tool_name: None, + tool_calls: None, + }); + } } Ok(Ok(successful_write_tool_ok_this_step)) @@ -2941,13 +2951,60 @@ impl Agent

{ .inferred_validator .clone() { - if runtime_checkpoint.validation_state.satisfied + let last_shell_ok = observed_tool_executions + .last() + .is_some_and(|execution| execution.name == "shell" && execution.ok); + if last_shell_ok { + if let Some(next_step) = crate::agent::validators::next_pending_step( + &validator, + &runtime_checkpoint + .validation_state + .inferred_validator_outcomes, + ) { + runtime_checkpoint.phase = crate::agent_runtime::state::RunPhase::Validating; + runtime_checkpoint.validation_state.required_command = + Some(next_step.command.clone()); + runtime_checkpoint.validation_state.satisfied = false; + messages.push(Message { + role: Role::Developer, + content: Some( + crate::agent::completion_policy::required_validation_phase_message( + &next_step.command, + ), + ), + tool_call_id: None, + tool_name: None, + tool_calls: None, + }); + self.emit_event( + run_id, + step, + EventKind::StepBlocked, + serde_json::json!({ + "reason": "inferred_validator_next_step", + "validator_kind": validator.kind, + "validator_label": validator.label, + "command": next_step.command, + "outcome": validator.run_outcome( + &runtime_checkpoint + .validation_state + .inferred_validator_outcomes + ) + }), + ); + return Ok(PhaseStepDispatch::ContinueAgentStep); + } + } + if crate::agent::validators::all_steps_passed( + &validator, + &runtime_checkpoint + .validation_state + .inferred_validator_outcomes, + ) && runtime_checkpoint.validation_state.satisfied && !runtime_checkpoint .validation_state .exact_final_answer_required - && observed_tool_executions - .last() - .is_some_and(|execution| execution.name == "shell" && execution.ok) + && last_shell_ok { let outcome = crate::agent::validators::ValidatorOutcome::Passed; self.emit_event( @@ -2959,14 +3016,19 @@ impl Agent

{ "validator_kind": validator.kind, "validator_label": validator.label, "command": validator.command, - "outcome": outcome + "outcome": outcome, + "validator_run": validator.run_outcome( + &runtime_checkpoint + .validation_state + .inferred_validator_outcomes + ) }), ); return Err(self.finalize_ok_with_end( step, run_id.to_string(), started_at.to_string(), - format!("Validation passed: `{}`.", validator.command), + "Validation passed: Rust validators.".to_string(), messages.clone(), observed_tool_calls.to_vec(), observed_tool_decisions.to_vec(), diff --git a/src/agent/completion_policy.rs b/src/agent/completion_policy.rs index 8b1f374..c453a65 100644 --- a/src/agent/completion_policy.rs +++ b/src/agent/completion_policy.rs @@ -615,6 +615,7 @@ mod tests { validation_state: crate::agent_runtime::state::ValidationState { required_command: Some("cargo test".to_string()), inferred_validator: None, + inferred_validator_outcomes: Vec::new(), satisfied: false, repair_mode: false, exact_final_answer_required: false, @@ -670,6 +671,7 @@ mod tests { validation_state: crate::agent_runtime::state::ValidationState { required_command: Some("cargo test".to_string()), inferred_validator: None, + inferred_validator_outcomes: Vec::new(), satisfied: true, repair_mode: false, exact_final_answer_required: true, diff --git a/src/agent/phase_transitions.rs b/src/agent/phase_transitions.rs index 894839b..f6c6a0c 100644 --- a/src/agent/phase_transitions.rs +++ b/src/agent/phase_transitions.rs @@ -16,6 +16,7 @@ pub(crate) enum PostToolPhaseRefreshEffect { EmitCompletionBlocked { transition: RuntimePhaseTransitionDecision, reason: String, + developer_message: Option, }, StopAfterRepairBudgetExhausted { reason: String, @@ -132,6 +133,36 @@ pub(crate) fn refresh_phase_state_from_tool_facts( .exact_final_answer_required, &tool_facts, ); + let mut validation_facts = validation_facts; + if let Some(validator) = runtime_checkpoint + .validation_state + .inferred_validator + .as_ref() + { + if let Some(required_command) = required_command { + if validation_facts.satisfied || validation_facts.repair_needed { + crate::agent::validators::record_step_outcome_for_command( + validator, + &mut runtime_checkpoint + .validation_state + .inferred_validator_outcomes, + required_command, + validation_facts.satisfied, + ); + } + } + if validation_facts.satisfied + && crate::agent::validators::next_pending_step( + validator, + &runtime_checkpoint + .validation_state + .inferred_validator_outcomes, + ) + .is_some() + { + validation_facts.satisfied = false; + } + } runtime_checkpoint.validation_state.satisfied = validation_facts.satisfied; runtime_checkpoint.last_tool_fact_envelopes = tool_fact_envelopes_from_tool_facts(&tool_facts, &runtime_checkpoint.phase); @@ -142,9 +173,31 @@ pub(crate) fn refresh_phase_state_from_tool_facts( let budget = crate::agent::validators::RepairBudget::one_repair( runtime_checkpoint.retry_state.validation_repair_turn_count, ); + let remaining_before_repair = budget.max.saturating_sub(budget.used); + let compact_evidence = runtime_checkpoint + .validation_state + .inferred_validator + .as_ref() + .map(|validator| { + crate::agent::validators::compact_outcome_evidence( + validator, + &runtime_checkpoint + .validation_state + .inferred_validator_outcomes, + remaining_before_repair, + ) + }); if !budget.can_authorize_repair() { return PostToolPhaseRefreshEffect::StopAfterRepairBudgetExhausted { - reason: "validation failed after the single allowed repair turn".to_string(), + reason: compact_evidence + .map(|evidence| { + format!( + "validation failed after the single allowed repair turn\n{evidence}" + ) + }) + .unwrap_or_else(|| { + "validation failed after the single allowed repair turn".to_string() + }), }; } runtime_checkpoint.retry_state.validation_repair_turn_count = runtime_checkpoint @@ -157,6 +210,7 @@ pub(crate) fn refresh_phase_state_from_tool_facts( PostToolPhaseRefreshEffect::EmitCompletionBlocked { transition: validation_resume_execution_transition_decision(), reason: "validation failed and runtime requires a code-fix repair step".to_string(), + developer_message: compact_evidence, } } ValidationPhaseTransitionDecision::EnterPostValidationFinalAnswerOnly => { @@ -172,6 +226,7 @@ pub(crate) fn refresh_phase_state_from_tool_facts( reason: post_validation_final_answer_transition_decision() .completion_reason .to_string(), + developer_message: None, } } ValidationPhaseTransitionDecision::ClearRepair => { @@ -223,6 +278,10 @@ pub(crate) fn apply_verified_write_follow_on( runtime_checkpoint.validation_state.required_command = Some(follow_on.command.clone()); runtime_checkpoint.validation_state.inferred_validator = follow_on.inferred_validator.clone(); + runtime_checkpoint + .validation_state + .inferred_validator_outcomes + .clear(); Some(VerifiedWriteFollowOnUpdate { control: PhaseLoopControl::ContinueAgentStep, developer_message: follow_on.developer_message.clone(), diff --git a/src/agent/run_finalize.rs b/src/agent/run_finalize.rs index ba0411f..8529fe8 100644 --- a/src/agent/run_finalize.rs +++ b/src/agent/run_finalize.rs @@ -174,15 +174,18 @@ impl Agent

{ if !self.validation_shell_available() { return None; } - inferred_validator.map(|validator| { - crate::agent::completion_policy::RequiredValidationFollowOn { - developer_message: - crate::agent::completion_policy::required_validation_phase_message( - &validator.command, - ), - command: validator.command.clone(), - inferred_validator: Some(validator), - } + inferred_validator.and_then(|validator| { + let first_step = validator.first_step()?; + Some( + crate::agent::completion_policy::RequiredValidationFollowOn { + developer_message: + crate::agent::completion_policy::required_validation_phase_message( + &first_step.command, + ), + command: first_step.command, + inferred_validator: Some(validator), + }, + ) }) }; match crate::agent::completion_policy::decide_verified_write_completion( diff --git a/src/agent/runtime_effects.rs b/src/agent/runtime_effects.rs index bd4c1ac..c60d79b 100644 --- a/src/agent/runtime_effects.rs +++ b/src/agent/runtime_effects.rs @@ -30,6 +30,7 @@ pub(crate) enum GuardEffect { pub(crate) struct CompletionBlockedEffect { pub(crate) transition: RuntimePhaseTransitionDecision, pub(crate) reason: String, + pub(crate) developer_message: Option, } pub(crate) fn apply_required_validation_guard_decision( @@ -108,9 +109,15 @@ pub(crate) fn completion_blocked_effect_from_post_tool_refresh( ) -> Option { match effect { PostToolPhaseRefreshEffect::None => None, - PostToolPhaseRefreshEffect::EmitCompletionBlocked { transition, reason } => { - Some(CompletionBlockedEffect { transition, reason }) - } + PostToolPhaseRefreshEffect::EmitCompletionBlocked { + transition, + reason, + developer_message, + } => Some(CompletionBlockedEffect { + transition, + reason, + developer_message, + }), PostToolPhaseRefreshEffect::StopAfterRepairBudgetExhausted { .. } => None, } } diff --git a/src/agent/validators.rs b/src/agent/validators.rs index adaf418..c3e76a9 100644 --- a/src/agent/validators.rs +++ b/src/agent/validators.rs @@ -8,11 +8,48 @@ pub(crate) enum ValidatorKind { Rust, } +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub(crate) enum ValidatorStepName { + Fmt, + Test, + Clippy, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub(crate) struct ValidatorStep { + pub(crate) name: ValidatorStepName, + pub(crate) command: String, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub(crate) enum ValidatorStepStatus { + Pass, + Fail, + Skipped, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub(crate) struct ValidatorStepOutcome { + pub(crate) step: ValidatorStep, + pub(crate) status: ValidatorStepStatus, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub(crate) struct ValidatorRunOutcome { + pub(crate) kind: ValidatorKind, + pub(crate) label: String, + pub(crate) steps: Vec, +} + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub(crate) struct ValidatorCommand { pub(crate) kind: ValidatorKind, pub(crate) label: String, pub(crate) command: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub(crate) steps: Vec, } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] @@ -74,10 +111,12 @@ pub(crate) fn infer_validator_command(trigger: &ValidatorTrigger) -> Option ValidatorCommand { + let steps = rust_validator_steps(); ValidatorCommand { kind: ValidatorKind::Rust, label: "rust_default".to_string(), command: "cargo fmt --check && cargo test && cargo clippy -- -D warnings".to_string(), + steps, } } @@ -86,11 +125,157 @@ fn path_triggers_rust_validator(path: &str) -> bool { normalized.ends_with(".rs") || normalized == "cargo.toml" || normalized.ends_with("/cargo.toml") } +fn rust_validator_steps() -> Vec { + vec![ + ValidatorStep { + name: ValidatorStepName::Fmt, + command: "cargo fmt --check".to_string(), + }, + ValidatorStep { + name: ValidatorStepName::Test, + command: "cargo test".to_string(), + }, + ValidatorStep { + name: ValidatorStepName::Clippy, + command: "cargo clippy -- -D warnings".to_string(), + }, + ] +} + +impl ValidatorCommand { + pub(crate) fn steps_or_legacy_command(&self) -> Vec { + if self.steps.is_empty() { + return vec![ValidatorStep { + name: ValidatorStepName::Test, + command: self.command.clone(), + }]; + } + self.steps.clone() + } + + pub(crate) fn first_step(&self) -> Option { + self.steps_or_legacy_command().into_iter().next() + } + + pub(crate) fn run_outcome(&self, outcomes: &[ValidatorStepOutcome]) -> ValidatorRunOutcome { + ValidatorRunOutcome { + kind: self.kind, + label: self.label.clone(), + steps: outcomes.to_vec(), + } + } +} + +pub(crate) fn record_step_outcome_for_command( + validator: &ValidatorCommand, + outcomes: &mut Vec, + command: &str, + passed: bool, +) -> bool { + let steps = validator.steps_or_legacy_command(); + let Some(step_index) = steps + .iter() + .position(|step| commands_match(&step.command, command)) + else { + return false; + }; + let step = steps[step_index].clone(); + if outcomes + .iter() + .any(|outcome| outcome.step.name == step.name) + { + return false; + } + outcomes.push(ValidatorStepOutcome { + step, + status: if passed { + ValidatorStepStatus::Pass + } else { + ValidatorStepStatus::Fail + }, + }); + if !passed { + for skipped in steps.into_iter().skip(step_index + 1) { + outcomes.push(ValidatorStepOutcome { + step: skipped, + status: ValidatorStepStatus::Skipped, + }); + } + } + true +} + +pub(crate) fn next_pending_step( + validator: &ValidatorCommand, + outcomes: &[ValidatorStepOutcome], +) -> Option { + let steps = validator.steps_or_legacy_command(); + steps.into_iter().find(|step| { + !outcomes + .iter() + .any(|outcome| outcome.step.name == step.name) + }) +} + +pub(crate) fn all_steps_passed( + validator: &ValidatorCommand, + outcomes: &[ValidatorStepOutcome], +) -> bool { + let steps = validator.steps_or_legacy_command(); + !steps.is_empty() + && steps.iter().all(|step| { + outcomes.iter().any(|outcome| { + outcome.step.name == step.name && outcome.status == ValidatorStepStatus::Pass + }) + }) +} + +pub(crate) fn failed_step(outcomes: &[ValidatorStepOutcome]) -> Option<&ValidatorStepOutcome> { + outcomes + .iter() + .find(|outcome| outcome.status == ValidatorStepStatus::Fail) +} + +pub(crate) fn compact_outcome_evidence( + validator: &ValidatorCommand, + outcomes: &[ValidatorStepOutcome], + repair_budget_remaining: u32, +) -> String { + let failed = failed_step(outcomes); + let mut lines = Vec::new(); + if let Some(failed) = failed { + lines.push(format!("validator_failed: {}", failed.step.command)); + } else if all_steps_passed(validator, outcomes) { + lines.push("validator_passed: rust_default".to_string()); + } else { + lines.push("validator_progress: rust_default".to_string()); + } + let skipped = outcomes + .iter() + .filter(|outcome| outcome.status == ValidatorStepStatus::Skipped) + .map(|outcome| outcome.step.command.clone()) + .collect::>(); + if !skipped.is_empty() { + lines.push("skipped:".to_string()); + lines.extend(skipped.into_iter().map(|command| format!("- {command}"))); + } + lines.push(format!( + "repair_budget_remaining: {repair_budget_remaining}" + )); + lines.join("\n") +} + +fn commands_match(left: &str, right: &str) -> bool { + left.trim().eq_ignore_ascii_case(right.trim()) +} + #[cfg(test)] mod tests { use super::{ - infer_validator_command, infer_validator_command_from_executions, RepairBudget, - ValidatorKind, ValidatorTrigger, + all_steps_passed, compact_outcome_evidence, infer_validator_command, + infer_validator_command_from_executions, next_pending_step, + record_step_outcome_for_command, RepairBudget, ValidatorKind, ValidatorStepStatus, + ValidatorTrigger, }; use crate::agent_impl_guard::ToolExecutionRecord; @@ -101,9 +286,19 @@ mod tests { }) .expect("rust validator"); assert_eq!(command.kind, ValidatorKind::Rust); - assert!(command.command.contains("cargo fmt --check")); - assert!(command.command.contains("cargo test")); - assert!(command.command.contains("cargo clippy -- -D warnings")); + let step_commands = command + .steps + .iter() + .map(|step| step.command.as_str()) + .collect::>(); + assert_eq!( + step_commands, + vec![ + "cargo fmt --check", + "cargo test", + "cargo clippy -- -D warnings" + ] + ); } #[test] @@ -141,4 +336,76 @@ mod tests { assert!(RepairBudget::one_repair(0).can_authorize_repair()); assert!(!RepairBudget::one_repair(1).can_authorize_repair()); } + + #[test] + fn records_all_rust_validator_steps_as_passed() { + let validator = infer_validator_command(&ValidatorTrigger { + changed_paths: vec!["src/lib.rs".into()], + }) + .expect("rust validator"); + let mut outcomes = Vec::new(); + assert!(record_step_outcome_for_command( + &validator, + &mut outcomes, + "cargo fmt --check", + true + )); + assert_eq!( + next_pending_step(&validator, &outcomes) + .expect("next") + .command, + "cargo test" + ); + assert!(record_step_outcome_for_command( + &validator, + &mut outcomes, + "cargo test", + true + )); + assert!(record_step_outcome_for_command( + &validator, + &mut outcomes, + "cargo clippy -- -D warnings", + true + )); + assert!(all_steps_passed(&validator, &outcomes)); + assert!(outcomes + .iter() + .all(|outcome| outcome.status == ValidatorStepStatus::Pass)); + } + + #[test] + fn fmt_failure_skips_test_and_clippy() { + let validator = infer_validator_command(&ValidatorTrigger { + changed_paths: vec!["src/lib.rs".into()], + }) + .expect("rust validator"); + let mut outcomes = Vec::new(); + assert!(record_step_outcome_for_command( + &validator, + &mut outcomes, + "cargo fmt --check", + false + )); + let statuses = outcomes + .iter() + .map(|outcome| (&outcome.step.command, outcome.status)) + .collect::>(); + assert_eq!( + statuses, + vec![ + (&"cargo fmt --check".to_string(), ValidatorStepStatus::Fail), + (&"cargo test".to_string(), ValidatorStepStatus::Skipped), + ( + &"cargo clippy -- -D warnings".to_string(), + ValidatorStepStatus::Skipped + ), + ] + ); + let evidence = compact_outcome_evidence(&validator, &outcomes, 1); + assert!(evidence.contains("validator_failed: cargo fmt --check")); + assert!(evidence.contains("- cargo test")); + assert!(evidence.contains("- cargo clippy -- -D warnings")); + assert!(evidence.contains("repair_budget_remaining: 1")); + } } diff --git a/src/agent_runtime/checkpoint.rs b/src/agent_runtime/checkpoint.rs index 1e8841c..027d686 100644 --- a/src/agent_runtime/checkpoint.rs +++ b/src/agent_runtime/checkpoint.rs @@ -299,6 +299,7 @@ pub(super) fn runtime_state_checkpoint_for_outcome( validation_state: ValidationState { required_command: required_command.clone(), inferred_validator: None, + inferred_validator_outcomes: Vec::new(), satisfied: crate::agent::required_validation_command_satisfied_from_facts( required_command.as_deref(), &tool_fact_envelopes @@ -1008,6 +1009,7 @@ mod tests { validation_state: ValidationState { required_command: Some("cargo test".to_string()), inferred_validator: None, + inferred_validator_outcomes: Vec::new(), satisfied: true, repair_mode: false, exact_final_answer_required: false, @@ -1035,6 +1037,7 @@ mod tests { validation_state: ValidationState { required_command: Some("cargo test".to_string()), inferred_validator: None, + inferred_validator_outcomes: Vec::new(), satisfied: false, repair_mode: false, exact_final_answer_required: false, diff --git a/src/agent_runtime/state.rs b/src/agent_runtime/state.rs index 8f004a8..bfc584d 100644 --- a/src/agent_runtime/state.rs +++ b/src/agent_runtime/state.rs @@ -72,6 +72,8 @@ pub struct ValidationState { pub required_command: Option, #[serde(skip_serializing_if = "Option::is_none")] pub(crate) inferred_validator: Option, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub(crate) inferred_validator_outcomes: Vec, #[serde(default)] pub satisfied: bool, #[serde(default)] diff --git a/src/agent_tests.rs b/src/agent_tests.rs index 4f953ae..1ac0f3f 100644 --- a/src/agent_tests.rs +++ b/src/agent_tests.rs @@ -1,4 +1,4 @@ -use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; use tokio::time::{sleep, Duration}; @@ -6,7 +6,7 @@ use async_trait::async_trait; use serde_json::json; use super::{ - sanitize_user_visible_output, Agent, AgentExitReason, McpPinEnforcementMode, + sanitize_user_visible_output, Agent, AgentExitReason, AgentOutcome, McpPinEnforcementMode, PlanStepConstraint, PlanToolEnforcementMode, ToolCallBudget, }; use crate::compaction::{CompactionMode, CompactionSettings, ToolResultPersist}; @@ -1672,6 +1672,11 @@ struct ReadPatchThenDoneThenEditThenDoneProvider { calls: Arc, } +struct ReadPatchThenRepairOnValidatorEvidenceProvider { + calls: Arc, + repaired: Arc, +} + struct ReadThenRepeatedBadApplyPatchThenEditThenReadThenDoneProvider { calls: Arc, } @@ -2470,6 +2475,86 @@ impl ModelProvider for ReadPatchThenDoneThenEditThenDoneProvider { } } +#[async_trait] +impl ModelProvider for ReadPatchThenRepairOnValidatorEvidenceProvider { + async fn generate(&self, req: GenerateRequest) -> anyhow::Result { + let n = self.calls.fetch_add(1, Ordering::SeqCst); + match n { + 0 => Ok(GenerateResponse { + assistant: Message { + role: Role::Assistant, + content: Some(String::new()), + tool_call_id: None, + tool_name: None, + tool_calls: None, + }, + tool_calls: vec![crate::types::ToolCall { + id: "tc_read".to_string(), + name: "read_file".to_string(), + arguments: serde_json::json!({"path":"main.rs"}), + }], + usage: None, + }), + 1 => Ok(GenerateResponse { + assistant: Message { + role: Role::Assistant, + content: Some(String::new()), + tool_call_id: None, + tool_name: None, + tool_calls: None, + }, + tool_calls: vec![crate::types::ToolCall { + id: "tc_patch".to_string(), + name: "apply_patch".to_string(), + arguments: serde_json::json!({ + "path":"main.rs", + "patch":"@@ -1,3 +1,3 @@\n fn answer() -> i32 {\n- return 1;\n+ return 2;\n }\n" + }), + }], + usage: None, + }), + _ if req.messages.iter().any(|message| { + message + .content + .as_deref() + .is_some_and(|content| content.contains("validator_failed:")) + }) && !self.repaired.swap(true, Ordering::SeqCst) => + { + Ok(GenerateResponse { + assistant: Message { + role: Role::Assistant, + content: Some(String::new()), + tool_call_id: None, + tool_name: None, + tool_calls: None, + }, + tool_calls: vec![crate::types::ToolCall { + id: "tc_repair".to_string(), + name: "edit".to_string(), + arguments: serde_json::json!({ + "path":"main.rs", + "old_string":"return 2;", + "new_string":"return 3;" + }), + }], + usage: None, + }) + } + _ => Ok(GenerateResponse { + assistant: Message { + role: Role::Assistant, + content: Some("done".to_string()), + tool_call_id: None, + tool_name: None, + tool_calls: None, + }, + tool_calls: Vec::new(), + usage: None, + }), + } + } +} + #[async_trait] impl ModelProvider for ReadThenRepeatedBadStrReplaceThenPatchThenReadThenDoneProvider { async fn generate(&self, _req: GenerateRequest) -> anyhow::Result { @@ -3217,6 +3302,14 @@ struct AlwaysFailShellExecTarget { host: HostTarget, } +#[derive(Clone)] +struct FailCommandOnceShellExecTarget { + host: HostTarget, + fail_command: &'static str, + failed_once: Arc, + commands: Arc>>, +} + #[async_trait] impl ExecTarget for ShellSuccessExecTarget { fn kind(&self) -> ExecTargetKind { @@ -3355,6 +3448,66 @@ impl ExecTarget for AlwaysFailShellExecTarget { } } +#[async_trait] +impl ExecTarget for FailCommandOnceShellExecTarget { + fn kind(&self) -> ExecTargetKind { + ExecTargetKind::Host + } + + fn describe(&self) -> TargetDescribe { + self.host.describe() + } + + async fn exec_shell(&self, req: ShellReq) -> TargetResult { + let command = if req.args.is_empty() { + req.cmd.clone() + } else { + format!("{} {}", req.cmd, req.args.join(" ")) + }; + self.commands.lock().expect("lock").push(command.clone()); + if command == self.fail_command && !self.failed_once.swap(true, Ordering::SeqCst) { + return TargetResult { + ok: false, + content: format!("{command} failed\nlarge raw output omitted in compact evidence"), + truncated: false, + bytes: None, + exit_code: Some(1), + stderr_truncated: None, + stdout_truncated: None, + execution_target: ExecTargetKind::Host, + docker: None, + }; + } + TargetResult { + ok: true, + content: format!("{command} passed"), + truncated: false, + bytes: None, + exit_code: Some(0), + stderr_truncated: None, + stdout_truncated: None, + execution_target: ExecTargetKind::Host, + docker: None, + } + } + + async fn read_file(&self, req: ReadReq) -> TargetResult { + self.host.read_file(req).await + } + + async fn list_dir(&self, req: ListReq) -> TargetResult { + self.host.list_dir(req).await + } + + async fn write_file(&self, req: WriteReq) -> TargetResult { + self.host.write_file(req).await + } + + async fn apply_patch(&self, req: PatchReq) -> TargetResult { + self.host.apply_patch(req).await + } +} + #[async_trait] impl ModelProvider for StaticContentProvider { async fn generate(&self, _req: GenerateRequest) -> anyhow::Result { @@ -5556,16 +5709,22 @@ async fn inferred_rust_validator_runs_after_rust_write_and_success_stops() { .expect("seed"); let calls = Arc::new(AtomicUsize::new(0)); let events = Arc::new(Mutex::new(Vec::::new())); + let shell_commands = Arc::new(Mutex::new(Vec::::new())); let mut agent = coding_agent( ReadPatchThenDoneProvider { calls: calls.clone(), }, tmp.path().to_path_buf(), - Arc::new(ShellSuccessExecTarget::default()), + Arc::new(FailCommandOnceShellExecTarget { + host: HostTarget, + fail_command: "__never__", + failed_once: Arc::new(AtomicBool::new(false)), + commands: shell_commands.clone(), + }), true, true, events.clone(), - 6, + 8, ); let out = agent .run( @@ -5580,15 +5739,20 @@ async fn inferred_rust_validator_runs_after_rust_write_and_success_stops() { }], ) .await; - assert!(matches!(out.exit_reason, AgentExitReason::Ok), "{out:?}"); assert_eq!( - out.final_output, - "Validation passed: `cargo fmt --check && cargo test && cargo clippy -- -D warnings`." + shell_commands.lock().expect("lock").as_slice(), + &[ + "cargo fmt --check".to_string(), + "cargo test".to_string(), + "cargo clippy -- -D warnings".to_string(), + ] ); + assert!(matches!(out.exit_reason, AgentExitReason::Ok), "{out:?}"); + assert_eq!(out.final_output, "Validation passed: Rust validators."); assert_eq!( calls.load(Ordering::SeqCst), - 3, - "validation success should stop without a final-answer model turn" + 5, + "validation success should run three validator turns and stop without a final-answer model turn" ); let evs = events.lock().expect("lock"); assert!(evs.iter().any(|e| { @@ -5609,6 +5773,152 @@ async fn inferred_rust_validator_runs_after_rust_write_and_success_stops() { .and_then(|v| v.as_str()) .is_some_and(|s| s == "allow") })); + let completion = evs + .iter() + .find(|e| { + matches!(e.kind, crate::events::EventKind::CompletionSatisfied) + && e.data + .get("reason") + .and_then(|v| v.as_str()) + .is_some_and(|s| s == "inferred_validator_passed") + }) + .expect("completion event"); + let steps = completion + .data + .get("validator_run") + .and_then(|v| v.get("steps")) + .and_then(|v| v.as_array()) + .expect("validator steps"); + assert_eq!(steps.len(), 3); + assert!(steps.iter().all(|step| { + step.get("status") + .and_then(|v| v.as_str()) + .is_some_and(|status| status == "pass") + })); +} + +async fn run_validator_repair_with_failed_command( + failed_command: &'static str, +) -> ( + AgentOutcome, + Arc>>, + Arc>>, +) { + let tmp = tempfile::tempdir().expect("tmp"); + tokio::fs::write( + tmp.path().join("main.rs"), + "fn answer() -> i32 {\n return 1;\n}\n", + ) + .await + .expect("seed"); + let calls = Arc::new(AtomicUsize::new(0)); + let events = Arc::new(Mutex::new(Vec::::new())); + let shell_commands = Arc::new(Mutex::new(Vec::::new())); + let mut agent = coding_agent( + ReadPatchThenRepairOnValidatorEvidenceProvider { + calls, + repaired: Arc::new(AtomicBool::new(false)), + }, + tmp.path().to_path_buf(), + Arc::new(FailCommandOnceShellExecTarget { + host: HostTarget, + fail_command: failed_command, + failed_once: Arc::new(AtomicBool::new(false)), + commands: shell_commands.clone(), + }), + true, + true, + events.clone(), + 12, + ); + let out = agent + .run( + "Edit main.rs to return 2.", + vec![], + vec![Message { + role: Role::System, + content: Some(crate::agent::INTERNAL_ENFORCE_IMPLEMENTATION_GUARD_FLAG.to_string()), + tool_call_id: None, + tool_name: None, + tool_calls: None, + }], + ) + .await; + (out, shell_commands, events) +} + +fn developer_messages(out: &AgentOutcome) -> String { + out.messages + .iter() + .filter(|message| matches!(message.role, Role::Developer)) + .filter_map(|message| message.content.as_deref()) + .collect::>() + .join("\n") +} + +#[tokio::test] +async fn inferred_rust_validator_fmt_failure_records_skipped_steps() { + let (out, shell_commands, _events) = + run_validator_repair_with_failed_command("cargo fmt --check").await; + assert!(matches!(out.exit_reason, AgentExitReason::Ok), "{out:?}"); + let developer = developer_messages(&out); + assert!(developer.contains("validator_failed: cargo fmt --check")); + assert!(developer.contains("- cargo test")); + assert!(developer.contains("- cargo clippy -- -D warnings")); + assert!(developer.contains("repair_budget_remaining: 1")); + assert!(!developer.contains("large raw output omitted")); + assert_eq!( + shell_commands.lock().expect("lock").as_slice(), + &[ + "cargo fmt --check".to_string(), + "cargo fmt --check".to_string(), + "cargo test".to_string(), + "cargo clippy -- -D warnings".to_string(), + ] + ); +} + +#[tokio::test] +async fn inferred_rust_validator_test_failure_records_prior_pass_and_skipped_clippy() { + let (out, shell_commands, _events) = + run_validator_repair_with_failed_command("cargo test").await; + assert!(matches!(out.exit_reason, AgentExitReason::Ok), "{out:?}"); + let developer = developer_messages(&out); + assert!(developer.contains("validator_failed: cargo test")); + assert!(developer.contains("- cargo clippy -- -D warnings")); + assert!(developer.contains("repair_budget_remaining: 1")); + assert_eq!( + shell_commands.lock().expect("lock").as_slice(), + &[ + "cargo fmt --check".to_string(), + "cargo test".to_string(), + "cargo fmt --check".to_string(), + "cargo test".to_string(), + "cargo clippy -- -D warnings".to_string(), + ] + ); +} + +#[tokio::test] +async fn inferred_rust_validator_clippy_failure_records_final_step_failure() { + let (out, shell_commands, _events) = + run_validator_repair_with_failed_command("cargo clippy -- -D warnings").await; + assert!(matches!(out.exit_reason, AgentExitReason::Ok), "{out:?}"); + let developer = developer_messages(&out); + assert!(developer.contains("validator_failed: cargo clippy -- -D warnings")); + assert!(!developer.contains("skipped:")); + assert!(developer.contains("repair_budget_remaining: 1")); + assert_eq!( + shell_commands.lock().expect("lock").as_slice(), + &[ + "cargo fmt --check".to_string(), + "cargo test".to_string(), + "cargo clippy -- -D warnings".to_string(), + "cargo fmt --check".to_string(), + "cargo test".to_string(), + "cargo clippy -- -D warnings".to_string(), + ] + ); } #[tokio::test] diff --git a/src/cli_dispatch_eval_replay.rs b/src/cli_dispatch_eval_replay.rs index 9421b5d..26c7228 100644 --- a/src/cli_dispatch_eval_replay.rs +++ b/src/cli_dispatch_eval_replay.rs @@ -998,6 +998,7 @@ mod tests { crate::agent_runtime::state::ValidationState { required_command: Some("cargo test".to_string()), inferred_validator: None, + inferred_validator_outcomes: Vec::new(), satisfied: false, repair_mode: false, exact_final_answer_required: false,