fix: verify cross-repo checkpoint response identity before rendering - #2225
Merged
Conversation
checkpoint_api_reader.go decodes an entire-api cell's response identity fields (repo_full_name, checkpointId) but never checked them against what was actually requested. loadDetail cached under the requested id, and Read()/ReadSessionMetadataAndPrompts() relabeled whatever came back with the requested checkpointID, so a wrong-repo/wrong-checkpoint response from a buggy or compromised cell would render silently as belonging to the repo the caller asked about. Adds verifyResponseIdentity, checked before the response is cached or rendered, mirroring cell_target.go's existing self-check pattern (resolveProcessingPlacement's EqualFold check) at the content layer. Read()/ReadSessionMetadataAndPrompts() now source CheckpointID from the verified server response rather than the request param. Verified: two-server reproduction test with a mismatched repo_full_name in the response fails without the fix (silently accepted) and passes with it. Full mise run check deferred to a follow-up pass.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The identity check currently runs after a content-based guard and uses case-sensitive comparisons that can bypass or falsely trip the safeguard in edge cases.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens cross-repo checkpoint reads (checkpoint_api_reader.go) by validating that the entire-api cell response’s identity fields match the requested repo/checkpoint before caching or rendering, preventing silent mislabeling of foreign data.
Changes:
- Add response identity verification (
repo_full_name,checkpointId) during API checkpoint envelope loads. - Source rendered
CheckpointIDfrom the verified server response rather than the request parameter. - Add regression tests to ensure mismatched repo/checkpoint responses are rejected.
File summaries
| File | Description |
|---|---|
| cmd/entire/cli/checkpoint_api_reader.go | Adds identity verification and ensures rendering uses server-verified identity fields. |
| cmd/entire/cli/checkpoint_api_reader_test.go | Adds tests covering repo/checkpoint identity mismatch rejection and an allowed repo-id fallback. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses the three review comments on #2225. The identity check ran after the zero-sessions guard, so a foreign response that also tripped that guard was reported as a property of the checkpoint the caller asked for: "checkpoint <requested> in acme/widgets has no sessions to explain", about a checkpoint the server never answered about. Identity now runs immediately after the non-nil check, ahead of every content-based validation. Both identity comparisons now fold case. This is defensive rather than a live fix, and the comment says so: id.Validate admits only the canonical uppercase ULID alphabet, so a lowercase id cannot reach here from user input, and the cell returns canonical case today (verified against aws-us-east-2: repo_full_name "entireio/cli", checkpointId "01M1FVHFTZ3HC4TATD1JYMSE4M"). Folding cannot weaken the check -- two distinct valid IDs stay distinct under it -- whereas a strict compare fails closed on a server-side casing change, breaking `explain --repo` rather than protecting it. Two tests, both mutation-verified. Reverting the ordering makes IdentityCheckedBeforeContentGuards report the misleading error verbatim; reverting the fold makes IdentityComparisonFoldsCase fail with a mismatch on the very id it requested. fmt + lint clean; cmd/entire/cli green (87s). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1HASXYQJRGW0T5FZ9CK7CG4
Both defects are mine, from the previous commit's review-fix pass.
repo_full_name was guarded with `got != ""`, so a response that simply
omitted it skipped the repo check altogether. The only survivor was
checkpointId, which any wrong-repo response satisfies by echoing the ID
it was handed -- so the guard was something the verified party could opt
out of, which is not verification. Reproduced before fixing: a
foreign-repo payload with an empty repo_full_name rendered successfully
as acme/widgets data. It is now required; every 200 from a real cell
carries it (checked against aws-us-east-2: "entireio/cli").
The case fold rested on a false claim -- that id.Validate admits "only
the canonical uppercase ULID alphabet". It also admits legacy IDs, which
are 12 LOWERCASE hex (id.Pattern), while isULID pins ULIDs to canonical
uppercase via ParseStrict(s).String() == s. The two kinds have opposite
canonical spellings, so case is load-bearing: with the fold, requesting
legacy abc123def456 and being answered ABC123DEF456 verified clean and
then minted id.CheckpointID("ABC123DEF456"), a value id.Validate itself
rejects. Both comparisons are byte equality now. EqualFold stays only on
ownerRepo, where it belongs -- forge owner/repo names are
case-insensitive.
Two tests, both mutation-verified against the faithful pre-fix shapes:
restoring `got != ""` makes the missing-repo test render with no error;
restoring the fold makes the legacy-hex test accept the uppercased ID.
Replaces the previous IdentityComparisonFoldsCase test, whose premise
was the incorrect claim above. The no-sessions fixture gains a
repo_full_name so it still exercises the guard it was written for.
fmt + lint clean; cmd/entire/cli green (168s).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pjbgf
approved these changes
Sep 3, 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/1207
Summary
checkpoint_api_reader.godecodes an entire-api cell's response identity fields (repo_full_name,checkpointId) but never checked them against what was actually requested (r.ownerRepo, the requestedcheckpointID).loadDetailcached the response under the requested id, andRead()/ReadSessionMetadataAndPrompts()relabeled whatever came back with the requestedcheckpointIDrather than the server's own value — so a wrong-repo/wrong-checkpoint response (server bug, cache-key collision, authz bug) would render silently as belonging to the repo the caller asked about.verifyResponseIdentity, checked before the response is cached or rendered — mirroringcell_target.go's existing self-check pattern (resolveProcessingPlacement'sEqualFoldcheck) at the routing layer, brought up to the content layer.Read()/ReadSessionMetadataAndPrompts()now sourceCheckpointIDfrom the verified server response, not the request param, so a future regression in the check can't silently paper over a real mismatch by relabeling.Verification
httptest.Server-style reproduction test with a mismatchedrepo_full_name/checkpointIdin the response.go build ./cmd/entire/...and the package's own test suite (go test ./cmd/entire/cli/ -run TestAPICheckpointReader) pass.mise run check(fmt/lint/full test:ci) deferred to a follow-up pass to keep local CI load down; will run before merge.Test plan
mise run check(deferred, will run before merge)