Skip to content

fix(review): reject repository-local grafts - #2113

Open
dnlrsls wants to merge 3 commits into
Gentleman-Programming:mainfrom
dnlrsls:fix/reject-local-grafts
Open

fix(review): reject repository-local grafts#2113
dnlrsls wants to merge 3 commits into
Gentleman-Programming:mainfrom
dnlrsls:fix/reject-local-grafts

Conversation

@dnlrsls

@dnlrsls dnlrsls commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Linked Issue

Closes #1719


PR Type

  • type:bug - Bug fix (non-breaking change that fixes an issue)
  • type:feature - New feature
  • type:docs - Documentation only
  • type:refactor - Code refactoring
  • type:chore - Build, CI, or tooling changes
  • type:breaking-change - Breaking change

Summary

  • Reject repository-local Git grafts before exact-revision ancestry derivation can consume rewritten parents.
  • Resolve the actual Git common directory so primary and linked worktrees fail closed consistently.
  • Derive first-parent ancestry from the raw commit object so grafts cannot influence base-tree selection.
  • Register the integrity guard and cover both repository layouts with behavior-first regression tests.

Changes

File / Area What Changed
.guard-population-baseline.txt Registered the exact-revision history guard.
internal/cli/guard_population_test.go Updated the frozen guard declaration count.
internal/reviewtransaction/snapshot.go Rejects active common-dir info/grafts, derives parents from raw commit headers with replacement objects disabled, and declares external graft cleanup by design.
internal/reviewtransaction/snapshot_test.go Covers normal and linked-worktree graft influence, refusal, and ungrafted raw-parent/base-tree derivation.

Test Plan

Passed

  • git diff --check
  • go run ./internal/gofmtcheck
  • go test ./internal/reviewtransaction -run TestSnapshotBuilderExactRevision -count=1
  • go test ./internal/cli -run TestEveryProductionRefusalNamesResolutionOrDeclaresByDesign -count=1
  • go build ./...
  • go vet ./...
  • bench: go build ./... and go vet ./...

Incomplete / unrelated local-suite failures

  • go test ./... -count=1 did not complete cleanly in the Windows worktree. Unrelated update/release tests require unavailable WSL/POSIX tooling, and three review/SDD tests timed out outside the exact-revision path. Focused affected tests pass.
  • bench/go test ./... -count=1 still fails TestProbeCapabilityRejectsAMissingFlag, TestProbeAndHelpProbeDoNotShareACacheEntry, and TestReadBackBlanksGitTrace. The benchmark module is untouched, and these are the same runner failures previously reproduced on the exact base.
  • E2E was not run locally. Remote CI covers the repository runtime matrix, while the affected logic is exercised against real temporary Git repositories by the focused tests.

Automated Checks

All automated checks passed on commit 44a0aa45, including Unit Tests, Go Format, runtime checks, the complete E2E matrix, PR validation, and CodeRabbit. The authored diff is 124 changed lines (119 additions and 5 deletions), within the 400-line review budget.


Contributor Checklist

  • PR is linked to an issue with status:approved
  • PR stays within 400 changed lines
  • Exactly one type:bug label is applied
  • Full local unit and benchmark suites are not claimed green; bounded failures are disclosed above
  • Go format passes (go run ./internal/gofmtcheck)
  • E2E was not run locally; rationale is documented above
  • Benchmark build and vet pass; unchanged runner failures are documented above
  • Documentation is not required for this internal fail-closed correction
  • Commits follow Conventional Commits format
  • Commits have no Co-Authored-By trailers

Notes for Reviewers

Please focus on the common-directory boundary for primary and linked worktrees, and on raw first-parent derivation remaining immune to repository-local grafts.

@dnlrsls dnlrsls added the type:bug Bug fix label Jul 31, 2026
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Exact-revision snapshot resolution now rejects repository-local Git grafts and reads native commit ancestry. Tests cover primary and linked worktrees. Guard-population metadata includes the new fail-closed guard.

Changes

Exact-revision snapshot integrity

Layer / File(s) Summary
Exact-revision graft validation
internal/reviewtransaction/snapshot.go
Exact-revision resolution inspects the Git common directory, rejects an existing info/grafts file, propagates inspection errors, and reads commit parents from the raw commit object with GIT_NO_REPLACE_OBJECTS=1.
Regression and guard coverage
internal/reviewtransaction/snapshot_test.go, .guard-population-baseline.txt, internal/cli/guard_population_test.go
Tests verify unchanged ungrafted derivation, graft effects on Git results, native parent and tree resolution, and refusal during snapshot construction. Guard metadata and its expected declaration count now include exact-revision-history.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested reviewers: alan-thegentleman

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy #1719 by rejecting local grafts, resolving Git common directories, preserving ungrafted derivation, and adding regression tests.
Out of Scope Changes check ✅ Passed All changes support #1719, including guard registration, implementation updates, and regression coverage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: rejecting repository-local Git grafts during review snapshot derivation.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/reviewtransaction/snapshot.go`:
- Around line 1042-1045: The parent commit lookup in the snapshot-building flow
must be atomic with the graft validation: replace the post-check runGit rev-list
call with raw commit-object parsing from the repository used by
rejectRepositoryLocalGrafts, or query the raw object with grafts disabled, then
use that parent when selecting the base tree.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0f870111-3e5d-49f0-a465-43eea98ed275

📥 Commits

Reviewing files that changed from the base of the PR and between fdecaf5 and 77a5f51.

📒 Files selected for processing (4)
  • .guard-population-baseline.txt
  • internal/cli/guard_population_test.go
  • internal/reviewtransaction/snapshot.go
  • internal/reviewtransaction/snapshot_test.go

Comment thread internal/reviewtransaction/snapshot.go Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(review): reject local grafts when deriving immutable snapshots

1 participant