feat(core): D24 — snapshot/fork keyset portability contract#158
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds keyset portability controls for encrypted snapshot and fork operations, new ChangesSnapshot/fork keyset portability (D24)
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bpk-lib/crates/core/src/store/read_api.rs (1)
246-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
# Errorsdoc foropen_encrypted_payload_bytesomits the newKeysetMissingcase.The function can now return
StoreError::KeysetMissingwhen the keyset was absent on load, but the doc comment only listsPayloadDecryptFailedand the internalSerializationinvariant-break.📝 Proposed doc fix
/// # Errors /// [`StoreError::PayloadDecryptFailed`] if the key is present but the /// ciphertext/nonce/bound identity fails to authenticate (tamper), or an /// internal [`StoreError::Serialization`] if reached with no keyset (an /// invariant break — callers gate on `key_store.is_some()`). + /// [`StoreError::KeysetMissing`] if the requested key is absent because the + /// keyset FILE was never loaded (as opposed to a lawfully-destroyed scope).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bpk-lib/crates/core/src/store/read_api.rs` around lines 246 - 277, The `# Errors` documentation for `open_encrypted_payload_bytes` is missing the newly introduced `StoreError::KeysetMissing` path. Update the doc comment above `open_encrypted_payload_bytes` to list `KeysetMissing` alongside `PayloadDecryptFailed` and the internal `Serialization` invariant-break, matching the branch that returns `StoreError::KeysetMissing { event_id }` when `key_store.lock().was_absent_on_load()` is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bpk-lib/crates/core/src/store/keyscope.rs`:
- Around line 406-413: The missing-keyset state is being tracked too broadly in
keyscope logic, so a later mint clears the signal for all scopes and can make a
restored missing scope look shredded. Update the `KeyScope`/`get_or_create` flow
in `keyscope.rs` to track `absent_on_load` per scope (or equivalent per-scope
state) instead of a store-wide boolean, and keep `PayloadShredded` vs
`StoreError::KeysetMissing` decisions tied to the specific scope being read.
Also add coverage around restore → append/mint → read to verify one scope’s
minting does not affect another scope’s missing-keyset behavior.
---
Nitpick comments:
In `@bpk-lib/crates/core/src/store/read_api.rs`:
- Around line 246-277: The `# Errors` documentation for
`open_encrypted_payload_bytes` is missing the newly introduced
`StoreError::KeysetMissing` path. Update the doc comment above
`open_encrypted_payload_bytes` to list `KeysetMissing` alongside
`PayloadDecryptFailed` and the internal `Serialization` invariant-break,
matching the branch that returns `StoreError::KeysetMissing { event_id }` when
`key_store.lock().was_absent_on_load()` is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fcebc09-33a4-4367-b898-d92706388548
📒 Files selected for processing (22)
CHANGELOG.mdROADMAP.mdbpk-lib/crates/core/benches/fork_cost.rsbpk-lib/crates/core/src/lib.rsbpk-lib/crates/core/src/store/error.rsbpk-lib/crates/core/src/store/error/display.rsbpk-lib/crates/core/src/store/file_classification.rsbpk-lib/crates/core/src/store/fork_report.rsbpk-lib/crates/core/src/store/keyscope.rsbpk-lib/crates/core/src/store/keyscope/persist.rsbpk-lib/crates/core/src/store/lifecycle.rsbpk-lib/crates/core/src/store/lifecycle_api.rsbpk-lib/crates/core/src/store/lifecycle_fork.rsbpk-lib/crates/core/src/store/lifecycle_snapshot.rsbpk-lib/crates/core/src/store/mod.rsbpk-lib/crates/core/src/store/read_api.rsbpk-lib/crates/core/src/store/snapshot_report.rsbpk-lib/crates/core/tests/crypto_shred_snapshot_fork_keyset.rsbpk-lib/crates/core/tests/store_fork.rsbpk-lib/crates/core/tests/store_fork_isolation.rsbpk-lib/traceability/complexity_ratchet.yamlbpk-lib/traceability/public_api/batpak.txt
💤 Files with no reviewable changes (1)
- bpk-lib/traceability/complexity_ratchet.yaml
Comment on lines
+406
to
+413
| /// `true` when this keyset was rehydrated from an ABSENT keyset file — the | ||
| /// file did not exist at open, so no keys were ever loaded. Distinguishes a | ||
| /// lost/withheld keyset (a keys-excluded snapshot opened without its | ||
| /// out-of-band keyset) from a lawful per-scope crypto-shred: with this set, a | ||
| /// missing scope key on read is reported as [`StoreError::KeysetMissing`] | ||
| /// rather than a Shredded lookalike (D24). Never cleared — the original keys | ||
| /// are permanently absent even after new keys are later minted/flushed. | ||
| absent_on_load: bool, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any existing test exercises "restore with excluded keys, then append
# a new scope, then read an old missing scope" to confirm this gap is untested.
rg -n -B2 -A40 'fn restoring_without_the_keyset_reads_keyset_missing_not_shredded' \
bpk-lib/crates/core/tests/crypto_shred_snapshot_fork_keyset.rs
rg -n 'get_or_create' bpk-lib/crates/core/src/store/keyscope.rs bpk-lib/crates/core/src/store/write/*.rsRepository: freebatteryfactory/batpak
Length of output: 2157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== keyscope.rs relevant sections =="
sed -n '380,540p' bpk-lib/crates/core/src/store/keyscope.rs
echo
echo "== load_with_fs / new_absent call sites =="
rg -n -B2 -A4 'new_absent|absent_on_load|was_absent_on_load|get_or_create' bpk-lib/crates/core/src/store -g '!**/target/**'
echo
echo "== tests around the restore scenario =="
sed -n '120,220p' bpk-lib/crates/core/tests/crypto_shred_snapshot_fork_keyset.rsRepository: freebatteryfactory/batpak
Length of output: 32084
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any tests that exercise a restore, then append, then read an old event.
rg -n -A30 -B10 'ExcludeKeys|KeysetMissing|PayloadShredded|snapshot_with_evidence_with_options' \
bpk-lib/crates/core/tests -g '*.rs'
# Check whether get_or_create is used by code paths that could run before a read of old events.
rg -n -A3 -B3 'get_or_create\(' bpk-lib/crates/core/src -g '*.rs'Repository: freebatteryfactory/batpak
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the definitions to see how the flag is interpreted on reads.
sed -n '1,220p' bpk-lib/crates/core/src/store/keyscope.rs | rg -n -A4 -B4 'was_absent_on_load|absent_on_load|KeysetMissing|PayloadShredded|destroy|destroyed'Repository: freebatteryfactory/batpak
Length of output: 2924
Track absent-on-load per scope, not with a store-wide boolean bpk-lib/crates/core/src/store/keyscope.rs:484-490 — get_or_create clears absent_on_load globally on the first mint, so a keys-excluded restore can start returning PayloadShredded for pre-existing missing scopes after any later append that mints a new scope. The current tests only cover restore→immediate read, not restore→append→read; keep the lost-keyset vs shredded distinction per scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bpk-lib/crates/core/src/store/keyscope.rs` around lines 406 - 413, The
missing-keyset state is being tracked too broadly in keyscope logic, so a later
mint clears the signal for all scopes and can make a restored missing scope look
shredded. Update the `KeyScope`/`get_or_create` flow in `keyscope.rs` to track
`absent_on_load` per scope (or equivalent per-scope state) instead of a
store-wide boolean, and keep `PayloadShredded` vs `StoreError::KeysetMissing`
decisions tied to the specific scope being read. Also add coverage around
restore → append/mint → read to verify one scope’s minting does not affect
another scope’s missing-keyset behavior.
Source: Coding guidelines
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Snapshot/fork of a store with payload encryption active now FAIL CLOSED by default (StoreError::KeysetNotPortable): a copy without its keys is silently unrestorable, and a copy carrying its keys would let a restore resurrect crypto-shredded data. Opt into a keys-excluded copy with KeysetPolicy::ExcludeKeys (SnapshotOptions + Store::snapshot_with_evidence_with_options; ForkOptions::keyset_policy) — the report is stamped KeysExcluded and the keyset must then be managed out-of-band. Restore side: opening an encrypted store whose keyset FILE is entirely absent (a keys-excluded copy restored without its out-of-band keyset) now reports the new StoreError::KeysetMissing — loud and distinct from a lawful crypto-shred (PayloadShredded), never a Shredded lookalike. Distinguished via a KeyStore::absent_on_load flag cleared on first mint, so a normal fresh store's lawful shred still reads Shredded (regression-guarded by crypto_shred_payload). Also corrects the stale "Stage B / no encryption" comments in file_classification.rs to the real D24 rationale, and keeps snapshot/fork within the function-complexity budget by splitting (not pin-bumping): SnapshotCopyAcc and a fmt_cursor_checkpoint_violation helper. Tests: 5 D24 integration tests (fail-closed default / ExcludeKeys+marker / KeysetMissing != Shredded / backups-cannot-resurrect / plaintext-unaffected), plus Display coverage. Docs: CHANGELOG [0.9.0], lib.rs crypto-shred narrative, ROADMAP section 0. Public-API baseline (traceability/public_api/batpak.txt) re-blessed (10 additive lines) and rustdoc redundant-link warnings fixed. Gates run out-of-band: structural-check ok, traceability-check ok, clippy (all-features all-targets) clean, fmt clean, D24 + crypto_shred_payload green, public-api baseline matches, 0 rustdoc warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TnRLGgP2VtnoggMn4BtKpP
heyoub
force-pushed
the
feat/d24-snapshot-fork-keyset-contract
branch
from
ff8b9b6 to
05c9b82
Compare
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
1 participant
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.
Closes the last silent-degradation member of the 0.9.0 kill-list: an encrypted store's snapshot/fork could silently produce an unrestorable backup. D24 makes keyset portability fail-closed by construction. Lands before the v0.9.0 tag, so the fail-closed default needs no migration note (payload encryption has never shipped).
Contract
snapshot/forkof a store with payload encryption active returnStoreError::KeysetNotPortable. A copy without its keys is silently unrestorable; a copy carrying its keys would let a restore resurrect crypto-shredded data — neither is safe.KeysetPolicy::ExcludeKeys(SnapshotOptions+Store::snapshot_with_evidence_with_options;ForkOptions::keyset_policy). The op proceeds and the report is stampedKeysExcluded; the keyset must then be managed out-of-band.StoreError::KeysetMissing, loud and distinct from a lawful shred (PayloadShredded) — "operator lost the keys" ≠ "scope lawfully erased". Distinguished by aKeyStore::absent_on_loadflag cleared on first mint, so a normal fresh store's lawful shred still readsShredded.Tests (pin the contract both ways)
Fail-closed default · ExcludeKeys proceeds + stamps the report · restore-without-keyset →
KeysetMissing(notShredded) · shred → snapshot(ExcludeKeys) → restore → still unreadable (backups cannot resurrect) · plaintext store unaffected. Plus a regression run of the existingcrypto_shred_payloadsuite.Notes
SnapshotCopyAcc,fmt_cursor_checkpoint_violation).file_classification.rs"Stage B / no encryption" comments to the real rationale.StoreErrorvariants correctly absent under default features), rustdoc redundant-link warnings fixed.[0.9.0], lib.rs crypto-shred narrative, ROADMAP §0 (StoreError contract-mirror gap 9→11).Verified locally: structural-check, traceability-check, clippy
--all-features --all-targets, fmt, D24 5/5 + crypto_shred_payload 10/10, public-api baseline matches, 0 rustdoc warnings.🤖 Generated with Claude Code
Summary by CodeRabbit
SnapshotOptions,ForkOptions.keyset_policy, and evidence/reporting of keys intentionally excluded.KeysetMissing(and not a shredded-style lookalike).Greptile Summary
This PR adds fail-closed keyset portability behavior for encrypted store snapshots and forks. The main changes are:
StoreError::KeysetNotPortablewhen payload encryption is active.KeysetPolicy::ExcludeKeysallows explicit keys-excluded copies and stamps evidence reports withKeysExcluded.StoreError::KeysetMissinginstead of a shredded-payload result.Confidence Score: 5/5
This PR is safe to merge based on the reviewed changes.
The changed lifecycle paths gate encrypted snapshot/fork before destination mutation. Reports are stamped only for explicit
ExcludeKeyson encryption-active stores. The tests cover the main contract branches reviewed here. No new actionable issues were identified.No files require special attention.
What T-Rex did
Important Files Changed
ExcludeKeysevidence.ExcludeKeysevidence.KeysetMissinginstead of shredded plaintext.ExcludeKeysevidence, missing-keyset reads, and plaintext behavior.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Caller participant Store participant Gate as resolve_keyset_exclusion participant Copy as snapshot/fork copy participant Report as Evidence report participant Restored as Restored Store Caller->>Store: snapshot/fork(options) Store->>Gate: key_store + keyset_policy alt encryption active + Refuse Gate-->>Store: StoreError::KeysetNotPortable Store-->>Caller: typed refusal before destination mutation else encryption active + ExcludeKeys Gate-->>Store: "keys_excluded = true" Store->>Copy: copy eligible store files excluding keyset Copy->>Report: add KeysExcluded finding Store-->>Caller: evidence report Caller->>Restored: open keys-excluded copy without keyset Restored-->>Caller: StoreError::KeysetMissing on encrypted read else plaintext/no payload encryption Gate-->>Store: "keys_excluded = false" Store->>Copy: normal snapshot/fork Store-->>Caller: evidence report without KeysExcluded end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Caller participant Store participant Gate as resolve_keyset_exclusion participant Copy as snapshot/fork copy participant Report as Evidence report participant Restored as Restored Store Caller->>Store: snapshot/fork(options) Store->>Gate: key_store + keyset_policy alt encryption active + Refuse Gate-->>Store: StoreError::KeysetNotPortable Store-->>Caller: typed refusal before destination mutation else encryption active + ExcludeKeys Gate-->>Store: "keys_excluded = true" Store->>Copy: copy eligible store files excluding keyset Copy->>Report: add KeysExcluded finding Store-->>Caller: evidence report Caller->>Restored: open keys-excluded copy without keyset Restored-->>Caller: StoreError::KeysetMissing on encrypted read else plaintext/no payload encryption Gate-->>Store: "keys_excluded = false" Store->>Copy: normal snapshot/fork Store-->>Caller: evidence report without KeysExcluded endReviews (2): Last reviewed commit: "Merge branch 'main' into feat/d24-snapsh..." | Re-trigger Greptile