[bkataru]

What this fixes

pawan review printed "No changes to review" and exited 0 whenever a git diff probe failed. Both probes in run_review read stdout only and never consulted output.status or stderr. Command::output() yields Ok whenever the process spawns, so a git that ran and failed (not a repo, dubious ownership, corrupt .git) produced empty stdout and was reported as "no changes" — and the fallback probe failed identically, reading as corroboration.

Closes #102. Also closes the fresh-repo gap behind #108 (a staged first commit in a repo with no commits was invisible to both probes).

How

run_review now calls review_diff(), which classifies each probe into a tri-state DiffProbe (Found / Empty / NoHead / Failed):

• git_diff_stdout checks output.status and surfaces stderr. A non-zero exit is a probe failure, never evidence of an empty diff.
• git diff HEAD exiting 128 with "ambiguous argument 'HEAD'" is classified NoHead — the one legitimate failure (a repository with no commits) — treated as Empty, with the new git diff --cached probe covering staged work in that case.
• Emptiness is reported only when every probe ran clean-empty; any unexplained failure is surfaced as PawanError::Git instead of a false "No changes to review."

Relationship to #105 and #111

This supersedes both for #102:

• fix(pawan): surface git probe failures instead of failing open (#98, #102) #105 (checked_git_stdout) surfaces probe failures — covers pawan review prints "No changes to review" and exits 0 when git diff fails (same shape as #98) #102's call sites — but does not add the fresh-repo --cached probe.
• fix(review): staged work in a fresh repo must not read as 'no changes' #111 (git diff --cached) covers the fresh-repo case but its git_diff_stdout still ignores output.status/stderr, so it does not fix pawan review prints "No changes to review" and exits 0 when git diff fails (same shape as #98) #102 (its own docstring defers that to pawan commit reports "working tree clean" and exits 0 when the git probe fails (fails open) #98/pawan review prints "No changes to review" and exits 0 when git diff fails (same shape as #98) #102).
• The two also textually conflict inside review_diff (commit-sequenced merge produces conflict markers), and merging fix(review): staged work in a fresh repo must not read as 'no changes' #111 orphans fix(pawan): surface git probe failures instead of failing open (#98, #102) #105's checked_git_stdout away from run_review.

This branch combines both intents so neither is lost. Happy to reconcile differently if the owners of #105/#111 prefer.

Verification

gate	result
cargo check -p pawan	exit 0
cargo fmt -p pawan --check	clean
cargo clippy -p pawan --all-targets	exit 0 (only pre-existing pawan-core warnings)
cargo test -p pawan --bin pawan	6 new tests pass

Mutation proof: reverting git_diff_stdout's failure branch to Empty (the pre-fix shape, proven present by grep before running) makes the broken-git test fail on exactly the defect assertion — "a broken git must surface as an error, not 'no changes'" (rc=101). The test has teeth.

Six regression tests: staged work in a fresh repo (#108), clean repo, empty fresh repo (NoHead is not an error), unstaged fallback, broken git surfaces an error (#102), and the NoHead classifier.

[bkataru]

Review: strongest of the three fail-open fixes — correct mechanism, real tests, red gate is pre-existing drift

What the change does (main.rs +138/-45): extracts review_diff returning Result<Option<String>>, and introduces a DiffProbe enum that classifies each git probe outcome: Found / Empty / NoHead / Failed. The critical distinction #102 needs: a probe that ran and failed (non-zero exit for any reason other than the no-HEAD case) is never read as an empty diff — it surfaces as Err. The one legitimate failure, git diff HEAD exiting 128 in a repo with no commits, is classified NoHead and treated as Empty, with the staged --cached probe covering that case.

Addresses pawan#102? Yes, and it also subsumes the no-commit-repo false negative (#108) as a natural consequence — the staged probe finds staged work the HEAD probe cannot see. The probe-ordering note (--staged mode still runs the plain git diff fallback, preserving pre-existing review --staged behaviour) is an honest flag that a separate behavioural question exists; correctly left out of scope.

The tests are the real evidence. Five new tests with a scratch-repo fixture: (1) staged work in a no-commit repo is found — the #108 repro; (2) a genuinely clean repo reports nothing — the control, so the fix hasn't traded one wrong answer for another; (3) an empty fresh repo yields no diff, not an error — NoHead is not a hard failure; (4) unstaged work still found via the plain-diff fallback; (5) fault injection — .git removed so every probe fails with "not a git repository", asserting review_diff errors rather than reporting "No changes to review". Plus a unit test pinning the NoHead classification. This is the mutation-test-style discipline the estate asks for: the broken-git test would fail against the pre-fix code, so it has teeth.

CI: Check & Lint FAIL is the same pre-existing clippy::double_must_use drift (rust 1.99.0, #[async_trait] on LlmBackend, pawan-core/agent/backend/mod.rs:18) — untouched by this PR. Doc pass, Test (nextest) pass 16m0s, macOS TUI smoke pass. Approve on content.

[bkataru]

Amendments to my review (two measured corrections)

Pre-existing lint — now measured, not inferred. Main's latest Check & Lint (run 27495472815, headSha de893a9, 2026-06-14) is green but predates the rust 1.99.0 promotion of double_must_use; the failure site is unchanged on main and fails under today's toolchain on all three open PRs. Filed as #116.

Overlap/supersession with #105. #115 and #105 rewrite the same run_review probe region and textually conflict (git merge-tree --write-tree on the two heads → CONFLICT (content) in main.rs). This PR is the stronger resolution of the same #102 fail-open: typed DiffProbe vs #105's unwrap_or_default() residual swallow. This PR should be the one merged; #105 superseded.

Scope note (per an earlier review concern): the completed-but-nonzero-exit path is correct — the unchanged parse logic reads stdout regardless of exit status ("exit code is non-zero when vulnerabilities are present, but stdout still contains the JSON"). One residual, out of #102's scope: parse_audit_json returns an empty Vec on unparseable JSON, which reads as clean — worth a follow-up, not a blocker for this PR.

Body should add Fixes #108 — the test test_review_diff_finds_staged_work_in_a_repo_with_no_commits demonstrates it.

[bkataru]

Correction to my earlier note (which pointed at #117 — I've since closed that as a duplicate): the red Check & Lint here is not caused by this PR's diff. rust 1.99.0 promoted clippy::double_must_use, which false-fires on async-trait 0.1.89's macro-generated #[must_use] and is fatal under -D warnings — unchanged main was already red. The fix is the async-trait 0.1.89 -> 0.1.92 bump, which already exists as #94 (also #87, #92 — byte-identical Cargo.lock diffs). Once one of those lands, rebasing this PR should clear the red.

[bkataru]

Closing as redundant: the content is already on main (test block main.rs:256 via 60f2389/#28; fail-closed guard main.rs:1778 via 88f92f1). Verified by a lane + a pane. Nothing to land; merging would duplicate. #102 closes once the reproduction on origin/main confirms the symptom is gone.