Emit ULID checkpoint IDs under the git-refs store - #1629
Merged
Conversation
New checkpoints written under the git-refs primary now get a 26-char ULID instead of a 12-hex id, so a checkpoint's id encodes its storage backend (ULID ⟹ ref) — which lets reads route by id kind without config. - id.GenerateULID(): timestamp + crypto-random ULID (via oklog/ulid, already a dep), canonical and KindULID-valid. Generate() (hex) is unchanged. - checkpoint.GenerateCheckpointID(ctx): the single place the format is chosen — ULID when the primary is git-refs, else 12-hex. Fail-soft to hex on a missing/malformed config. - Route the checkpoint-id generation sites through it (attach, the two manual-commit hook paths, the two condensation paths). Turn ids and the investigate run id stay hex — they're format-agnostic correlation tokens. - Fix explain's auto-ambiguity guard: it assumed a fixed 12-char id width; use the new id.MaxIDLength (26) so a ULID target isn't wrongly treated as un-prefixable. Storage/read already handled both formats (ShardFor shards ULIDs on the last two chars, RefName/ParseRef/Validate accept them), so no store changes were needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ddfc647b27a2
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates checkpoint ID emission so that when the checkpoints primary backend is git-refs, newly minted checkpoint IDs are 26-character ULIDs (vs the legacy 12-character hex IDs). This makes the ID format itself convey which storage backend a checkpoint belongs to, enabling follow-up work to route reads based on ID kind.
Changes:
- Added ULID checkpoint ID generation (
id.GenerateULID) and aMaxIDLengthconstant for prefix/length reasoning. - Introduced
checkpoint.GenerateCheckpointID(ctx)as the single dispatcher (git-refs → ULID, otherwise → legacy hex) and routed key generation sites through it (hooks, condensation, attach). - Updated
explain’s ambiguity guard to useid.MaxIDLengthso ULID targets aren’t incorrectly excluded.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/strategy/manual_commit_hooks.go | Generates checkpoint IDs via the backend-aware dispatcher for commit-message trailer emission. |
| cmd/entire/cli/strategy/manual_commit_condensation.go | Uses backend-aware checkpoint ID generation during condensation flows. |
| cmd/entire/cli/explain.go | Fixes ambiguity guard to reason about ULID-length checkpoint IDs. |
| cmd/entire/cli/checkpoint/id/id.go | Adds GenerateULID() and MaxIDLength to support ULID emission and correct prefix checks. |
| cmd/entire/cli/checkpoint/id/id_test.go | Adds unit test coverage for ULID generation/validation/kind. |
| cmd/entire/cli/checkpoint/generate.go | New dispatcher for checkpoint ID generation based on configured primary backend. |
| cmd/entire/cli/checkpoint/generate_test.go | Tests that dispatcher emits ULID for git-refs and legacy hex otherwise. |
| cmd/entire/cli/attach.go | Routes attach checkpoint ID generation through the dispatcher (and threads context). |
Collaborator
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fbc294a. Configure here.
ulid.Timestamp uses t.Unix()/nanoseconds — the absolute epoch instant — so local vs UTC never changed the ID. Switch to the library's own ulid.Now() (= Timestamp(time.Now().UTC())) to make the UTC intent explicit, match the library idiom, and drop the manual time.Now() + time import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: f05349699fb0
attach's ensureCheckpointAvailable gated on the v1 branch existing before reading the configured store. Under the git-refs backend a checkpoint lives at its own ref (refs/entire/checkpoints/<shard>/<id>) and there is no v1 branch, so a valid ULID checkpoint was reported "missing" and attach refused — even though the ref existed locally. The refresh path also only fetched the v1 branch. Now: - checkpointPresentLocally reads through the configured primary store directly. The local-Primary-ref gate (which prevents counting origin remote-tracking presence and clobbering the remote on push) is kept for git-branch only; for git-refs the store read is already local-only (attach wires no ref fetcher). - refreshCheckpoint is backend-aware: git-refs fetches just this checkpoint's ref via FetchCheckpointRef, git-branch fetches the whole v1 branch as before. - the refuse error and suggested fetch command name the right storage per backend (shared suggestFetchCommand helper avoids duplication). Also isolate generate_test's default-primary case to an empty worktree so a developer dogfooding git-refs in their real .entire/settings.json can't turn the empty-env default (which falls through to the settings file) into a ULID. Adds TestAttach_GitRefsBackend_AppendsToExistingCheckpoint; the git-branch refuse/remote-tracking regression tests are unchanged and still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: daee20832ff1
Follow-ups from review of the git-refs ULID-emission PR (the id-kind read-routing item, #1, and the write-boundary kind guard, #2, are deferred — see below): - id.CheckpointID.DisplayShort() (new): kind-aware trim — legacy hex shows its 12-char prefix, a ULID is shown in full (front-truncating a ULID drops its entropy tail and yields an ambiguous, unresolvable prefix). - explain checkpoint-list view uses DisplayShort instead of a blind 12-char cut, so ULID checkpoints no longer collapse to near-identical timestamp prefixes. - blame --long sizes the Checkpoint/Session column to its content (attributionCheckpointColumnWidth) so a 26-char ULID + session renders whole instead of being clipped to a session-less 21-char prefix. - attach's fetch hint now resolves its target via resolveCheckpointFetchTarget — the same path FetchCheckpointRef uses — so the pasteable command matches the remote the fetch actually ran (fixes a bare "git fetch origin …" that fails in a token-only environment with an SSH origin). - id.MaxIDLength ties to oklog/ulid's EncodedSize instead of a third hardcoded 26. - eager-condense mints the checkpoint ID only after the skip checks, so a no-op session stop no longer pays the mint (and its checkpoints-config load). - docs/architecture/sessions-and-checkpoints.md: checkpoint IDs are 12-hex OR ULID (not fixed-width), minted via checkpoint.GenerateCheckpointID. Deferred to the id-kind read-routing follow-up (tracked separately): - #1 attach routes presence/refresh/fetch-hint by current config, not by the trailer ID's kind — after a git-branch⇄git-refs flip attach looks in the wrong place and can suggest an unfixable fetch. This is the read-routing work the PR body already names; the two concrete attach dead-ends belong in that issue. - #2 the ULID⇒refs invariant has no write-boundary guard. A naive "git-branch store saw a ULID → warn" misfires when git-branch is a *mirror* of a git-refs primary (a valid topology where ULIDs legitimately reach it), so the correct guard needs topology-role awareness and belongs with the routing work. Tests: TestCheckpointID_DisplayShort, TestAttributionCheckpointColumnWidth. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 14006b64f8d1
nodo
approved these changes
Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/753
New checkpoints written under the git-refs primary now get a 26-char ULID instead of a 12-hex id. This makes a checkpoint's id encode its storage backend — ULID ⟹ ref, legacy hex ⟹ branch — which is the missing piece for id-kind-aware read routing (a follow-up): reads can decide where a checkpoint lives from the id alone, no config, no guessing.
What changed
id.GenerateULID()— timestamp + crypto-random ULID viaoklog/ulid(already a dependency, previously import-for-parsing-only). Canonical, passesKindOf/ValidateasKindULID.id.Generate()(12-hex) is unchanged.checkpoint.GenerateCheckpointID(ctx)— the single place the format is decided: ULID when the primary is git-refs, else 12-hex. Fail-soft to hex on a missing/malformed checkpoints config. Layering stays clean (checkpointalready importsid+settings;settingsdoesn't importcheckpoint).attach, both manual-commit hook paths, both condensation paths. Turn ids and the investigate run id stay hex — they're format-agnostic correlation tokens, not stored checkpoints.explainguard fix — its auto-ambiguity guard assumed a fixed 12-char id width; it now usesid.MaxIDLength(26) so a ULID target isn't wrongly treated as un-prefixable.Why no store changes
Read/write already handled both formats:
ShardForshards ULIDs on the last two chars, andRefName/ParseRef/Validateaccept them. Only emission was missing (the deferred "Layer B" from the git-refs store PR).Testing
id.GenerateULIDunit test (valid, KindULID, 26 chars, unique);GenerateCheckpointIDtest (git-refs → ULID, default/git-branch → hex).Follow-up
The id-kind read routing (git-refs primary reading old branch-only checkpoints, and vice-versa) builds on this — now that ULID ⟹ ref holds.
🤖 Generated with Claude Code
Note
Medium Risk
Changes how new checkpoint IDs are created across attach, hooks, and condensation; mis-routing format vs primary could affect metadata writes, though tests and fail-soft hex fallback limit blast radius.
Overview
New committed checkpoints now get a 26-character ULID when the checkpoints primary is git-refs, and keep the legacy 12-hex ID for git-branch (and the default). The goal is to tie ID shape to storage (ULID → ref, hex → branch) so later read routing can pick the backend from the ID alone.
checkpoint.GenerateCheckpointID(ctx)is the single dispatcher: it reads checkpoints config and callsid.GenerateULID()orid.Generate(), failing soft to hex if config is missing or bad. Attach, manual-commit hook paths, and condensation (doctor / eager stop) now call this instead ofid.Generate()directly;resolveCheckpointIDtakescontextfor that.explain --generateambiguity handling no longer assumes a 12-character checkpoint width; it usesid.MaxIDLength(26) so ULID targets are not skipped incorrectly.Tests cover ULID generation and config-driven
GenerateCheckpointIDbehavior.Reviewed by Cursor Bugbot for commit fbc294a. Configure here.