Skip to content

Commit 9c2e157

Browse files
dipreeclaude
andcommitted
fix(attach): tolerate unpublished v1 branch on explicit-target refresh
Same gap as the git-refs fix, on the git-branch backend: a repo that has never published entire/checkpoints/v1 made the fail-closed refresh reject every explicit-target attach. Probe the branch with ls-remote first — absent means genuinely new (proceed); transport/auth failures still fail closed. Also extracts readAttachTranscript to keep runAttach under the maintidx threshold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 01KY56BD4NF340J1HV27JC0CZF
1 parent 0a245a8 commit 9c2e157

2 files changed

Lines changed: 165 additions & 23 deletions

File tree

cmd/entire/cli/attach.go

Lines changed: 61 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/entireio/cli/cmd/entire/cli/agent/types"
1818
cpkg "github.com/entireio/cli/cmd/entire/cli/checkpoint"
1919
"github.com/entireio/cli/cmd/entire/cli/checkpoint/id"
20+
checkpointremote "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote"
2021
"github.com/entireio/cli/cmd/entire/cli/interactive"
2122
"github.com/entireio/cli/cmd/entire/cli/logging"
2223
cliReview "github.com/entireio/cli/cmd/entire/cli/review"
@@ -286,26 +287,15 @@ func runAttach(ctx context.Context, w, errW io.Writer, sessionID string, agentNa
286287
reviewSkills = resolveReviewSkills(opts.ReviewSkillsOverride)
287288
}
288289

289-
transcriptData, err := ag.ReadTranscript(transcriptPath)
290+
transcriptData, storedTranscript, err := readAttachTranscript(logCtx, ag, transcriptPath)
290291
if err != nil {
291-
return fmt.Errorf("failed to read transcript: %w", err)
292-
}
293-
294-
// Normalize Gemini transcripts for storage.
295-
storedTranscript := transcriptData
296-
if ag.Type() == agent.AgentTypeGemini {
297-
if normalized, normErr := geminicli.NormalizeTranscript(transcriptData); normErr == nil {
298-
storedTranscript = normalized
299-
} else {
300-
logging.Warn(logCtx, "failed to normalize Gemini transcript, storing raw", "error", normErr)
301-
}
292+
return err
302293
}
303-
304294
meta := extractTranscriptMetadata(transcriptData)
305295
warnEmptyTranscriptMetadata(errW, ag.Name(), meta, opts)
296+
tokenUsage := agent.CalculateTokenUsage(logCtx, ag, transcriptData, 0, "")
306297

307-
// Determine checkpoint ID: an explicit target uses the caller-supplied ID
308-
// (new by construction, bound to opts.CommitSHA); otherwise reuse HEAD's
298+
// Explicit targets use the caller-supplied ID; otherwise reuse HEAD's
309299
// trailer if present or generate a fresh ID.
310300
var checkpointID id.CheckpointID
311301
var isExistingCheckpoint bool
@@ -343,6 +333,12 @@ func runAttach(ctx context.Context, w, errW io.Writer, sessionID string, agentNa
343333
}
344334
}
345335

336+
saveState := func() {
337+
if err := saveAttachSessionState(logCtx, repo, existingState, sessionID, ag.Type(), transcriptPath, checkpointID, meta, tokenUsage, opts, reviewSkills); err != nil {
338+
logging.Warn(logCtx, "failed to save session state", "error", err)
339+
}
340+
}
341+
346342
if opts.explicitTarget() {
347343
freshRepo, alreadyAttached, expErr := prepareExplicitTarget(ctx, logCtx, w, repo, refs, checkpointID, sessionID)
348344
if freshRepo != repo {
@@ -357,6 +353,10 @@ func runAttach(ctx context.Context, w, errW io.Writer, sessionID string, agentNa
357353
return expErr
358354
}
359355
if alreadyAttached {
356+
// The checkpoint write may have succeeded on an earlier attempt while
357+
// its best-effort state save failed. Repair local state on every
358+
// idempotent retry before returning success.
359+
saveState()
360360
return nil
361361
}
362362
}
@@ -366,8 +366,6 @@ func runAttach(ctx context.Context, w, errW io.Writer, sessionID string, agentNa
366366
return fmt.Errorf("failed to get git author: %w", err)
367367
}
368368

369-
tokenUsage := agent.CalculateTokenUsage(logCtx, ag, transcriptData, 0, "")
370-
371369
_, redactSpan := perf.Start(ctx, "redact_transcript")
372370
redactedTranscript, redactErr := redact.JSONLBytes(storedTranscript)
373371
redactSpan.End()
@@ -400,9 +398,7 @@ func runAttach(ctx context.Context, w, errW io.Writer, sessionID string, agentNa
400398
}
401399

402400
// Create or update session state.
403-
if err := saveAttachSessionState(logCtx, repo, existingState, sessionID, ag.Type(), transcriptPath, checkpointID, meta, tokenUsage, opts, reviewSkills); err != nil {
404-
logging.Warn(logCtx, "failed to save session state", "error", err)
405-
}
401+
saveState()
406402

407403
fmt.Fprintf(w, "Attached session %s\n", sessionID)
408404
printAttachFooter(w, meta, tokenUsage)
@@ -578,6 +574,24 @@ func ensureCheckpointAvailable(ctx, logCtx context.Context, repo *git.Repository
578574
return repo, missingCheckpointError(logCtx, checkpointID, primaryIsRefs)
579575
}
580576

577+
// readAttachTranscript reads the session transcript, normalizing Gemini
578+
// transcripts for storage (raw is kept when normalization fails).
579+
func readAttachTranscript(logCtx context.Context, ag agent.Agent, transcriptPath string) ([]byte, []byte, error) {
580+
transcriptData, err := ag.ReadTranscript(transcriptPath)
581+
if err != nil {
582+
return nil, nil, fmt.Errorf("failed to read transcript: %w", err)
583+
}
584+
storedTranscript := transcriptData
585+
if ag.Type() == agent.AgentTypeGemini {
586+
if normalized, normErr := geminicli.NormalizeTranscript(transcriptData); normErr == nil {
587+
storedTranscript = normalized
588+
} else {
589+
logging.Warn(logCtx, "failed to normalize Gemini transcript, storing raw", "error", normErr)
590+
}
591+
}
592+
return transcriptData, storedTranscript, nil
593+
}
594+
581595
// linkExistingSessionCheckpoint handles a session whose state already records
582596
// a checkpoint: reviews are refused (review-upgrade of an existing checkpoint
583597
// is unsupported), otherwise the existing checkpoint is offered as a trailer
@@ -655,8 +669,8 @@ func prepareExplicitTarget(ctx, logCtx context.Context, w io.Writer, repo *git.R
655669
// after the refresh is NOT an error — an explicit-target ID is normally brand
656670
// new; the refresh exists so a retry from a fresh clone sees the earlier
657671
// attempt instead of rebuilding the ID as an orphan that would clobber it on
658-
// push. For git-refs, an explicitly missing remote source ref proves the ID is
659-
// new and is safe to proceed; transport/auth/unknown failures still fail closed.
672+
// push. An explicitly missing remote source ref/branch proves the ID is new and
673+
// is safe to proceed; transport/auth/unknown failures still fail closed.
660674
func ensureExplicitCheckpointFreshness(ctx context.Context, repo *git.Repository, refs cpkg.PersistentRefs, checkpointID id.CheckpointID) (*git.Repository, bool, error) {
661675
cfg, err := settings.LoadCheckpointsConfig(ctx)
662676
if err != nil {
@@ -672,6 +686,16 @@ func ensureExplicitCheckpointFreshness(ctx context.Context, repo *git.Repository
672686
return repo, true, nil
673687
}
674688

689+
if !primaryIsRefs {
690+
remotePresent, probeErr := explicitTargetMetadataBranchPresent(ctx, refs)
691+
if probeErr != nil {
692+
return repo, false, fmt.Errorf("failed to probe explicit checkpoint metadata branch before attach: %w", probeErr)
693+
}
694+
if !remotePresent {
695+
return repo, false, nil
696+
}
697+
}
698+
675699
freshRepo, fetchErr := refreshCheckpoint(ctx, checkpointID, primaryIsRefs)
676700
if fetchErr != nil {
677701
if primaryIsRefs && errors.Is(fetchErr, errCheckpointRefNotFound) {
@@ -686,6 +710,21 @@ func ensureExplicitCheckpointFreshness(ctx context.Context, repo *git.Repository
686710
return freshRepo, present, nil
687711
}
688712

713+
// explicitTargetMetadataBranchPresent distinguishes a genuinely unpublished v1
714+
// branch from an inconclusive fetch failure. `git ls-remote` succeeds with empty
715+
// output when the branch is absent, while transport/auth failures return errors.
716+
func explicitTargetMetadataBranchPresent(ctx context.Context, refs cpkg.PersistentRefs) (bool, error) {
717+
fetchTarget, err := checkpointremote.FetchURL(ctx)
718+
if err != nil {
719+
return false, fmt.Errorf("resolve checkpoint fetch target: %w", err)
720+
}
721+
output, err := checkpointremote.LsRemoteInDir(ctx, "", fetchTarget, refs.Primary.String())
722+
if err != nil {
723+
return false, fmt.Errorf("list metadata branch on %s: %w", checkpointremote.RedactURL(fetchTarget), err)
724+
}
725+
return strings.TrimSpace(string(output)) != "", nil
726+
}
727+
689728
// refreshCheckpoint fetches the checkpoint referenced by HEAD from the remote and
690729
// returns a freshly-opened repo so go-git sees the newly-fetched refs/packfiles.
691730
// The fetch is backend-aware: git-refs fetches just this checkpoint's ref, while

cmd/entire/cli/attach_test.go

Lines changed: 104 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1171,7 +1171,9 @@ func TestAttach_ReviewWithExistingCheckpointErrors(t *testing.T) {
11711171
// when HEAD carries no Entire-Checkpoint trailer, and must never amend HEAD.
11721172
func TestAttach_ReviewExplicitTargetCreatesShaBoundCheckpoint(t *testing.T) {
11731173
setupAttachTestRepo(t)
1174-
setupAttachCheckpointOrigin(t)
1174+
// A reachable origin with no metadata branch is the normal first-checkpoint
1175+
// case and must be accepted as a genuinely new explicit target.
1176+
setupAttachBareOrigin(t)
11751177

11761178
repoRoot := mustGetwd(t)
11771179
repo, err := git.PlainOpen(repoRoot)
@@ -1307,6 +1309,107 @@ func TestAttach_ReviewExplicitTargetCreatesShaBoundCheckpoint(t *testing.T) {
13071309
}
13081310
}
13091311

1312+
// An idempotent retry must repair session state as well as avoid duplicating
1313+
// checkpoint data. This models a first attach whose checkpoint write succeeded
1314+
// but whose best-effort state save failed.
1315+
func TestAttach_ExplicitTargetRetryRepairsMissingSessionState(t *testing.T) {
1316+
setupAttachTestRepo(t)
1317+
setupAttachCheckpointOrigin(t)
1318+
1319+
repoRoot := mustGetwd(t)
1320+
sessionID := "review-session-retry-state-repair"
1321+
setupClaudeTranscript(t, sessionID, `{"type":"user","message":{"role":"user","content":"review retry state"},"uuid":"u1"}
1322+
`)
1323+
targetID := id.CheckpointID("aabbccddeeff")
1324+
opts := attachOptions{
1325+
Force: true,
1326+
Review: true,
1327+
ReviewSkillsOverride: []string{"/security-review"},
1328+
ReviewPromptOverride: "review the target commit for security regressions",
1329+
CheckpointID: targetID,
1330+
}
1331+
1332+
var out bytes.Buffer
1333+
if err := runAttach(context.Background(), &out, &out, sessionID, agent.AgentNameClaudeCode, opts); err != nil {
1334+
t.Fatalf("first explicit-target attach failed: %v", err)
1335+
}
1336+
stateFile := filepath.Join(repoRoot, ".git", "entire-sessions", sessionID+".json")
1337+
if err := os.Remove(stateFile); err != nil {
1338+
t.Fatalf("remove state to simulate failed first save: %v", err)
1339+
}
1340+
1341+
out.Reset()
1342+
if err := runAttach(context.Background(), &out, &out, sessionID, agent.AgentNameClaudeCode, opts); err != nil {
1343+
t.Fatalf("idempotent retry failed: %v", err)
1344+
}
1345+
if !strings.Contains(out.String(), "already attached") {
1346+
t.Fatalf("retry did not take idempotent path: %q", out.String())
1347+
}
1348+
1349+
stateStore, err := session.NewStateStore(context.Background())
1350+
if err != nil {
1351+
t.Fatal(err)
1352+
}
1353+
state, err := stateStore.Load(context.Background(), sessionID)
1354+
if err != nil {
1355+
t.Fatal(err)
1356+
}
1357+
if state == nil {
1358+
t.Fatal("retry did not recreate session state")
1359+
}
1360+
if state.AgentType != agent.AgentTypeClaudeCode {
1361+
t.Errorf("AgentType = %q, want %q", state.AgentType, agent.AgentTypeClaudeCode)
1362+
}
1363+
if state.Kind != session.KindAgentReview {
1364+
t.Errorf("Kind = %q, want %q", state.Kind, session.KindAgentReview)
1365+
}
1366+
if !reflect.DeepEqual(state.ReviewSkills, opts.ReviewSkillsOverride) {
1367+
t.Errorf("ReviewSkills = %v, want %v", state.ReviewSkills, opts.ReviewSkillsOverride)
1368+
}
1369+
if state.ReviewPrompt != opts.ReviewPromptOverride {
1370+
t.Errorf("ReviewPrompt = %q, want %q", state.ReviewPrompt, opts.ReviewPromptOverride)
1371+
}
1372+
if !state.LastCheckpointID.IsEmpty() || state.BaseCommit != "" {
1373+
t.Errorf("retry incorrectly trailer-linked explicit checkpoint: LastCheckpointID=%s BaseCommit=%q", state.LastCheckpointID, state.BaseCommit)
1374+
}
1375+
}
1376+
1377+
// A missing metadata branch on a reachable origin is safe, but a failed branch
1378+
// probe is inconclusive and must not create a local v1 orphan under the supplied
1379+
// checkpoint ID.
1380+
func TestAttach_ExplicitTargetGitBranchRejectsProbeFailure(t *testing.T) {
1381+
setupAttachTestRepo(t)
1382+
1383+
repoRoot := mustGetwd(t)
1384+
runGit(t, repoRoot, "remote", "add", "origin", filepath.Join(t.TempDir(), "missing.git"))
1385+
sessionID := "review-session-broken-metadata-remote"
1386+
setupClaudeTranscript(t, sessionID, `{"type":"user","message":{"role":"user","content":"review retry"},"uuid":"u1"}
1387+
`)
1388+
targetID := id.CheckpointID("aabbccddeeff")
1389+
1390+
var out bytes.Buffer
1391+
err := runAttach(context.Background(), &out, &out, sessionID, agent.AgentNameClaudeCode, attachOptions{
1392+
Force: true,
1393+
Review: true,
1394+
CheckpointID: targetID,
1395+
})
1396+
if err == nil || !strings.Contains(err.Error(), "failed to probe explicit checkpoint metadata branch") {
1397+
t.Fatalf("expected inconclusive metadata probe to fail closed, got: %v", err)
1398+
}
1399+
1400+
stateStore, err := session.NewStateStore(context.Background())
1401+
if err != nil {
1402+
t.Fatal(err)
1403+
}
1404+
state, err := stateStore.Load(context.Background(), sessionID)
1405+
if err != nil {
1406+
t.Fatal(err)
1407+
}
1408+
if state != nil {
1409+
t.Fatalf("session state was written after an inconclusive metadata probe: %+v", state)
1410+
}
1411+
}
1412+
13101413
// Under git-refs, a brand-new explicit-target ID has no source ref on the
13111414
// remote. Git reports that as a failed fetch; attach must recognize this
13121415
// specific absence as the safe, normal create path.

0 commit comments

Comments
 (0)