Repository navigation
fix(spec): report what a review run wrote outside its spec folder - #3117
mlorentedev wants to merge 2 commits into
Conversation
A reviewer's only sanctioned output is review.md, but nothing checked it: probes, scratch dumps and staged restores were left in the reviewed tree and found by hand five times (#1649). spec deadline, which wraps every reviewer in both modes, now snapshots git status before and after the run when --review is given, names each path changed outside specs/<id>/, and exits 4 when the run otherwise exited 0. A snapshot that fails is reported as unchecked, never as clean. The prompt tells the reviewer to keep probes under mktemp -d and that leftovers fail the run.
mlorentedev
added
the
skip-sdd
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 56 minutes. View limit details
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. CodeRabbit Verify
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func (before TreeSnapshot) StrayWrites(after TreeSnapshot) []string { | ||
| var stray []string | ||
| for path, code := range after.entries { | ||
| if before.entries[path] != code { | ||
| stray = append(stray, path) | ||
| } | ||
| } | ||
| sort.Strings(stray) | ||
| return stray | ||
| } |
There was a problem hiding this comment.
Blind spot: deletions of pre-existing untracked files
Class: THEORETICAL
StrayWrites iterates only after.entries, so a path present in the before
snapshot but absent after is never reported. If the reviewer deletes an
untracked file that already existed before the run (an operator's scratch
file, a .env.local, an untracked config), the tree changed outside the
spec folder yet the run exits 0 with no mention. Same for a file moved
from outside the spec folder into it, which silently relocates operator
content under specs//. The doc comment acknowledges state-vs-content
comparison but not this deletion direction. Fix: also walk before.entries
and report paths missing from after.
| func reportStrayWrites(errw io.Writer, review string, before spec.TreeSnapshot, snapErr error) bool { | ||
| if review == "" { | ||
| return false | ||
| } | ||
| after, err := before, snapErr | ||
| if err == nil { | ||
| after, err = spec.SnapshotTree(review) | ||
| } | ||
| if err != nil { | ||
| _, _ = fmt.Fprintf(errw, "[review] writes outside the spec folder were NOT checked: %v\n", err) | ||
| return false | ||
| } | ||
| stray := before.StrayWrites(after) | ||
| for _, p := range stray { | ||
| _, _ = fmt.Fprintf(errw, "[review] wrote outside %s: %s\n", before.Folder(), p) | ||
| } | ||
| return len(stray) > 0 | ||
| } |
There was a problem hiding this comment.
Snapshot failure exits clean
Class: THEORETICAL
When the snapshot cannot be taken (snapErr non-nil, e.g. --review points
outside a git repository), reportStrayWrites prints "NOT checked" and
returns false, so a run whose reviewer left strays exits 0. The issue's
acceptance says the verdict "says so rather than exiting clean"; a stderr
line satisfies the letter of the PR description, but an operator scripting
on exit status treats an unchecked run as clean. Consider a distinct exit
code or at least confirming this asymmetry is intended.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
⚙️ Agent run details
|
A path only the before snapshot names was dirty and is now clean or gone: an operator's untracked file the reviewer deleted left no status line afterwards and passed unreported.
Review triage
Reviewer output dispositioned: github-actions (PR-Agent), 2026-11-03T06:30:22Z. CodeRabbit did not review (rate limited). The PR author did this triage. |
Part of #1649 (option 2, the floor). Option 1, running the reviewer in an isolated worktree, stays open on the issue.
Summary
A reviewer's only sanctioned output is
review.md, but nothing checked that. Probes, scratch dumps and staged restores were left in the reviewed tree and found by hand five times.spec deadlinewraps every reviewer in both modes. When--reviewis given it now snapshotsgit statusbefore and after the run, and names each path whose status changed outsidespecs/<id>/:[review] wrote outside specs/<id>/: <path>.NOT checked, never as clean.mktemp -d, and that leftovers fail the run.SDD skip rationale
The owner approved
skip-sddfor this change. Exit 4 is the acceptance criterion the issue itself wrote: the run's verdict says so rather than exiting clean.spec deadlineis hidden plumbing that only the review launcher calls, so no user-facing contract changes. A spec gated on #1649 would have to close the issue, while option 1 is still open on it. Most of the 249 lines are tests.Verification
stray_writes_test.gohas no build tag and was only run on macOS. Thetest (windows-latest)job is its first run on Windows. The end-to-end test inspec_deadline_stray_unix_test.gois Unix-only, because it needs a shell runner.Knowledge