Repository navigation
id(artifact): finish the receipt contract — frontier policy is Forbidden, builders refuse what receipt refuses - #5576
Conversation
![developing-today[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…den, builders refuse what receipt refuses Implements the remaining gaps of the #3231 receipt contract (task 9606, issue #5539): - verify(): after-frontier policy failures (too many / malformed / duplicate hashes) are now Refusal::Forbidden naming the rule, split from check_authenticity so hash/signature failures stay Refusal::Unauthenticated (#3196). - SignedArtifact::sign_after refuses the same frontier violations at construction; the two receipt tests that crafted invalid frontiers through it now sign raw via a test helper. - parsed(): friend_remove request IDs must be unique on fresh receipt (#3146 remainder); replay stays lenient, mirroring the #3187 grandfathering, since the removed-request effect is idempotent. - home_declared checks extracted to check_home_declared, shared by parsed() and Directory::declare_home, which now refuses a duplicate endpoint or out-of-bound world_id before signing (#3187 producer seam). - artifact.rs module docs state the receipt contract (#3231); the field-by-field closing audit remains with #3232. Tests: cargo test -p id --lib — 2136 passed, 0 failed (artifact+directory filtered run: 167 passed). New/updated tests cover Forbidden classification, builder refusal, duplicate request refusal at insert, legacy duplicate-request replay, and declare_home producer refusal.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 078f33886c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !replay { | ||
| let mut named = HashSet::with_capacity(body.requests.len()); | ||
| if body.requests.iter().any(|request| !named.insert(request)) { | ||
| return Err(forbidden("a friend removal names the same request twice")); |
There was a problem hiding this comment.
If an older server stored a removal containing duplicate request IDs and crashed between appending the artifact and its directory journal entry, upgrading now silently loses that removal's effect. Although ArtifactLog::open accepts the legacy artifact through parsed_for_replay, the recovery scans in Directory::open and Directory::replay_read_only still call strict artifact.parsed() (directory.rs:465 and directory.rs:569), which now rejects it. The friendship consequently remains active and the request IDs are not tombstoned. Both directory recovery scans need to use the replay-aware interpretation as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 101a19f (task 9606, lane code-y): both directory recovery scans — Directory::open and Directory::replay_read_only — now interpret stored artifacts with parsed_for_replay (widened to pub(crate)), mirroring the artifact log replay path. The live receipt path (receive_artifact) stays on strict parsed, so new receipts still refuse duplicate request IDs.
Regression test: a_legacy_removal_with_duplicate_request_ids_still_ends_the_friendship_on_recovery signs a removal naming the formed-by request ID twice, asserts strict parsed() refuses it, appends it to the artifact log with no journal entry, and asserts both recovery paths end the friendship (and, for writer open, tombstone the request ID). Targeted tests on the laptop: cargo test --lib directory:: 53 passed, cargo test --lib artifact:: 37 passed.
) Directory::open and Directory::replay_read_only used strict parsed() when scanning the artifact log for unjournaled friend removals, so a legacy removal with duplicate request IDs (stored before #3146 refused them at receipt) was silently dropped on recovery: the friendship stayed active and its request IDs were never tombstoned. Both scans now use parsed_for_replay (widened to pub(crate)), mirroring ArtifactLog replay; the live receipt path in receive_artifact stays strict. Regression test covers both recovery paths.
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Implements the remaining gaps of the #3231 receipt contract for issue #5539 (queue task 9606).
What was already on main
#3187 receipt rules (duplicate endpoints by validated identity, world_id bounds, replay grandfathering), #3192 receipt NFC checks, #3146 count/shape pins, and the duplicate-
aftercheck insideverify's helper.This PR
verify()now splits frontier policy from authenticity —afterviolations (too many / malformed / duplicate hashes) returnRefusal::Forbiddennaming the rule; hash/signature failures stayRefusal::Unauthenticated.SignedArtifact::sign_afterrefuses the same frontier violations at construction. The two receipt tests that crafted invalid frontiers throughsign_afternow sign raw via a test helper, and assert theForbiddenclassification.requestsis the one known-kind body parsed() never validates — any count, any shape, any length passes, and every string lands permanently in the directory's removed_requests void guard #3146 remainder:friend_remove.requestsmust be unique on fresh receipt (the producer sorts/dedups; receipt refuses rather than deduplicates). Replay stays lenient, mirroring the id(artifact): home_declared world_id gets only an is_empty check in parsed() — a 100 KB, whitespace-only, control- or bidi-bearing world_id is accepted and stored, and the same endpoint can be listed twice with different worlds #3187 grandfathering — the removed-request effect is idempotent.check_home_declared, shared byparsed()andDirectory::declare_home, which now refuses a duplicate endpoint or out-of-boundworld_idbefore signing.artifact.rsmodule docs state the receipt contract. The field-by-field closing audit remains with Phase 3 (audit) of #3231 — field-by-field receipt-validation audit for pkgs/id artifacts + contract statement #3232, which is gated on Steps 1–2.Tests
cargo test -p id --libon the laptop: 2136 passed, 0 failed (artifact+directory filtered run: 167 passed).sign_after, duplicate request IDs refused at insert, legacy duplicate-request removal still replays,declare_homeproducer refusal.Refs #5539, #3231, #3196, #3146, #3187. Does not close #3232 (closing audit still owed).
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.