Skip to content

fix(review): bind reviewers to captured Git revisions - #290

Merged
ferueda merged 3 commits into
mainfrom
fix/change-review-revision-isolation
Sep 9, 2026
Merged

ferueda merged 3 commits into
mainfrom
fix/change-review-revision-isolation

Conversation

@ferueda

@ferueda ferueda commented Sep 9, 2026 •

Copy link
Copy Markdown
Owner

Problem and validation

Fix the change-review integrity findings from section 2 of the audit, verified against ac1cd589 before implementation.

  • Advancing the head branch between rev-parse and git diff produces a patch for a different commit than the recorded headSha. A deterministic Git-wrapper regression fails on the original source and passes with this fix.
  • An annotated head tag previously records the tag object rather than its commit; that regression also fails on the original source and passes with this fix.
  • Synthetic Git reproductions confirm that porcelain-only comparison cannot distinguish further edits to an already-dirty file or a switch between two clean commits.

Fix

Resolve base and head to commit IDs once. Run each selected change reviewer in its own temporary detached Git worktree at that captured head instead of the caller's live checkout. Validate the worktree root, exact HEAD, and clean status before accepting results; unreadable verification fails closed. Preserve raw reviewer output and any successful sibling even when revision validation fails. Always attempt owned-worktree cleanup, with checkout hooks disabled during creation.

Diff/plan artifact references remain readable from the isolated checkout. The caller's local changes are not committed, stashed, reset, or included. Dry runs and plan reviews retain their existing execution path.

This addresses the dirty-baseline guard problem at the change-review boundary rather than adding a fingerprinting framework to the generic provider guard. No new dependencies, CLI flags, provider interfaces, cache, or orchestration layer.

Verification

  • Full make check: PASS in the existing Tests workflow, exercising PR head 4ec28ff16d3db2f7aac28f25a326dd6b22d4ac87 merged with base ac1cd589. Formatting, lint, typecheck, tests, build, and distribution smoke all passed on Node 24 with locked dependencies.
  • Added ten deterministic Git regression scenarios covering caller-edit isolation, changed checkout rejection, clean HEAD switches, unavailable post-run verification, cleanup on provider failure, parallel isolation, hook suppression, unrelated-directory preservation, annotated tags, and the moving-ref race.
  • Added five workflow integration cases using the existing injected-provider seam: both provider names receive captured code and readable copied context; content/head/verification failures preserve raw evidence and the successful sibling. These cases ran as part of the passing full CI suite.
  • The ten focused Git scenarios also passed locally against the changed production modules using Node's native test runner (only the test-runner import was adapted). Local dependency installation was unavailable, so the full-gate evidence is the actual CI run, not those adapted local checks.
  • Independent implementation review and live provider execution have not run. No independent reviewer pass is claimed.

Boundaries

This is revision isolation, not a new security sandbox. Codex's configured read-only execution and Cursor's different enforcement remain distinct. Ignored dependencies, environment files, and uncommitted specialist guidance are not copied or symlinked into review worktrees; unavailable checks remain explicit proof limits. Git-linked worktrees share the repository's object/ref storage. See the updated architecture notes.

Kept draft because independent implementation review remains outstanding, despite the passing full verification gate. Not merged.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a9744bf4-02ef-4a26-8bd0-bba21edf4c67


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ferueda
ferueda marked this pull request as ready for review September 9, 2026 17:41
Post-run revision checks were replacing timeout, abort, and workspace-guard
errors with a generic mismatch even after raw evidence was written.
@ferueda
ferueda merged commit 8762bda into main Sep 9, 2026
5 checks passed
@ferueda
ferueda deleted the fix/change-review-revision-isolation branch September 9, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant