Skip to content

Commit 4a32e34

Browse files
peyton-altclaude
andcommitted
review: debug-log anchor resolution, pin wiring + preserve-on-rewrite in tests
Addresses toolkit review findings on #1825: a Debug line records the resolved anchor (support can tell why an import is unanchored or stale), an integration assertion pins the import_cmd wiring end to end (omitempty would otherwise hide a dropped wire), a store test covers the summary preserve-on-rewrite branch, and comment polish (canonical CommitSHA doc on WriteOptions, softened default-branch over-claims). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 01KY39WXXNWBZ6SQQDE9TMJWN8
1 parent d2df272 commit 4a32e34

7 files changed

Lines changed: 120 additions & 48 deletions

File tree

api/checkpoint/metadata.go

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,13 @@ type WriteOptions struct {
3838
Branch string
3939

4040
// CommitSHA links this checkpoint to an existing commit without a trailer.
41-
// Set only by `entire import`: imported history has no Entire-Checkpoint
42-
// trailer (we never rewrite existing commits), so import stamps the default
43-
// branch's head SHA here as an anchor — "imported at this point in time" —
44-
// not attribution. Empty for all other writers.
41+
// It is an anchor — "imported at this point in time" — not attribution.
42+
// Currently set only by `entire import`: imported history has no
43+
// Entire-Checkpoint trailer (we never rewrite existing commits), so import
44+
// stamps the resolved anchor commit here (the default branch head when
45+
// resolvable; see resolveImportLinkCommitSHA for the fallback order).
46+
// Empty for all other writers. This comment is the canonical description;
47+
// Metadata.CommitSHA and CheckpointSummary.CommitSHA point back here.
4548
CommitSHA string
4649

4750
// Transcript is the session transcript content (full.jsonl).
@@ -329,9 +332,9 @@ type Metadata struct {
329332
Strategy string `json:"strategy"`
330333
CreatedAt time.Time `json:"created_at"`
331334
Branch string `json:"branch,omitempty"` // Branch where checkpoint was created (empty if detached HEAD)
332-
// CommitSHA anchors an imported checkpoint to an existing commit (the
333-
// default branch head at import time). Empty for non-imported checkpoints,
334-
// which link to commits via the Entire-Checkpoint trailer instead.
335+
// CommitSHA anchors an imported checkpoint to an existing commit; empty for
336+
// non-imported checkpoints, which link via the Entire-Checkpoint trailer.
337+
// See WriteOptions.CommitSHA for the full semantics.
335338
CommitSHA string `json:"commit_sha,omitempty"`
336339
CheckpointsCount int `json:"checkpoints_count"`
337340
// SaveStepCount is the number of SaveStep-recorded steps for this session.
@@ -485,10 +488,11 @@ type SessionFilePaths struct {
485488
//
486489
//nolint:revive // Named CheckpointSummary to avoid conflict with existing Summary struct
487490
type CheckpointSummary struct {
488-
CLIVersion string `json:"cli_version,omitempty"`
489-
CheckpointID id.CheckpointID `json:"checkpoint_id"`
490-
Strategy string `json:"strategy"`
491-
Branch string `json:"branch,omitempty"`
491+
CLIVersion string `json:"cli_version,omitempty"`
492+
CheckpointID id.CheckpointID `json:"checkpoint_id"`
493+
Strategy string `json:"strategy"`
494+
Branch string `json:"branch,omitempty"`
495+
// CommitSHA: import-only anchor; see WriteOptions.CommitSHA.
492496
CommitSHA string `json:"commit_sha,omitempty"`
493497
CheckpointsCount int `json:"checkpoints_count"`
494498
FilesTouched []string `json:"files_touched"`

cmd/entire/cli/agentimport/agentimport.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,8 @@ type Options struct {
101101

102102
// LinkCommitSHA, when non-empty, is written to each imported checkpoint's
103103
// metadata as commit_sha — the anchor commit the UI shows imported sessions
104-
// against. The caller resolves it (default branch head); Run does not.
104+
// against. The caller resolves it (default branch head when resolvable;
105+
// see resolveImportLinkCommitSHA); Run does not.
105106
LinkCommitSHA string
106107
}
107108

cmd/entire/cli/checkpoint/persistent.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -818,6 +818,9 @@ func (s *treeWriter) writeCheckpointSummary(opts WriteOptions, basePath string,
818818
if !imported {
819819
imported = existingSummary.Imported
820820
}
821+
// A later write to the same checkpoint (e.g. a review session
822+
// attached to it) carries no CommitSHA; the imported anchor
823+
// must survive that rewrite rather than be cleared.
821824
if commitSHA == "" {
822825
commitSHA = existingSummary.CommitSHA
823826
}

cmd/entire/cli/checkpoint/persistent_imported_test.go

Lines changed: 68 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,11 @@ import (
1515
"github.com/entireio/cli/redact"
1616
)
1717

18-
func TestWrite_ImportedSurfacesOnList(t *testing.T) {
19-
t.Parallel()
18+
// newImportedTestStore builds a GitStore over a fresh temp repo with one
19+
// commit (so HEAD exists) and returns it with a redacted one-line transcript,
20+
// shared setup for the imported-checkpoint tests below.
21+
func newImportedTestStore(t *testing.T) (*GitStore, redact.RedactedBytes) {
22+
t.Helper()
2023
tempDir := t.TempDir()
2124
testutil.InitRepo(t, tempDir)
2225
repo, err := git.PlainOpen(tempDir)
@@ -38,12 +41,17 @@ func TestWrite_ImportedSurfacesOnList(t *testing.T) {
3841
t.Fatal(err)
3942
}
4043

41-
store := NewGitStore(repo, DefaultV1Refs())
4244
red, err := redact.JSONLBytes([]byte(`{"type":"user","uuid":"u1","message":{"role":"user","content":"hi"}}` + "\n"))
4345
if err != nil {
4446
t.Fatal(err)
4547
}
46-
err = store.Write(context.Background(), Session{
48+
return NewGitStore(repo, DefaultV1Refs()), red
49+
}
50+
51+
func TestWrite_ImportedSurfacesOnList(t *testing.T) {
52+
t.Parallel()
53+
store, red := newImportedTestStore(t)
54+
err := store.Write(context.Background(), Session{
4755
CheckpointID: id.MustCheckpointID("aabbccddeeff"),
4856
SessionID: "s1",
4957
Strategy: "import",
@@ -68,37 +76,12 @@ func TestWrite_ImportedSurfacesOnList(t *testing.T) {
6876

6977
func TestImportedCheckpoint_CommitSHAPersisted(t *testing.T) {
7078
t.Parallel()
71-
tempDir := t.TempDir()
72-
testutil.InitRepo(t, tempDir)
73-
repo, err := git.PlainOpen(tempDir)
74-
if err != nil {
75-
t.Fatalf("open repo: %v", err)
76-
}
77-
// Initial commit so HEAD exists.
78-
wt, err := repo.Worktree()
79-
if err != nil {
80-
t.Fatal(err)
81-
}
82-
testutil.WriteFile(t, tempDir, "f.txt", "x")
83-
if _, err := wt.Add("f.txt"); err != nil {
84-
t.Fatal(err)
85-
}
86-
if _, err := wt.Commit("init", &git.CommitOptions{
87-
Author: &object.Signature{Name: "Test", Email: "test@test.com"},
88-
}); err != nil {
89-
t.Fatal(err)
90-
}
91-
92-
store := NewGitStore(repo, DefaultV1Refs())
93-
red, err := redact.JSONLBytes([]byte(`{"type":"user","uuid":"u1","message":{"role":"user","content":"hi"}}` + "\n"))
94-
if err != nil {
95-
t.Fatal(err)
96-
}
79+
store, red := newImportedTestStore(t)
9780

9881
const commitSHA = "b01b59663fd4860fd15a9939499be44a14dbf168"
9982
ctx := context.Background()
10083
cid := id.MustCheckpointID("aabbccddeeff")
101-
err = store.Write(ctx, Session{
84+
err := store.Write(ctx, Session{
10285
CheckpointID: cid,
10386
SessionID: "s1",
10487
Strategy: "import",
@@ -161,3 +144,57 @@ func TestImportedCheckpoint_CommitSHAPersisted(t *testing.T) {
161144
t.Fatalf("expected marshaled metadata to omit commit_sha when unset, got %s", rawMD)
162145
}
163146
}
147+
148+
// TestImportedCheckpoint_CommitSHASurvivesSummaryRewrite proves the root
149+
// summary's preserve-on-rewrite: a later write to the SAME checkpoint that
150+
// carries no CommitSHA (e.g. a review session attached to it) must not clear
151+
// the stamped anchor from the root CheckpointSummary. Session-level Metadata
152+
// is deliberately NOT preserved the same way — each session's metadata.json
153+
// records what its own write carried.
154+
func TestImportedCheckpoint_CommitSHASurvivesSummaryRewrite(t *testing.T) {
155+
t.Parallel()
156+
store, red := newImportedTestStore(t)
157+
158+
const commitSHA = "b01b59663fd4860fd15a9939499be44a14dbf168"
159+
ctx := context.Background()
160+
cid := id.MustCheckpointID("ddeeff001122")
161+
err := store.Write(ctx, Session{
162+
CheckpointID: cid,
163+
SessionID: "s1",
164+
Strategy: "import",
165+
Kind: "imported",
166+
Agent: agent.AgentTypeClaudeCode,
167+
Transcript: red,
168+
Prompts: []string{"hi"},
169+
CheckpointsCount: 1,
170+
CommitSHA: commitSHA,
171+
})
172+
if err != nil {
173+
t.Fatalf("write imported checkpoint: %v", err)
174+
}
175+
176+
// Second write to the same checkpoint, different session, no CommitSHA.
177+
err = store.Write(ctx, Session{
178+
CheckpointID: cid,
179+
SessionID: "s2",
180+
Strategy: "manual-commit",
181+
Agent: agent.AgentTypeClaudeCode,
182+
Transcript: red,
183+
Prompts: []string{"review it"},
184+
CheckpointsCount: 1,
185+
})
186+
if err != nil {
187+
t.Fatalf("second write to same checkpoint: %v", err)
188+
}
189+
190+
summary, err := store.Read(ctx, cid)
191+
if err != nil {
192+
t.Fatalf("read checkpoint summary: %v", err)
193+
}
194+
if summary.CommitSHA != commitSHA {
195+
t.Fatalf("root summary commit_sha must survive a CommitSHA-less rewrite: expected %q, got %q", commitSHA, summary.CommitSHA)
196+
}
197+
if len(summary.Sessions) != 2 {
198+
t.Fatalf("expected both sessions in the rewritten summary, got %d", len(summary.Sessions))
199+
}
200+
}

cmd/entire/cli/import_cmd.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"github.com/spf13/cobra"
88

99
"github.com/entireio/cli/cmd/entire/cli/agentimport"
10+
"github.com/entireio/cli/cmd/entire/cli/logging"
1011
"github.com/entireio/cli/cmd/entire/cli/paths"
1112
"github.com/entireio/cli/cmd/entire/cli/strategy"
1213
)
@@ -66,10 +67,15 @@ fails even with --dry-run.`, imp.AgentType()),
6667
// it only always-on secret scanning would run on imported history.
6768
strategy.EnsureRedactionConfigured()
6869

70+
// Logged so support can tell why an import has no anchor (empty
71+
// sha: nothing resolved) or a stale one (origin tip not fetched).
72+
linkCommitSHA := resolveImportLinkCommitSHA(repo)
73+
logging.Debug(ctx, "import: resolved link commit", "commit_sha", linkCommitSHA)
74+
6975
res, err := agentimport.Run(ctx, repo, imp, agentimport.Options{
7076
RepoRoot: repoRoot, OverridePath: pathFlag, SessionFilter: sessions,
7177
Now: time.Now(), DryRun: dryRun,
72-
LinkCommitSHA: resolveImportLinkCommitSHA(repo),
78+
LinkCommitSHA: linkCommitSHA,
7379
})
7480
if err != nil {
7581
return fmt.Errorf("import %s: %w", imp.Name(), err)

cmd/entire/cli/import_link.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,11 @@ import (
99

1010
// resolveImportLinkCommitSHA returns the commit SHA imported checkpoints are
1111
// anchored to: the default branch's head at import time. Preference order:
12-
// origin's tip of the default branch (the commit the server already knows),
13-
// then the local branch tip, then HEAD. Best-effort — returns "" when nothing
14-
// resolves (e.g. an empty repo); import proceeds without a link.
12+
// origin's tip of the default branch (the commit most likely already known to
13+
// the server), then the local branch tip, then HEAD. Best-effort — returns ""
14+
// when nothing resolves (e.g. an empty repo); import proceeds without a link.
15+
// This function is the source of truth for the order; the architecture docs
16+
// describe it but defer here.
1517
func resolveImportLinkCommitSHA(repo *git.Repository) string {
1618
if name := strategy.GetDefaultBranchName(repo); name != "" {
1719
if ref, err := repo.Reference(plumbing.NewRemoteReferenceName("origin", name), true); err == nil {

cmd/entire/cli/integration_test/import_claude_test.go

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,30 @@ func TestImportClaudeCode_EndToEnd(t *testing.T) {
3131
require.Contains(t, out, "Would import 2", "dry-run should count 2 turns; got: %s", out)
3232
require.NotContains(t, env.RunCLI("checkpoint", "list"), "[imported]", "dry-run must not write checkpoints")
3333

34+
// Diverge the feature branch from the default branch before importing, so
35+
// the commit_sha assertion below can tell the default-branch tip apart
36+
// from HEAD (they are identical right after NewFeatureBranchEnv).
37+
env.WriteFile("diverge.txt", "x")
38+
env.GitAdd("diverge.txt")
39+
env.GitCommit("diverge feature branch")
40+
3441
// 2. Real import writes the imported checkpoints (onto the v1 metadata branch).
3542
out = env.RunCLI("import", "claude-code")
3643
require.Contains(t, out, "Imported 2", "got: %s", out)
3744

45+
// 2b. Every imported checkpoint carries the default branch's tip as its
46+
// commit_sha anchor (NOT the feature branch's HEAD). This pins the
47+
// command-level wiring (import_cmd → resolveImportLinkCommitSHA →
48+
// agentimport.Options): omitempty would silently hide a dropped wire.
49+
defaultTip := gitOutput(t, env.RepoDir, "rev-parse", "master")
50+
importedID := agentimport.DeriveCheckpointID(sessionID, "u1").String()
51+
sessionMD := gitOutput(t, env.RepoDir, "show", "entire/checkpoints/v1:"+SessionMetadataPath(importedID))
52+
require.Contains(t, sessionMD, `"commit_sha": "`+defaultTip+`"`,
53+
"imported session metadata should anchor to the default branch tip; got: %s", sessionMD)
54+
rootMD := gitOutput(t, env.RepoDir, "show", "entire/checkpoints/v1:"+CheckpointSummaryPath(importedID))
55+
require.Contains(t, rootMD, `"commit_sha": "`+defaultTip+`"`,
56+
"imported root summary should anchor to the default branch tip; got: %s", rootMD)
57+
3858
// 3. checkpoint list surfaces imported entries labeled [imported], and does
3959
// NOT duplicate them as [temporary] (regression: the imports were once
4060
// mis-read by the shadow-branch scanner).
@@ -44,7 +64,6 @@ func TestImportClaudeCode_EndToEnd(t *testing.T) {
4464

4565
// 4. explain resolves an imported checkpoint by ID (regression: explain once
4666
// only consulted the committed/shadow paths and missed imports).
47-
importedID := agentimport.DeriveCheckpointID(sessionID, "u1").String()
4867
explainOut := env.RunCLI("checkpoint", "explain", importedID)
4968
require.Contains(t, explainOut, "first", "explain should show the imported turn's prompt; got: %s", explainOut)
5069

0 commit comments

Comments
 (0)