Prevent Unauthorized Metadata Ref Creation

All 5 angle finders are running in parallel. Waiting for them to complete.

Angle D complete. Waiting on remaining 4 angles.

Angle A complete. Awaiting B, C, E.

Angle B complete. Awaiting C and E.

Angle C complete. Note: Angle C mentions uncommitted changes in metadata_reconcile.go — but initial git status was clean. I'll verify after Angle E returns.

Here are the verified findings, ranked most-severe first.

Workspace note: the worktree is currently in a merge state from a concurrent git merge origin/main you ran during the review (commit 5c0219a7af mirror reconciled metadata refs was added — it closes the gap in ReconcileDisconnectedMetadataBranch). cmd/entire/cli/dispatch/mode_local.go shows UU (merge conflict). Findings below reflect HEAD (a56d43642d) including that commit; review scope is git diff origin/main...HEAD.

Summary

13 findings ranked by severity. The dominant cluster is v1.1 read-side staleness on fresh clones, post-opt-in, and after external git fetch — caused by removing read-time sync without seeding the v1.1 mirror from the bootstrap paths (EnsureMetadataBranch, fetchMetadataBranchIfMissing, DeleteOrphanedCheckpoints, promoteRemoteTrackingMetadataBranch). The single user-facing recovery hint (git fetch entire/checkpoints/v1:entire/checkpoints/v1) is itself broken for v1.1 mode. Findings 7–13 are narrower (ctx expiry, error misclassification, future topology hazards, doc/test gaps).

Workspace note again: please resolve the cmd/entire/cli/dispatch/mode_local.go merge conflict left by the in-progress git merge origin/main.

Re-verified against current HEAD (a56d43642d). The branch state didn't shift much since my last check — the new merge from origin/main was resolved but no new functional changes landed. Findings still verify against the current source.

Filtering through your stated intent — "mirror advances after entire-managed v1 writes/fetches; missing paths should be visible, not hidden" — the read-staleness findings (#3, #5, #11–13) drop out as accepted. What remains are entire-managed v1 writes that don't mirror plus silent failures inside the mirror code itself — both of which run directly against the stated goal.

Findings that remain under the PR intent

Dropped from earlier list under your intent

• Stale v1.1 reads after git fetch entire/checkpoints/v1:entire/checkpoints/v1 (was #3): external git op; missing-path exposure is intended.
• fetchMetadataBranchIfMissing fast-paths out without mirror (was #5): no fetch happens by design; mirror gap closes when the user-initiated v1 write fires through a hook. Mostly subsumed by #1 if entire enable mirrors.
• BestEffort re-resolves CommittedRefs (was #11): perf/concurrency only, doesn't fight the intent.
• cleanup.go:172 uses NewGitStore for committed read (was #12) and getCheckpointLog uses write store (was #13): forward-compat hazards on a topology flip, no current user impact.
• Force-set rewrites mirror without rewind check: tests assert this is intentional.

Suggested ordering for fixes

1. Cluster #1, #2, #3 (entire-managed v1 writes that don't mirror) — directly close the missed paths.
2. #5 (misclassification) and #4 (silent nil return) — small mirror-code hygiene; align logging with intent.
3. #6 (ctx propagation) and #7 (silent branch-name skip) — defense in depth against silent skips.
4. #8 (docs) — keep CLAUDE.md/sessions-and-checkpoints.md in sync once the above land.

Test passes (all 5 EnsureMetadataBranch subtests, including the new one). The pre-existing lint diagnostics flagged are in unrelated functions (lines 247/1126/1522/1534, not the lines I touched).

Fix 1 of 8 — EnsureMetadataBranch now mirrors after v1 writes

Files changed

• cmd/entire/cli/strategy/common.go — added MirrorCommittedMetadataRefBestEffort(context.Background(), repo) after each of the three SetReference paths that advance the local v1 branch:
  • empty-orphan → remote tip (~line 478)
  • local-missing → create-from-remote (~line 504)
  • no-remote → create empty orphan (~line 563)
• cmd/entire/cli/strategy/common_test.go — added TestEnsureMetadataBranch_MirrorsV11WhenSeedingFromRemote (not parallel, uses t.Chdir so settings.Load picks up the v1.1 opt-in).

Decisions

• Used context.Background() rather than threading a new ctx parameter. Rationale: the function already uses context.Background() internally (existing Debug call), EnsureMetadataBranch is part of entire enable setup where caller cancellation isn't load-bearing, and changing the signature would touch ~9 test sites + the one production caller for no behavioral gain.
• Mirror call placed after SetReference but before the stdout/stderr print so a mirror failure can't desync without at least a Warn log preceding the "Created/Updated" message.
• Mirror call added on the orphan-creation path too, even though the orphan tree is empty — keeps the invariant clean ("any v1 write mirrors") rather than having a quiet exception.

Verification: go test -run TestEnsureMetadataBranch -count=1 ./cmd/entire/cli/strategy/ → ok. Failing test reproduced first, then passes after the fix.

Ready for review. Reply when you'd like me to move on to finding #2 (promoteRemoteTrackingMetadataBranch + the v1-only gate in resume.go).

Honest answer: no good principled reason. I picked it to avoid widening the signature change, and rationalized it by pointing at the existing context.Background() Debug log at line 472 — but that line is itself a pre-existing wart, not a pattern to follow. Justifying a new wart by an old one is weak.

Threading ctx properly is the right call:

• EnsureSetup (the one production caller, common.go:79) already has ctx — trivial to pass through.
• MirrorCommittedMetadataRefBestEffort → ResolveCommittedRefs → settings.Load is where the ctx actually does work (cancellation, deadlines, future trace correlation). Background severs all of that.
• Test sites that need updating: ~9 callers across common_test.go and metadata_reconcile_test.go, each gets context.Background() or t.Context(). Mechanical.

Want me to redo it with a ctx context.Context parameter on EnsureMetadataBranch? I'd also clean up the existing context.Background() Debug log on the same pass so the wart goes away rather than getting duplicated.

Counting:

Strictly required (16 edits):

• 1 signature change at common.go:456
• 1 production caller at common.go:79 (EnsureSetup already has ctx)
• 14 test call sites: resume_test.go (2), metadata_reconcile_test.go (4), common_test.go (8 — includes the one I just added)

Each test site is the same mechanical edit: EnsureMetadataBranch(repo) → EnsureMetadataBranch(ctx, repo) or EnsureMetadataBranch(t.Context(), repo).

Optional cleanup in the same pass (2 more edits):

• common.go:487 — pre-existing context.Background() in a Debug log
• common.go:548 — pre-existing context.Background() passed to checkpoint.SignCommitBestEffort

Both are inside EnsureMetadataBranch; with ctx available, swap them too so the wart is fully gone rather than half-fixed.

Total: ~16 mechanical edits, or 18 with the cleanup. No call-site logic changes — every test already has either a context.Background() or t.Context() they can hand over.

Want me to proceed with threading ctx (including the pre-existing cleanup)?

All green. Summary of the ctx-threading pass:

Fix 1 (revised) — EnsureMetadataBranch now takes ctx

Files changed (4)

• cmd/entire/cli/strategy/common.go
  • Signature: EnsureMetadataBranch(ctx context.Context, repo *git.Repository) error
  • Caller in EnsureSetup (line 79) now passes ctx
  • Inside the function, 5 context.Background() uses replaced with the passed ctx:
    • 3 new MirrorCommittedMetadataRefBestEffort calls
    • 1 pre-existing logging.Debug (cleanup of the wart I'd cited earlier)
    • 1 pre-existing checkpoint.SignCommitBestEffort (same cleanup)
• cmd/entire/cli/strategy/common_test.go — 8 callers → t.Context()
• cmd/entire/cli/strategy/metadata_reconcile_test.go — 4 callers → t.Context()
• cmd/entire/cli/resume_test.go — 2 callers → t.Context()

Verification

• go build ./... — clean
• go test -run TestEnsureMetadataBranch ./cmd/entire/cli/strategy/ — ok
• go test -run TestReconcile ./cmd/entire/cli/strategy/ — ok
• go test -run TestResume ./cmd/entire/cli/ — ok

Lint diagnostics emitted by the harness are all pre-existing and in unrelated files (explain_test.go, search_tui.go, activity_render.go, plus earlier ones at common.go:247/1126/1522/1534) — none in the lines I touched.

Ready for review. Reply when you'd like me to proceed to finding #2 (promoteRemoteTrackingMetadataBranch doesn't mirror + the v1-only gate at resume.go:193).

All resume tests pass.

Fix 2 of 8 — promoteRemoteTrackingMetadataBranch mirrors + v1-only gate removed

Files changed (2)

• cmd/entire/cli/resume.go
  • promoteRemoteTrackingMetadataBranch (line ~714): after a successful SafelyAdvanceLocalRef, calls strategy.MirrorCommittedMetadataRefBestEffort(ctx, repo). Also added an early return on the SafelyAdvanceLocalRef error so we don't mirror a ref that didn't advance.
  • resumeFromCurrentBranch (line 193): removed the if store.CommittedReadRef() == v1 branch { promote } gate; promote always fires now. Added a one-line comment explaining why (v1 is source of truth, promote mirrors when v1.1 is configured).
• cmd/entire/cli/resume_test.go
  • New test TestPromoteRemoteTrackingMetadataBranch_MirrorsV11 (not parallel, t.Chdir): seeds refs/remotes/origin/entire/checkpoints/v1, deletes local v1 and the mirror, calls promote, asserts both refs/heads/entire/checkpoints/v1 and refs/entire/checkpoints/v1.1 end up at the origin hash.
  • Added assert + require imports.

Decisions

• The existing TestResumeFromCurrentBranch_V11DoesNotSeedFromV1 still passes unchanged. Its scenario sets up local v1 with metadata but no origin/entire/checkpoints/v1 remote-tracking ref, so the now-always-invoked promote no-ops and the test's assertions about the "Try: entire explain" hint still hold.
• The early return after the SafelyAdvanceLocalRef error is new (previously the function fell through and would have called mirror anyway if the call had been added later — better to skip mirror when the advance itself failed).

Verification

• go test -run 'TestPromoteRemoteTrackingMetadataBranch_MirrorsV11|TestResumeFromCurrentBranch_V11DoesNotSeedFromV1' -count=1 ./cmd/entire/cli/ → ok
• go test -run TestResume -count=1 ./cmd/entire/cli/ → ok

Pre-existing lint diagnostics in search_tui.go are unrelated.

Ready for review.

Reverted. Working tree is clean again (zero diff vs HEAD) — both the gate restoration and the removal of the mirror call inside promoteRemoteTrackingMetadataBranch, and the test/imports rolled back. go build ./... clean, TestResume ok.

Marking finding #2 as not-to-fix: the gate is intentional protection, and the mirror staying stale in this path is the kind of "visible missing path" the PR is preserving on purpose.

Quick gut-check on the other open findings so I don't repeat the same misread:

• Fix #1 (EnsureMetadataBranch mirrors) — already shipped. It only writes the v1.1 ref when v1.1 is opted in (HasMirror() gates it inside MirrorCommittedMetadataRefBestEffort). Want me to revert that too, or is fix #1 fine because the user has explicitly opted in by setting checkpoints_version: "1.1" before running entire enable?
• Finding #4 (DeleteOrphanedCheckpoints doesn't mirror) — same shape as #1: only mirrors when v1.1 opted in. Probably acceptable, but I want to check first.
• Findings #6 (FetchMetadataBranch swallows OpenRepository error), #7 (ctx-expiry silently skips mirror), #8 (ErrReferenceNotFound misclassification) — these are silent-skip bugs inside the mirror code itself. They only fire when v1.1 is already opted in. Should be safe to fix, but confirm.
• Findings #9 (docs) and #10 (mirrorSyncedMetadataBranch silent skip) — non-behavioral / future-topology.

Two questions before I touch anything else:

1. Should I revert fix #1, or is "opted into v1.1 in settings" enough consent for the mirror to be created at entire enable time?
2. For the remaining findings — are silent-skip-in-mirror-code fixes (#6/#7/#8) OK to land, since they only execute when v1.1 is already opted in?

Test passes. Full strategy package tests green.

Fix 4 of 8 — DeleteOrphanedCheckpoints mirrors after v1 cleanup commit

Files changed (2)

• cmd/entire/cli/strategy/cleanup.go — added MirrorCommittedMetadataRefBestEffort(ctx, repo) after the SetReference that advances the v1 branch with the cleanup commit (line ~336).
• cmd/entire/cli/strategy/v1_custom_ref_mirror_test.go — added TestDeleteOrphanedCheckpoints_MirrorsV1CustomRefWhenEnabled (not parallel, t.Chdir), matching the existing TestCondenseSession_MirrorsV1CustomRefWhenEnabled / REDACTED shape.

Setting-check parity with fix #1

• The mirror call goes through MirrorCommittedMetadataRefBestEffort → ResolveCommittedRefs(ctx) → settings.MirrorsToV1CustomRef(ctx) → settings.Load(ctx). Same setting-gated path. If the user toggles checkpoints_version off, the next entire clean mirror call is a no-op and the (now-orphaned) v1.1 ref stops being touched.

Verification

• Failing test reproduced first, then passes.
• Full go test ./cmd/entire/cli/strategy/ → ok (20.9s).

Ready for review. Reply when you'd like me to commit and continue to fix #6 (FetchMetadataBranch swallows OpenRepository error).

All FetchMetadataBranch tests pass.

Fix 6 of 8 — FetchMetadataBranch surfaces post-promote mirror failure

File changed (1)

• cmd/entire/cli/strategy/checkpoint_remote.go — replaced the logging.Warn(...) + return nil after a failed OpenRepository with return fmt.Errorf("metadata fetch succeeded but failed to open repo to mirror committed-ref: %w", err).

Behavior change

• Before: fetch + promote ran successfully, but if the post-promote OpenRepository failed, the function logged a Warn and reported success to the caller. The mirror was silently stale until a later code path happened to mirror.
• After: the same scenario returns an error wrapping the OpenRepository failure. The error message preserves the "fetch succeeded" context so the caller (and operators reading the log) can distinguish this from a genuine fetch failure.

Why no new test

• TDD here would need to inject an OpenRepository failure between PromoteTmpRefSafely and the mirror call. The existing code calls OpenRepository(ctx) directly with no seam, and CLAUDE.md forbids adding mutable function-variable seams just for tests. The realistic ways to make OpenRepository fail (locked .git, removed dir, perms flip) all also break the fetch/promote earlier in the function.
• The existing TestFetchMetadataBranch_MirrorsV11Ref covers the happy path and still passes — confirming the change doesn't regress the normal case.
• The behavioral change is a 4-line → 1-line shift on an explicit error branch; visual review is reliable here.

Caller impact

• resume.checkRemoteMetadata (resume.go:632): if fetchErr := strategy.FetchMetadataBranch(...); fetchErr == nil { ... }. With this change the caller skips the read-after-fetch path when mirror failed, then falls through to promoteRemoteTrackingMetadataBranch / FetchMetadataBranch(ctx) (no-URL variant, which doesn't hit this path) — so the error doesn't dead-end the user.
• FetchMetadataFromCheckpointRemote (git_operations.go:492): already wraps with "failed to fetch from checkpoint remote: %w". The double-wrap is verbose but accurate.

Verification

• go build ./... clean.
• go test -run TestFetchMetadataBranch -count=1 ./cmd/entire/cli/ ./cmd/entire/cli/strategy/ → ok.

Ready for review. Reply when you'd like me to commit and continue to fix #9 (docs cleanup).

Clean revert — git diff --stat and go build ./... both empty. Fix #6 is dropped.

Status of open findings after this discussion:

• #1 ✅ done
• #2 ❌ not-fix (gate intentional)
• #4 ✅ done
• #6 ❌ not-fix (mirror is best-effort by design)
• #7, #8 — not approved, skipping
• #9 (docs) — your approval still stands, but the target needs updating given the conversation

For #9, the docs should now say:

• mirror is best-effort, gated on the checkpoints_version: "1.1" setting
• updated after entire-managed v1 writes (EnsureMetadataBranch, condensation, post-commit attribution, finalize turns, DeleteOrphanedCheckpoints, reconcile) and fetches (FetchMetadataBranch, fetchMetadataFromOrigin, fetchAndRebaseSessionsCommon)
• not updated by promoteRemoteTrackingMetadataBranch — the resume bootstrap deliberately stays out of v1/v1.1 ref territory when v1.1 is opted in
• mirror failures are logged but never fail the primary operation

Want me to draft the docs edit (CLAUDE.md + docs/architecture/sessions-and-checkpoints.md) along those lines, or do you want to refine the wording first?

Two doc files updated. Both add three things to the existing prose:

1. mirror is best-effort; failures logged but never fail the primary op
2. the resume bootstrap (origin → local v1 promotion) is the deliberate exception that doesn't mirror and is skipped in v1.1 mode
3. (CLAUDE.md only) tightened "active v1 write/fetch paths" → "entire-managed v1 writes and fetches"

I avoided naming promoteRemoteTrackingMetadataBranch directly in the docs — described the role ("resume bootstrap that promotes local v1 from origin's remote-tracking ref") so it doesn't rot if the function is renamed.

No verification needed beyond visual review (markdown only).

Ready for review. Reply when you'd like me to commit. After that, all approved findings are done — the remaining ones (#7, #8, #10) you didn't approve, so unless you want me to revisit any of them, this work is complete.

Committed 0cfc8ed646. All approved findings are now done.

Summary of the review-fix cycle

Committed (3 fixes):

Reverted after discussion (2 findings closed as not-fix):

• #2 — promoteRemoteTrackingMetadataBranch gate is intentional protection against touching v1.1 refs without opt-in
• #6 — FetchMetadataBranch's Warn + return nil matches the best-effort-mirror invariant for hook callers

Not addressed (3 findings, no approval given):

• #7 — MirrorCommittedMetadataRefBestEffort under expiring 2-min fetch ctx → settings.Load may silently fall back to v1-only topology
• #8 — MirrorCommittedMetadataRefBestEffort misclassifies SetReference ErrReferenceNotFound as "primary missing" (Debug instead of Warn)
• #10 — mirrorSyncedMetadataBranch silently no-ops on branch-name mismatch (future-topology hazard)

The work you approved is complete. Want me to look at any of #7/#8/#10 (each is small — sub-10 lines), or are we done with this branch?

Committed c3ae8f5014.

Final summary

5 commits on this branch addressing the review:

Closed as not-fix after discussion:

• #2 (promoteRemoteTrackingMetadataBranch + gate) — gate is intentional protection
• #6 (FetchMetadataBranch error return) — Warn + nil matches the best-effort invariant for hook callers

Final verification: go test ./cmd/entire/cli/strategy/ and mise run lint --new-from-rev=origin/main both clean.