From 3c3d1e7498bb04ca0caae88f9f8c8ed2a3278685 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 20:50:41 +0200 Subject: [PATCH 1/3] Fix resume when local metadata branch is stale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit promoteRemoteTrackingMetadataBranch returned early whenever the local entire/checkpoints/v1 ref existed, even when origin/entire/checkpoints/v1 was ahead. The committed-checkpoint store consulted by RestoreLogsOnly and resumeSingleSession only falls back to origin/... when the local ref is missing entirely, so `entire resume` printed "session log not available" for checkpoints already present in refs/remotes/origin/entire/checkpoints/v1. Drop the early-return so SafelyAdvanceLocalRef runs unconditionally — it no-ops when local is at or ahead of the target (preserving any unpushed local commits) and fast-forwards when behind. Same pattern FetchMetadataBranch already uses after a real fetch. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 6372b938a1f7 --- cmd/entire/cli/resume.go | 12 ++- cmd/entire/cli/resume_test.go | 159 ++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 4 deletions(-) diff --git a/cmd/entire/cli/resume.go b/cmd/entire/cli/resume.go index f2c9000dfa..c028552e28 100644 --- a/cmd/entire/cli/resume.go +++ b/cmd/entire/cli/resume.go @@ -845,12 +845,16 @@ func checkRemoteMetadata(ctx context.Context, w, errW io.Writer, checkpointID id return nil } +// promoteRemoteTrackingMetadataBranch aligns the local entire/checkpoints/v1 +// branch with origin's remote-tracking ref. It creates the local ref when it +// does not exist and fast-forwards a stale local ref when origin has new +// commits — without this, callers reading checkpoint metadata via the local +// ref (e.g. the committed-checkpoint store in RestoreLogsOnly) miss +// checkpoints that have already been fetched into refs/remotes/origin/... +// SafelyAdvanceLocalRef no-ops when the local ref is at or ahead of the +// target, so unpushed local checkpoints are preserved. func promoteRemoteTrackingMetadataBranch(ctx context.Context, repo *git.Repository) { localRefName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) - if _, err := repo.Reference(localRefName, true); err == nil { - return - } - remoteRef, err := repo.Reference(plumbing.NewRemoteReferenceName("origin", paths.MetadataBranchName), true) if err != nil { return diff --git a/cmd/entire/cli/resume_test.go b/cmd/entire/cli/resume_test.go index 453fb09296..b2aab1fb47 100644 --- a/cmd/entire/cli/resume_test.go +++ b/cmd/entire/cli/resume_test.go @@ -921,6 +921,165 @@ func TestCheckRemoteMetadata_CheckpointNotOnRemote(t *testing.T) { } } +// TestPromoteRemoteTrackingMetadataBranch_FastForwardsStaleLocal verifies that +// promoteRemoteTrackingMetadataBranch advances a stale local entire/checkpoints/v1 +// to match origin/entire/checkpoints/v1 when the remote-tracking branch is ahead. +// Before the fix, this function returned early whenever the local ref existed, +// even if it was behind remote — so callers reading checkpoint metadata from the +// local branch could miss checkpoints already fetched into the remote-tracking ref. +func TestPromoteRemoteTrackingMetadataBranch_FastForwardsStaleLocal(t *testing.T) { + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + repo, _, _ := setupResumeTestRepo(t, tmpDir, false) + + // Initial position of the local metadata branch (from EnsureMetadataBranch). + localRefName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) + initialRef, err := repo.Reference(localRefName, true) + if err != nil { + t.Fatalf("Failed to read initial metadata branch ref: %v", err) + } + initialHash := initialRef.Hash() + + // Add a checkpoint commit on top — this advances the local metadata branch. + _ = createCheckpointOnMetadataBranch(t, repo, "2025-01-01-test-session-uuid") + descendantRef, err := repo.Reference(localRefName, true) + if err != nil { + t.Fatalf("Failed to read advanced metadata branch ref: %v", err) + } + descendantHash := descendantRef.Hash() + if descendantHash == initialHash { + t.Fatalf("expected the metadata branch to advance after createCheckpointOnMetadataBranch") + } + + // Mirror the descendant into the remote-tracking ref ("origin has the new commit"). + remoteRefName := plumbing.NewRemoteReferenceName("origin", paths.MetadataBranchName) + if err := repo.Storer.SetReference(plumbing.NewHashReference(remoteRefName, descendantHash)); err != nil { + t.Fatalf("Failed to set remote-tracking ref: %v", err) + } + + // Rewind the LOCAL branch back to before the checkpoint, simulating a stale + // local ref while the remote-tracking ref is up to date. + if err := repo.Storer.SetReference(plumbing.NewHashReference(localRefName, initialHash)); err != nil { + t.Fatalf("Failed to rewind local metadata branch: %v", err) + } + + // Sanity check: local is stale. + pre, err := repo.Reference(localRefName, true) + if err != nil { + t.Fatalf("Failed to re-read local ref: %v", err) + } + if pre.Hash() != initialHash { + t.Fatalf("precondition: local should be at initial hash, got %s want %s", pre.Hash(), initialHash) + } + + // Promote. + promoteRemoteTrackingMetadataBranch(context.Background(), repo) + + // Post-condition: local should match the remote-tracking ref. + post, err := repo.Reference(localRefName, true) + if err != nil { + t.Fatalf("Failed to read local ref after promote: %v", err) + } + if post.Hash() != descendantHash { + t.Errorf("local should be fast-forwarded to remote-tracking ref: got %s, want %s", + post.Hash(), descendantHash) + } +} + +// TestResumeFromCurrentBranch_FastForwardsStaleLocalMetadata exercises the +// end-to-end resume flow when the local entire/checkpoints/v1 branch is behind +// origin/entire/checkpoints/v1 (a fresh checkpoint has been pushed but the user +// hasn't fast-forwarded their local ref). Before the fix the resume command +// printed "session log not available" because the committed-checkpoint reader +// in RestoreLogsOnly/resumeSingleSession only consults the local ref (it only +// falls back to origin/... when the local ref is missing entirely). After the +// fix, promoteRemoteTrackingMetadataBranch fast-forwards local from remote +// before the metadata reads happen, so the message is no longer printed. +func TestResumeFromCurrentBranch_FastForwardsStaleLocalMetadata(t *testing.T) { + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + // Redirect Claude session writes into the temp dir. + claudeDir := filepath.Join(tmpDir, "claude-projects") + t.Setenv("ENTIRE_TEST_CLAUDE_PROJECT_DIR", claudeDir) + + repo, w, _ := setupResumeTestRepo(t, tmpDir, false) + + // Capture the local metadata branch's initial position (no checkpoints yet). + localRefName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) + initialRef, err := repo.Reference(localRefName, true) + if err != nil { + t.Fatalf("Failed to read initial metadata branch ref: %v", err) + } + initialHash := initialRef.Hash() + + // Write a real checkpoint with agent metadata so RestoreLogsOnly can resolve + // the agent and write the restored session file. This advances local. + ctx := context.Background() + sessionID := "2025-01-01-test-session-uuid" + cpID := id.MustCheckpointID("abc123def456") + rawTranscript := []byte(`{"type":"user","message":{"content":[{"type":"text","text":"hi"}]}}` + "\n") + + v1Store := checkpoint.NewGitStore(repo) + if err := v1Store.WriteCommitted(ctx, checkpoint.WriteCommittedOptions{ + CheckpointID: cpID, + SessionID: sessionID, + Strategy: "manual-commit", + Transcript: redact.AlreadyRedacted(rawTranscript), + Agent: types.AgentType("Claude Code"), + AuthorName: "Test", + AuthorEmail: "test@example.com", + }); err != nil { + t.Fatalf("WriteCommitted: %v", err) + } + + advancedRef, err := repo.Reference(localRefName, true) + if err != nil { + t.Fatalf("Failed to read advanced metadata branch ref: %v", err) + } + descendantHash := advancedRef.Hash() + + // Mirror the new commit into the remote-tracking ref… + remoteRefName := plumbing.NewRemoteReferenceName("origin", paths.MetadataBranchName) + if err := repo.Storer.SetReference(plumbing.NewHashReference(remoteRefName, descendantHash)); err != nil { + t.Fatalf("Failed to set remote-tracking ref: %v", err) + } + // …then rewind the LOCAL ref to before the checkpoint, leaving the local + // branch stale while the remote-tracking ref is ahead. + if err := repo.Storer.SetReference(plumbing.NewHashReference(localRefName, initialHash)); err != nil { + t.Fatalf("Failed to rewind local metadata branch: %v", err) + } + + // Create a feature-style commit on master carrying the checkpoint trailer. + featureFile := filepath.Join(tmpDir, "feature.txt") + if err := os.WriteFile(featureFile, []byte("feature content"), 0o644); err != nil { + t.Fatalf("write feature file: %v", err) + } + if _, err := w.Add("feature.txt"); err != nil { + t.Fatalf("add feature file: %v", err) + } + commitMsg := "Add feature\n\nEntire-Checkpoint: " + cpID.String() + if _, err := w.Commit(commitMsg, &git.CommitOptions{ + Author: &object.Signature{Name: "Test", Email: "test@example.com"}, + }); err != nil { + t.Fatalf("Commit: %v", err) + } + + // Run resume from the current branch with force=true (skip interactive prompt). + var stdout, stderr bytes.Buffer + if err := resumeFromCurrentBranch(ctx, &stdout, &stderr, "master", true); err != nil { + t.Fatalf("resumeFromCurrentBranch error: %v\nstdout: %s\nstderr: %s", + err, stdout.String(), stderr.String()) + } + + combined := stdout.String() + stderr.String() + if strings.Contains(combined, "session log not available") { + t.Errorf("resumeFromCurrentBranch reported missing log even though origin has the checkpoint metadata (local metadata branch was not fast-forwarded from remote-tracking ref):\n%s", + combined) + } +} + func TestResumeFromCurrentBranch_NoMetadataAvailable(t *testing.T) { tmpDir := t.TempDir() t.Chdir(tmpDir) From 31e6f2b8a55b2d5f3a31c038ac9fa42a956bbb25 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 22:38:19 +0200 Subject: [PATCH 2/3] Simplify resume tests after review - Extract makeLocalMetadataBranchStale + readMetadataBranchHash helpers to dedupe the "advance local, mirror to remote, rewind local" setup shared by both new tests. - Replace types.AgentType("Claude Code") magic string with the existing agent.AgentTypeClaudeCode constant. - Trim the 8-line docstring on promoteRemoteTrackingMetadataBranch and drop inline test comments that narrated WHAT obvious code does; keep the non-obvious WHY (the bug class). Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 857472164f84 --- cmd/entire/cli/resume.go | 13 ++- cmd/entire/cli/resume_test.go | 149 ++++++++++++---------------------- 2 files changed, 57 insertions(+), 105 deletions(-) diff --git a/cmd/entire/cli/resume.go b/cmd/entire/cli/resume.go index c028552e28..1b84657517 100644 --- a/cmd/entire/cli/resume.go +++ b/cmd/entire/cli/resume.go @@ -845,14 +845,11 @@ func checkRemoteMetadata(ctx context.Context, w, errW io.Writer, checkpointID id return nil } -// promoteRemoteTrackingMetadataBranch aligns the local entire/checkpoints/v1 -// branch with origin's remote-tracking ref. It creates the local ref when it -// does not exist and fast-forwards a stale local ref when origin has new -// commits — without this, callers reading checkpoint metadata via the local -// ref (e.g. the committed-checkpoint store in RestoreLogsOnly) miss -// checkpoints that have already been fetched into refs/remotes/origin/... -// SafelyAdvanceLocalRef no-ops when the local ref is at or ahead of the -// target, so unpushed local checkpoints are preserved. +// promoteRemoteTrackingMetadataBranch advances the local entire/checkpoints/v1 +// ref to match origin's remote-tracking ref. Without this, callers reading +// checkpoint metadata via the local ref miss checkpoints already fetched into +// refs/remotes/origin/...: the committed-checkpoint store only falls back to +// origin/... when the local ref is *missing*, not when it's behind. func promoteRemoteTrackingMetadataBranch(ctx context.Context, repo *git.Repository) { localRefName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) remoteRef, err := repo.Reference(plumbing.NewRemoteReferenceName("origin", paths.MetadataBranchName), true) diff --git a/cmd/entire/cli/resume_test.go b/cmd/entire/cli/resume_test.go index b2aab1fb47..dd8f9d09f1 100644 --- a/cmd/entire/cli/resume_test.go +++ b/cmd/entire/cli/resume_test.go @@ -921,137 +921,95 @@ func TestCheckRemoteMetadata_CheckpointNotOnRemote(t *testing.T) { } } -// TestPromoteRemoteTrackingMetadataBranch_FastForwardsStaleLocal verifies that -// promoteRemoteTrackingMetadataBranch advances a stale local entire/checkpoints/v1 -// to match origin/entire/checkpoints/v1 when the remote-tracking branch is ahead. -// Before the fix, this function returned early whenever the local ref existed, -// even if it was behind remote — so callers reading checkpoint metadata from the -// local branch could miss checkpoints already fetched into the remote-tracking ref. -func TestPromoteRemoteTrackingMetadataBranch_FastForwardsStaleLocal(t *testing.T) { - tmpDir := t.TempDir() - t.Chdir(tmpDir) - - repo, _, _ := setupResumeTestRepo(t, tmpDir, false) - - // Initial position of the local metadata branch (from EnsureMetadataBranch). +// makeLocalMetadataBranchStale advances origin/entire/checkpoints/v1 to the +// current local hash and rewinds the local ref back to baseHash, leaving the +// local metadata branch behind its remote-tracking counterpart. Returns the +// hash the remote-tracking ref now points at. +func makeLocalMetadataBranchStale(t *testing.T, repo *git.Repository, baseHash plumbing.Hash) plumbing.Hash { + t.Helper() localRefName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) - initialRef, err := repo.Reference(localRefName, true) - if err != nil { - t.Fatalf("Failed to read initial metadata branch ref: %v", err) - } - initialHash := initialRef.Hash() - - // Add a checkpoint commit on top — this advances the local metadata branch. - _ = createCheckpointOnMetadataBranch(t, repo, "2025-01-01-test-session-uuid") - descendantRef, err := repo.Reference(localRefName, true) + current, err := repo.Reference(localRefName, true) if err != nil { - t.Fatalf("Failed to read advanced metadata branch ref: %v", err) + t.Fatalf("read advanced metadata branch ref: %v", err) } - descendantHash := descendantRef.Hash() - if descendantHash == initialHash { - t.Fatalf("expected the metadata branch to advance after createCheckpointOnMetadataBranch") + if current.Hash() == baseHash { + t.Fatalf("makeLocalMetadataBranchStale: local ref must have advanced past baseHash before calling") } - - // Mirror the descendant into the remote-tracking ref ("origin has the new commit"). remoteRefName := plumbing.NewRemoteReferenceName("origin", paths.MetadataBranchName) - if err := repo.Storer.SetReference(plumbing.NewHashReference(remoteRefName, descendantHash)); err != nil { - t.Fatalf("Failed to set remote-tracking ref: %v", err) + if err := repo.Storer.SetReference(plumbing.NewHashReference(remoteRefName, current.Hash())); err != nil { + t.Fatalf("set remote-tracking ref: %v", err) } - - // Rewind the LOCAL branch back to before the checkpoint, simulating a stale - // local ref while the remote-tracking ref is up to date. - if err := repo.Storer.SetReference(plumbing.NewHashReference(localRefName, initialHash)); err != nil { - t.Fatalf("Failed to rewind local metadata branch: %v", err) + if err := repo.Storer.SetReference(plumbing.NewHashReference(localRefName, baseHash)); err != nil { + t.Fatalf("rewind local metadata branch: %v", err) } + return current.Hash() +} - // Sanity check: local is stale. - pre, err := repo.Reference(localRefName, true) +// readMetadataBranchHash returns the current hash of refs/heads/entire/checkpoints/v1. +func readMetadataBranchHash(t *testing.T, repo *git.Repository) plumbing.Hash { + t.Helper() + ref, err := repo.Reference(plumbing.NewBranchReferenceName(paths.MetadataBranchName), true) if err != nil { - t.Fatalf("Failed to re-read local ref: %v", err) - } - if pre.Hash() != initialHash { - t.Fatalf("precondition: local should be at initial hash, got %s want %s", pre.Hash(), initialHash) + t.Fatalf("read metadata branch ref: %v", err) } + return ref.Hash() +} + +// Before the fix, promoteRemoteTrackingMetadataBranch returned early whenever +// the local ref existed, even when it was behind the remote-tracking ref — +// so downstream metadata readers using the local ref missed checkpoints +// already fetched into refs/remotes/origin/... +func TestPromoteRemoteTrackingMetadataBranch_FastForwardsStaleLocal(t *testing.T) { + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + repo, _, _ := setupResumeTestRepo(t, tmpDir, false) + + initialHash := readMetadataBranchHash(t, repo) + _ = createCheckpointOnMetadataBranch(t, repo, "2025-01-01-test-session-uuid") + descendantHash := makeLocalMetadataBranchStale(t, repo, initialHash) - // Promote. promoteRemoteTrackingMetadataBranch(context.Background(), repo) - // Post-condition: local should match the remote-tracking ref. - post, err := repo.Reference(localRefName, true) - if err != nil { - t.Fatalf("Failed to read local ref after promote: %v", err) - } - if post.Hash() != descendantHash { - t.Errorf("local should be fast-forwarded to remote-tracking ref: got %s, want %s", - post.Hash(), descendantHash) + if got := readMetadataBranchHash(t, repo); got != descendantHash { + t.Errorf("local should be fast-forwarded to remote-tracking ref: got %s, want %s", got, descendantHash) } } -// TestResumeFromCurrentBranch_FastForwardsStaleLocalMetadata exercises the -// end-to-end resume flow when the local entire/checkpoints/v1 branch is behind -// origin/entire/checkpoints/v1 (a fresh checkpoint has been pushed but the user -// hasn't fast-forwarded their local ref). Before the fix the resume command -// printed "session log not available" because the committed-checkpoint reader -// in RestoreLogsOnly/resumeSingleSession only consults the local ref (it only -// falls back to origin/... when the local ref is missing entirely). After the -// fix, promoteRemoteTrackingMetadataBranch fast-forwards local from remote -// before the metadata reads happen, so the message is no longer printed. +// End-to-end coverage for the same bug: when a fresh checkpoint has been +// pushed to origin but the user's local entire/checkpoints/v1 ref is behind, +// `entire resume` previously printed "session log not available" because the +// committed-checkpoint reader only falls back to origin/... when the local +// ref is missing entirely. func TestResumeFromCurrentBranch_FastForwardsStaleLocalMetadata(t *testing.T) { tmpDir := t.TempDir() t.Chdir(tmpDir) - // Redirect Claude session writes into the temp dir. - claudeDir := filepath.Join(tmpDir, "claude-projects") - t.Setenv("ENTIRE_TEST_CLAUDE_PROJECT_DIR", claudeDir) + t.Setenv("ENTIRE_TEST_CLAUDE_PROJECT_DIR", filepath.Join(tmpDir, "claude-projects")) repo, w, _ := setupResumeTestRepo(t, tmpDir, false) + initialHash := readMetadataBranchHash(t, repo) - // Capture the local metadata branch's initial position (no checkpoints yet). - localRefName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) - initialRef, err := repo.Reference(localRefName, true) - if err != nil { - t.Fatalf("Failed to read initial metadata branch ref: %v", err) - } - initialHash := initialRef.Hash() - - // Write a real checkpoint with agent metadata so RestoreLogsOnly can resolve - // the agent and write the restored session file. This advances local. ctx := context.Background() - sessionID := "2025-01-01-test-session-uuid" cpID := id.MustCheckpointID("abc123def456") rawTranscript := []byte(`{"type":"user","message":{"content":[{"type":"text","text":"hi"}]}}` + "\n") + // Agent must be set so RestoreLogsOnly can resolve a session-write target. v1Store := checkpoint.NewGitStore(repo) if err := v1Store.WriteCommitted(ctx, checkpoint.WriteCommittedOptions{ CheckpointID: cpID, - SessionID: sessionID, + SessionID: "2025-01-01-test-session-uuid", Strategy: "manual-commit", Transcript: redact.AlreadyRedacted(rawTranscript), - Agent: types.AgentType("Claude Code"), + Agent: agent.AgentTypeClaudeCode, AuthorName: "Test", AuthorEmail: "test@example.com", }); err != nil { t.Fatalf("WriteCommitted: %v", err) } - advancedRef, err := repo.Reference(localRefName, true) - if err != nil { - t.Fatalf("Failed to read advanced metadata branch ref: %v", err) - } - descendantHash := advancedRef.Hash() - - // Mirror the new commit into the remote-tracking ref… - remoteRefName := plumbing.NewRemoteReferenceName("origin", paths.MetadataBranchName) - if err := repo.Storer.SetReference(plumbing.NewHashReference(remoteRefName, descendantHash)); err != nil { - t.Fatalf("Failed to set remote-tracking ref: %v", err) - } - // …then rewind the LOCAL ref to before the checkpoint, leaving the local - // branch stale while the remote-tracking ref is ahead. - if err := repo.Storer.SetReference(plumbing.NewHashReference(localRefName, initialHash)); err != nil { - t.Fatalf("Failed to rewind local metadata branch: %v", err) - } + _ = makeLocalMetadataBranchStale(t, repo, initialHash) - // Create a feature-style commit on master carrying the checkpoint trailer. featureFile := filepath.Join(tmpDir, "feature.txt") if err := os.WriteFile(featureFile, []byte("feature content"), 0o644); err != nil { t.Fatalf("write feature file: %v", err) @@ -1059,14 +1017,12 @@ func TestResumeFromCurrentBranch_FastForwardsStaleLocalMetadata(t *testing.T) { if _, err := w.Add("feature.txt"); err != nil { t.Fatalf("add feature file: %v", err) } - commitMsg := "Add feature\n\nEntire-Checkpoint: " + cpID.String() - if _, err := w.Commit(commitMsg, &git.CommitOptions{ + if _, err := w.Commit("Add feature\n\nEntire-Checkpoint: "+cpID.String(), &git.CommitOptions{ Author: &object.Signature{Name: "Test", Email: "test@example.com"}, }); err != nil { t.Fatalf("Commit: %v", err) } - // Run resume from the current branch with force=true (skip interactive prompt). var stdout, stderr bytes.Buffer if err := resumeFromCurrentBranch(ctx, &stdout, &stderr, "master", true); err != nil { t.Fatalf("resumeFromCurrentBranch error: %v\nstdout: %s\nstderr: %s", @@ -1075,8 +1031,7 @@ func TestResumeFromCurrentBranch_FastForwardsStaleLocalMetadata(t *testing.T) { combined := stdout.String() + stderr.String() if strings.Contains(combined, "session log not available") { - t.Errorf("resumeFromCurrentBranch reported missing log even though origin has the checkpoint metadata (local metadata branch was not fast-forwarded from remote-tracking ref):\n%s", - combined) + t.Errorf("resume reported missing log even though origin has the checkpoint metadata:\n%s", combined) } } From d4366c651cd77ca2c4651fcfbbeca45f8040f2db Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 23:18:43 +0200 Subject: [PATCH 3/3] Preserve diverged local refs in SafelyAdvanceLocalRef MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous contract overwrote any local ref the target couldn't reach, which silently discarded unpushed local commits whenever a fetch landed sibling commits — e.g. checkpoint metadata produced on another machine sharing the same orphan-style refs (entire/checkpoints/v1, the V2 main ref). The objects survived in the loose-objects pool until git gc, but the branch ref no longer pointed at them. Tighten the helper so it only sets the ref when the move is non-destructive: missing → create, local at/ahead → no-op (existing protection), local strictly behind → fast-forward, diverged or unrelated history → no-op with a debug log. All three production callers (promoteRemoteTrackingMetadataBranch, FetchMetadataBranch, PromoteTmpRefSafely) sync orphan branches where this is the correct semantic; resume falls through to remote-tracking-tree reads in the rare divergent case. Update TestFetchV2MainFromURL_UpdatesExistingRef to use the existing advanceV2MainOnTop helper — the prior setup advanced the remote via a second call to createV2MainRef, which always produces an unrelated orphan commit. That setup was only "updating" via the now-removed diverged-overwrite path; a descendant commit on top of the previous tip is what the real condensation flow produces. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 4197ff1b1da6 --- .../cli/strategy/checkpoint_remote_test.go | 8 +- cmd/entire/cli/strategy/common.go | 31 ++- .../strategy/safely_advance_local_ref_test.go | 183 ++++++++++++++++++ 3 files changed, 213 insertions(+), 9 deletions(-) create mode 100644 cmd/entire/cli/strategy/safely_advance_local_ref_test.go diff --git a/cmd/entire/cli/strategy/checkpoint_remote_test.go b/cmd/entire/cli/strategy/checkpoint_remote_test.go index 08e815cb3a..4bdd6fceaf 100644 --- a/cmd/entire/cli/strategy/checkpoint_remote_test.go +++ b/cmd/entire/cli/strategy/checkpoint_remote_test.go @@ -918,10 +918,12 @@ func TestFetchV2MainFromURL_UpdatesExistingRef(t *testing.T) { require.NoError(t, err) hash1 := strings.TrimSpace(string(hash1Out)) - // Add a second commit on the remote's v2 ref - createV2MainRef(ctx, t, remoteDir) // Creates a new orphan commit, updating the ref + // Advance the remote's v2 /main with a child commit so the new tip is a + // strict descendant of hash1 (the realistic CLI flow — condensation + // appends a new checkpoint commit on top of the previous tip). + advanceV2MainOnTop(ctx, t, remoteDir, hash1) - // Fetch again — should update + // Fetch again — should fast-forward require.NoError(t, FetchV2MainFromURL(ctx, remoteDir)) hashCmd = exec.CommandContext(ctx, "git", "rev-parse", paths.V2MainRefName) diff --git a/cmd/entire/cli/strategy/common.go b/cmd/entire/cli/strategy/common.go index ce49de7d1e..104a138427 100644 --- a/cmd/entire/cli/strategy/common.go +++ b/cmd/entire/cli/strategy/common.go @@ -126,13 +126,24 @@ func PromoteTmpRefSafely(ctx context.Context, tmpRefName, destRefName plumbing.R return nil } -// SafelyAdvanceLocalRef updates localRefName to point at targetHash, except -// when the existing local ref is already at or ahead of targetHash. In that -// case it leaves the local ref unchanged to avoid rewinding locally-ahead -// work. Otherwise (local missing, behind, or diverged) it updates the ref to -// targetHash. +// SafelyAdvanceLocalRef updates localRefName to point at targetHash only when +// the move is non-destructive: // -// The ancestry check walks from the local ref (which has full history), so +// - local missing → create at target +// - local == target → no-op +// - local at or ahead of target (target is an ancestor of local) → no-op +// - local strictly behind target (local is an ancestor of target) → fast-forward +// - diverged or unrelated history (neither ref is an ancestor of the other) +// → no-op, logged at debug level +// +// The diverged case is the dangerous one this guard exists for. The CLI +// maintains orphan-style refs (entire/checkpoints/v1, the V2 main ref) where +// unpushed local commits encode user work — a fetch that landed sibling +// commits from another machine must not silently rewind that work. In the +// diverged case readers fall through to the remote-tracking tree or to v1, +// so resume keeps working while local-only commits stay reachable. +// +// The ancestry checks walk from the local ref (which has full history), so // callers that fetched with --depth=1 do not break the check. func SafelyAdvanceLocalRef(ctx context.Context, repo *git.Repository, localRefName plumbing.ReferenceName, targetHash plumbing.Hash) error { currentLocal, localErr := repo.Reference(localRefName, true) @@ -143,6 +154,14 @@ func SafelyAdvanceLocalRef(ctx context.Context, repo *git.Repository, localRefNa if IsAncestorOf(ctx, repo, targetHash, currentLocal.Hash()) { return nil } + if !IsAncestorOf(ctx, repo, currentLocal.Hash(), targetHash) { + logging.Debug(ctx, "skipping advance: local ref has diverged from target", + slog.String("ref", string(localRefName)), + slog.String("local", currentLocal.Hash().String()), + slog.String("target", targetHash.String()), + ) + return nil + } } newRef := plumbing.NewHashReference(localRefName, targetHash) diff --git a/cmd/entire/cli/strategy/safely_advance_local_ref_test.go b/cmd/entire/cli/strategy/safely_advance_local_ref_test.go new file mode 100644 index 0000000000..57d650a815 --- /dev/null +++ b/cmd/entire/cli/strategy/safely_advance_local_ref_test.go @@ -0,0 +1,183 @@ +package strategy + +import ( + "context" + "testing" + "time" + + "github.com/entireio/cli/cmd/entire/cli/testutil" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" +) + +const safelyAdvanceTestRef plumbing.ReferenceName = "refs/heads/safely-advance-test" + +// newSafelyAdvanceTestRepo opens an empty git repository for ref-manipulation +// tests. It uses testutil.InitRepo so author/GPG config matches the rest of +// the suite, but the tests themselves operate purely via plumbing — no +// worktree state needed. +func newSafelyAdvanceTestRepo(t *testing.T) *git.Repository { + t.Helper() + dir := t.TempDir() + testutil.InitRepo(t, dir) + repo, err := git.PlainOpen(dir) + if err != nil { + t.Fatalf("PlainOpen: %v", err) + } + return repo +} + +// makeEmptyTreeCommit writes a commit with an empty tree and the given parents +// and message. Different messages produce different hashes, so callers can +// create distinct commits (including divergent siblings) without touching +// files. +func makeEmptyTreeCommit(t *testing.T, repo *git.Repository, parents []plumbing.Hash, msg string) plumbing.Hash { + t.Helper() + emptyTree := object.Tree{} + emptyTreeObj := repo.Storer.NewEncodedObject() + if err := emptyTree.Encode(emptyTreeObj); err != nil { + t.Fatalf("encode empty tree: %v", err) + } + treeHash, err := repo.Storer.SetEncodedObject(emptyTreeObj) + if err != nil { + t.Fatalf("store empty tree: %v", err) + } + sig := object.Signature{Name: "T", Email: "t@example.com", When: time.Unix(0, 0).UTC()} + commit := &object.Commit{ + TreeHash: treeHash, + Message: msg, + Author: sig, + Committer: sig, + ParentHashes: parents, + } + obj := repo.Storer.NewEncodedObject() + if err := commit.Encode(obj); err != nil { + t.Fatalf("encode commit %q: %v", msg, err) + } + hash, err := repo.Storer.SetEncodedObject(obj) + if err != nil { + t.Fatalf("store commit %q: %v", msg, err) + } + return hash +} + +// forceSetTestRef points safelyAdvanceTestRef at hash unconditionally — +// tests deliberately rewind/diverge the ref to set up the scenarios under +// test. +func forceSetTestRef(t *testing.T, repo *git.Repository, hash plumbing.Hash) { + t.Helper() + if err := repo.Storer.SetReference(plumbing.NewHashReference(safelyAdvanceTestRef, hash)); err != nil { + t.Fatalf("SetReference %s: %v", safelyAdvanceTestRef, err) + } +} + +// readTestRef reads safelyAdvanceTestRef and fatals if it is missing. Used +// after a call to SafelyAdvanceLocalRef to inspect the resulting ref state. +func readTestRef(t *testing.T, repo *git.Repository) plumbing.Hash { + t.Helper() + ref, err := repo.Reference(safelyAdvanceTestRef, true) + if err != nil { + t.Fatalf("read %s: %v", safelyAdvanceTestRef, err) + } + return ref.Hash() +} + +func TestSafelyAdvanceLocalRef_LocalMissing_SetsToTarget(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + a := makeEmptyTreeCommit(t, repo, nil, "A") + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, a); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != a { + t.Errorf("local ref = %s, want %s", got, a) + } +} + +func TestSafelyAdvanceLocalRef_LocalEqualsTarget_NoOp(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + a := makeEmptyTreeCommit(t, repo, nil, "A") + forceSetTestRef(t, repo, a) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, a); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != a { + t.Errorf("local ref = %s, want %s (unchanged)", got, a) + } +} + +func TestSafelyAdvanceLocalRef_LocalAhead_NoOp(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + a := makeEmptyTreeCommit(t, repo, nil, "A") + b := makeEmptyTreeCommit(t, repo, []plumbing.Hash{a}, "B") + forceSetTestRef(t, repo, b) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, a); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != b { + t.Errorf("locally-ahead ref must not rewind: got %s, want %s", got, b) + } +} + +func TestSafelyAdvanceLocalRef_LocalBehind_FastForwards(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + a := makeEmptyTreeCommit(t, repo, nil, "A") + b := makeEmptyTreeCommit(t, repo, []plumbing.Hash{a}, "B") + forceSetTestRef(t, repo, a) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, b); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != b { + t.Errorf("local ref should have fast-forwarded: got %s, want %s", got, b) + } +} + +// Before this fix, SafelyAdvanceLocalRef overwrote any local ref whose history +// the target couldn't reach. That destroyed unpushed local commits on orphan +// refs the CLI maintains (entire/checkpoints/v1, the V2 main ref) whenever a +// fetch landed sibling commits — e.g. checkpoint metadata produced on another +// machine. The objects survived in the loose-objects pool until git gc, but +// the branch ref no longer pointed at them. +func TestSafelyAdvanceLocalRef_Diverged_PreservesLocal(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + base := makeEmptyTreeCommit(t, repo, nil, "base") + localTip := makeEmptyTreeCommit(t, repo, []plumbing.Hash{base}, "local-only-work") + targetTip := makeEmptyTreeCommit(t, repo, []plumbing.Hash{base}, "remote-only-work") + forceSetTestRef(t, repo, localTip) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, targetTip); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != localTip { + t.Errorf("diverged local ref must be preserved: got %s, want %s (overwrote to target %s?)", + got, localTip, targetTip) + } +} + +// Unrelated histories (no common ancestor at all) are a strict subset of +// "diverged" — neither ref is an ancestor of the other. Confirms the +// protection extends to that case. +func TestSafelyAdvanceLocalRef_UnrelatedHistory_PreservesLocal(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + localOnly := makeEmptyTreeCommit(t, repo, nil, "local-orphan") + targetOnly := makeEmptyTreeCommit(t, repo, nil, "target-orphan") + forceSetTestRef(t, repo, localOnly) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, targetOnly); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != localOnly { + t.Errorf("unrelated-history local ref must be preserved: got %s, want %s", got, localOnly) + } +}