feat(pr-review): auto-fix findings and rebuild the review presentation - #693
Conversation
GitGuardex code-assist only described problems; every fix was a human click. It also had five defects that made the posted review fragile: findings were never validated against the diff (one bad line number 422s the whole review, which blocks a merge in gate mode), each run stacked a duplicate review, multi-line fixes were unexpressible, the gate verdict was invisible, and an uncapped diff let a truncated review read as clean. Review side: - src/review-diff.js parses the diff into commentable RIGHT-side lines; unanchored findings move to the summary instead of failing the POST, and a rejected POST retries once as summary-only. - fingerprint markers let a re-run skip what it already reported. - start_line/start_side forwarded for range suggestions; commit_id pins the anchors to the PR head. - summary is now a report card (severity mix, gate verdict, findings table, footer); inline bodies use GitHub alert callouts with the tail folded into <details>. - prompt requires `suggestion` for bounded edits and adds `category`. - diff capped at 220k chars, with truncation surfaced as partial. - extractJsonPayload tries raw JSON before the fence extractor, so a fenced `suggestion` inside valid JSON no longer hijacks the parse. Fix side: - src/review-fix.js runs the provider in edit mode and commits the result. Refuses protected branches and uncommitted tracked edits, stages only paths the fix touched, and is pinned to the target branch. - `gx pr-review --fix` applies findings locally. - `gx branch finish --gate-autofix [--gate-autofix-rounds N]` lets the merge gate repair blocking findings: fix, push, then re-review with a fresh provider run. Bounded rounds; a no-op fix stops the loop and the gate still blocks. All flags default off. Verified: npm test failing set byte-identical to main (41 pre-existing), biome 1.9.4 lint clean, openspec change + specs valid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NagyVikt
left a comment
There was a problem hiding this comment.
Code review — PR #693
Decision: APPROVE with comments — 0 CRITICAL, 0 HIGH, 2 MEDIUM, 2 LOW.
Validation
| Check | Result |
|---|---|
| Lint (biome 1.9.4, pinned) | Pass — 187 files, no findings |
Tests (npm test) |
786/828 pass, 41 fail — failing set byte-identical to main (pre-existing baseline red), 1 skipped |
node --check bin/multiagent-safety.js |
Pass |
bash scripts/check-script-symlinks.sh |
Pass — 14 pairs |
openspec validate <change> --type change --strict |
Pass |
openspec validate --specs |
Pass — 133/133 |
Correctness properties verified
- The gate decides on all findings, not the posted subset.
runPrReviewreturns every normalized finding regardless of anchoring or dedupe, andevaluate(review.findings)consumes that. So demoting a finding out of the inline view, or skipping it as a duplicate, can never soften the merge gate. - New flags never reach the shell script.
gx finishbuildsfinishArgsfrom an explicit allowlist (src/finish/index.js:451);gx branch finishstrips them insplitGateReviewFlags. Confirmed empirically: passing--gate-autofixto the script surfaces no unknown-argument error. - Auto-fix cannot write to the wrong place. Four independent guards: protected-branch refusal, uncommitted-tracked-edits refusal,
expectBranchpin, and explicit path staging (neveradd -A). - The fixer never certifies itself. The gate loop always re-invokes
runPrReviewafter a fix; the fix result is never used as the verdict.
MEDIUM
-
src/pr-review.js—postGithubReviewtreats any non-zero POST status as an anchoring failure and retries summary-only, reporting "inline anchoring rejected by GitHub". A transient 5xx or network blip therefore loses the inline comments and prints a misleading cause. Behavior is still safe (the review posts, the gate is unaffected); the message is just wrong for non-422 failures. Distinguishing 422 from other statuses would need the response body, whichgh apiwrites to stderr — a follow-up. -
src/review-fix.js— a fix that touches a very large number of files stages them in onegit add -- <paths>argv. Well under Linux ARG_MAX for any realistic fix, but unbounded in principle.
LOW
-
src/review-diff.js:12—GIT_HEADER_REis greedy; a path literally containingb/would split at the wrong point. Backtracking resolves the normal case correctly, and+++ b/<path>overrides the provisional value whenever git emits it. -
src/review-fix.js—commitMessagedoes not cap per-finding body line length, so a verbose finding produces a long commit-body line.
Notes
- All new behavior is opt-in:
--fix,--gate-autofix,--gate-autofix-roundsdefault off. The only unconditional change to existing runs is the review presentation plus the anchoring/dedupe safety, both of which strictly reduce failure modes. extractJsonPayloadreordering (raw JSON before fence extraction) fixes a live bug found by the new tests: valid JSON whosesuggestioncontains a code fence previously parsed as the fence contents.
Summary
Test plan