Handle Checkpoint Updates in Resume Logic

Claude Code·Opus 4.8[1m]·

Soph·2mo ago·25hr 17min·2 Checkpoints·10 file changes·+697/-74·255K tokens

I just did entire resume mirror-perms-sync in ../entiredb it worked and put the session in the right place and I then used claude -r <sessionid> but then I noticed the session is not at the state it was on my other machine. So I went there and did a push. Now it did not mean a new code commit was pushed but the checkpoint was updated. But entire resume is not catching this now, since it's not handling the case that a checkpoint might have been updated.

Can you take a look and make a few suggestions for a fix?

2mo ago·2m

I've traced the whole flow. Here's what's going on and how I'd fix it.

Root cause

The bug is in RestoreLogsOnly at cmd/entire/cli/strategy/manual_commit_rewind.go:670-677:

classifySessionsForRestore already computes a four-way status by comparing the local log's last timestamp against the checkpoint's (ClassifyTimestamps, line 924):

StatusNew — no local file
StatusUnchanged — same
StatusCheckpointNewer — the checkpoint has entries the local log doesn't
StatusLocalNewer — local has entries the checkpoint doesn't (real conflict)

But the skip logic collapses all of Unchanged / CheckpointNewer / LocalNewer into "skip". So the one case that should refresh the local log — StatusCheckpointNewer — is silently skipped. That's exactly your scenario: machine A pushed an advanced checkpoint (no new code commit, just updated entire/checkpoints/v1), machine B already has the older restored log on disk, so resume sees "file exists, not New → skip" and keeps the stale transcript. claude -r then opens the old state.

Two more notes:

The fallback path resumeSingleSession (resume.go:1000-1007) has the same flaw — it only os.Stats the file and keeps it if present, with no timestamp comparison.
PromptOverwriteNewerLogs (line 962) was clearly built for this and is never called from non-test code — the machinery exists but isn't wired up.

Suggested fixes

Option A — restore when the checkpoint is strictly ahead (recommended, minimal). In the !force branch, only add to skipExisting for StatusLocalNewer (and StatusUnchanged, which is a harmless no-op). Let StatusCheckpointNewer fall through to the normal write path, with a message like Updating local session log (checkpoint has newer entries). This is safe by construction: the checkpoint is a superset, so nothing local is lost. Directly fixes the reported case, and mirror the same change into resumeSingleSession.

Option B — prompt on checkpoint-newer. Same as A but wire up PromptOverwriteNewerLogs (or a tailored confirm) before overwriting. Given there's nothing to lose when the checkpoint is ahead, I'd lean toward A (or B defaulting to yes / auto-yes under --force) to avoid friction.

Option C — also surface the real conflict. Independently of A, StatusLocalNewer is currently silently kept too. That's the genuinely dangerous case and is where PromptOverwriteNewerLogs belongs — warn and let the user choose, instead of quietly keeping. Worth doing in the same change so both "ahead" and "diverged" are handled deliberately.

Option D — robustness (optional). Timestamp comparison is a proxy. If claude -r on machine B appended even one entry, local can look newer in wall-clock while actually being a divergent branch. A sturdier check is a prefix test: if the checkpoint transcript starts with the local content, it's a clean fast-forward (overwrite freely); otherwise it's a true divergence (prompt). More work than A, but it removes the timestamp-skew failure mode if you want to harden this.

My recommendation: A + C together — restore automatically when the checkpoint is ahead, prompt (via the existing PromptOverwriteNewerLogs) when local and checkpoint have genuinely diverged, and apply the same logic to the resumeSingleSession fallback so both code paths agree.

Want me to implement A + C?

All green. Done. Here's a summary of what I implemented (A + C).

The bug

entire resume (without --force) skipped restoring any session whose local log already existed — including the case where the checkpoint had newer entries than the local copy. So after you pushed an advanced checkpoint from machine A, machine B kept its stale local log and claude -r opened the old state.

The fix

The skip decision now uses the four-way timestamp classification that was already computed but collapsed into a binary "exists → skip":

StatusNew (no local file) → restore
StatusUnchanged → keep local (no-op)
StatusCheckpointNewer → restore (A) — the checkpoint is a strict superset, nothing local is lost. Prints Checkpoint has newer entries than the local log; updating it.
StatusLocalNewer → prompt before overwriting (C) — wires up the previously-dead PromptOverwriteNewerLogs. Keeps the local log on decline, and in non-interactive mode keeps it with a use --force hint rather than erroring.

--force still overwrites unconditionally.

Files changed

strategy/manual_commit_rewind.go — extracted the per-session decision into decideRestoreActions (keeps RestoreLogsOnly under the complexity limit), added the checkpoint-newer notice in the write loop, and exported ConfirmOverwriteNewerLocalLogs (interactive prompt / non-interactive keep).
resume.go — resumeSingleSession (the legacy single-session fallback) gets the same timestamp-aware logic instead of a bare os.Stat.
Tests: new TestRestoreLogsOnly_OverwritesWhenCheckpointNewer, TestResumeSingleSession_OverwritesWhenCheckpointNewer, and rewrote the integration test that previously asserted the buggy behavior (...KeptEvenWhenCheckpointNewer → ...RefreshedWhenCheckpointNewer).
CLAUDE.md — updated the resume behavior description.

Every real resume entrypoint (entire resume, session resume, the interactive picker, the remote-metadata fallback) funnels through these two functions, so they're all covered. Per your call, trail needed no change — trail checkout only switches branches and doesn't restore session logs.

One thing worth flagging: while wrapping up I found that running mise run fmt mangled an unrelated file, cmd/entire/cli/agentimport/agentimport.go (it stripped a //nolint:ireturn directive, causing a lint error). I reverted it — it's not part of this change — but since mise run check runs fmt, that pre-existing quirk may resurface and is worth a separate look.

Confirmed the root cause of that churn: mise run lint runs golangci-lint run **--fix**, and with ireturn disabled in the default profile, --fix strips the //nolint:ireturn directive on agentimport.go as "unused" — which then makes a later run flag ireturn. It's a pre-existing repo quirk, fully unrelated to this change; I've reverted that file and left it out.

Summary of the second fix (the gap you spotted)

You were right: the A+C fix only handled "the local checkpoint is newer than the local log." But in the happy path, resume read the checkpoint straight from the local store and never checked the remote when the checkpoint ID was already resolvable locally. The actual fetch logic lived only in checkRemoteMetadata, which runs as a fallback when the local read fails — so a locally-present-but-stale checkpoint was used as-is. promoteRemoteTrackingPrimary only advanced the local ref to match an already-fetched tracking ref; it never fetched.

Fix: added refreshMetadataFromRemote(ctx) — a best-effort fetch of the metadata branch (checkpoint remote if configured, else a cheap tree-only fetch from origin) that runs before reading the checkpoint, in both resumeFromCurrentBranch (branch-arg path) and resumeByCheckpointID (picker path). It reuses the same fetch helpers as the fallback (which advance the local ref via SafelyAdvanceLocalRef, so local-ahead commits are never dropped), runs before the repo is opened so the fresh handle sees the fetched refs, and is fully best-effort — offline / no-remote / branch-not-on-origin just logs and falls back to local data.

The two fixes compose: the refresh makes the local checkpoint current; the A+C comparison then sees it as newer and refreshes the stale log.

Test: TestResume_RefreshesUpdatedCheckpointFromRemote reproduces your exact scenario — machine A commits + pushes, machine B clones and resumes (gets T1), machine A re-pushes updated checkpoint content for the same ID with no new code commit, machine B resumes again. I verified it fails without the fix (prints "Keeping existing local session log", keeps T1) and passes with it (fetches the update, "Checkpoint has newer entries", restores T2).

Final state: 6 files changed (resume.go, resume_test.go, strategy/manual_commit_rewind.go, strategy/rewind_test.go, integration_test/resume_test.go, CLAUDE.md), lint-clean, all resume/strategy unit + integration tests passing.

/simplify → 4 cleanup agents in parallel → apply the fixes

You are improving the quality of the changed code, not hunting for bugs. Review it for reuse, simplification, efficiency, and altitude issues, then fix what you find. Do not look for correctness bugs — that is what /code-review is for.

Phase 0 — Gather the diff

Run git diff @{upstream}...HEAD (or git diff main...HEAD / git diff HEAD~1 if there's no upstream) to get the unified diff under review. If there are uncommitted changes, or the range diff is empty, also run git diff HEAD and include the working-tree changes in scope — the review often runs before the commit. If a PR number, branch name, or file path was passed as an argument, review that target instead. Treat this diff as the review scope.

Phase 1 — Review (4 cleanup agents in parallel)

Launch 4 independent review agents via the Agent tool, all in a single message so they run concurrently. Pass each agent the diff and one of the four angles below. Each returns its findings with file, line, a one-line summary, and the concrete cost (what is duplicated, wasted, or harder to maintain).

Reuse

Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.

Simplification

Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.

Efficiency

Flag wasted work the diff introduces: redundant computation or repeated I/O, independent operations run sequentially, blocking work added to startup or hot paths. Also flag long-lived objects built from closures or captured environments — they keep the entire enclosing scope alive for the object's lifetime (a memory leak when that scope holds large values); prefer a class/struct that copies only the fields it needs. Name the cheaper alternative.

Altitude

Check that each change is implemented at the right depth, not as a fragile bandaid. Special cases layered on shared infrastructure are a sign the fix isn't deep enough — prefer generalizing the underlying mechanism over adding special cases.

Phase 2 — Apply the fixes

Wait for all four agents to complete, dedup findings that point at the same line or mechanism, and fix each remaining one directly. Skip any finding whose fix would change intended behavior, require changes well outside the reviewed diff, or that you judge to be a false positive — note the skip rather than arguing with it. Finish with a brief summary of what was fixed and what was skipped (or confirm the code was already clean).

2mo ago·2m

Done. Clean tree, lint passes, all tests green.

Spinner (your request)

The slow part is the new metadata fetch in refreshMetadataFromRemote. I wrapped it with the existing startSpinner helper (the same one explain/mirror use), writing to stderr per convention:

Behavior: it only animates once the fetch exceeds ~250ms (no flicker on warm runs), erases the line when done (stop(false)) so it's purely transient progress, and prints nothing on non-terminal writers — so CI/script output and the integration-test assertions are unaffected. Your example would now show a spinner during the fetch instead of a silent hang.

/simplify findings — applied

Simplification #1 — decideRestoreActions returned two parallel maps (skipExisting + statusByID) that had to stay in sync. Collapsed to a single map[string]SessionRestoreStatus; a declined local-newer conflict is now demoted to StatusUnchanged (= keep) instead of needing a second map. The write loop reads one map.
Simplification #2 — resumeSingleSession hand-wrote the "To continue this session" 2-liner in four places; the diff had only added a closure for one of them. Hoisted printResumeCmd to function scope and used it at all four sites.

/simplify findings — skipped (with reasons)

Altitude #1 / Reuse #2 (reuse getMetadataTree/attach's refreshCheckpointRefs instead of refreshMetadataFromRemote): skipped — would change behavior. refreshMetadataFromRemote deliberately (a) guards on ReadBootstrappableFromOrigin() to skip network entirely for local-only ref configs, and (b) does only a lightweight checkpoint-remote/tree-only fetch. getMetadataTree has neither — it always attempts fetches and falls back to a heavier full-blob fetch. Reusing it would make every resume do more network work and attempt fetches on local-only setups. The duplication is ~6 lines of fetch ordering, which the reuse agent itself rated low-priority. Worth a follow-up only if someone wants to factor a guarded, lightweight shared helper.
Altitude #2 / Reuse #1 (funnel resumeSingleSession through decideRestoreActions): skipped — genuinely different shapes (single-session fallback with already-read content + known agent vs. store/summary-driven multi-session loop). Both already share the real primitives (ClassifyTimestamps, ConfirmOverwriteNewerLocalLogs).
Altitude #3 / Simplification "note block" (one message at one layer): skipped — the wording differs by layer and each message sits where the write actually happens; centralizing would couple layers for a cosmetic string.
Efficiency #3 (double ReadSessionContent per session): skipped — real but pre-existing, not introduced by this diff; out of scope.

All green, tree clean. Here's what I added.

The real gap

My earlier spinner only wrapped refreshMetadataFromRemote — the metadata branch fetch. But that fetch is tree-only; the actual transcript blobs are pulled lazily by FetchBlobsByHash after that spinner stops, while reading the checkpoint and restoring the session. That on-demand fetch (which can even fall back to a full metadata fetch) was completely silent — the long hang you saw.

Fix

Added a spinner inside FetchBlobsByHash itself (git_operations.go), writing to os.Stderr:

This is at the mechanism level rather than per-call-site, so it covers every on-demand blob fetch across all the read/restore phases (and benefits explain/attach/tokens too, which share this fetcher). It's safe everywhere:

startSpinner is a no-op on non-terminal writers (it returns immediately without even spawning the goroutine), so hooks, CI, and the integration tests stay silent — confirmed by the passing integration test that exercises this exact clone-fetch path.
The 250ms initial delay means fast/cached fetches show nothing (no flicker).
stop(false) erases the transient line, and each blob fetch completes before its caller prints, so there's no interleaving with resume's stdout output.

Net result

Resume now shows progress across the whole remote window:

⣾ Fetching latest checkpoint data from remote — during the metadata-branch fetch (refreshMetadataFromRemote)
⣾ Downloading checkpoint data from remote — during the transcript/blob fetch (FetchBlobsByHash)

then the normal restore output. No more silent hang.

/simplify → 4 cleanup agents in parallel → apply the fixes

Phase 0 — Gather the diff

Phase 1 — Review (4 cleanup agents in parallel)

Reuse

Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.

Simplification

Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.

Efficiency

Altitude

Phase 2 — Apply the fixes

2mo ago·1m

/simplify complete. The spinner code was largely clean — three of four agents found nothing actionable. Summary:

Fixed

Altitude #1 (the real one) — FetchBlobsByHash is the global strategy's blob fetcher (config.go → GetStrategy()), so its spinner wasn't actually scoped to resume/explain — it also fires from git hooks, doctor, clean, reset, rewind, and lifecycle. Its only gate was IsTerminalWriter(os.Stderr), which ignores the agent sentinels (GEMINI_CLI, COPILOT_CLI, GIT_TERMINAL_PROMPT=0), CI, etc. that interactive.CanPromptInteractively() honors — so an agent-invoked hook over a pty could animate the spinner into the agent's output. Gated the spinner on interactive.CanPromptInteractively() (a one-line tightening; the defer inside the if is function-scoped so it still fires at return) and rewrote the misleading "resume/explain" comment to state the real blast radius.
Simplification copy note — the two spinners showed near-identical messages ("Fetching latest checkpoint data…" / "Downloading checkpoint data…"). Renamed the metadata-graph fetch to "Checking remote for newer checkpoints" so the two phases read distinctly (check → download).

Skipped (with reasons)

Altitude #1 full fix (thread a writer/progress-sink through the BlobFetchFunc seam so the store reports to the owning command's stderr): the correct deep fix, but a package-wide refactor across checkpoint and all 6 fetcher-wiring sites — out of scope. The interactive gate is the agreed in-scope mitigation.
Efficiency #2 (per-blob spinner flicker when a read path iterates FetchingTree.File() without PreFetch): real but low-severity, on-terminal only; the agent itself recommended leaving it. The 250ms draw delay plus the new interactive gate keep it benign. If flicker is ever observed, the fix is to ensure callers PreFetch-batch — noted, not done.
Reuse, other Simplification, Altitude #2/#3: confirmed clean (correct startSpinner reuse; os.Stderr forced by the writer-less BlobFetchFunc signature; two-phase progress is legitimate granularity).

Final tree: 7 files, lint clean, resume unit + integration tests green.

Done. Codex's review pass found one real issue, I fixed it, and a second Codex pass confirmed it's resolved with no new findings.

What Codex found

[P2] Checkpoint-newer overwrites weren't divergence-safe. StatusCheckpointNewer overwrote the local log on timestamp ordering alone — ClassifyTimestamps never verified the checkpoint was actually a superset. Two machines continuing the same session independently (the remote one with a newer last-timestamp) would silently lose local-only entries without --force. This was the "Option D" robustness gap we deferred when you first chose A+C — and my code comments even asserted "strict superset, nothing local is lost" without verifying it.

Fix (per your "prefix guard + prompt on divergence" choice)

Added TranscriptIsCleanContinuation(local, checkpoint) — line-based check that the local log is a prefix of the checkpoint (append-only JSONL ⇒ overwriting only appends, loses nothing).
New StatusDiverged status: StatusCheckpointNewer is now only assigned when the local log is a clean prefix; otherwise it's StatusDiverged.
StatusDiverged routes through the same conflict path as StatusLocalNewer — prompts before overwriting, keeps the local log on decline / when non-interactive. Wired into both RestoreLogsOnly/decideRestoreActions (multi-session) and resumeSingleSession (fallback).
Updated the conflict prompt/keep wording to cover both "newer" and "diverged" ("entries the checkpoint doesn't have"), and StatusToText.

Tests

Converted the two "overwrite when checkpoint newer" tests to realistic clean-continuation fixtures (checkpoint = local + appended line) so they still verify seamless refresh.
Added divergence tests at both layers (TestRestoreLogsOnly_DivergedKeptWithoutForce, TestResumeSingleSession_DivergedKeptWithoutForce).
Repurposed the integration test to TestResume_DivergedLocalLog_KeptWithoutForce, and updated TestResume_RefreshesUpdatedCheckpointFromRemote to push a genuine clean continuation (restored log + appended entry) so it still exercises the end-to-end refresh.
Updated CLAUDE.md to document the prefix/divergence semantics + the remote refresh.

Lint clean, all unit + resume integration tests green, and the agentimport.go lint-autofix artifact reverted again. One known tradeoff stands (you accepted it): OPF re-redaction could in principle change earlier checkpoint bytes and trip a false "diverged" prompt on an otherwise-clean continuation — erring toward a prompt rather than silent data loss.

Done.

Committed

dbd6f31c5 on soph/resume-improve — one cohesive commit (7 files, +622/−50):

resume: refresh stale logs, fetch remote checkpoints, show progress — refresh on clean checkpoint-newer continuation, divergence guard (StatusDiverged → prompt/keep), remote metadata refresh before read, gated progress spinners, plus the internal cleanups (single decision map, shared conflict prompt, deduped resume-command print).

Trail opened

entire trail create pushed the branch to origin and created trail #679:

URL: https://entire.io/gh/entireio/cli/trails/679
ID: 019f0a14-0575-7a1f-8bf7-b6ce39dcf750
Branch: soph/resume-improve

One thing worth flagging: I committed all the work as a single commit since it's one tightly-coupled feature (the refresh/divergence logic, spinners, tests, and docs all depend on each other). If you'd prefer it split into logical commits (e.g. core keep/overwrite/divergence vs. remote-refresh vs. progress-spinners), say the word and I can reset --soft and restage into separate commits.

Checked and addressed. PR #1547 had one review (Copilot, no general/human comments yet) with 3 inline comments — all valid. I fixed all three and pushed.

The 3 comments and fixes

#	Comment	Fix
1	Privacy (`manual_commit_rewind.go`): non-interactive keep message echoed `info.Prompt` (user content) into stderr/CI logs	Identify the conflict by session ID only in the non-interactive branch — never echo prompt text
2	Memory (`TranscriptIsCleanContinuation`): `strings.Split(string(...))` copied both full transcripts	Rewrote to walk lines in place with `bytes.IndexByte` (no allocation, no `bufio` line-length cap); added a focused table test for the edge cases
3	Consistency (`resume.go`): `refreshMetadataFromRemote`'s spinner wasn't gated on `CanPromptInteractively()` like `FetchBlobsByHash`'s	Gated it the same way, so agent/CI invocations stay silent even on a pty

All three were genuine — #1 enforces the project's "don't log user content" rule, #2 is a real allocation win on large transcripts, #3 closes the exact pty-non-interactive gap the earlier altitude review raised (I'd only applied the gate to one of the two spinners).

Pushed as 9a2b3b58a → PR #1547 updated; lint clean, all resume unit + integration tests green, new TestTranscriptIsCleanContinuation passing.

Want me to reply to the Copilot review threads on the PR (resolving each with a note on the fix), or leave them as-is for the push to speak for itself?