Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a post-commit cache optimization to reduce redundant Git object reads during the PostCommit hook execution. The implementation pre-resolves HEAD and parent trees once, then threads them through multiple per-session functions that previously performed duplicate reads.
Changes:
- Added caching fields to
postCommitActionHandlerto store pre-resolved Git trees (headTree, parentTree, shadowRef, shadowTree) - Refactored PostCommit to extract per-session processing into
postCommitProcessSessionfunction - Updated multiple functions to accept optional pre-resolved tree parameters (
condenseOpts,overlapOpts,attributionOpts)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/strategy/postcommit_bench_test.go | Added benchmark tests for PostCommit hook with single and multiple sessions |
| cmd/entire/cli/strategy/phase_postcommit_test.go | Updated test calls to pass nil parameters to filesChangedInCommit |
| cmd/entire/cli/strategy/manual_commit_hooks.go | Refactored PostCommit to pre-resolve trees and extract session processing logic |
| cmd/entire/cli/strategy/manual_commit_condensation.go | Added caching support for shadow ref/tree resolution in CondenseSession and attribution |
| cmd/entire/cli/strategy/content_overlap.go | Added optional tree parameters to overlap detection functions |
| .claude/worktrees/cached-spinning-nebula | Added Git subproject commit reference |
Comments suppressed due to low confidence (1)
cmd/entire/cli/strategy/content_overlap.go:110
- The comment "Check each file in filesTouched" appears to be orphaned after the refactoring. The code that builds the
touchedSetmap was removed, but this comment remains. The loop below doesn't need this comment as it's self-explanatory. Consider removing this line.
// Check each file in filesTouched
| } else if shadowRef == nil { | ||
| // Cache provided a shadow tree but shadowRef is nil — this shouldn't happen | ||
| // in practice, but handle gracefully: use HEAD as shadow. | ||
| logging.Debug(logCtx, "attribution: using HEAD as shadow (no shadow ref, ignoring cached tree)") | ||
| shadowTree = headTree |
There was a problem hiding this comment.
The logic at lines 372-377 handles an unexpected state where shadowTree is cached but shadowRef is nil. However, this condition cannot occur in the calling code (line 826-827 in postCommitProcessSession) because both fields are set together from the same resolution. This defensive code adds unnecessary complexity. Consider removing this branch or adding a comment explaining the specific edge case it protects against.
| } else if shadowRef == nil { | |
| // Cache provided a shadow tree but shadowRef is nil — this shouldn't happen | |
| // in practice, but handle gracefully: use HEAD as shadow. | |
| logging.Debug(logCtx, "attribution: using HEAD as shadow (no shadow ref, ignoring cached tree)") | |
| shadowTree = headTree |
PR SummaryMedium Risk Overview Refactors the post-commit loop into Written by Cursor Bugbot for commit 1835022. Configure here. |
Entire-Checkpoint: 0e42eb4462d9
…tion Tests verify that pre-resolved git objects (cache hit) produce identical results to fallback resolution (cache miss) for filesOverlapWithContent, filesWithRemainingAgentChanges, and filesChangedInCommit. Also covers partial cache (some trees nil) and initial commit (no parent) edge cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 3a8a8ee935de
shadowRef and shadowTree are always resolved together from the same repo.Reference call, so the cached-tree-but-nil-ref state cannot occur. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 88a7d2762f60
1835022 to
ee47d5b
Compare
Entire-Checkpoint: 0e42eb4462d9