Entire

I’ll inspect the current branch, working tree, and the live entire command guidance first, then compare the branch against its PR/base before reading the changed code.

The working tree is clean on feat/checkpoint-ulid-emission. I’m going to identify the PR metadata and base branch, then review the actual diff rather than only the branch name.

PR #1629 targets main and has two commits. I’m reading the branch diff now, starting with the changed file set and then drilling into the strategy/checkpoint paths.

The change is small but touches ID generation from hooks and condensation, so I’m checking all remaining generation sites and the config/store routing code to make sure the new ULID behavior is complete and doesn’t break legacy paths.

There are still id.Generate() calls, but most are tests, benchmarks, or turn IDs. I’m separating checkpoint IDs from unrelated short IDs before deciding whether any generation site was missed.

The config loader is central to this change, so I’m checking its error behavior. A mismatch there would decide whether “fail-soft” ID generation really matches the committed-store behavior.

I’m also checking the changed call sites in context. The important question is whether the ID format chosen before writing is driven by the same settings context that the store uses when it actually writes.

The explain ambiguity change widens the guard from 12 to 26 characters. I’m reading that flow because “longer than legacy ID” targets are common command names/paths, and a guard bug there could show up as a user-facing regression.

I’ve covered the primary paths. I’m going to run the focused package tests next so the review can distinguish verified behavior from static inspection only.

The focused Go tests for the touched CLI, checkpoint, ID, and strategy packages pass. I’m doing a final pass over tests and edge cases now, especially config combinations like git-refs primary with git-branch mirrors.

One adjacent area is attach/resume behavior for missing committed checkpoints. Since this PR changes the ID format used there, I’m checking whether fetch/help paths still line up with git-refs storage.

I found one suspicious attach path: presence checks still gate on the v1 metadata branch even when the configured persistent store is per-checkpoint refs. I’m verifying whether that is just a stale comment or an actual false-negative before I call it out.

That path is a real false-negative for refs-only repositories: refs.Primary is still the v1 branch topology, while the store selected by settings may be git-refs. I’m looking for any other similar v1 assumptions in changed flows before I finalize findings.

The rest of the diff looks mechanically consistent: hook and condensation generation now route through the new helper, and focused tests pass. I’m checking PR status/checks once so I can include any external signal separately from the code findings.

Findings

cmd/entire/cli/attach.go:561 checkpointPresentLocally still requires refs.Primary to exist before reading the configured store. refs.Primary is the v1 branch, so in a refs-only repo a valid ULID checkpoint stored at refs/entire/checkpoints/... is treated as missing. ensureCheckpointAvailable then refreshes/suggests the v1 branch, so a later entire attach on a commit that already has a new ULID trailer can fail even though the checkpoint ref exists. This should read through the configured primary store directly, and the refresh path should use checkpoint-ref fetching when the primary is git-refs.
cmd/entire/cli/checkpoint/generate_test.go:23 The “default primary” test can read the repo’s real .entire/settings*.json because it uses context.Background() and only sets ENTIRE_CHECKPOINTS_PRIMARY to an empty string, which means “fall back to file.” A developer dogfooding git-refs in local settings would get a false failure. Use an isolated temp worktree context with no checkpoint config for the default case.

Verification

I ran:

Both passed. PR checks had lint, core tests, canaries, binary-size, and license passing; integration checks were still pending when I checked.

I’ll re-check the PR head and local branch first so I’m not reviewing stale state, then I’ll re-read the updated diff and focus on whether the previous findings were fixed cleanly.

Local HEAD matches the PR head (e1112a7), and the working tree is clean. The PR has been updated since my last review, so I’m reading the new branch diff and the specific attach/config test changes.

The new commit addresses both previous findings directly. I’m now checking the backend-aware fetch path and local-only read behavior, since that is where subtle regressions usually hide for git-refs versus v1 branch storage.

The git-refs fetch uses the existing single-ref fetch helper and reopens the repo before reading again, which is the right shape. I’m going to run the focused tests now, then do one final scan for missed generation sites and test isolation issues.

The focused packages still pass. I’m checking remaining id.Generate() users and diff hygiene now; at this point I’m looking for edge cases rather than broad failures.

The remaining id.Generate() calls are still non-checkpoint IDs or benchmark/test fixtures, so the new dispatcher coverage looks intentional. I’m checking current CI status once more before writing the review result.

One migration edge case I checked is legacy IDs under a git-refs primary. This PR is scoped to emission and keeps store selection config-driven, so I’m treating ID-kind read routing as follow-up work rather than a regression in this branch.

Findings

No remaining issues found on the updated PR.

The two previous findings look addressed: attach now checks/fetches git-refs checkpoints without requiring the v1 branch, and the default ID-generation test is isolated from local repo settings.

Verified locally:

All passed. Current PR checks: lint, binary-size, license, and both canaries are passing; test-core and integration shards were still pending when checked.