From 069bd835624fd2106da8fbe7ea5c85494de0c7d0 Mon Sep 17 00:00:00 2001 From: Hunter B Date: Tue, 7 Jul 2026 21:43:56 -0700 Subject: [PATCH 1/2] feat(tools): agent-callable verify/critique tool (#4196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `verify`, a tool the agent can CHOOSE to call to run an adversarial self-review of its own recent work before claiming a non-trivial change done. It spends extra test-time compute on the model's own judgment: the agent states a claim plus optional scope (recent git diff / staged diff / named files / the requirement), and an independent critic runs at elevated reasoning (default Max, independent of the session tier) tasked to REFUTE it, returning structured findings (issue, severity, evidence, suggested fix) plus a verdict and an unresolved-risk flag. Design: - Critic mechanism: a single-shot adversarial model call (mirrors the `review` tool) with reasoning_effort set explicitly, so elevated reasoning — the test-time-compute lever — never inherits a low tier. - Bounded / no runaway (hard requirement): the critic request carries `tools: None`, so the critic literally cannot invoke `verify` (or any tool) — recursion is impossible by construction rather than by a denylist. A task-local re-entry guard adds defense in depth. - Fail-safe parsing: unstructured or "upheld-but-has-serious-findings" critic output is treated as unresolved risk, guarding the green-CI-but-wrong trap. - Disjoint from subagent/mod.rs (parallel #4193 change) — calls no sub-agent internals; evidence is gathered deterministically by the tool. - Opt-out: `Feature::Verify` (`[features] verify_tool`, default on) gates registration in the agent runtime surface. Tests: tool contract (name/schema/registered/model-visible), a critic-finds-a-planted-bug test via MockLlmClient (also asserting the outgoing request is elevated + toolless), the recursion re-entry guard, fail-safe verdict normalization, and the feature-gated registration. --- config.example.toml | 4 + crates/tui/src/core/engine/tool_setup.rs | 1 + crates/tui/src/features.rs | 8 + crates/tui/src/tools/mod.rs | 1 + crates/tui/src/tools/registry.rs | 73 ++ crates/tui/src/tools/verify.rs | 971 +++++++++++++++++++++++ 6 files changed, 1058 insertions(+) create mode 100644 crates/tui/src/tools/verify.rs diff --git a/config.example.toml b/config.example.toml index 313bf647e..b9eb02a3e 100644 --- a/config.example.toml +++ b/config.example.toml @@ -745,6 +745,10 @@ apply_patch = true mcp = true exec_policy = true # vision_model = false # enable vision model for image_analyze tool +# verify_tool = false # disable the agent-callable `verify` self-critique tool + # (#4196). On by default; the agent decides when to spend + # the extra reasoning, so cost is only incurred on demand. + # Set false to remove it from the model's tool catalog. # ───────────────────────────────────────────────────────────────────────────────── # Vision Model Configuration (optional) diff --git a/crates/tui/src/core/engine/tool_setup.rs b/crates/tui/src/core/engine/tool_setup.rs index 53bd5bc78..c29ff162f 100644 --- a/crates/tui/src/core/engine/tool_setup.rs +++ b/crates/tui/src/core/engine/tool_setup.rs @@ -28,6 +28,7 @@ impl Engine { }; options.speech_output_dir = self.config.speech_output_dir.clone(); options.goal_state = Some(self.config.goal_state.clone()); + options.verify_tool_enabled = self.config.features.enabled(Feature::Verify); options } diff --git a/crates/tui/src/features.rs b/crates/tui/src/features.rs index 3fa3d1df4..7e9d24354 100644 --- a/crates/tui/src/features.rs +++ b/crates/tui/src/features.rs @@ -46,6 +46,8 @@ pub enum Feature { ExecPolicy, /// Enable vision model for image analysis. VisionModel, + /// Enable the agent-callable `verify` adversarial self-critique tool (#4196). + Verify, } impl fmt::Display for Stage { @@ -255,6 +257,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Beta, default_enabled: false, }, + FeatureSpec { + id: Feature::Verify, + key: "verify_tool", + stage: Stage::Stable, + default_enabled: true, + }, ]; #[cfg(test)] diff --git a/crates/tui/src/tools/mod.rs b/crates/tui/src/tools/mod.rs index 8b94928d5..a7bf7061f 100644 --- a/crates/tui/src/tools/mod.rs +++ b/crates/tui/src/tools/mod.rs @@ -59,6 +59,7 @@ pub mod truncate; pub mod user_input; pub mod validate_data; pub mod verifier; +pub mod verify; pub mod web_run; pub mod web_search; pub mod workflow; diff --git a/crates/tui/src/tools/registry.rs b/crates/tui/src/tools/registry.rs index 102081e18..dd5d92f95 100644 --- a/crates/tui/src/tools/registry.rs +++ b/crates/tui/src/tools/registry.rs @@ -501,6 +501,9 @@ pub struct AgentToolSurfaceOptions { pub vision_config: Option, pub speech_output_dir: Option, pub goal_state: Option, + /// Register the agent-callable `verify` self-critique tool (#4196). + /// Gated by `Feature::Verify` (`[features] verify_tool`), default on. + pub verify_tool_enabled: bool, } impl AgentToolSurfaceOptions { @@ -514,6 +517,7 @@ impl AgentToolSurfaceOptions { vision_config: None, speech_output_dir: None, goal_state: None, + verify_tool_enabled: true, } } } @@ -867,6 +871,15 @@ impl ToolRegistryBuilder { self.with_tool(Arc::new(ReviewTool::new(client, model))) } + /// Include the agent-callable `verify` self-critique tool (#4196). The + /// critic runs at elevated reasoning (default `Max`) independent of the + /// session tier and is given no tools, so it cannot recurse into `verify`. + #[must_use] + pub fn with_verify_tool(self, client: Option, model: String) -> Self { + use super::verify::VerifyTool; + self.with_tool(Arc::new(VerifyTool::new(client, model))) + } + /// Include note tool. #[must_use] pub fn with_note_tool(self) -> Self { @@ -1012,6 +1025,8 @@ impl ToolRegistryBuilder { plan_state: super::plan::SharedPlanState, ) -> Self { let speech_client = client.clone(); + let verify_client = client.clone(); + let verify_model = model.clone(); let mut builder = self .with_agent_tools_policy(options.shell_policy) .with_todo_tool(todo_list) @@ -1022,6 +1037,9 @@ impl ToolRegistryBuilder { .with_fim_tool(client, model) .with_speech_tools(speech_client, options.speech_output_dir.clone()); + if options.verify_tool_enabled { + builder = builder.with_verify_tool(verify_client, verify_model); + } if let Some(goal_state) = options.goal_state { builder = builder.with_goal_tools(goal_state); } @@ -1737,6 +1755,61 @@ mod tests { assert!(registry.contains("finance")); } + #[test] + fn with_verify_tool_registers_and_exposes_verify() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + + let registry = ToolRegistryBuilder::new() + .with_verify_tool(None, "test-model".to_string()) + .build(ctx); + + assert!( + registry.contains("verify"), + "verify tool should be registered" + ); + let api_names = registry + .to_api_tools() + .into_iter() + .map(|tool| tool.name) + .collect::>(); + assert!( + api_names.iter().any(|name| name == "verify"), + "verify tool should be model-visible" + ); + } + + #[test] + fn agent_runtime_surface_gates_verify_on_option() { + use super::AgentToolSurfaceOptions; + use crate::worker_profile::ShellPolicy; + + let build_surface = |verify_enabled: bool| { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + let mut options = AgentToolSurfaceOptions::new(ShellPolicy::Full); + options.verify_tool_enabled = verify_enabled; + ToolRegistryBuilder::new() + .with_agent_runtime_surface( + None, + "test-model".to_string(), + options, + crate::tools::todo::new_shared_todo_list(), + crate::tools::plan::new_shared_plan_state(), + ) + .build(ctx) + }; + + assert!( + build_surface(true).contains("verify"), + "verify should register when enabled" + ); + assert!( + !build_surface(false).contains("verify"), + "verify should be absent when the opt-out disables it" + ); + } + #[test] fn test_builder_with_agent_tools_includes_finance() { let tmp = tempdir().expect("tempdir"); diff --git a/crates/tui/src/tools/verify.rs b/crates/tui/src/tools/verify.rs new file mode 100644 index 000000000..070529fb2 --- /dev/null +++ b/crates/tui/src/tools/verify.rs @@ -0,0 +1,971 @@ +//! `verify` — agent-callable adversarial self-critique (#4196). +//! +//! This tool lets the agent DECIDE to spend extra test-time compute on a +//! self-review of its own recent work before claiming a change done. It runs +//! an INDEPENDENT adversarial critic pass at elevated reasoning (High/Max, +//! regardless of the session tier) whose job is to REFUTE the agent's claim — +//! surfacing correctness gaps, missed requirements, and edge cases as +//! structured findings the agent must then address. +//! +//! # Why elevated reasoning is the mechanism +//! +//! The critic request explicitly sets `reasoning_effort` to a high tier +//! ([`VerifyTool::critic_effort`], default [`ReasoningEffort::Max`]). Elevated +//! reasoning IS the test-time-compute lever, so the critic never inherits a low +//! session tier — [`build_critic_request`] threads the effort onto the outgoing +//! [`MessageRequest`], which the client forwards to the provider. +//! +//! # Bounded / no runaway (hard requirement) +//! +//! A verify call must not be able to trigger another verify. Two independent +//! guards enforce this: +//! +//! 1. **Structural (primary):** the critic is a single model call with +//! `tools: None` (see [`build_critic_request`]). With no tools of any kind, +//! the critic literally cannot invoke `verify` — recursion is impossible by +//! construction, not by a denylist that could be forgotten. +//! 2. **Re-entry guard (defense in depth):** [`VerifyTool::execute`] refuses if +//! it is entered while a critique is already in progress on the same task +//! (tracked via the [`struct@VERIFY_ACTIVE`] task-local). This protects any +//! future path that might run the critic inside a tool loop. +//! +//! # Relationship to neighbouring tools +//! +//! - `review` critiques a specific target (file/diff/PR) as a code review. +//! - `run_verifiers` executes external test/build gates (pytest, cargo, …). +//! - `verify` (this tool) is an adversarial reasoning pass over a *claim* and +//! its supporting evidence — "is what I just did actually correct and +//! complete?" — not a linter and not a test runner. + +use std::path::Path; + +use async_trait::async_trait; +use serde::{Deserialize, Serialize}; +use serde_json::{Value, json}; + +use crate::client::DeepSeekClient; +use crate::dependencies::ExternalTool; +use crate::features::Feature; +use crate::llm_client::LlmClient; +use crate::models::{ContentBlock, Message, MessageRequest, SystemPrompt, Usage}; +use crate::tui::app::ReasoningEffort; +use crate::utils::truncate_with_ellipsis; + +use super::spec::{ + ApprovalRequirement, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec, + optional_str, required_str, +}; + +/// Total evidence budget handed to the critic. Kept well under a turn so the +/// critic has room to reason. Large diffs/files are truncated with a marker. +const DEFAULT_MAX_EVIDENCE_CHARS: usize = 120_000; +/// Per-file evidence cap so a single huge file can't crowd out the rest. +const PER_FILE_MAX_CHARS: usize = 40_000; +/// Response budget for the critic's structured JSON. +const CRITIC_MAX_TOKENS: u32 = 2_048; +/// Cap on the raw-text fallback summary when the critic returns non-JSON. +const FALLBACK_SUMMARY_MAX_CHARS: usize = 4_000; + +// Task-local marker set for the duration of a critic pass. Presence means "a +// verify critique is already running on this task", which `VerifyTool::execute` +// treats as illegal re-entry. This is defense-in-depth on top of the structural +// `tools: None` guard in `build_critic_request`. +tokio::task_local! { + static VERIFY_ACTIVE: (); +} + +const CRITIC_SYSTEM_PROMPT: &str = "You are an adversarial critic performing a rigorous \ +self-review of a code change on behalf of the engineer who wrote it. Your job is to REFUTE the \ +claim, not to praise it. Assume the change is WRONG or INCOMPLETE until the evidence proves \ +otherwise.\n\ +\n\ +Hunt specifically for: correctness bugs; requirements that are only partially met or silently \ +dropped; unhandled edge cases (empty / huge / malformed input, concurrency and re-entrancy, \ +error and failure paths, off-by-one, integer overflow, null/None); regressions in existing \ +behaviour; and tests that pass but assert the wrong thing (green-CI-but-wrong). Prefer a small \ +number of concrete, evidence-backed findings over vague concerns. Cite `path:line` from the \ +evidence whenever you can. If, after a genuine effort to break it, you cannot refute the claim, \ +say so honestly rather than inventing problems.\n\ +\n\ +Return ONLY valid JSON (no prose, no markdown fences) matching this schema:\n\ +{\n\ + \"verdict\": \"refuted\" | \"upheld\" | \"uncertain\",\n\ + \"summary\": \"<= 3 sentence adversarial assessment\",\n\ + \"findings\": [\n\ + {\n\ + \"severity\": \"critical\" | \"high\" | \"medium\" | \"low\",\n\ + \"issue\": \"what is wrong or unproven\",\n\ + \"evidence\": \"where/why, path:line when possible\",\n\ + \"suggested_fix\": \"concrete, actionable fix\"\n\ + }\n\ + ],\n\ + \"unresolved_risk\": true | false\n\ +}\n\ +Set verdict=refuted if you found at least one critical or high finding; upheld only if you \ +genuinely could not refute the claim; uncertain if the evidence was insufficient to decide. Set \ +unresolved_risk=true whenever any unaddressed correctness risk remains."; + +/// A single adversarial finding the agent should address. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct CritiqueFinding { + /// Normalized to one of `critical` / `high` / `medium` / `low`. + #[serde(default)] + pub severity: String, + /// What is wrong or unproven. + #[serde(default)] + pub issue: String, + /// Where/why, ideally `path:line`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub evidence: Option, + /// Concrete, actionable fix. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub suggested_fix: Option, +} + +/// Structured result of a verify/critique pass. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct CritiqueReport { + /// `refuted` (found a real problem), `upheld` (could not refute), or + /// `uncertain` (insufficient evidence / unstructured critic output). + #[serde(default)] + pub verdict: String, + /// Short adversarial assessment. + #[serde(default)] + pub summary: String, + /// Concrete findings the agent must address before claiming done. + #[serde(default)] + pub findings: Vec, + /// True whenever an unaddressed correctness risk remains. Biased toward + /// `true` on ambiguity so "green-but-wrong" changes are not waved through. + #[serde(default)] + pub unresolved_risk: bool, +} + +impl CritiqueReport { + /// Parse the critic's raw response text into a structured report, tolerating + /// bare JSON, a fenced ```json block, or free-form prose (fallback). + #[must_use] + pub fn from_model_text(raw: &str) -> Self { + if let Some(parsed) = parse_report_json(raw) { + return parsed.normalize(); + } + if let Some(block) = extract_json_block(raw) + && let Some(parsed) = parse_report_json(block) + { + return parsed.normalize(); + } + Self::fallback(raw).normalize() + } + + /// The critic returned something we could not parse as JSON. Fail safe: + /// treat it as unresolved risk rather than a clean bill of health. + fn fallback(raw: &str) -> Self { + let trimmed = raw.trim(); + let summary = if trimmed.is_empty() { + "Critic returned no output; treat the change as unverified.".to_string() + } else { + format!( + "Critic returned unstructured output (treated as unresolved risk):\n{}", + truncate_with_ellipsis(trimmed, FALLBACK_SUMMARY_MAX_CHARS, "\n...[truncated]\n") + ) + }; + Self { + verdict: "uncertain".to_string(), + summary, + findings: Vec::new(), + unresolved_risk: true, + } + } + + /// Canonicalize severities/verdict and derive a fail-safe `unresolved_risk`. + fn normalize(mut self) -> Self { + self.summary = self.summary.trim().to_string(); + for finding in &mut self.findings { + finding.severity = normalize_severity(&finding.severity); + finding.issue = finding.issue.trim().to_string(); + finding.evidence = normalize_optional(finding.evidence.take()); + finding.suggested_fix = normalize_optional(finding.suggested_fix.take()); + } + + let has_serious = self + .findings + .iter() + .any(|f| matches!(f.severity.as_str(), "critical" | "high")); + + // Verdict: honour an explicit, recognized value; otherwise infer. + self.verdict = match self.verdict.trim().to_ascii_lowercase().as_str() { + "refuted" | "rejected" | "fail" | "failed" => "refuted".to_string(), + "upheld" | "confirmed" | "pass" | "passed" | "ok" => { + // Don't let the critic mark a change clean while it also reported + // serious findings — that is exactly the green-but-wrong trap. + if has_serious { + "refuted".to_string() + } else { + "upheld".to_string() + } + } + "" => { + if has_serious { + "refuted".to_string() + } else { + "uncertain".to_string() + } + } + _ => "uncertain".to_string(), + }; + + // Fail safe: any serious finding => unresolved risk, regardless of what + // the model set. + self.unresolved_risk = self.unresolved_risk || has_serious; + self + } + + /// Highest severity present, or "none". + #[must_use] + fn highest_severity(&self) -> &'static str { + for level in ["critical", "high", "medium", "low"] { + if self.findings.iter().any(|f| f.severity == level) { + return level; + } + } + "none" + } +} + +/// Evidence gathered by the tool and handed to the critic. +struct CritiqueInput { + claim: String, + requirement: Option, + focus: Option, + evidence: Vec, + /// True when no diff or file contents could be gathered. + no_code_evidence: bool, +} + +struct EvidenceBlock { + label: String, + body: String, +} + +/// Outcome of a single critic invocation, plus accounting for metadata. +struct CritiqueRun { + report: CritiqueReport, + response_model: String, + usage: Usage, +} + +/// Agent-callable adversarial self-critique tool. +pub struct VerifyTool { + client: Option, + model: String, + /// Reasoning tier the critic runs at, independent of the session tier. + critic_effort: ReasoningEffort, +} + +impl VerifyTool { + /// Construct with the default critic effort ([`ReasoningEffort::Max`]). + #[must_use] + pub fn new(client: Option, model: String) -> Self { + Self { + client, + model, + critic_effort: ReasoningEffort::Max, + } + } + + /// Override the critic reasoning tier. Values below `High` are clamped up to + /// `High` — elevated reasoning is the whole point of this tool. This is the + /// seam for a future `[verify] critic_effort` config knob; production + /// registration currently uses the `Max` default from [`Self::new`]. + #[allow(dead_code)] + #[must_use] + pub fn with_critic_effort(mut self, effort: ReasoningEffort) -> Self { + self.critic_effort = clamp_to_elevated(effort); + self + } +} + +#[async_trait] +impl ToolSpec for VerifyTool { + fn name(&self) -> &'static str { + "verify" + } + + fn description(&self) -> &'static str { + "Run an INDEPENDENT adversarial critic over your own recent work before you claim it is \ +done. You state a claim (what you believe your change accomplishes) plus optional scope (the \ +recent git diff, specific files, the original requirement); an independent critic runs at \ +elevated reasoning and tries to REFUTE it, returning structured findings (issue, severity, \ +suggested fix). Call this when it is worth spending extra thinking: before claiming a non-trivial \ +change complete, after a risky or subtle edit, or when you are unsure the change fully satisfies \ +the requirement and handles edge cases. Skip it for trivial or mechanical changes. This is not a \ +test runner (use run_verifiers) or a code review of an arbitrary target (use review) — it is a \ +self-check of whether what you just did is actually correct and complete." + } + + fn input_schema(&self) -> Value { + json!({ + "type": "object", + "properties": { + "claim": { + "type": "string", + "description": "What you believe your recent change accomplishes and why it is correct and complete. State it as an assertion the critic will try to REFUTE." + }, + "requirement": { + "type": "string", + "description": "Optional: the original requirement / task / acceptance criteria the change must satisfy. The critic checks the change against THIS, not against your restatement of it." + }, + "scope": { + "type": "string", + "enum": ["diff", "staged", "none"], + "default": "diff", + "description": "Code evidence to gather for the critic. 'diff' = uncommitted working-tree changes; 'staged' = git staged changes; 'none' = rely only on `files` and the claim text." + }, + "base": { + "type": "string", + "description": "Optional git base ref for the diff (e.g. origin/main). Defaults to the plain working-tree/staged diff." + }, + "files": { + "type": "array", + "items": { "type": "string" }, + "description": "Optional explicit file paths (relative to the workspace) whose current contents to include as evidence." + }, + "focus": { + "type": "string", + "description": "Optional: a specific risk to scrutinize (e.g. 'concurrency', 'the empty-input case', 'error handling on network failure')." + } + }, + "required": ["claim"] + }) + } + + fn capabilities(&self) -> Vec { + // Read-only: it inspects the workspace (git diff, file reads) and calls + // the model. It never mutates the workspace. + vec![ToolCapability::ReadOnly, ToolCapability::Network] + } + + fn approval_requirement(&self) -> ApprovalRequirement { + ApprovalRequirement::Auto + } + + async fn execute(&self, input: Value, context: &ToolContext) -> Result { + // Opt-out (defense in depth; the primary gate is registration-time in + // `with_agent_runtime_surface`). Honours `[features] verify_tool = false` + // and lets saved-transcript replays respect a disabled toggle. + if !context.features.enabled(Feature::Verify) { + return Err(ToolError::not_available( + "verify tool is disabled ([features] verify_tool = false)".to_string(), + )); + } + + // Re-entry guard: refuse if a critique is already running on this task. + // Checked BEFORE anything else so it cannot be bypassed via a missing + // client or bad input. + if VERIFY_ACTIVE.try_with(|_| ()).is_ok() { + return Err(ToolError::not_available( + "verify cannot run inside its own critic pass (recursion guard)".to_string(), + )); + } + + // Validate the request shape before checking client availability, so a + // malformed call gets a precise input error rather than a generic + // "no client" one. + let claim = required_str(&input, "claim")?.trim().to_string(); + if claim.is_empty() { + return Err(ToolError::invalid_input("claim cannot be empty")); + } + let requirement = optional_str(&input, "requirement") + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string); + let focus = optional_str(&input, "focus") + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string); + let base = optional_str(&input, "base") + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string); + + let scope = optional_str(&input, "scope").unwrap_or("diff").trim(); + let staged = match scope { + "diff" | "" => false, + "staged" => true, + "none" => { + // handled below by skipping diff gathering + false + } + other => { + return Err(ToolError::invalid_input(format!( + "unknown scope '{other}' (expected diff | staged | none)" + ))); + } + }; + let gather_diff_scope = scope != "none"; + + let files = extract_string_array(&input, "files"); + + let Some(client) = self.client.clone() else { + return Err(ToolError::not_available( + "verify tool requires an active model client".to_string(), + )); + }; + + // --- Deterministic evidence gathering --- + let mut evidence: Vec = Vec::new(); + if gather_diff_scope { + match gather_git_diff(context.workspace.as_path(), staged, base.as_deref()).await? { + Some(diff) => evidence.push(EvidenceBlock { + label: if staged { + "git diff --cached".to_string() + } else { + "git diff (working tree)".to_string() + }, + body: diff, + }), + None => { /* no diff — recorded via no_code_evidence below */ } + } + } + evidence.extend(gather_files(&files, context)); + + let no_code_evidence = evidence.is_empty(); + + let critique_input = CritiqueInput { + claim, + requirement, + focus, + evidence, + no_code_evidence, + }; + + // Run the critic under the re-entry marker so any (future) nested tool + // call to `verify` is refused. + let run = VERIFY_ACTIVE + .scope( + (), + run_critique(&client, &self.model, self.critic_effort, &critique_input), + ) + .await?; + + let metadata = json!({ + "tool": "verify", + "verdict": run.report.verdict, + "finding_count": run.report.findings.len(), + "highest_severity": run.report.highest_severity(), + "unresolved_risk": run.report.unresolved_risk, + "critic_effort": self.critic_effort.as_setting(), + "child_model": run.response_model, + "child_input_tokens": run.usage.input_tokens, + "child_output_tokens": run.usage.output_tokens, + }); + + let result = ToolResult::json(&run.report) + .map_err(|e| ToolError::execution_failed(e.to_string()))?; + Ok(result.with_metadata(metadata)) + } +} + +/// Run one adversarial critic pass. Generic over [`LlmClient`] so tests can +/// drive it with `MockLlmClient` without a network call. +async fn run_critique( + client: &C, + model: &str, + effort: ReasoningEffort, + input: &CritiqueInput, +) -> Result { + let prompt = build_critic_prompt(input, DEFAULT_MAX_EVIDENCE_CHARS); + let request = build_critic_request(model, effort, prompt); + let response = client + .create_message(request) + .await + .map_err(|e| ToolError::execution_failed(format!("verify critic request failed: {e}")))?; + let text = extract_text(&response.content); + Ok(CritiqueRun { + report: CritiqueReport::from_model_text(&text), + response_model: response.model, + usage: response.usage, + }) +} + +/// Build the critic's outgoing request. **The recursion guarantee lives here:** +/// `tools` is always `None`, so the critic cannot invoke `verify` (or any other +/// tool). `reasoning_effort` is set explicitly so the critic runs elevated +/// regardless of the session tier. +fn build_critic_request(model: &str, effort: ReasoningEffort, prompt: String) -> MessageRequest { + MessageRequest { + model: model.to_string(), + messages: vec![Message { + role: "user".to_string(), + content: vec![ContentBlock::Text { + text: prompt, + cache_control: None, + }], + }], + max_tokens: CRITIC_MAX_TOKENS, + system: Some(SystemPrompt::Text(CRITIC_SYSTEM_PROMPT.to_string())), + // Hard bound: the critic gets NO tools, so it cannot recurse into verify. + tools: None, + tool_choice: None, + metadata: None, + thinking: None, + // Test-time compute: elevated reasoning, independent of session tier. + reasoning_effort: Some(clamp_to_elevated(effort).as_setting().to_string()), + stream: Some(false), + temperature: Some(0.1), + top_p: Some(0.9), + } +} + +fn build_critic_prompt(input: &CritiqueInput, max_chars: usize) -> String { + let mut out = String::new(); + out.push_str("CLAIM (to be refuted):\n"); + out.push_str(&input.claim); + out.push('\n'); + + if let Some(req) = &input.requirement { + out.push_str("\nORIGINAL REQUIREMENT (verify the change against THIS):\n"); + out.push_str(req); + out.push('\n'); + } + if let Some(focus) = &input.focus { + out.push_str("\nFOCUS (scrutinize this in particular):\n"); + out.push_str(focus); + out.push('\n'); + } + + // Evidence gets its own budget so the claim/requirement always survive. + let header_len = out.len(); + let evidence_budget = max_chars.saturating_sub(header_len).max(1_000); + + out.push_str("\n=== EVIDENCE ===\n"); + if input.no_code_evidence { + out.push_str( + "No code diff or file contents were available. Critique the claim on its own terms, \ +and explicitly note in your summary that you could not inspect the actual change.\n", + ); + } else { + let mut evidence_text = String::new(); + for block in &input.evidence { + evidence_text.push_str("--- "); + evidence_text.push_str(&block.label); + evidence_text.push_str(" ---\n"); + evidence_text.push_str(&block.body); + if !block.body.ends_with('\n') { + evidence_text.push('\n'); + } + evidence_text.push('\n'); + } + out.push_str(&truncate_with_ellipsis( + &evidence_text, + evidence_budget, + "\n...[evidence truncated]...\n", + )); + } + out.push_str("=== END EVIDENCE ===\n\nRefute the claim. Return ONLY the JSON object."); + out +} + +/// Gather a git diff, or `None` when there is nothing to diff / git is absent. +/// Unlike `review`, an empty diff is not an error — a claim can be about +/// reasoning, and `files` may carry the evidence instead. +async fn gather_git_diff( + workspace: &Path, + staged: bool, + base: Option<&str>, +) -> Result, ToolError> { + let Some(mut cmd) = crate::dependencies::Git::command() else { + // git not installed: degrade gracefully rather than failing the tool. + return Ok(None); + }; + cmd.arg("diff"); + if staged { + cmd.arg("--cached"); + } + if let Some(base) = base.filter(|b| !b.trim().is_empty()) { + cmd.arg(format!("{base}...HEAD")); + } + cmd.current_dir(workspace); + + let output = tokio::task::spawn_blocking(move || cmd.output()) + .await + .map_err(|e| ToolError::execution_failed(format!("git diff task panicked: {e}")))? + .map_err(|e| ToolError::execution_failed(format!("failed to run git diff: {e}")))?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(ToolError::execution_failed(format!( + "git diff failed: {}", + stderr.trim() + ))); + } + let diff = String::from_utf8_lossy(&output.stdout).to_string(); + if diff.trim().is_empty() { + Ok(None) + } else { + Ok(Some(diff)) + } +} + +/// Read the requested files as evidence, recording read/path failures as inline +/// notes so the critic knows evidence was requested but unavailable. +fn gather_files(files: &[String], context: &ToolContext) -> Vec { + let mut blocks = Vec::new(); + for raw in files { + let raw = raw.trim(); + if raw.is_empty() { + continue; + } + match context.resolve_path(raw) { + Ok(path) => match std::fs::read_to_string(&path) { + Ok(content) => { + let display = path + .strip_prefix(&context.workspace) + .unwrap_or(&path) + .to_string_lossy() + .to_string(); + let numbered = number_lines(&content); + blocks.push(EvidenceBlock { + label: format!("file: {display}"), + body: truncate_with_ellipsis( + &numbered, + PER_FILE_MAX_CHARS, + "\n...[file truncated]...\n", + ), + }); + } + Err(e) => blocks.push(EvidenceBlock { + label: format!("file: {raw} (unreadable)"), + body: format!(""), + }), + }, + Err(e) => blocks.push(EvidenceBlock { + label: format!("file: {raw} (rejected)"), + body: format!(""), + }), + } + } + blocks +} + +fn number_lines(content: &str) -> String { + content + .lines() + .enumerate() + .map(|(idx, line)| format!("{:>4} | {line}", idx + 1)) + .collect::>() + .join("\n") +} + +/// Reasoning tiers below `High` defeat the purpose; clamp them up. +fn clamp_to_elevated(effort: ReasoningEffort) -> ReasoningEffort { + match effort { + ReasoningEffort::High | ReasoningEffort::Max => effort, + // Off / Low / Medium / Auto → High (still elevated, provider-normalized + // at the client boundary). + _ => ReasoningEffort::High, + } +} + +fn extract_string_array(input: &Value, key: &str) -> Vec { + input + .get(key) + .and_then(Value::as_array) + .map(|arr| { + arr.iter() + .filter_map(|v| v.as_str()) + .map(str::to_string) + .collect() + }) + .unwrap_or_default() +} + +fn extract_text(blocks: &[ContentBlock]) -> String { + let mut out = String::new(); + for block in blocks { + if let ContentBlock::Text { text, .. } = block { + out.push_str(text); + } + } + out +} + +fn parse_report_json(raw: &str) -> Option { + serde_json::from_str::(raw.trim()).ok() +} + +/// Extract a JSON object from prose: prefer a fenced ```json block, else the +/// span from the first `{` to the last `}`. +fn extract_json_block(raw: &str) -> Option<&str> { + if let Some(start) = raw.find("```json") { + let after = &raw[start + "```json".len()..]; + if let Some(end) = after.find("```") { + return Some(after[..end].trim()); + } + } + let start = raw.find('{')?; + let end = raw.rfind('}')?; + if end > start { + Some(raw[start..=end].trim()) + } else { + None + } +} + +fn normalize_severity(value: &str) -> String { + match value.trim().to_ascii_lowercase().as_str() { + "critical" | "crit" | "blocker" | "severe" => "critical", + "high" | "major" | "important" => "high", + "low" | "minor" | "nit" | "trivial" => "low", + // Default unknown/empty to medium so a finding is never dropped, but is + // also not over-escalated to serious. + _ => "medium", + } + .to_string() +} + +fn normalize_optional(value: Option) -> Option { + value + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::llm_client::mock::MockLlmClient; + use serde_json::json; + use std::path::Path; + + fn ctx() -> ToolContext { + ToolContext::new(Path::new(".")) + } + + fn text_response(model: &str, body: &str) -> crate::models::MessageResponse { + crate::models::MessageResponse { + id: "msg_test".to_string(), + r#type: "message".to_string(), + role: "assistant".to_string(), + content: vec![ContentBlock::Text { + text: body.to_string(), + cache_control: None, + }], + model: model.to_string(), + stop_reason: Some("stop".to_string()), + stop_sequence: None, + container: None, + usage: Usage::default(), + } + } + + fn planted_bug_input() -> CritiqueInput { + // A change that claims to handle all inputs but has an obvious + // divide-by-zero / empty-slice defect in the diff. + CritiqueInput { + claim: "average() now correctly computes the mean for any input slice".to_string(), + requirement: Some("Must not panic on empty input.".to_string()), + focus: None, + evidence: vec![EvidenceBlock { + label: "git diff (working tree)".to_string(), + body: "+fn average(xs: &[f64]) -> f64 {\n+ xs.iter().sum::() / xs.len() as f64\n+}\n" + .to_string(), + }], + no_code_evidence: false, + } + } + + // === Contract tests === + + #[test] + fn tool_contract_name_and_schema() { + let tool = VerifyTool::new(None, "test-model".to_string()); + assert_eq!(tool.name(), "verify"); + let schema = tool.input_schema(); + assert_eq!(schema["type"], "object"); + assert!(schema["properties"]["claim"].is_object()); + assert!(schema["properties"]["scope"]["enum"].is_array()); + let required = schema["required"].as_array().expect("required array"); + assert!(required.iter().any(|v| v == "claim")); + // Read-only + network; approval auto. + assert!(tool.capabilities().contains(&ToolCapability::ReadOnly)); + assert!(tool.is_read_only()); + assert_eq!(tool.approval_requirement(), ApprovalRequirement::Auto); + assert!(tool.model_visible()); + } + + // === Elevated-reasoning + no-recursion structural guard === + + #[test] + fn critic_request_is_elevated_and_toolless() { + // Even if someone constructs the tool at Low, the critic must run + // elevated and carry NO tools (so it cannot recurse into verify). + let req = build_critic_request("m", ReasoningEffort::Low, "prompt".to_string()); + assert_eq!( + req.reasoning_effort.as_deref(), + Some("high"), + "Low must clamp up to elevated reasoning" + ); + assert!( + req.tools.is_none(), + "critic must be given NO tools — this is the structural recursion guard" + ); + + let req_max = build_critic_request("m", ReasoningEffort::Max, "prompt".to_string()); + assert_eq!(req_max.reasoning_effort.as_deref(), Some("max")); + assert!(req_max.tools.is_none()); + } + + #[test] + fn with_critic_effort_clamps_below_high() { + let tool = VerifyTool::new(None, "m".to_string()).with_critic_effort(ReasoningEffort::Low); + assert_eq!(tool.critic_effort, ReasoningEffort::High); + let tool = VerifyTool::new(None, "m".to_string()).with_critic_effort(ReasoningEffort::Max); + assert_eq!(tool.critic_effort, ReasoningEffort::Max); + } + + // === Critic-finds-a-planted-bug (mocked model) === + + #[tokio::test] + async fn critic_surfaces_planted_bug() { + let mock = MockLlmClient::new(vec![]); + // Canonical adversarial JSON the critic would return for the defect. + mock.push_message_response(text_response( + "mock-critic", + r#"{ + "verdict": "refuted", + "summary": "average() divides by len() with no empty-slice guard.", + "findings": [ + { + "severity": "critical", + "issue": "Divide-by-zero / NaN when xs is empty; violates the no-panic requirement.", + "evidence": "average(): xs.len() as f64 is 0 for empty input", + "suggested_fix": "Return 0.0 or Option::None when xs.is_empty()." + } + ], + "unresolved_risk": true + }"#, + )); + + let run = run_critique( + &mock, + "mock-critic", + ReasoningEffort::Max, + &planted_bug_input(), + ) + .await + .expect("critique runs"); + + assert_eq!(run.report.verdict, "refuted"); + assert!(run.report.unresolved_risk); + assert_eq!(run.report.findings.len(), 1); + assert_eq!(run.report.findings[0].severity, "critical"); + assert!( + run.report.findings[0] + .issue + .to_lowercase() + .contains("empty"), + "finding should name the empty-input defect" + ); + assert_eq!(run.report.highest_severity(), "critical"); + + // The outgoing critic request carried elevated reasoning and NO tools. + let sent = mock.last_request().expect("request captured"); + assert_eq!(sent.reasoning_effort.as_deref(), Some("max")); + assert!(sent.tools.is_none()); + // Evidence and requirement were threaded into the prompt. + let prompt = match &sent.messages[0].content[0] { + ContentBlock::Text { text, .. } => text.clone(), + _ => panic!("expected text content"), + }; + assert!(prompt.contains("CLAIM")); + assert!(prompt.contains("Must not panic on empty input")); + assert!(prompt.contains("average(")); + } + + #[tokio::test] + async fn unstructured_critic_output_is_unresolved_risk() { + let mock = MockLlmClient::new(vec![]); + mock.push_message_response(text_response("m", "I think it looks fine, ship it.")); + let run = run_critique(&mock, "m", ReasoningEffort::High, &planted_bug_input()) + .await + .expect("runs"); + // Fail safe: non-JSON critic output must not read as a clean pass. + assert_eq!(run.report.verdict, "uncertain"); + assert!(run.report.unresolved_risk); + } + + #[test] + fn upheld_with_serious_finding_is_downgraded_to_refuted() { + // Guards the green-but-wrong trap: a critic can't declare "upheld" while + // simultaneously reporting a high-severity finding. + let report = CritiqueReport::from_model_text( + r#"{"verdict":"upheld","summary":"looks ok","findings":[{"severity":"high","issue":"missing null check"}],"unresolved_risk":false}"#, + ); + assert_eq!(report.verdict, "refuted"); + assert!(report.unresolved_risk); + } + + // === Recursion / re-entry guard === + + #[tokio::test] + async fn execute_refuses_reentry() { + // Simulate being inside a critic pass; execute must refuse before it + // even looks at the (absent) client or input. + let tool = VerifyTool::new(None, "m".to_string()); + let err = VERIFY_ACTIVE + .scope((), async { + tool.execute(json!({ "claim": "x" }), &ctx()).await + }) + .await + .expect_err("re-entry must be refused"); + let msg = err.to_string().to_lowercase(); + assert!( + msg.contains("recursion") || msg.contains("inside its own"), + "expected a recursion-guard error, got: {err}" + ); + } + + #[tokio::test] + async fn execute_without_client_is_not_available() { + let tool = VerifyTool::new(None, "m".to_string()); + let err = tool + .execute(json!({ "claim": "did the thing" }), &ctx()) + .await + .expect_err("no client"); + assert!(err.to_string().to_lowercase().contains("client")); + } + + #[tokio::test] + async fn execute_rejects_empty_and_unknown_scope() { + let tool = VerifyTool::new(None, "m".to_string()); + // Empty claim is rejected before the (absent) client is consulted. + let err = tool + .execute(json!({ "claim": " " }), &ctx()) + .await + .expect_err("empty claim"); + assert!(err.to_string().to_lowercase().contains("claim"), "{err}"); + + // Unknown scope is a precise input error, not a generic client error. + let err = tool + .execute(json!({ "claim": "ok", "scope": "everything" }), &ctx()) + .await + .expect_err("unknown scope"); + assert!(err.to_string().to_lowercase().contains("scope"), "{err}"); + } + + #[test] + fn parses_fenced_json_block() { + let raw = "Here is my critique:\n```json\n{\"verdict\":\"refuted\",\"summary\":\"s\",\"findings\":[],\"unresolved_risk\":true}\n```\nDone."; + let report = CritiqueReport::from_model_text(raw); + assert_eq!(report.verdict, "refuted"); + assert!(report.unresolved_risk); + } + + #[test] + fn severity_normalization() { + assert_eq!(normalize_severity("BLOCKER"), "critical"); + assert_eq!(normalize_severity("Major"), "high"); + assert_eq!(normalize_severity("nit"), "low"); + assert_eq!(normalize_severity("weird"), "medium"); + assert_eq!(normalize_severity(""), "medium"); + } +} From 8666e4ca2b03c0091819ea4a3bee7eff78b7267a Mon Sep 17 00:00:00 2001 From: Hunter B Date: Tue, 7 Jul 2026 22:01:45 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(tools):=20verify=20=E2=80=94=20flag=20m?= =?UTF-8?q?edium=20findings=20+=20include=20worktree=20diff=20(#4196)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two correctness gaps from review of PR #4199 (design-agnostic; the direct-call-vs-sub-agent decision is unchanged, still pending maintainer review). (1) normalize() only forced unresolved_risk on critical/high findings, so verdict=upheld + a MEDIUM finding + critic-set unresolved_risk=false reported "no unresolved risk" — the exact green-but-wrong case this tool exists to catch. Now any finding at MEDIUM severity or above forces unresolved_risk=true and forbids an `upheld` verdict (downgraded to `uncertain`; critical/high still downgrade to `refuted`). `low` nits remain exempt so they don't block a claim. (2) gather_git_diff() built `git diff {base}...HEAD`, which captures branch commits but OMITS the uncommitted working-tree changes the agent usually wants to verify before claiming done. Replaced with gather_diff_evidence(): for the working-tree scope with a base it now emits BOTH the committed-since-base diff (`git diff base...HEAD`) AND the uncommitted diff (`git diff HEAD`, with a plain-worktree fallback on an unborn HEAD), as two labelled evidence blocks. Tests: - upheld_with_medium_finding_flags_unresolved_risk: medium finding => unresolved_risk true and verdict uncertain (not upheld). - upheld_with_only_low_finding_stays_upheld: low-only nit stays upheld, no unresolved risk (documents the carve-out). - diff_scope_with_base_includes_uncommitted_worktree_changes: real dirty repo + base ref => evidence contains both the committed-since-base and the uncommitted marker, in two distinct labelled blocks. --- crates/tui/src/tools/verify.rs | 233 +++++++++++++++++++++++++++++---- 1 file changed, 205 insertions(+), 28 deletions(-) diff --git a/crates/tui/src/tools/verify.rs b/crates/tui/src/tools/verify.rs index 070529fb2..a578f8852 100644 --- a/crates/tui/src/tools/verify.rs +++ b/crates/tui/src/tools/verify.rs @@ -135,8 +135,10 @@ pub struct CritiqueReport { /// Concrete findings the agent must address before claiming done. #[serde(default)] pub findings: Vec, - /// True whenever an unaddressed correctness risk remains. Biased toward - /// `true` on ambiguity so "green-but-wrong" changes are not waved through. + /// True whenever an unaddressed correctness risk remains. Forced `true` by + /// any finding at `medium` severity or above (only `low` nits are exempt), + /// regardless of what the critic self-reported, so "green-but-wrong" + /// changes are not waved through. #[serde(default)] pub unresolved_risk: bool, } @@ -187,19 +189,35 @@ impl CritiqueReport { finding.suggested_fix = normalize_optional(finding.suggested_fix.take()); } + // `has_serious` (critical/high) refutes the claim outright. let has_serious = self .findings .iter() .any(|f| matches!(f.severity.as_str(), "critical" | "high")); + // `has_blocking` (medium-or-above, i.e. anything that is not a `low` + // nit) is a real unresolved correctness concern: it must force + // `unresolved_risk` and forbid an `upheld` verdict, even if the critic + // self-reported `unresolved_risk = false`. This closes the green-but- + // wrong gap where verdict=upheld + a MEDIUM finding + critic-set + // `unresolved_risk=false` reported "no unresolved risk". `low`-only + // findings are intentionally exempt so nits don't block a claim. + let has_blocking = self + .findings + .iter() + .any(|f| matches!(f.severity.as_str(), "critical" | "high" | "medium")); // Verdict: honour an explicit, recognized value; otherwise infer. + // Precedence: any serious finding => refuted; else any blocking + // (medium) finding => uncertain (never upheld); else honour the critic. self.verdict = match self.verdict.trim().to_ascii_lowercase().as_str() { "refuted" | "rejected" | "fail" | "failed" => "refuted".to_string(), "upheld" | "confirmed" | "pass" | "passed" | "ok" => { - // Don't let the critic mark a change clean while it also reported - // serious findings — that is exactly the green-but-wrong trap. if has_serious { "refuted".to_string() + } else if has_blocking { + // A medium finding is a genuine open concern — the claim + // cannot be "upheld" while it stands. + "uncertain".to_string() } else { "upheld".to_string() } @@ -214,9 +232,9 @@ impl CritiqueReport { _ => "uncertain".to_string(), }; - // Fail safe: any serious finding => unresolved risk, regardless of what - // the model set. - self.unresolved_risk = self.unresolved_risk || has_serious; + // Fail safe: any medium-or-above finding => unresolved risk, regardless + // of what the model set. + self.unresolved_risk = self.unresolved_risk || has_blocking; self } @@ -415,17 +433,9 @@ self-check of whether what you just did is actually correct and complete." // --- Deterministic evidence gathering --- let mut evidence: Vec = Vec::new(); if gather_diff_scope { - match gather_git_diff(context.workspace.as_path(), staged, base.as_deref()).await? { - Some(diff) => evidence.push(EvidenceBlock { - label: if staged { - "git diff --cached".to_string() - } else { - "git diff (working tree)".to_string() - }, - body: diff, - }), - None => { /* no diff — recorded via no_code_evidence below */ } - } + evidence.extend( + gather_diff_evidence(context.workspace.as_path(), staged, base.as_deref()).await?, + ); } evidence.extend(gather_files(&files, context)); @@ -566,24 +576,75 @@ and explicitly note in your summary that you could not inspect the actual change out } -/// Gather a git diff, or `None` when there is nothing to diff / git is absent. -/// Unlike `review`, an empty diff is not an error — a claim can be about -/// reasoning, and `files` may carry the evidence instead. -async fn gather_git_diff( +/// Gather git-diff evidence for the requested scope. Returns zero or more +/// labelled blocks; an empty result is not an error (unlike `review`) — a claim +/// can be about reasoning, and `files` may carry the evidence instead. +/// +/// The key correctness point (fix for the base-omits-worktree gap): when a +/// `base` ref is supplied for the working-tree scope, we emit BOTH the committed +/// changes since the merge-base (`git diff base...HEAD`) AND the uncommitted +/// working-tree changes (`git diff HEAD`). `git diff base...HEAD` alone captures +/// branch commits but drops the uncommitted edits the agent usually wants to +/// verify before claiming done. +async fn gather_diff_evidence( workspace: &Path, staged: bool, base: Option<&str>, -) -> Result, ToolError> { +) -> Result, ToolError> { + let base = base.filter(|b| !b.trim().is_empty()); + let mut blocks = Vec::new(); + + if staged { + // Staged scope: the index (optionally vs an explicit base). + let mut args: Vec = vec!["--cached".to_string()]; + if let Some(base) = base { + args.push(base.to_string()); + } + if let Some(diff) = run_git_diff(workspace, &args).await? { + blocks.push(EvidenceBlock { + label: "git diff --cached (staged)".to_string(), + body: diff, + }); + } + return Ok(blocks); + } + + // Working-tree scope. + if let Some(base) = base { + // Committed changes on this branch since the merge-base with `base`. + if let Some(diff) = run_git_diff(workspace, &[format!("{base}...HEAD")]).await? { + blocks.push(EvidenceBlock { + label: format!("committed changes since {base} (git diff {base}...HEAD)"), + body: diff, + }); + } + } + // Uncommitted changes (staged + unstaged) — what "before claiming done" + // usually means. `git diff HEAD` captures both; fall back to a plain + // working-tree diff on an unborn HEAD (empty repo / no commits yet). + let worktree = match run_git_diff(workspace, &["HEAD".to_string()]).await { + Ok(diff) => diff, + Err(_) => run_git_diff(workspace, &[]).await?, + }; + if let Some(diff) = worktree { + blocks.push(EvidenceBlock { + label: "uncommitted changes (git diff HEAD, working tree)".to_string(), + body: diff, + }); + } + Ok(blocks) +} + +/// Run `git diff ` in `workspace`. Returns `Ok(None)` for an empty diff or +/// when git is unavailable, and `Err` when git runs but reports failure. +async fn run_git_diff(workspace: &Path, args: &[String]) -> Result, ToolError> { let Some(mut cmd) = crate::dependencies::Git::command() else { // git not installed: degrade gracefully rather than failing the tool. return Ok(None); }; cmd.arg("diff"); - if staged { - cmd.arg("--cached"); - } - if let Some(base) = base.filter(|b| !b.trim().is_empty()) { - cmd.arg(format!("{base}...HEAD")); + for arg in args { + cmd.arg(arg); } cmd.current_dir(workspace); @@ -904,6 +965,40 @@ mod tests { assert!(report.unresolved_risk); } + #[test] + fn upheld_with_medium_finding_flags_unresolved_risk() { + // The exact green-but-wrong gap: verdict=upheld + a MEDIUM finding + + // critic-set unresolved_risk=false must still surface as unresolved + // risk, and the verdict must not stay "upheld". + let report = CritiqueReport::from_model_text( + r#"{"verdict":"upheld","summary":"seems fine","findings":[{"severity":"medium","issue":"unhandled empty-input case"}],"unresolved_risk":false}"#, + ); + assert!( + report.unresolved_risk, + "a medium finding must force unresolved_risk=true" + ); + assert_ne!( + report.verdict, "upheld", + "cannot remain 'upheld' with an open medium finding" + ); + assert_eq!( + report.verdict, "uncertain", + "medium (not serious) downgrades upheld to uncertain, not refuted" + ); + } + + #[test] + fn upheld_with_only_low_finding_stays_upheld() { + // Intentional carve-out: low-severity nits do not block a claim, so an + // otherwise-clean "upheld" with only a low finding stays upheld and + // does not raise unresolved_risk. + let report = CritiqueReport::from_model_text( + r#"{"verdict":"upheld","summary":"clean","findings":[{"severity":"low","issue":"nit: rename variable"}],"unresolved_risk":false}"#, + ); + assert_eq!(report.verdict, "upheld"); + assert!(!report.unresolved_risk); + } + // === Recursion / re-entry guard === #[tokio::test] @@ -968,4 +1063,86 @@ mod tests { assert_eq!(normalize_severity("weird"), "medium"); assert_eq!(normalize_severity(""), "medium"); } + + // === git diff evidence gathering === + + fn run_git(dir: &Path, args: &[&str]) { + let mut cmd = crate::dependencies::Git::command().expect("git available"); + cmd.args(args).current_dir(dir); + let out = cmd.output().expect("run git"); + assert!( + out.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + + #[tokio::test] + async fn diff_scope_with_base_includes_uncommitted_worktree_changes() { + // Regression for the base-omits-worktree gap: `git diff base...HEAD` + // captures branch commits but drops the uncommitted edits the agent + // usually wants to verify before claiming done. With a base set, the + // gatherer must include BOTH the committed-since-base changes AND the + // uncommitted working-tree changes. + if crate::dependencies::Git::command().is_none() { + return; // no git in this environment — nothing to exercise. + } + + let tmp = tempfile::tempdir().expect("tempdir"); + let repo = tmp.path(); + run_git(repo, &["init", "-q"]); + run_git(repo, &["config", "user.email", "t@example.com"]); + run_git(repo, &["config", "user.name", "Test"]); + run_git(repo, &["config", "commit.gpgsign", "false"]); + + // Base commit. + std::fs::write(repo.join("f.txt"), "line1\n").expect("write"); + run_git(repo, &["add", "."]); + run_git(repo, &["commit", "-q", "-m", "base"]); + + // A committed change on top of the base. + std::fs::write(repo.join("f.txt"), "line1\nCOMMITTED_MARKER\n").expect("write"); + run_git(repo, &["add", "."]); + run_git(repo, &["commit", "-q", "-m", "second"]); + + // An UNCOMMITTED working-tree change. + std::fs::write( + repo.join("f.txt"), + "line1\nCOMMITTED_MARKER\nUNCOMMITTED_MARKER\n", + ) + .expect("write"); + + let blocks = gather_diff_evidence(repo, false, Some("HEAD~1")) + .await + .expect("gather diff"); + let joined = blocks + .iter() + .map(|b| format!("[{}]\n{}", b.label, b.body)) + .collect::>() + .join("\n"); + + assert!( + joined.contains("UNCOMMITTED_MARKER"), + "uncommitted working-tree change must appear in evidence with a base set:\n{joined}" + ); + assert!( + joined.contains("COMMITTED_MARKER"), + "committed-since-base change must also appear:\n{joined}" + ); + // Two distinct labelled blocks: committed-since-base and uncommitted. + assert!( + blocks + .iter() + .any(|b| b.label.contains("committed changes since")), + "expected a committed-since-base block: {:?}", + blocks.iter().map(|b| &b.label).collect::>() + ); + assert!( + blocks + .iter() + .any(|b| b.label.contains("uncommitted changes")), + "expected an uncommitted working-tree block: {:?}", + blocks.iter().map(|b| &b.label).collect::>() + ); + } }