fix: harden git-backed change review - #175
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Waishnav
changed the title
Greptile SummaryThis PR hardens git-backed change reviews by adding unborn-repository support, HEAD re-anchoring, whitespace filtering, and oversized-patch degradation.
Confidence Score: 2/5This PR should not merge until re-anchoring preserves pending edits, persisted checkpoints restore HEAD tracking, and oversized numstat output degrades safely. The new checkpoint flow can silently baseline unreviewed edits after HEAD movement, skip history-change detection after a manager restart, and fail outright on sufficiently large file lists. Files Needing Attention: src/review-checkpoints.ts, src/review-checkpoints.test.ts
|
| Filename | Overview |
|---|---|
| src/review-checkpoints.ts | Adds the git-hardening behavior, but re-anchoring can consume pending edits, persisted managers lose HEAD tracking, and numstat overflow bypasses degradation. |
| src/review-checkpoints.test.ts | Adds useful regression coverage but omits HEAD movement with pending edits, restart-plus-HEAD movement, and numstat overflow. |
| docs/configuration.md | Documents journal and git review modes, checkpoint behavior, whitespace filtering, and oversized-patch fallback. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[show_changes in git mode] --> B{HEAD moved?}
B -- Yes --> C[Snapshot current working tree]
C --> D[Set open and baseline refs]
D --> E[Return empty review]
B -- No --> F[Snapshot current working tree]
F --> G[Generate patch with 10 MB fallback]
G --> H[Generate numstat with 10 MB hard limit]
H --> I[Summarize files]
I --> J{Mark reviewed?}
J -- Yes --> K[Advance baseline]
J -- No --> L[Return review]
K --> L
Reviews (1): Last reviewed commit: "docs: document change review modes and r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const snapshot = await createWorkingTreeSnapshot(state.gitRoot); | ||
| await git(state.gitRoot, ["update-ref", state.openRef, snapshot.commit]); | ||
| await git(state.gitRoot, ["update-ref", state.baselineRef, snapshot.commit]); |
There was a problem hiding this comment.
When HEAD moves while unreviewed working-tree edits are present, createWorkingTreeSnapshot includes those edits in both new checkpoints and returns an empty review, causing the pending edits to disappear from subsequent reviews.
| } else { | ||
| state.openRefAvailable = openCommit !== undefined; | ||
| state.baselineRefAvailable = baselineCommit !== undefined; | ||
| } |
There was a problem hiding this comment.
When a manager restarts with existing review refs, this branch leaves headTracked unset, so subsequent HEAD movement bypasses re-anchoring and can report committed or unrelated branch changes as pending workspace changes.
| const numstat = (await git(state.gitRoot, ["diff", ...whitespaceArgs, "--numstat", "-z", baseline, current.commit], { | ||
| maxBuffer: 10 * 1024 * 1024, | ||
| })).stdout; |
There was a problem hiding this comment.
When git diff --numstat -z produces more than 10 MB of output, this uncaught buffer limit makes show_changes fail instead of returning the documented file-list fallback.
7395469 to
657bff7
Compare
Review checkpoints were commits created with -p HEAD from an index in the system temp directory, and the whole path failed on repositories with no commits yet. Snapshots are now root commits built from a temporary index inside the git common directory with fsmonitor and the untracked cache disabled, so capture is isolated from the real index, immune to HEAD ancestry, and works on unborn repositories. The checkpoint manager records the HEAD used at capture and re-anchors both refs to fresh snapshots when the repository's HEAD moves, so reviews never diff across unrelated histories. Existing checkpoints created before this change are left untouched. Diffs now ignore whitespace-only changes by default, use the standard 10 MB output cap, and degrade to a file list with a note instead of failing when a patch exceeds it.
The unborn-repository test now asserts the improved behavior: an empty repository is reviewable immediately, re-anchors after the first commit, and continues reviewing from the new baseline. New tests cover whitespace-only changes being ignored by default but visible on request, and an oversized diff degrading to a file list.
Describe the journal-based review, the DEVSPACE_REVIEW_MODE=git fallback, and how checkpoints are stored and re-anchored.
Re-anchoring used to bake the whole working tree, unreviewed edits included, into the new baseline snapshot, which made pending changes vanish from every subsequent review. The baseline now points at the new HEAD commit itself, so unreviewed working-tree edits stay visible. HEAD tracking is restored when a restart finds persisted review refs, so a HEAD move after a restart is absorbed by re-anchoring instead of reporting committed changes as pending workspace changes. The degraded diff path no longer runs the 10 MB-capped numstat call that could overflow and fail the whole review; oversized diffs now fall back to a name-only file list, and a review that cannot list files either reports the degradation instead of claiming there were no changes.
657bff7 to
a371fb3
Compare
1 participant
Hardens the git-backed review fallback (
DEVSPACE_REVIEW_MODE=git).What changed
HEAD, so capture is isolated from the real index and works on repositories with no commits yet.DEVSPACE_REVIEW_MODE, and the checkpoint refs (refs/devspace/review/<workspace>/openand/baseline).Why
The fallback is the path users choose when they want repository-backed review; it should be as robust as the journal. Root commits and re-anchoring remove the snapshot's coupling to a moving
HEAD; the new unborn-repository behavior makes empty repos reviewable immediately.Top PR of three; sits on #174.