Skip to content

fix(strategy): compare redo content in the commit index - #2330

Open
MuskanPaliwal wants to merge 9 commits into
entireio:mainfrom
MuskanPaliwal:fix-hook-git-context
Open

MuskanPaliwal wants to merge 9 commits into
entireio:mainfrom
MuskanPaliwal:fix-hook-git-context

Conversation

@MuskanPaliwal

@MuskanPaliwal MuskanPaliwal commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

git commit -a and path-limited commits prepare the commit in a temporary GIT_INDEX_FILE. Redo detection (inheritReplacedCommitsTrailers) read staged paths from that index but blob IDs from go-git's ordinary index, so it compared the dropped commit with content that was not being committed. It also let git's rename detection hide staged deletions. The inherited Entire-Checkpoint trailer was wrong in both directions.

Problem

After git reset HEAD~1:

  • Recommitting the dropped commit's exact content with git commit -a, git commit -- <path> or git commit --amend -a loses its trailer.
  • Staging that content, rewriting a file, then committing with -a or -- <path> inherits the trailer for work the commit no longer contains.
  • Deleting a file the dropped commit changed, while adding a similar file, also inherits the trailer, even with a plain git add and git commit. Git reports the pair as a rename, so the deletion is never compared.

A plain git add followed by git commit was otherwise correct, because both indexes agree.

Cause

stagedBlobs (manual_commit_redo.go, added in 6170bc1) listed paths with getStagedFiles, which runs git diff --cached and so honors GIT_INDEX_FILE. It then looked up each blob in repo.Storer.Index(), which reads .git/index. During -a, path-limited and --amend -a commits, those are different indexes.

git diff --cached also detects renames, following the user's diff.renames (on by default). A rename's raw record carries only its destination path, so the source's staged deletion never reached the comparison.

The existing redo tests call PrepareCommitMsg directly, where no temporary index exists, so they could not see the difference.

Fix

getStagedChanges reads paths and full blob IDs in one git diff --cached --raw -z --no-abbrev query, through the existing parseStagedRaw (which already skips submodule gitlinks and reports rename destinations). getStagedFiles and stagedBlobs both build on it, and the second index read is removed. The query deliberately keeps the hook environment: GIT_INDEX_FILE identifies the commit Git is preparing.

stagedBlobs adds --no-renames, so a rename's source is compared as the staged deletion it is. getStagedFiles keeps reporting a rename's new path, so its squash and session-overlap callers do not change.

git-safety.md moves the hook-environment rules into their own section and adds the --no-renames rule. CLAUDE.md now also covers hook queries of the hook's own refs, which the redo path runs.

How this PR changed

This PR originally fixed the same mismatch in session linking (stagedFilesOverlapWithContent). Upstream 389e855 then made new-file linking match by name, and prepare-commit-msg stopped reading the index there, so that change no longer applies and was dropped while syncing with main. The same mismatch had meanwhile appeared in redo detection, which this PR now fixes.

I started this from the side finding in #2303 about getStagedFiles, which suggests stripping the hook environment. Doing that would read both paths and blob IDs from .git/index. They'd agree with each other and still miss what git commit -a or a path-limited commit records. This fix keeps GIT_INDEX_FILE, so the comparison reads the index Git is committing from.

Related: #2111. This does not fix its empty-tree corruption.

Tests

TestRedo_ComparesTheCommitIndex runs real git commit through Entire's hooks, with the arguments the installed hooks pass. The dropped commit edits a.txt and b.txt and deletes d.txt.

Verified RED against main:

--- FAIL: TestRedo_ComparesTheCommitIndex/all                []string(nil) does not contain "a1b2c3d4e5f6"
--- FAIL: TestRedo_ComparesTheCommitIndex/partial            []string(nil) does not contain "a1b2c3d4e5f6"
--- FAIL: TestRedo_ComparesTheCommitIndex/amend-all          []string(nil) does not contain "a1b2c3d4e5f6"
--- FAIL: TestRedo_ComparesTheCommitIndex/all-rewritten      []string{"a1b2c3d4e5f6"} should not contain "a1b2c3d4e5f6"
--- FAIL: TestRedo_ComparesTheCommitIndex/partial-rewritten  []string{"a1b2c3d4e5f6"} should not contain "a1b2c3d4e5f6"
--- FAIL: TestRedo_ComparesTheCommitIndex/renamed-deletion   []string{"a1b2c3d4e5f6"} should not contain "a1b2c3d4e5f6"

With the commit-index change alone, only renamed-deletion still fails; --no-renames fixes it. The staged case passes throughout, as a control. All seven pass with the fix. TestParseStagedRaw also asserts each path's blob ID and deletions.

Test plan

  • TestRedo_ComparesTheCommitIndex (7 cases), plus the existing redo and adversarial redo tests
  • mise run lint: 0 issues
  • mise run check: passes except TestExternalCommand_SigintReachesPlugin, which failed once under -race load (its plugin never reached the ready state) and passes 3/3 when run alone
  • Paid real-agent E2E tests not run

馃 Generated with Claude Code

Read staged filenames and blob IDs together from Git's selected commit
index. During partial commits and commit -a, the previous content check
read the ordinary worktree index while filenames came from Git's temporary
index, omitting valid checkpoint links or adding unwanted trailers.

Pass one read-only snapshot through fresh and carry-forward content checks,
preserving GIT_INDEX_FILE and the existing non-blocking hook behavior.
Document why commit-index queries retain the hook environment.

Add a 48-case real-Git regression matrix covering both checkpoint backends,
ordinary and linked worktrees, staged/partial/-a commits, and kept/replaced
agent content. The original implementation fails 32 cases; the fix passes
all 48. Add native Git and raw-format parser coverage.

Validation: mise run check passed, including race-enabled unit/integration
tests and 60 deterministic canaries. The focused 48-case regression also
passed on a subsequent rerun. No paid real-agent E2E tests ran.

Entire-Checkpoint: 01M213A52SASJDN4ZRT42KMY2J
@MuskanPaliwal
MuskanPaliwal marked this pull request as ready for review September 9, 2026 09:23
@MuskanPaliwal
MuskanPaliwal requested a review from a team as a code owner September 9, 2026 09:23
Copilot AI balanced review requested due to automatic review settings October 14, 2026 12:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@MuskanPaliwal MuskanPaliwal changed the title fix(strategy): use commit index for session linking fix(strategy): compare redo content in the commit index Oct 14, 2026
@MuskanPaliwal

Copy link
Copy Markdown
Contributor Author

The two red checks (test-core, and test, which only reports test-core's result) come from four checkpoint-preview tests that fail on main as well, not from this change:

  • TestPreviewNextCheckpoint
  • TestCheckpointListPending_PreviewText
  • TestCheckpointListPending_PreviewJSON
  • TestCheckpointListPending_PreviewEmptyArraysPresent

Their fixtures (from 6142ebe) use fixed session timestamps on 2026-10-07. session.StaleSessionThreshold is 7 days, so since about 2026-10-14 12:00 UTC those sessions load as stale and the previews come back empty. They fail the same way on an unmodified checkout of main (ffc17a1).

#2769 fixes the three TestCheckpointListPending_* tests by deriving the session times from time.Now(). TestPreviewNextCheckpoint (cmd/entire/cli/strategy/manual_commit_preview_test.go) has the same fixed date and isn't covered there yet. No redo, staged-file, or integration test failed here.

MuskanPaliwal and others added 3 commits October 14, 2026 21:01
Redo detection read staged changes with git's rename detection on, so a
staged deletion that git paired with a similar addition reported only the
destination. A dropped commit that changed the deleted file then looked
recommitted, and its trailer was inherited for work the commit no longer
contains. The result also depended on the user's diff.renames setting.
The redo query now passes --no-renames; getStagedFiles keeps reporting a
rename's new path.

The real-git test helper now passes the arguments the installed hooks
pass, so amends work through it, and writes its hook scripts under
ForkLock to avoid ETXTBSY. TestRedo_ComparesTheCommitIndex adds amend,
deletion and renamed-deletion cases.

git-safety.md gets its own section for git subprocesses in hooks, and
CLAUDE.md covers hook queries of the hook's own refs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Entire-Checkpoint: 01M4ZJJPGR2CSMQS435CEY4CB7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants