feat(tools): agent-callable verify/critique tool (#4196)

[Hmbown]

Fixes #4196

What

Adds 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 (elevated reasoning + an independent critic), on demand, not as an always-on watcher.

The agent states a claim plus optional scope (recent git diff / staged diff / named files / the original requirement / a focus risk). An independent critic then runs at elevated reasoning (default Max, independent of the session tier), tasked to REFUTE the claim, and returns structured findings.

Design (for maintainer review before merge)

• Name: verify (distinct from review = code review of a target, and run_verifiers = external test/build gates).
• Input schema: claim (required); optional requirement, scope (diff|staged|none), base, files[], focus.
• Critic mechanism: a single-shot adversarial model call (mirrors the existing review tool) with reasoning_effort set explicitly on the outgoing request, so the test-time-compute lever never inherits a Low session tier. The tool gathers evidence deterministically (git diff by scope + named file contents) and hands it to the critic — it does not touch subagent/mod.rs (parallel fix(fleet): interactive TUI in-process spawn ignores profile provider pin (#4093 TUI follow-up) #4193), keeping the change additive and disjoint.
• Output: { verdict: refuted|upheld|uncertain, summary, findings: [{severity, issue, evidence, suggested_fix}], unresolved_risk } + usage metadata.
• When-to-call guidance is baked into the tool description (before claiming a non-trivial change done; after a risky/subtle edit; when uncertain — the agent decides; skip trivial changes).

Bounded / no runaway (hard requirement)

• Structural: the critic request carries tools: None, so the critic literally cannot invoke verify (or any tool) — recursion is impossible by construction, not by a denylist that could be forgotten.
• Defense in depth: a task-local re-entry guard makes verify refuse if entered while a critique is already in progress.
• Fail-safe: unstructured critic output, or an "upheld" verdict that still carries a high/critical finding, is normalized to unresolved risk — guarding the green-CI-but-wrong trap.

Opt-out

Feature::Verify ([features] verify_tool, default on) gates registration in the agent runtime surface; cost is only incurred when the agent chooses to call it.

Tests

• Tool contract: name/schema, registered + model-visible, feature-gated registration on/off.
• Critic-finds-a-planted-bug via MockLlmClient (also asserts the outgoing request is elevated + toolless, and threads the claim/requirement/evidence).
• Recursion re-entry guard; fail-safe verdict normalization; fenced-JSON parsing; input validation.

Verification gate

• cargo fmt --all --check — clean
• cargo clippy --workspace --all-features --locked -D warnings … — clean
• cargo test --workspace --locked — 6004 passed; only the 2 known pre-existing unrelated failures (git_repo_root_reports_attempted_paths_when_no_repo_found, skill_hotbar_action_activates_skill_through_dollar_alias), confirmed failing identically on pristine origin/main.
• cargo build --release -p codewhale-tui — success

Note

Do not merge yet — this is a new product capability; the DESIGN should be reviewed with the maintainer first. A follow-up could swap the critic mechanism to a spawned sub-agent (autonomous exploration) behind the same tool contract once #4193 settles; the recursion guard would then also need to exclude verify from that sub-agent's tool allowlist.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 3m 30s —— View job

---

Review: verify tool (#4199)

• Gather context (diff, changed files, AGENTS.md)
• Review crates/tui/src/tools/verify.rs
• Review registry.rs, tool_setup.rs, features.rs, mod.rs, config.example.toml wiring
• Check provider/model/route safety invariant (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608)
• Check recursion guard / bounded critic design claims
• Post review summary

This is a well-scoped, well-tested addition. The design goals hold up under inspection:

• Recursion is genuinely structural. build_critic_request (verify.rs:495-518) hard-codes tools: None, and the critic response is only ever text-extracted/JSON-parsed (extract_text, CritiqueReport::from_model_text) — it's never fed back into the engine's tool-dispatch loop, so even a hallucinated tool-call-shaped string in the critic's output can't trigger anything. The VERIFY_ACTIVE task-local re-entry guard is genuine defense-in-depth on top of that.
• Provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608): clean. VerifyTool just threads through the session's client/model exactly like review/fim/rlm (registry.rs:1028-1041) — no provider is inferred from a model-string prefix. reasoning_effort is set explicitly ("high"/"max"), and per-provider translation is handled centrally by the existing apply_reasoning_effort (client.rs:1862), the same mechanism every other elevated-reasoning caller already relies on — so this doesn't bypass or duplicate that logic.
• Fail-safe parsing (normalize(), verify.rs:180-221) correctly forces unresolved_risk = true whenever a critical/high finding is present, regardless of what the model claims — this is well tested (upheld_with_serious_finding_is_downgraded_to_refuted).

Findings

1. Correctness — scope: "staged" + base produces an invalid git invocation.
crates/tui/src/tools/verify.rs:577-599 (gather_git_diff): when staged is true and a base is supplied, the tool runs git diff --cached <base>...HEAD. git diff --cached does not accept a <commit>...<commit> range — it's a documented usage error (verified locally: git diff --cached HEAD...HEAD exits 129 with a usage message). Since both scope (enum incl. "staged") and base are independently valid per the input schema (verify.rs:318-327) with no cross-field validation or docs saying they're mutually exclusive, the agent can construct this combination and verify will hard-fail with execution_failed instead of degrading gracefully like the "no diff" path does. Note this exact pattern already exists in review.rs's resolve_diff_target (review.rs:629-643), so it's an inherited bug rather than novel — but worth fixing here (and ideally in review.rs too) since the doc comment explicitly frames verify as "mirroring" that tool. Fix this →

2. Robustness — claim/requirement/focus are unbounded while evidence is carefully capped.
crates/tui/src/tools/verify.rs:520-539 (build_critic_prompt): only the evidence blocks are truncated (DEFAULT_MAX_EVIDENCE_CHARS / PER_FILE_MAX_CHARS); claim, requirement, and focus are appended verbatim with no size limit. evidence_budget = max_chars.saturating_sub(header_len).max(1_000) means an oversized claim/requirement just steals the evidence budget down to a 1,000-char floor rather than being bounded itself — so the stated design goal ("Total evidence budget handed to the critic. Kept well under a turn", verify.rs:59-61) doesn't actually bound the total prompt size if the agent pastes something large into claim. Low risk given it's agent-authored text within the same trust boundary, but worth a cap for consistency with the rest of the budget logic.

3. Test coverage gap.
No test exercises gather_git_diff (including the bug above) or gather_files, and no test asserts evidence truncation (PER_FILE_MAX_CHARS/DEFAULT_MAX_EVIDENCE_CHARS) actually fires. The existing tests thoroughly cover the critic-prompt/parsing/guard logic but skip the deterministic evidence-gathering half of the tool.

Not blocking, worth noting

• Feature::Verify is registered with Stage::Stable (features.rs:260-265) even though the PR explicitly asks for a design review before merge and calls this "a new product capability" — Stage::Beta (as used for VisionModel) seems more consistent with that framing, though this is a labeling nit, not a functional issue.

Everything else — schema, opt-out gating, metadata, severity/verdict normalization, and the test suite for the critic path itself — looks solid.

[claude]

Claude finished @Hmbown's task in 59s —— View job

---

Re-review after push (8666e4c)

• Diff the new commit against the previously reviewed one (069bd83)
• Verify fixes for prior findings Fix cargo fmt formatting issues #1 (staged+base) and check status of Simplify UI and improve footer display (v0.3.14) #2/fix: UTF-8 safe truncation, deduplicate url_encode, Display impls, pricing fix, model simplification #3
• Post updated review

New commit 8666e4c ("verify — flag medium findings + include worktree diff") addresses two correctness gaps and is a clean improvement:

1. normalize() now forces unresolved_risk/blocks upheld on any medium-or-above finding, not just critical/high (verify.rs:189-232). This closes a real green-but-wrong gap: previously verdict: upheld + a medium finding + critic-set unresolved_risk: false passed through untouched. low-only findings are intentionally still exempt. Covered by upheld_with_medium_finding_flags_unresolved_risk and upheld_with_only_low_finding_stays_upheld.
2. gather_git_diff → gather_diff_evidence (verify.rs:583-634): for the working-tree scope with a base set, it now emits both the committed-since-base diff (base...HEAD) and the uncommitted working-tree diff (git diff HEAD), instead of only the former — fixing a real "agent verifies stale state" gap. Covered by diff_scope_with_base_includes_uncommitted_worktree_changes using a real repo fixture.

This also fixes my prior finding #1 (staged+base invalid git invocation): the staged branch now runs git diff --cached <base> (a plain tree-ish, valid usage) instead of the old git diff --cached <base>...HEAD (range syntax + --cached, exit 129). Good fix, and structurally sound since it dropped the range entirely rather than special-casing.

Still open from the original review (not touched by this commit, still worth addressing before merge):

• Simplify UI and improve footer display (v0.3.14) #2 — unbounded claim/requirement/focus. build_critic_prompt (verify.rs:530-571) still only truncates evidence blocks; evidence_budget = max_chars.saturating_sub(header_len).max(1_000) means an oversized claim/requirement/focus silently steals budget down to a 1,000-char evidence floor rather than being capped itself. Low severity (agent-authored text, same trust boundary), but the stated "total budget" design goal doesn't actually hold.
• fix: UTF-8 safe truncation, deduplicate url_encode, Display impls, pricing fix, model simplification #3 — no test for gather_files or evidence truncation. gather_files (verify.rs:672) and the PER_FILE_MAX_CHARS/DEFAULT_MAX_EVIDENCE_CHARS truncation logic still have no direct test, even though this commit added good coverage for the diff-gathering half.

Not blocking, unchanged from before: Feature::Verify is still Stage::Stable despite the PR's own "review the design before merge" framing — Stage::Beta would read more consistently, but it's a labeling nit.

Net: the pushed fix is correct and well-tested for what it targets. Recommend addressing #2/#3 (small, low-risk) before merge, but neither blocks on correctness grounds.