feat: Commit Review, fix-with-agent + iterations/coverage + review trailer (v3.7.0 part 2) - #160
Closed
devlint wants to merge 12 commits into
Closed
feat: Commit Review, fix-with-agent + iterations/coverage + review trailer (v3.7.0 part 2)#160devlint wants to merge 12 commits into
devlint wants to merge 12 commits into
Conversation
added 12 commits
August 17, 2026 10:47
…ch worktree Task 3 of v3.7.0: adds buildReviewFixPrompt (pure, no secrets, PTY-safe), a Fix with agent control in CommitReviewModal (tool picker + scratch worktree checkbox), and useCommitReview.armReReview/onStagedSetChanged, the real staged-set-watcher trigger for the one-shot re-review after a fix handoff (decision D7: the prompt is typed into the agent PTY without pressing Enter). Factors confirmNewAiTask's scratch-worktree sequence into a shared helper. Un-hides the previously dangling commitReviewAutoReReview setting now that it has a real consumer. disabled
Task 4 of v3.7.0: adds commitReviewState.ts, a pure, localStorage-persisted store (pattern: usePrCache.ts) tracking review->fix->review iterations and content-hash based coverage of the staged diff's added lines, so a fix that shifts line numbers never destroys coverage. Wires iterations/coverage into useCommitReview (recorded only for a non-aborted run, guarding against the same stale-run race already covered for running/progress), refreshes iterations from persisted state on repo switch, and clears state after a successful commit (a new commit starts a new review cycle). disabled
…ed/skipped decision Task 5 of v3.7.0: adds buildReviewTrailer, the pure commit-gate helpers (resolveCommitReviewGate, effectiveReviewDecision, appendReviewTrailer) and CommitReviewDecisionModal.vue, wired into App.vue's handleCommitRequest via a shared proceedToCommit path (also used by the secrets "Commit anyway" route, so both funnel through the same gate). Same non-blocking UX contract as the v3.5.0 secrets scanner: cancelling the decision modal cancels the commit rather than silently recording "skipped" (decision D8). "Review now" leaves the commit pending; a later commit click with iterations > 0 proceeds without re-prompting and records "ran". A successful commit clears the review-state cycle. disabled
…flakiness The real-timer version of the debounce/one-shot re-review test had only a few ms of margin between its waits and the debounce window, and failed once under load during full-suite verification. Switches it to fake timers (pattern: useSecretsScanner.test.ts), scoped to just this one test. disabled
….ts binary hashLineKey's template literal had a literal, embedded NUL byte instead of the \0 escape sequence, which made the whole file register as binary to git (git diff showed "Binary files differ", GitHub would render it as "Binary file not shown", and git blame/merge/diff were broken on it going forward). Replaces the raw byte with the \0 escape sequence in source, so the runtime hash values are byte-identical (verified: every existing hashLineKey test still passes unmodified) while the file itself is plain UTF-8 text. Adds a regression guard that reads the source file's own bytes and fails if a raw NUL character ever reappears. disabled
…onest between reviews Two related correctness fixes to the iterations/coverage tracking: 1. Coverage no longer asserts 100% right after a plain staged-set change. onStagedSetChanged now recomputes it against the CURRENT staged diff (a plain git diff fetch, no LLM call), guarded by a generation counter so a stale in-flight refresh can never clobber a newer one or a real run(). Previously a brand-new unreviewed file staged after a completed review kept showing coverage:100% until the next explicit review click. 2. iterations is now bound to the repo's HEAD commit, not just the review count. run() resolves and stamps HEAD via a single git rev-parse HEAD call per run, and reconcileIterationsForHead (awaited by the commit gate before it ever reads iterations) resets the count to 0 when HEAD moved since the last recorded review. Without this, a commit made outside the app (amend, terminal commit, any external tool) left a stale iterations count that let the gate skip the decision modal and write a "ran" trailer for a review that never happened against what was actually being committed. Also resets iterations when snapshot pruning empties a repo's snapshot list entirely (aged-out evidence). disabled
… now, harden the agent handoff
- proceedToCommit awaits commitReview.reconcileIterationsForHead before
resolveCommitReviewGate ever reads iterations, so a commit made outside
the app since the last review is caught before the gate decision (see
the companion commitReviewState/useCommitReview fix).
- onCommitReviewDecisionReviewNow ("Review now" in the decision modal) now
reuses onReviewStagedClicked directly instead of a weaker duplicate:
it only opens the findings modal when a review actually completed
without error, and surfaces "no AI provider configured" via repoError
when the run never even attempted. Previously it always popped an empty
"No findings" modal regardless of outcome.
- onCommitReviewFixWithAgent adds a short readiness wait before typing the
prompt into a freshly spawned agent PTY, and surfaces scratch-worktree
and terminal-session failures instead of silently doing nothing after
the review modal has already closed.
Manual QA performed against real claude and codex CLIs via the dev-server's
node-pty backend (see PR report for the full write-up): once an agent is
at its normal ready-to-chat input, writing the whole multi-line prompt as
one burst lands as unsent text with no premature submission. A first-run
"trust this directory?" onboarding screen (always hit by the scratch-
worktree path, since it is always a brand-new directory) is a separate,
unresolved risk this readiness wait does not cover, flagged in code
comments and the PR report for explicit sign-off before merge.
disabled
sortFindingsForReview lived in composables/useCommitReviewNav.ts, but utils/reviewFixPrompt.ts (a pure utils module) needed the same order and was importing it from there, inverting the established utils-do-not- depend-on-composables direction (Task 0's unifiedDiff.ts/editableTarget.ts precedent). Moves it to utils/reviewFindingsSort.ts; useCommitReviewNav.ts re-exports it verbatim for back-compat with CommitReviewModal.vue's import. disabled
Deliberate scope-narrowing for PR2, not a bug fix. Manual QA against real
claude/codex CLIs (see the previous commit's report) found that a brand-new
scratch worktree always hits a first-run "trust this directory?" onboarding
screen that misinterprets the piped fix prompt as menu navigation, which
drove a real brew upgrade --cask codex attempt in testing. The user was
shown this finding and decided to disable the scratch-worktree option for
this PR rather than ship it with that risk.
CommitReviewModal.vue's "Fix with agent" footer no longer offers a scratch
checkbox; its fix-with-agent emit payload drops the scratch flag. App.vue's
onCommitReviewFixWithAgent always targets the current repo (already
trusted, no onboarding screen) and no longer calls
createAiTaskScratchWorktree, which stays untouched for its other caller,
confirmNewAiTask ("New AI task"). Removed the now-unreachable
commitReview.fixInScratch i18n key from all 5 locales after confirming it
was not used anywhere else. Updated ROADMAP.md's Fix with agent bullet to
record the cut and the follow-up: revisit scratch-worktree support once
there is a real fix for the onboarding-trust-screen problem (pre-trusting
the directory before launching the agent, or detecting the onboarding
screen before writing).
disabled
…fore the trailer is written Second verifier pass, HIGH: coverage:100% was still reachable in two real scenarios even after the previous round's fix. Scenario B: stage a.ts, review it (coverage 100), then edit a.ts further and restage it. The staged file COUNT never changes (still one file, just more content), so App.vue's staged-set watcher (keyed on repoStats.staged, a count) never fires onStagedSetChanged, the coverage refresh never runs, and the commit gets a stale coverage:100% for content that was never reviewed. This is the single most common review-fix-review cycle. Scenario C: run()'s own coverage computation compared the just-reviewed, file/byte-capped subset against itself, a tautology that always yields 100% even when the file-count cap dropped most of a large staged tree. Fix: a new shared pure helper, coverageFromDiffText, always computes coverage against the FULL parsed staged diff, never the capped subset sent to the AI. run() now uses it for both of its own coverage snapshots. A new computeCurrentCoverage method wraps it with a fresh git diff fetch and is awaited by App.vue's proceedToCommit right before buildReviewTrailer, so the trailer's coverage number is correct at the moment of commit regardless of staged-set-watcher granularity or truncation. Also fixes a stale doc comment on createAiTaskScratchWorktree that still claimed it was shared with Commit Review's scratch-worktree option (removed in the previous commit) — it is now only used by "New AI task". disabled
…nt finding text Second verifier pass, low priority. sanitizeLine only stripped \r and collapsed newlines before a finding's title/detail got written raw into a PTY. That text is parsed from an AI response to an arbitrary diff, so a crafted diff could in principle smuggle other control bytes (ANSI escape sequences, NUL, BEL) into the raw PTY write. Broadens the strip to the full C0 control range plus DEL after the existing \r/newline handling. disabled
… 100 when disabled mid-cycle Third verifier pass, item L1: computeCurrentCoverage and refreshCoverageForCurrentDiff both defaulted next to 100 whenever the feature was disabled or the diff fetch failed, discarding whatever truthful coverage the ref already held. Toggling Commit Review off between a real review and the commit could launder a genuinely partial coverage back to a false 100% in the trailer, the same failure class already fixed twice for other code paths in this PR.
This was referenced Aug 19, 2026
Owner
Author
|
Superseded by #164, which combines all three PRs into a single review surface for the full v3.7.0 Commit Review feature. |
devlint
added a commit
that referenced
this pull request
Aug 19, 2026
Combines the full v3.7.0 "Commit Review" feature, originally built and adversarially reviewed across three stacked PRs (#159, #160, #163), then further product- and code-reviewed as a whole and fixed here (see commit history for the detailed round-by-round verification trail). - Review staged changes: AI pass over the staged diff, inline findings with severity badges, n/p/x navigation - Fix with agent: pipe findings into a terminal AI agent session - Iterations & coverage tracking, bound to HEAD - Review / Vouch / Skip commit-time decision, recorded as a GitWand-Review trailer - Per-repo .gitwandrc opt-in overriding the global setting in either direction - Composable pre-commit hook merging the shipped secrets-scanner section with a new warn-only review reminder - Fixed a core reactivity bug where routine background polling silently wiped findings, plus 13 other findings from a dedicated product/code review round 1067 core + 959 desktop tests, parity suite green, clean build, zero new Tauri commands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second slice of ROADMAP.md's v3.7.0 "Commit Review" feature, stacked on #159 (part 1: staged-diff review engine + issue navigation). This PR adds:
brew upgrade). "Fix with agent" in this PR only ever targets the current, already-trusted repo. Scratch-worktree support for this feature is left as a follow-up once there's a real fix for the onboarding-screen risk (e.g. pre-trusting the directory before launching the agent).iter:N) and the share of the staged diff already reviewed (coverage:X%), content-hash based so line-number shifts from a fix don't destroy the count. The review cycle is bound to HEAD, so an amend or an out-of-app commit correctly starts a fresh cycle instead of reusing stale state. Coverage is recomputed against the full current staged diff immediately before the commit trailer is written, so it can't lie because of watcher granularity or a review's file/byte truncation cap.GitWand-Review: ran|vouched|skipped (iter:N, coverage:X%)via the existing commit-trailers support. The parenthetical is omitted for a plainskippedwith no review activity.This PR went through three rounds of implementation + adversarial verification. Round 1 found a blocking binary-file corruption (a literal NUL byte in a source file) plus real logic gaps in the coverage/iteration accounting and the PTY safety story. Round 2's fix for the coverage accounting was itself found to be incomplete (still wrong in the most common edit-and-restage-the-same-file workflow, and after a truncated review) and was fixed and re-verified in round 3. A final low-severity finding (an optimistic 100% fallback when the feature is toggled off mid-cycle) was fixed directly after the third pass confirmed everything else.
Test plan
pnpm --filter @gitwand/core run test -- --run— 1056/1056 passing (unchanged)cd apps/desktop && pnpm test -- --run— 831/831 passing (103 files, +101 tests vs PR1's baseline: fix-with-agent prompt building, iteration/coverage state incl. HEAD-cycle binding and snapshot pruning, the decision modal, and multiple race-condition regression tests)cd apps/desktop && pnpm build— cleanvue-tsc --noEmit+vite buildclaudeandcodexsessions (via the dev-server's PTY backend) confirmed the no-auto-submit design holds once an agent is at its normal ready inputclaudebut not fully forcodex/opencode— a bracketed-paste wrap was considered as a more robust alternative to the current boot-timing wait but not implemented (further manual QA attempts risked repeating the onboarding-screen misfire); left as a documented follow-up, not a blocker, consistent with dropping scratch-worktree support in this PR