Review Checkpoint Metadata Fix Changes

Reviewed trail #1320 (krk/doctor-shrink-checkpoint-metadata, 3 commits, +2021 lines). I read the diff myself and ran a second review pass in parallel; where the two agreed I re-verified against source or by measurement.

Overall: the design is sound and the write-up is unusually good — the cap/scan/repair split is right, --force correctly excluded, contentReachable comparing trees rather than hashes is a genuinely subtle catch, and it reuses readV1Tip/atomicSetV1Ref/SafelyAdvanceLocalRef from the OPF rewrite instead of re-inventing them. But the always-on scan has two problems that hit the default configuration, and I could reproduce both in this repo.

Blocking

1. The scan hard-fails on any partial clone, and takes entire doctor down with it — metadata_shrink.go:307

repo.BlobObject(hash) is a bare go-git read, and go-git does not lazy-fetch promisor objects. Measured in this very worktree (filtered_fetches: true, remote.origin.partialclonefilter=blob:none): 3,815 of 15,680 distinct metadata.json blobs on entire/checkpoints/v1 are promisor-absent. ScanOversizedCheckpointMetadata returned:

That propagates findOversizedMetadataBlobs → ScanOversizedCheckpointMetadata (:135) → checkOversizedCheckpointMetadata → runSessionsFix, which prints Error: checkpoint metadata size check failed and sets finalErr — so doctor exits non-zero on a healthy repo, on the one command you run when things are broken. Any git clone --filter=blob:none hits this too.

The codebase already knows this: checkpoint/fetching_tree.go:251 documents exactly this hazard and uses GIT_NO_LAZY_FETCH=1 + cat-file --batch-check. The remote half of this scan is already fail-soft via scan.RemoteErr; the local half needs the same — skip-and-report a missing blob, never abort.

2. The scan costs 9 minutes on a real branch, on every doctor run — metadata_shrink.go:281

PrimaryIsRefs is false unless checkpoints.primary.type is explicitly git-refs, so the default (git-branch) path always runs a full go-git history walk before any network decision. Measured on this repo's v1 branch (12,901 commits, walk made blob-tolerant so it could finish):

The last one is the fast path worth having: if no blob in the whole object store exceeds the threshold, no blob on v1 does either, so doctor can answer OK in 0.4s and only pay the attribution walk when something is actually oversized. It also solves finding 1 for free — git reports missing instead of erroring.

3. Remote-only bloat on a dedicated checkpoint_remote is reported as healthy — metadata_shrink.go:169

The DedicatedCheckpointRemote branch calls clearRemote(), which nils scan.Remote. If the bloat is remote-only, Empty() now returns true, and doctor_metadata_size.go:58 checks scan.Empty() before scan.DedicatedCheckpointRemote — so it prints ✓ Checkpoint metadata size: OK and the "rewrite by hand" advisory is unreachable. shrink-checkpoint-metadata likewise says "nothing to repair". Set the flag without clearing Local/Remote; clear only RemoteTip/RemoteAhead if the tip must not be acted on.

Should fix

• metadata_shrink.go:505 — the push runs even when rw.stillOversized still holds blobs over 100 MiB. GitHub rejects the pack, the user gets PushFailedError and is told to "re-run once the remote is reachable" — advice that fails identically forever, because reachability was never the problem. Gate the push on nothing remaining above gitHubBlobHardLimit.
• metadata_shrink.go:484 — BlobsShrunk/StillOversized are assigned only after the remote pass, so an error in rewriteHistory(RemoteTip) or reconcileRewrittenTips returns BlobsShrunk == 0 after the local ref was already moved. applyCheckpointMetadataShrink only special-cases *PushFailedError, so the user gets a bare error with no hint that their branch was rewritten.
• doctor_metadata_size.go:168 — an election error collapses to remoteName = "", which every downstream message renders as "no checkpoint sync remote is configured". That is the exact misreading the function's own doc comment says must not happen ("must not read as 'no remote'").
• metadata_shrink.go:174 — a settings-load failure sets PushDisabled = true, later narrated as "checkpoint pushing is disabled in settings". Fail-safe is right; the reason string is a false statement of cause.
• persistent.go:1795 / fsstore.go:332 — CapPromptAttributions(context.Background(), …). logging.log falls back to slog.Default() when the ctx carries no logger, and nothing calls slog.SetDefault in production — so the drop never reaches .entire/logs (contradicting the function's own doc comment) and instead prints a WARN to the hook's stderr. Both sites have a real ctx within reach; persistent.go:737 already uses it.
• doctor_metadata_size.go:75 — the guard is !CanPromptInteractively(), not !force, so entire doctor --force on a TTY still prompts, against the flag's own help ("Auto-fix all issues without prompting"). The behaviour looks deliberate; the help text should say so. TestRunSessionsFix_Force_DoesNotRewriteCheckpointBranch passes only because go test is non-interactive.

Nits

• humanBytes (doctor_metadata_size.go:282) duplicates humanizeBytes (explain.go:1469) in the same package — same 1024 logic, different suffixes. The new labels are the correct ones; worth fixing the old function rather than adding a second.
• settings.LoadForWorktreeRoot duplicates the existing settings.WithWorktreeRoot(ctx, root) + Load(ctx) — Load's first statement is byte-for-byte what the wrapper does, and the ctx-carried form is the one that propagates.
• rewriteCommit copies Encoding/ExtraHeaders but drops object.Commit.MergeTag, which go-git does re-emit (commit.go:384). Near-zero impact on a linear branch, but inconsistent with the care taken two lines above.
• remote.FetchOptions.Dir/PushOptions.Dir are left empty, so both subprocesses run against the process cwd — latent, since the code deliberately reads settings for the passed repo rather than the cwd.
• strings.Contains(output, "couldn't find remote ref") is locale-dependent; this repo pins LC_ALL=C in three other places for exactly that reason, and remote.newCommand does not.
• rewriteBlob collapses "unparseable JSON" and "no such field" into one silent path, discarding the parse error, then reports the blob as "large for another reason".

No test covers a history with an unreadable blob, which is why finding 1 got through.

Want me to post any of this to the trail? I haven't written anything to it — and I can publish the full review as a shareable page if it's going to the author.

No new changes to review.

Branch: krk/doctor-shrink-checkpoint-metadata is still at d9c31d994 — same head I reviewed, confirmed against git ls-remote (not a cached gh view). No new commits, no force-push.

Trail #1320: updated_at moved 09-13 16:30Z → 09-15 09:17Z, but nothing visible changed — 0 findings (any status, any freshness, including dismissed), 0 approvals, same description, still open / reviewing, unassigned reviewers. That looks like a background gate or re-index rather than author or reviewer activity.

What did move: main gained 15 commits under the branch. The only file both sides touch is CLAUDE.md, and git merge-tree reports a clean merge — so no rebase is needed and my review still applies verbatim.

So all fifteen findings from yesterday stand unaddressed, including the three blocking ones: the promisor hard-failure that takes entire doctor non-zero on any partial clone (metadata_shrink.go:307), the 9m13s full-history walk on every doctor run (:281), and the remote-only bloat reported as healthy under a dedicated checkpoint_remote (:169).

Still happy to post the review to the trail or publish it as a shareable page if you want the author to see it — I haven't written anything to the trail.

Still no change to the code under review — but the branch has gone stale against main, and that is new since yesterday.

Branch: unchanged at d9c31d994 (confirmed via git ls-remote). Same three commits, same diff.

Trail #1320: updated_at 2026-09-15T11:18Z — moved ~2h after my last check, but still 0 findings (any status/freshness, including dismissed), 0 approvals, no reviewers assigned, still open/reviewing. It doesn't appear in the 12 most recently-updated trails, so nothing has touched it in ~13h.

What's new: the branch now conflicts with main. Yesterday main was 15 commits ahead and git merge-tree merged clean. It's now 105 commits ahead and conflicts:

The cause is trivial to resolve — one commit on main (4ba7510a09) edited the doctor Long help text, changing the summary-provider example from opencode to factoryai-droid, in the same block this branch renumbered checks 2→7 when it inserted the new oversized-metadata check. Same hunk, different edits. CLAUDE.md still auto-merges.

So the state is: unreviewed, unapproved, and now needs a rebase. All fifteen findings stand, including the three blocking ones — the promisor hard-failure that takes entire doctor non-zero on any partial clone (metadata_shrink.go:307), the 9m13s full-history walk on every doctor run (:281), and remote-only bloat reported as healthy under a dedicated checkpoint_remote (:169).

Given it's sat two days with no reviewer, posting the review to the trail is probably the thing that unblocks it. Say the word and I'll post it — I still haven't written anything there.

Replied to cli-90. Two things from that check are worth your attention, since they change what I told you on the 16th:

The trail I reviewed is GitHub PR entireio/cli#2404 — I hadn't connected the two before; my earlier updates only tracked the trail.

The branch moved, but the code didn't. Head is now 878a05e18 (was d9c31d994). The only new commit is a merge of origin/main — no non-merge commits from the author, and metadata_shrink.go, doctor_metadata_size.go and prompt_attributions_cap.go are byte-identical to what I reviewed. So the merge conflict I flagged is resolved (mergeable: MERGEABLE) and all fifteen findings stand untouched.

Current PR state: OPEN, not draft, REVIEW_REQUIRED, mergeStateStatus: BLOCKED, updated 09-17. 15 checks pass, 1 fails: Entire Gates, which points back at trail 1320 — the same trail showing 0 findings and 0 approvals. So the gate is almost certainly failing on the absent review, not on anything in the code.

That last point is the practical one: the PR is blocked on a review that doesn't exist, and I'm holding one that identifies three blocking bugs — the promisor hard-failure that takes entire doctor non-zero on any partial clone, the 9m13s full-history walk on every doctor run, and remote-only bloat reported as healthy under a dedicated checkpoint_remote. Want me to post it to the trail or the PR?

Here's where it actually stands — and the world moved under this PR while it sat.

PR #2404 is stalled and now effectively stale. Open since 09-13, untouched since 09-17, still REVIEW_REQUIRED with zero reviews and zero trail findings. It is 2,338 commits behind main and conflicts in four files: persistent.go, fsstore.go, doctor.go, docs/development/checkpoint-implementation.md. The author has no commits on main since Oct 1.

Half the PR is now moot. The shadow-branch/attribution removal (#2692, merged 10-08) deleted the producer: PromptAttributionsJSON no longer exists outside tests, and the field was renamed to Metadata.LegacyPromptAttributions. So the 4 MiB write-time cap — the PR's "forward prevention" — guards a write path nothing takes anymore. Those two conflicts in persistent.go/fsstore.go are semantic, not textual: the patch targets code that no longer exists in that shape.

But two things survive, and one of them got more attractive.

The repair is still real: existing repos still carry 70–106 MiB metadata.json blobs frozen in entire/checkpoints/v1 history, and that still makes the branch unpushable to GitHub. History doesn't age out, and git-branch is still the default backend (PrimaryIsRefs requires explicit config).

And persistent.go:671-675 on main still does this on every session-slot overwrite:

It reads the bloat back out of the old blob and copies it into the new one, forever. Nothing writes the field, nothing reads it — it's pure ballast being propagated indefinitely. Capping there is now a ~3-line change that is strictly safer than it was in September, because the data is unambiguously dead.

What I'd do

1. Post the review to the trail (my recommendation, and cheap). The sole failing check is Entire Gates, which points at trail 1320 — it's almost certainly failing on the absent review, not on the code. Right now the PR is blocked on a review that doesn't exist while I'm holding one with three blocking findings. Posting it either gets the author moving or gives whoever triages it the grounds to close it.

2. Salvage the small win as its own PR — cap LegacyPromptAttributions in that carry-forward. No conflict, no dependency on #2404, independently useful, and it's the one piece of the original work that today's main still needs.

3. Leave the doctor repair unless someone has actually hit it recently. Reviving it isn't a rebase, it's a rewrite of the scan: the promisor hard-failure (metadata_shrink.go:307) and the 9m13s full-history walk both need replacing with the git cat-file --batch-all-objects --batch-check approach I measured at 0.42s. And the cost/benefit got worse — paying a multi-minute walk on every entire doctor to detect a legacy condition most repos no longer accumulate is a poor trade. Worth doing if the unpushable-branch problem is live for someone; not worth it speculatively.

The deciding question I can't answer from here is whether anyone is still hitting the unpushable v1 branch. If you know of a repo that is, option 3 moves up.

Want me to post the review to trail 1320, or start on the carry-forward cap?

I have the change complete and verified locally; the full CI suite is running. Summary of what's in the branch while that finishes:

soph/cap-legacy-prompt-attributions (off e2b69f168a, current main)

The scope grew once I traced it properly. My original read — one carry-forward site in persistent.go, one in fsstore.go — was incomplete. The git store has three places that marshal a session metadata.json into a blob, and updateSessionMetadata's three mutators sit behind one of them. Capping only where the field is read forward would have left a summary backfill, a transcript finalize, a skill-events replacement, and a compact-transcript marker update each still re-committing an oversized blob, because every one of those decodes the whole Metadata and re-encodes it.

So the cap went into a new choke point, treeWriter.writeSessionMetadataBlob, that all three routes now go through. That cost threading ctx into updateSessionMetadata, replaceSkillEvents and setCompactTranscriptStart — every call site already had one in scope, so no context.Background() anywhere, which is the defect I flagged in #2404's version of this.

The measurements that set the threshold. Over this repo's own v1 history — 11,929 metadata.json blobs readable in this clone, only 11 of them over 64 KiB:

4 MiB clears every healthy record by better than 2x and fires only on the 70–106 MiB pathological shape. initial_attribution is left uncapped — one snapshot per session rather than one per prompt, and at 254 bytes it isn't a bloat vector.

Tests, each mutation-checked rather than trusted:

• TestCapLegacyPromptAttributions — pass-through, boundary, drop. Narrowing <= to < makes the at-the-cap case fail, so the boundary assertion is real.
• TestRewritesDropOversizedLegacyPromptAttributions — a table over all four rewrite shapes. With the cap reverted, the three re-emission subtests fail and the untouched-slot subtest still passes, which proves it discriminates per path rather than passing wholesale.
• TestStore_RewriteDropsOversizedLegacyPromptAttributions — the same for fsstore.

The fourth subtest pins a deliberate non-behaviour: attaching a second session leaves the first slot's blob untouched, so there's nothing to re-emit and nothing to cap. That slot's field goes when the slot is next written.

mise run fmt and mise run lint are clean (0 issues, run after fmt); the affected packages passed in full before the choke-point refactor and I'll confirm the final tree when test:ci lands.

Two things I have not done, both needing your say-so: nothing is committed yet, and nothing is pushed. I'll report the CI result and then commit and open the PR unless you'd rather look at the diff first.

Neither mirror flake is in checkpoint/remote, so I don't yet have a named cause. Waiting on the second full run to name the failing test rather than guess.

Where things stand:

The change is complete and self-consistent. mise run fmt and mise run lint clean, and the targeted checkpoint packages pass repeatedly — checkpoint 62s, fsstore 2.6s, api/checkpoint 0.3s, and checkpoint/remote passes in isolation at 35s.

Two test failures seen, neither yet attributed to the change:

TestOpenCodeSeedRepoPlantsDeps in e2e/agents — confirmed pre-existing. I stashed my work, ran it against pristine origin/main, and it failed identically (node_modules is not a symlink into the shared tree). Unrelated to checkpoint metadata; it's about opencode plugin-dep seeding.

cmd/entire/cli/checkpoint/remote — failed once in a full-suite run at 84s, having passed in mise run test:ci and in two isolated runs. That pattern is the signature of a load-dependent flake, and this repo has three registered ones of exactly that shape, but all three live in cmd/entire/cli/, not this package, so I can't claim a match. I'm re-running the full suite with output captured to get the actual test name instead of asserting "probably a flake".

I won't commit or push until that resolves. If it names a test my change could plausibly touch, I'll chase it; if it's green or names something unrelated, I'll report and then ask before pushing.

One correction to my previous message: I reported test:ci as pending and implied I'd confirm a pass. It completed and failed — the exit code 0 I was notified with came from tail at the end of the pipeline, not from the suite. The only failure in it was the pre-existing opencode one, but the suite did not pass, and I should have checked the output rather than the exit status.

Green across the board. Committing and opening the PR: