fix(attribution): review feedback — correctness fixes, deduplication, and tests

- Fix tree/hash fallback mismatch: keep nonAgentBaseTree and
  nonAgentBaseCommit in sync when parent resolution fails, preventing
  inconsistent attribution from dual-path getAllChangedFiles
- Remove redundant parentCommitHash re-derivation in
  postCommitProcessSession — use the already-threaded parameter
- Extract resolveCommitContext helper to deduplicate ~25 lines of
  HEAD/parent tree resolution between PostCommit and CondenseSessionByID;
  also adds missing warnings in CondenseSessionByID when repo.Head() fails
- Add attrBase divergence warning in computeCombinedAttribution
- Only accumulate condensed sessions for combined attribution (snapshot
  PromptAttributions before condensation, commit to accumulator after)
- Add nil guard to UpdateCheckpointSummary to avoid unnecessary git commits
- Add 4 unit tests for UpdateCheckpointSummary (happy path, not found,
  field preservation, nil no-op)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: peyton-alt

cmd/entire/cli/checkpoint/committed.go

@@ -1106,6 +1106,10 @@ func (s *GitStore) UpdateSummary(ctx context.Context, checkpointID id.Checkpoint
 // for an existing checkpoint, setting CombinedAttribution.
 // Returns ErrCheckpointNotFound if the checkpoint does not exist.
 func (s *GitStore) UpdateCheckpointSummary(ctx context.Context, opts UpdateCheckpointSummaryOptions) error {
+	if opts.CombinedAttribution == nil {
+		return nil // No-op: nothing to patch
+	}
+
 	if err := ctx.Err(); err != nil {
 		return err //nolint:wrapcheck // Propagating context cancellation
 	}

cmd/entire/cli/checkpoint/committed_update_test.go

@@ -580,5 +580,122 @@ func TestGetGitAuthorFromRepo_NoConfig(t *testing.T) {
 	}
 }
 
+func TestUpdateCheckpointSummary_SetsCombinedAttribution(t *testing.T) {
+	t.Parallel()
+	_, store, cpID := setupRepoForUpdate(t)
+
+	combined := &InitialAttribution{
+		AgentLines:      15,
+		HumanAdded:      3,
+		HumanModified:   1,
+		HumanRemoved:    0,
+		TotalCommitted:  18,
+		AgentPercentage: 83.3,
+	}
+
+	err := store.UpdateCheckpointSummary(context.Background(), UpdateCheckpointSummaryOptions{
+		CheckpointID:        cpID,
+		CombinedAttribution: combined,
+	})
+	if err != nil {
+		t.Fatalf("UpdateCheckpointSummary() error = %v", err)
+	}
+
+	summary, err := store.ReadCommitted(context.Background(), cpID)
+	if err != nil {
+		t.Fatalf("ReadCommitted() error = %v", err)
+	}
+
+	if summary.CombinedAttribution == nil {
+		t.Fatal("CombinedAttribution is nil after update")
+	}
+	if summary.CombinedAttribution.AgentLines != 15 {
+		t.Errorf("AgentLines = %d, want 15", summary.CombinedAttribution.AgentLines)
+	}
+	if summary.CombinedAttribution.HumanAdded != 3 {
+		t.Errorf("HumanAdded = %d, want 3", summary.CombinedAttribution.HumanAdded)
+	}
+	if summary.CombinedAttribution.AgentPercentage != 83.3 {
+		t.Errorf("AgentPercentage = %.1f, want 83.3", summary.CombinedAttribution.AgentPercentage)
+	}
+}
+
+func TestUpdateCheckpointSummary_NonexistentCheckpoint(t *testing.T) {
+	t.Parallel()
+	_, store, _ := setupRepoForUpdate(t)
+
+	err := store.UpdateCheckpointSummary(context.Background(), UpdateCheckpointSummaryOptions{
+		CheckpointID: id.MustCheckpointID("deadbeef1234"),
+		CombinedAttribution: &InitialAttribution{
+			AgentLines: 10,
+		},
+	})
+	if err == nil {
+		t.Fatal("expected error for nonexistent checkpoint, got nil")
+	}
+}
+
+func TestUpdateCheckpointSummary_PreservesExistingFields(t *testing.T) {
+	t.Parallel()
+	_, store, cpID := setupRepoForUpdate(t)
+
+	summaryBefore, err := store.ReadCommitted(context.Background(), cpID)
+	if err != nil {
+		t.Fatalf("ReadCommitted() before error = %v", err)
+	}
+
+	err = store.UpdateCheckpointSummary(context.Background(), UpdateCheckpointSummaryOptions{
+		CheckpointID: cpID,
+		CombinedAttribution: &InitialAttribution{
+			AgentLines:      10,
+			TotalCommitted:  10,
+			AgentPercentage: 100.0,
+		},
+	})
+	if err != nil {
+		t.Fatalf("UpdateCheckpointSummary() error = %v", err)
+	}
+
+	summaryAfter, err := store.ReadCommitted(context.Background(), cpID)
+	if err != nil {
+		t.Fatalf("ReadCommitted() after error = %v", err)
+	}
+
+	if summaryAfter.CheckpointID != summaryBefore.CheckpointID {
+		t.Errorf("CheckpointID changed: %q -> %q", summaryBefore.CheckpointID, summaryAfter.CheckpointID)
+	}
+	if summaryAfter.Strategy != summaryBefore.Strategy {
+		t.Errorf("Strategy changed: %q -> %q", summaryBefore.Strategy, summaryAfter.Strategy)
+	}
+	if len(summaryAfter.Sessions) != len(summaryBefore.Sessions) {
+		t.Errorf("Sessions count changed: %d -> %d", len(summaryBefore.Sessions), len(summaryAfter.Sessions))
+	}
+	if summaryAfter.CombinedAttribution == nil {
+		t.Fatal("CombinedAttribution should be set after update")
+	}
+}
+
+func TestUpdateCheckpointSummary_NilCombinedAttributionIsNoop(t *testing.T) {
+	t.Parallel()
+	_, store, cpID := setupRepoForUpdate(t)
+
+	// Nil CombinedAttribution should be a no-op (no error, no unnecessary commit)
+	err := store.UpdateCheckpointSummary(context.Background(), UpdateCheckpointSummaryOptions{
+		CheckpointID:        cpID,
+		CombinedAttribution: nil,
+	})
+	if err != nil {
+		t.Fatalf("UpdateCheckpointSummary(nil) error = %v", err)
+	}
+
+	summary, err := store.ReadCommitted(context.Background(), cpID)
+	if err != nil {
+		t.Fatalf("ReadCommitted() error = %v", err)
+	}
+	if summary.CombinedAttribution != nil {
+		t.Errorf("CombinedAttribution should remain nil, got %+v", summary.CombinedAttribution)
+	}
+}
+
 // Verify go-git config import is used (compile-time check).
 var _ = config.GlobalScope

cmd/entire/cli/strategy/manual_commit_attribution.go

@@ -248,24 +248,25 @@ func CalculateAttributionWithAccumulated(
 	// Calculate total user edits to non-agent files (files not in filesTouched).
 	// Scope this to the attributed commit only by diffing parent→head. If parent
 	// context is unavailable, fall back to the session base behavior.
+	// IMPORTANT: nonAgentBaseTree and nonAgentBaseCommit must reference the same
+	// commit — getAllChangedFiles has a fast path (CLI diff-tree using hashes) and
+	// a slow path (go-git tree walk using trees). If they diverge, attribution
+	// produces inconsistent results depending on which path fires.
 	nonAgentBaseTree := baseTree
+	nonAgentBaseCommit := attributionBaseCommit
 	if parentTree != nil {
 		nonAgentBaseTree = parentTree
+		nonAgentBaseCommit = parentCommitHash
 	} else if parentCommitHash != "" {
 		// parentCommitHash is set but parentTree is nil — parent object resolution
-		// failed (e.g. shallow clone, pack corruption). Fall back to session base,
-		// but warn because non-agent file counts may be inflated.
+		// failed (e.g. shallow clone, pack corruption). Fall back to session base
+		// for both tree and hash to keep them in sync.
 		logging.Warn(logging.WithComponent(ctx, "attribution"),
 			"attribution: parent tree unavailable despite parent hash being set; non-agent file counts may be inflated",
 			slog.String("parent_commit_hash", parentCommitHash),
 		)
 	}
 
-	nonAgentBaseCommit := attributionBaseCommit
-	if parentCommitHash != "" {
-		nonAgentBaseCommit = parentCommitHash
-	}
-
 	allChangedFiles, err := getAllChangedFiles(ctx, nonAgentBaseTree, headTree, repoDir, nonAgentBaseCommit, headCommitHash)
 	if err != nil {
 		logging.Warn(logging.WithComponent(ctx, "attribution"),

cmd/entire/cli/strategy/manual_commit_condensation.go

@@ -88,15 +88,11 @@ func (s *ManualCommitStrategy) getCheckpointLog(ctx context.Context, checkpointI
 // sessionAttrData captures per-session data needed to compute combined attribution
 // after the condensation loop has cleared state.PromptAttributions.
 type sessionAttrData struct {
+	condenseOpts // Embedded: shadowRef, headTree, parentTree, repoDir, headCommitHash, parentCommitHash
+
 	promptAttributions []PromptAttribution
 	filesTouched       []string
-	shadowRef          *plumbing.Reference
-	headTree           *object.Tree
-	parentTree         *object.Tree
-	repoDir            string
 	attrBase           string
-	headCommit         string
-	parentCommit       string
 }
 
 // computeCombinedAttribution merges PromptAttributions from all sessions sharing
@@ -125,10 +121,22 @@ func computeCombinedAttribution(
 	}
 
 	first := sessions[0]
+	attrBase := first.attrBase
+	for i := 1; i < len(sessions); i++ {
+		if sessions[i].attrBase != attrBase {
+			logging.Warn(logging.WithComponent(ctx, "attribution"),
+				"combined attribution: sessions have divergent attribution base commits; using first session's base",
+				slog.String("first_base", attrBase),
+				slog.String("divergent_base", sessions[i].attrBase),
+				slog.Int("session_index", i),
+			)
+			break
+		}
+	}
 	syntheticState := &SessionState{
 		PromptAttributions:    merged,
-		AttributionBaseCommit: first.attrBase,
-		BaseCommit:            first.attrBase,
+		AttributionBaseCommit: attrBase,
+		BaseCommit:            attrBase,
 	}
 	syntheticData := &ExtractedSessionData{
 		FilesTouched: allFilesTouched,
@@ -139,19 +147,53 @@ func computeCombinedAttribution(
 		parentTree:            first.parentTree,
 		repoDir:               first.repoDir,
 		attributionBaseCommit: first.attrBase,
-		headCommitHash:        first.headCommit,
-		parentCommitHash:      first.parentCommit,
+		headCommitHash:        first.headCommitHash,
+		parentCommitHash:      first.parentCommitHash,
 	})
 }
 
+// resolveCommitContext extracts HEAD tree, parent tree, and parent commit hash
+// from a commit object. Best-effort: warns on failure and leaves fields nil/empty.
+// Both parentTree and parentCommitHash are set together or not at all, so callers
+// can rely on them being in sync.
+func resolveCommitContext(ctx context.Context, commit *object.Commit) (headTree, parentTree *object.Tree, parentCommitHash string) {
+	logCtx := logging.WithComponent(ctx, "checkpoint")
+
+	if t, err := commit.Tree(); err != nil {
+		logging.Warn(logCtx, "failed to resolve HEAD tree; attribution will be skipped",
+			slog.String("commit", commit.Hash.String()),
+			slog.String("error", err.Error()))
+	} else {
+		headTree = t
+	}
+
+	if commit.NumParents() > 0 {
+		rawHash := commit.ParentHashes[0]
+		if parent, err := commit.Parent(0); err != nil {
+			logging.Warn(logCtx, "failed to load parent commit; parent-scoped attribution unavailable",
+				slog.String("parent_hash", rawHash.String()),
+				slog.String("error", err.Error()))
+		} else if t, err := parent.Tree(); err != nil {
+			logging.Warn(logCtx, "failed to load parent tree; parent-scoped attribution unavailable",
+				slog.String("parent_hash", rawHash.String()),
+				slog.String("error", err.Error()))
+		} else {
+			parentTree = t
+			parentCommitHash = rawHash.String()
+		}
+	}
+
+	return headTree, parentTree, parentCommitHash
+}
+
 // condenseOpts provides pre-resolved git objects to avoid redundant reads.
 type condenseOpts struct {
 	shadowRef        *plumbing.Reference // Pre-resolved shadow branch ref (nil = resolve from repo)
 	headTree         *object.Tree        // Pre-resolved HEAD tree (passed through to calculateSessionAttributions)
 	repoDir          string              // Repository worktree path for git CLI commands
 	headCommitHash   string              // HEAD commit hash (passed through for attribution)
-	parentTree       *object.Tree        // HEAD's first parent tree (nil iff parentCommitHash is empty)
-	parentCommitHash string              // HEAD's first parent hash (empty iff parentTree is nil — initial commit or resolution failure)
+	parentTree       *object.Tree        // HEAD's first parent tree (nil when parentCommitHash is empty or parent resolution failed)
+	parentCommitHash string              // HEAD's first parent hash (empty when parentTree is nil — initial commit or resolution failure)
 }
 
 // CondenseSession condenses a session's shadow branch to permanent storage.
@@ -912,31 +954,17 @@ func (s *ManualCommitStrategy) CondenseSessionByID(ctx context.Context, sessionI
 	if repoDir, rdErr := paths.WorktreeRoot(ctx); rdErr == nil {
 		headCommitOpts.repoDir = repoDir
 	}
-	if headRef, hrErr := repo.Head(); hrErr == nil {
+	if headRef, hrErr := repo.Head(); hrErr != nil {
+		logging.Warn(logCtx, "condense-by-id: failed to resolve HEAD; attribution context unavailable",
+			slog.String("error", hrErr.Error()))
+	} else {
 		headCommitOpts.headCommitHash = headRef.Hash().String()
-		if headCommit, hcErr := repo.CommitObject(headRef.Hash()); hcErr == nil {
-			if t, tErr := headCommit.Tree(); tErr != nil {
-				logging.Warn(logCtx, "condense-by-id: failed to resolve HEAD tree; attribution will be skipped",
-					slog.String("commit", headRef.Hash().String()),
-					slog.String("error", tErr.Error()))
-			} else {
-				headCommitOpts.headTree = t
-			}
-			if headCommit.NumParents() > 0 {
-				rawHash := headCommit.ParentHashes[0]
-				if parent, pErr := headCommit.Parent(0); pErr != nil {
-					logging.Warn(logCtx, "condense-by-id: failed to load parent commit; parent-scoped attribution unavailable",
-						slog.String("parent_hash", rawHash.String()),
-						slog.String("error", pErr.Error()))
-				} else if t, tErr := parent.Tree(); tErr != nil {
-					logging.Warn(logCtx, "condense-by-id: failed to load parent tree; parent-scoped attribution unavailable",
-						slog.String("parent_hash", rawHash.String()),
-						slog.String("error", tErr.Error()))
-				} else {
-					headCommitOpts.parentTree = t
-					headCommitOpts.parentCommitHash = rawHash.String()
-				}
-			}
+		if headCommit, hcErr := repo.CommitObject(headRef.Hash()); hcErr != nil {
+			logging.Warn(logCtx, "condense-by-id: failed to load HEAD commit object",
+				slog.String("commit", headRef.Hash().String()),
+				slog.String("error", hcErr.Error()))
+		} else {
+			headCommitOpts.headTree, headCommitOpts.parentTree, headCommitOpts.parentCommitHash = resolveCommitContext(ctx, headCommit)
 		}
 	}
 

cmd/entire/cli/strategy/manual_commit_hooks.go

@@ -846,31 +846,7 @@ func (s *ManualCommitStrategy) PostCommit(ctx context.Context) error { //nolint:
 	// per-session functions (filesOverlapWithContent, filesWithRemainingAgentChanges,
 	// calculateSessionAttributions).
 	_, resolveTreesSpan := perf.Start(ctx, "resolve_commit_trees")
-	var headTree *object.Tree
-	if t, err := commit.Tree(); err != nil {
-		logging.Warn(logCtx, "post-commit: failed to resolve HEAD tree; attribution will be skipped",
-			slog.String("commit", commit.Hash.String()),
-			slog.String("error", err.Error()))
-	} else {
-		headTree = t
-	}
-	var parentTree *object.Tree
-	var parentCommitHash string
-	if commit.NumParents() > 0 {
-		rawHash := commit.ParentHashes[0]
-		if parent, err := commit.Parent(0); err != nil {
-			logging.Warn(logCtx, "post-commit: failed to load parent commit; parent-scoped attribution unavailable",
-				slog.String("parent_hash", rawHash.String()),
-				slog.String("error", err.Error()))
-		} else if t, err := parent.Tree(); err != nil {
-			logging.Warn(logCtx, "post-commit: failed to load parent tree; parent-scoped attribution unavailable",
-				slog.String("parent_hash", rawHash.String()),
-				slog.String("error", err.Error()))
-		} else {
-			parentTree = t
-			parentCommitHash = rawHash.String()
-		}
-	}
+	headTree, parentTree, parentCommitHash := resolveCommitContext(ctx, commit)
 
 	committedFileSet := filesChangedInCommit(ctx, worktreePath, commit, headTree, parentTree)
 	resolveTreesSpan.End()
@@ -1043,28 +1019,28 @@ func (s *ManualCommitStrategy) postCommitProcessSession(
 		slog.Any("files", filesTouchedBefore),
 	)
 
-	// Collect PromptAttributions BEFORE condensation clears them.
+	// Snapshot PromptAttributions BEFORE condensation clears them.
 	// condenseAndUpdateState sets state.PromptAttributions = nil, so we must
-	// capture them here for the combined attribution pass after the session loop.
+	// capture them here. The snapshot is only committed to attrAccumulator after
+	// we confirm condensation occurred (below), to avoid computing combined
+	// attribution for sessions that weren't actually condensed.
 	attrBase := state.AttributionBaseCommit
 	if attrBase == "" {
 		attrBase = state.BaseCommit
 	}
-	var parentCommit string
-	if commit.NumParents() > 0 {
-		parentCommit = commit.ParentHashes[0].String()
-	}
-	attrAccumulator[shadowBranchName] = append(attrAccumulator[shadowBranchName], sessionAttrData{
+	snapshotAttr := sessionAttrData{
+		condenseOpts: condenseOpts{
+			shadowRef:        shadowRef,
+			headTree:         headTree,
+			parentTree:       parentTree,
+			repoDir:          repoDir,
+			headCommitHash:   newHead,
+			parentCommitHash: parentCommitHash,
+		},
 		promptAttributions: append([]PromptAttribution(nil), state.PromptAttributions...),
 		filesTouched:       append([]string(nil), filesTouchedBefore...),
-		shadowRef:          shadowRef,
-		headTree:           headTree,
-		parentTree:         parentTree,
-		repoDir:            repoDir,
 		attrBase:           attrBase,
-		headCommit:         newHead,
-		parentCommit:       parentCommit,
-	})
+	}
 
 	// Run the state machine transition with handler for strategy-specific actions.
 	_, transitionAndCondenseSpan := perf.Start(ctx, "transition_and_condense")
@@ -1095,6 +1071,13 @@ func (s *ManualCommitStrategy) postCommitProcessSession(
 	}
 	transitionAndCondenseSpan.End()
 
+	// Only include this session in the combined attribution accumulator if it
+	// was actually condensed. Non-condensed sessions haven't written their data
+	// to the checkpoint yet, so including them would produce premature results.
+	if handler.condensed {
+		attrAccumulator[shadowBranchName] = append(attrAccumulator[shadowBranchName], snapshotAttr)
+	}
+
 	// Record checkpoint ID for ACTIVE sessions so HandleTurnEnd can finalize
 	// with full transcript. IDLE/ENDED sessions already have complete transcripts.
 	// NOTE: This check runs AFTER TransitionAndLog updated the phase. It relies on