Review Branch and Refactor Shard Methods

Review complete.

Findings: none blocking.

Notes:

• The implementation consistently separates legacy 12-hex Pattern from widened checkpoint matching via CheckpointPattern, which avoids accidentally widening run IDs in provenance.
• Trailer parsing and E2E assertions now reuse the canonical pattern, which is a good drift-prevention change.
• Tests cover validation, kind detection, sharding behavior, JSON round-trips, and trailer parsing for mixed legacy/ULID cases.

Minor non-blocking cleanup:

• cmd/entire/cli/checkpoint/id/id.go still has a top-level comment saying CheckpointID is a 12-character hex identifier; that should probably be updated to mention ULIDs.
• trailers.go’s CheckpointTrailerKey comment still says format is 12 hex characters.

Validation run:

• go test ./cmd/entire/cli/... ✅
• go test ./e2e/testutil ✅

Review complete.

Finding:

• cmd/entire/cli/checkpoint/id/id.go:34 — ULIDPattern accepts any Crockford base32 char in the first position, but canonical ULIDs must fit 128 bits, so the first char should be 0-7. As written, IDs like Z1KVBJCWYA4YW6J5M9GP655HZN validate as “ULID” even though they are not valid ULIDs. Consider:

Everything else looks good. The comment cleanups from the previous pass are addressed.

Validation run:

• go test ./cmd/entire/cli/checkpoint/id ./cmd/entire/cli/trailers ./cmd/entire/cli ✅

Yes. Common Go choices:

• github.com/oklog/ulid/v2 — de facto standard, small, widely used.
• github.com/segmentio/ksuid is similar concept but not ULID.
• github.com/ulid/spec is the spec repo, not a Go validation package.

For validation with oklog/ulid/v2:

Use ParseStrict, not Parse, if you want canonical 26-char ULIDs and correct Crockford/base32/range validation.

That said, pulling in a dependency only for validation may be more than this PR needs. A stricter regex is likely fine here:

But if future work will generate ULIDs too, adding github.com/oklog/ulid/v2 now would be reasonable.

Review complete.

Finding:

• e2e/testutil/assertions.go:29-32 / :168-173 — AssertCheckpointIDFormat still uses CheckpointPattern, but CheckpointPattern is now intentionally a loose extraction regex. It accepts ULID-shaped strings that production validation rejects, e.g. timestamp overflow starting with 8-Z. Since this helper claims to assert a “valid checkpoint ID”, it should probably call checkpointid.Validate(checkpointID) instead of regex matching.

Validation run:

• go test ./cmd/entire/cli/checkpoint/id ./cmd/entire/cli/trailers ./cmd/entire/cli ./e2e/testutil ✅

Review complete.

Findings: none blocking.

The previous issue is addressed: AssertCheckpointIDFormat now calls checkpointid.Validate, so E2E assertions match production validation instead of the loose extraction regex.

Validation run:

• go test ./cmd/entire/cli/checkpoint/id ./cmd/entire/cli/trailers ./cmd/entire/cli ./e2e/testutil ✅