feat: write file content end to end - #907
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR adds authenticated per-version content keys, streaming content writes, bounded staging admission, staged publication and cleanup, and write APIs across the engine, WASM boundary, worker protocol, transports, and client facade. ChangesContent-key protocol
Engine content plane
WASM and client write API
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
The settings-record tag landed on main at 0x0b first, so the content-key tag moves to 0x0c to keep the registry byte-ordered and 1-based dense. The tag is AAD-bound, so the content_key KAT vectors are regenerated from kat_gen; only the AEAD tags move, no other vector family changes.
57e5f51 to
35a662c
Compare
|
Rebased onto #903 claimed struct tag The struct tag is AAD-bound, so the Green locally: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fuse/src/error.rs (1)
172-192: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the missing
TooManyWritescause to this test.The test iterates
[StagingLimit, DeviceFull, StagingBacklog, StorageUnmeasured, AccountQuota].OverBudgetCausealso has aTooManyWritesvariant (derived fromRefusal::TooManyWritesincrates/engine/src/facade.rs). The test's own doc comment states every cause "has to survive the crossing", so leave no variant untested.🧪 Proposed fix
for cause in [ OverBudgetCause::StagingLimit, OverBudgetCause::DeviceFull, OverBudgetCause::StagingBacklog, OverBudgetCause::StorageUnmeasured, OverBudgetCause::AccountQuota, + OverBudgetCause::TooManyWrites, ] {🤖 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 `@crates/fuse/src/error.rs` around lines 172 - 192, Update the cause list in the each_budget_keeps_its_own_cause test to include OverBudgetCause::TooManyWrites, ensuring every OverBudgetCause variant is verified through the VfsError conversion.
🧹 Nitpick comments (18)
blueprint/core.md (1)
165-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the content-key KAT set, like the neighboring tags.
The registry entry is correct and matches
STRUCT_TAG_CONTENT_KEYandmanifest.json(content-key: 12). Every other recently added tag (owner-write-blob,op-record,settings-record) also documents its accept/reject vector families in this section. Thecontent-keyfamilies (content_key_accept,content_key_reject) are not described here.🤖 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 `@blueprint/core.md` around lines 165 - 166, Update the content-key section in the manifest documentation to describe its KAT vector families, content_key_accept and content_key_reject, consistently with the neighboring tag entries.crates/engine/src/content/write.rs (2)
241-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe realloc test also documents the stall.
the_pending_buffer_is_never_reallocatedbreaks out of the loop whenremaining.len() == rest.len(), which is exactly the no-progress condition. The test therefore encodes a caller contract that differs from thepushdoc comment. Ifpushstarts to fail closed on an over-push, assert the error here instead of breaking on the stalled slice.🤖 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 `@crates/engine/src/content/write.rs` around lines 241 - 268, Update the test the_pending_buffer_is_never_reallocated to assert the expected push error when the input exceeds the declared size, rather than breaking when remaining.len() equals rest.len(). Preserve the capacity and observed_size assertions after validating the fail-closed behavior.
74-92: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a small guard in
pushso stale unit loops cannot hide over-length input.
push_chunk_innerrejects excess input beforepush, butContentWriterstill accepts larger initial chunks by declaring one full chunk whileobserved_size()remains smaller. This leaves thepush-only test behavior inconsistent with the write-handles path. Add a bound check inpushforobserved + bytes.len() > declared_sizeand fail closed before buffering excess bytes.🤖 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 `@crates/engine/src/content/write.rs` around lines 74 - 92, Update ContentWriter::push to reject input when self.observed plus bytes.len() exceeds the declared size, returning the appropriate existing error before extending pending or updating observed. Preserve normal chunk sealing and remainder handling for inputs within the declared bound.crates/engine/src/content/mod.rs (1)
12-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
pub(crate) mod budget;for consistency.Every item in
budgetispub(crate)(Refused,StagingLedger,sealed_total_bytes,ReservationId,MAX_OPEN_WRITES), sopub mod budgetexposes no public API.limitsusespub(crate) modfor the same situation.🤖 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 `@crates/engine/src/content/mod.rs` around lines 12 - 22, The budget module declaration uses pub mod while all of its exports (Refused, StagingLedger, sealed_total_bytes, ReservationId, MAX_OPEN_WRITES) are pub(crate), which means the module exposes no public API. Change pub mod budget to pub(crate) mod budget to align with the consistency pattern already established by the limits module in the same visibility scope.crates/core/examples/kat_gen.rs (1)
6437-6493: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider adding a low-order
encreject vector for the content-key family.The settings-record family pins
hpke-non-contributorywith a short and a low-orderenc. The content-key family only variesencby one bit (tampered_enc) and by width (wide_enc), so the contributory check on this family's open path is not pinned by its own vector. A low-orderencvector would freeze that behavior foropen_content_key.🤖 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 `@crates/core/examples/kat_gen.rs` around lines 6437 - 6493, Add a new reject vector in build_content_key_reject that uses reframe_content_key to set the enc field to a low-order value, similar to how tampered_enc and wide_enc are constructed. This vector should mirror the low-order enc vector pattern used in the settings-record family to pin the contributory check behavior on the open_content_key path, alongside the existing tampered_enc and wide_enc variants.crates/core/src/seal/mod.rs (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm whether
seal::content_keyshould remain public
seal::content_keyexposes only the already re-exportedcontent_keyhelpers, and no current code imports it through that path. If onlyseal::seal_*/open_*and constants are part of the intended public API, makecontent_keyprivate.🤖 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 `@crates/core/src/seal/mod.rs` at line 15, Review the public API exposed by the seal module and make the content_key module private if its helpers are already re-exported elsewhere and no callers require the seal::content_key path. Update the module declaration in seal and preserve the existing seal_* and open_* functions and constants as the intended public API.crates/engine/examples/kat_gen.rs (1)
241-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
leaf_cidsfor the later assertion and the hex output.Lines 254 and 262 rebuild the same CID list from
leaves. Reuseleaf_cidsso the vector, the assertion, and the hex encoding cannot drift.♻️ Proposed refactor
- assert_eq!( - manifest.leaf_cids, - leaves.iter().map(|l| l.cid.clone()).collect::<Vec<_>>(), - "{name}: links preserve file order" - ); + assert_eq!( + manifest.leaf_cids, leaf_cids, + "{name}: links preserve file order" + );- leaf_cids: leaves.iter().map(|l| hex::encode(&l.cid)).collect(), + leaf_cids: leaf_cids.iter().map(hex::encode).collect(),🤖 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 `@crates/engine/examples/kat_gen.rs` around lines 241 - 246, Reuse the existing leaf_cids variable from the assembly setup for the later assertion and hexadecimal output in the kat generation flow, removing the duplicate leaves.iter CID collection at those locations. Keep the current assertion and encoding behavior unchanged while ensuring both consume the same vector.crates/engine/src/content/chunk.rs (1)
16-20: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin
SEALED_LEAF_OVERHEADagainst a real sealed leaf.The staging reservation is exact only while
SEALED_LEAF_OVERHEADequals the true per-leaf overhead ofseal_chunk. Nothing in this file asserts that. A change to the core wire layout would silently make every reservation wrong. Add a test that seals one chunk and compares the lengths.💚 Proposed test
#[test] fn sealed_leaf_overhead_matches_a_real_sealed_leaf() { let key = ContentKey::from_bytes([2u8; KEY_LEN]); let chunk = vec![0xABu8; 13]; let leaf = seal_one_chunk(&key, &chunk, &mut SeededEntropy::new(4)).unwrap(); assert_eq!( leaf.sealed.len() as u64, chunk.len() as u64 + SEALED_LEAF_OVERHEAD, "the reservation arithmetic must match the sealed wire layout" ); }🤖 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 `@crates/engine/src/content/chunk.rs` around lines 16 - 20, Add a test function to verify that the SEALED_LEAF_OVERHEAD constant matches the actual per-leaf overhead produced by seal_one_chunk. The test should create a sample ContentKey and plaintext chunk, call seal_one_chunk to produce a sealed leaf, and then assert that the sealed output length equals the plaintext chunk length plus SEALED_LEAF_OVERHEAD. This pins the constant against the real wire layout so that future changes to the sealing format are caught before they silently break the staging admission ledger reservation calculation.crates/engine/src/content/dag.rs (1)
572-599: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the 3-byte to 5-byte CBOR head boundary.
The comment states the test holds at every leaf count where a CBOR head width changes. The counts stop at 300.
head_lenalso changes at 0x1_0000, and both the links-array count and thesizeuint cross that width on real production files. At 65536 leaves the production root is about 2.5 MiB, so it still assembles and the case is reachable. Add 65535 and 65536 so the sizing arithmetic is pinned at that width too.💚 Proposed change
- for leaves in [0u64, 1, 23, 24, 255, 256, 300] { + for leaves in [0u64, 1, 23, 24, 255, 256, 300, 65_535, 65_536] {🤖 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 `@crates/engine/src/content/dag.rs` around lines 572 - 599, The test root_block_len_matches_the_assembled_root does not cover the CBOR head width boundary at 0x1_0000 where both the links-array count and size uint widen from 3 bytes to 5 bytes. Extend the leaves array (currently [0u64, 1, 23, 24, 255, 256, 300]) to include 65535 and 65536 so the sizing arithmetic is pinned at this boundary transition and validates at every leaf count where a CBOR head width changes.crates/engine/src/sync/staging.rs (1)
128-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo copies of the write-handle framing loop. Both files reproduce the same
ContentWriterpush/finish loop to build a version's sealed leaves and root block. The shared root cause is the absence of one test helper for that loop, so a change to theContentWritercontract must be applied twice.
crates/engine/src/sync/staging.rs#L128-L157: replace the body offramedwith a call to a shared helper exposed fromcrate::testkit.crates/engine/tests/write_plane.rs#L899-L921: replaceframe_versionwith the sametestkithelper, returning the root CID it already derives fromfinished.content.content_cid().
testkitis reachable from both an in-crate unit test module and an integration test, so one definition serves both call sites.🤖 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 `@crates/engine/src/sync/staging.rs` around lines 128 - 157, The duplicated ContentWriter framing loop should be centralized in a shared testkit helper. In crates/engine/src/sync/staging.rs lines 128-157, update framed to call the helper while preserving its blocks, root block, and StagedContent outputs; in crates/engine/tests/write_plane.rs lines 899-921, replace frame_version’s duplicated loop with the same helper and continue returning the root CID derived from finished.content.content_cid(). Expose one helper from crate::testkit for both call sites.crates/engine/tests/write_plane.rs (1)
1004-1119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a staged-byte assertion to the two block-loss tests.
Both tests assert the dead letter and the absent publish. Neither asserts what happens to the remaining staged blocks. The key-failure test at Line 992 pins the release side, so the preservation side of the same rule stays unasserted.
blueprint/engine.mdstates that an unrecoverable record dead-letters with staged content preserved, while#818releases blocks only for the no-key case. A regression that released the surviving blocks here would pass both tests.Add an assertion on
staged_keys()(orstaged_bytes_total()) after the tick in each test.The two tests also differ only by the removed leaf index. Consider one helper parameterized on that index.
🤖 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 `@crates/engine/tests/write_plane.rs` around lines 1004 - 1119, Add assertions after tick in an_evicted_prefix_of_the_block_set_is_loss_not_progress and a_hole_in_the_staged_block_suffix_is_loss_and_fails_closed verifying the remaining staged content is preserved via staged_keys() or staged_bytes_total(), matching the documented unrecoverable-record behavior. Keep the existing dead-letter and no-publish assertions, and optionally extract their shared setup/assertion flow into a helper parameterized by the removed leaf index.packages/client/src/correlatedTransport.ts (1)
68-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a named alias for the durable op id.
commitWritereturns a durableopId, whilebeginWrite,pushChunk, andabortWriteuseWriteHandle. A newOpIdtype would distinguish write handles from op ids in the worker protocol and across implementers.🤖 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 `@packages/client/src/correlatedTransport.ts` around lines 68 - 71, Introduce a named OpId type for durable operation identifiers and update commitWrite to return Promise<OpId> instead of Promise<bigint>. Propagate this alias through the worker protocol and all implementations or consumers of commitWrite, while keeping WriteHandle for beginWrite, pushChunk, and abortWrite.Source: Coding guidelines
packages/client/test/browser/leadership.ts (1)
104-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider preserving the original error if
abortWritealso fails.If
abortWritethrows inside thecatchblock, its error replaces the originalpushChunk/commitWritefailure, so the root cause is lost from the harness's return value. This only affects diagnostics in this Playwright test harness.🔧 Optional improvement
try { await client!.facade.pushChunk(handle, content.buffer); await client!.facade.commitWrite(handle); } catch (error) { - await client!.facade.abortWrite(handle); + await client!.facade.abortWrite(handle).catch(() => {}); throw error; }🤖 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 `@packages/client/test/browser/leadership.ts` around lines 104 - 119, Update the inner error handling in window.cbCreateFile so an abortWrite failure cannot replace the original pushChunk or commitWrite error; preserve and rethrow the initial failure while still attempting abortWrite, allowing the outer settle(error) path to return the root cause.packages/client/src/writeQueue.ts (1)
1-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct unit test for
WriteQueue.The header states two guarantees: steps for one handle run in call order, and a rejected step does not stall the remaining steps for that handle.
serve.test.tscovers the class only indirectly throughserveEngine. A direct test pins both guarantees and the map pruning, and it fails loudly ifprevious.then(step, step)is ever simplified toprevious.then(step).Suggested cases:
- Interleave two handles. Assert that handle A's steps complete in call order and that handle B does not wait for A.
- Reject the first step for one handle. Assert that the second step still runs and that
runrejects for the failed step only.Do you want me to generate the test file?
🤖 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 `@packages/client/src/writeQueue.ts` around lines 1 - 33, Add a direct unit test for the WriteQueue class covering per-handle call-order serialization, independent progress for interleaved handles, and continuation after a rejected step. Assert that only the failed run rejects, the subsequent run executes, and the internal chains map is pruned after each handle drains.packages/client/test/browser/journalEngine.worker.ts (1)
79-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn an op id that differs from the handle.
commitWritereturns the write handle as the durable op id. The two are separate id spaces:beginWriteyields a handle andcommitWriteyields an op id. A client that conflates them passes against this fake.ReadHostinpackages/client/src/worker/serve.test.tskeeps them distinct (11nfor the handle,2048nfor the commit).♻️ Proposed change
class JournalHost implements EngineHostLike { private nextHandle = 1n; + private nextOpId = 1000n; private readonly open = new Map<bigint, { size: number; received: number }>();this.open.delete(handle); await journal('commitWrite'); - return handle; + return this.nextOpId++; }🤖 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 `@packages/client/test/browser/journalEngine.worker.ts` around lines 79 - 86, Update commitWrite in the journalEngine worker fake to return a distinct durable operation id rather than the input handle, matching the separate handle/op-id contract and the ReadHost values used by serve.test.ts.packages/client/src/worker/engineHost.ts (1)
71-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow
WriteTargetonce.The method tests
'node' in targetthree times. A single branch makes the two exclusive shapes explicit and removes the chance that a later edit updates one test and not the others.♻️ Proposed refactor
beginWrite(target: WriteTarget, size: number): Promise<WriteHandle> { - const node = 'node' in target ? this.wasm.NodeId.fromBytes(target.node) : undefined; - const parent = 'node' in target ? undefined : this.wasm.NodeId.fromBytes(target.parent); - const name = 'node' in target ? undefined : target.name; - return this.handle.beginWrite(parent, name, node, size); + return 'node' in target + ? this.handle.beginWrite(undefined, undefined, this.wasm.NodeId.fromBytes(target.node), size) + : this.handle.beginWrite( + this.wasm.NodeId.fromBytes(target.parent), + target.name, + undefined, + size + ); }🤖 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 `@packages/client/src/worker/engineHost.ts` around lines 71 - 76, Refactor beginWrite so it narrows target once using a single branch on the exclusive 'node' shape, deriving node, parent, and name within the respective branches before calling this.handle.beginWrite. Preserve the existing NodeId conversions and arguments for both target variants.packages/client/src/worker/commandCodec.test.ts (1)
34-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the parent argument too.
The test pins the argument count, the name, and the kind. It leaves
calls[0][0]unchecked, so a swap that passed the wrong bytes toNodeId.fromByteswould still pass. The stub returns{ bytes }, so the assertion is one line.♻️ Proposed assertion
expect(calls).toHaveLength(1); expect(calls[0]).toHaveLength(3); + expect(calls[0][0]).toEqual({ bytes: new Uint8Array(16).fill(1) }); expect(calls[0][1]).toBe('a.txt'); expect(calls[0][2]).toBe(fakeWasmEnums.NodeKind.File);🤖 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 `@packages/client/src/worker/commandCodec.test.ts` around lines 34 - 37, Update the test assertions around the recorded call to also verify calls[0][0] equals the expected parent argument bytes, using the stubbed { bytes } value returned by NodeId.fromBytes. Keep the existing assertions for argument count, filename, and node kind unchanged.crates/engine/src/sync/overlay.rs (1)
279-297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the empty-file case to the version-rendering test.
The test contrasts
NewNode::File { content: Some(..) }withNewNode::Folder. It does not coverNewNode::File { content: None }, which is the third branch the create arm discriminates. A future change that keyedcontent_versionoffnode.kind()instead ofstaged_content()would pass this test and still render a phantom version on every empty file.♻️ Proposed extra case
let bare = Op::create(id(2), id(0), "dir", NewNode::Folder, 1, AT); + let empty = Op::create(id(3), id(0), "e.txt", NewNode::File { content: None }, 1, AT); - let view = apply_overlay(&base, &[with_content, bare]); + let view = apply_overlay(&base, &[with_content, bare, empty]); assert_eq!(view.node(id(1)).unwrap().content_version, Some(1)); assert_eq!(view.node(id(2)).unwrap().content_version, None); + assert_eq!( + view.node(id(3)).unwrap().content_version, + None, + "an empty file create authors no version" + );🤖 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 `@crates/engine/src/sync/overlay.rs` around lines 279 - 297, Extend the test function overlay_renders_the_one_version_a_content_bearing_create_authors with a NewNode::File { content: None } create operation, apply it alongside the existing operations, and assert its rendered node has content_version set to None. Keep the existing assertions for content-bearing files and folders unchanged.
🤖 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 `@crates/engine/src/content/budget.rs`:
- Around line 119-120: Align the documentation for the reservation API,
including the `reserved()` docs around the `committed` calculation and the
related text near the later reservation logic, to state that each reservation
remains held in full until release. Keep the existing accounting implementation
unchanged; do not introduce staged-progress tracking.
In `@crates/engine/src/facade.rs`:
- Around line 1691-1746: Validate WriteTarget::Version in begin_write before
calling staged_bytes_total or admitting the reservation: require that the node
exists and is a file, rejecting unknown nodes and NodeKind::Folder with the
appropriate EngineError. Keep WriteTarget::NewFile behavior unchanged, and
ensure invalid version targets consume no staging work, node IDs, entropy, or
reservation.
- Around line 1914-1924: The error mapping for the seal_content_key call is
incorrectly classifying the failure as EngineError::Seam, which is documented
for host I/O failures and retryable scenarios, but seal_content_key failures are
deterministic and non-retryable. Replace the EngineError::Seam variant in the
.map_err block with either a new ContentKeySealFailed variant (modeling the seal
failure separately from host seams) or reuse the existing seal_error taxonomy
that already handles sealing errors. Preserve the error's check field in the
mapped result.
In `@crates/engine/src/sync/drain.rs`:
- Around line 1400-1411: The test section at lines 293-340 in
crates/engine/tests/sync.rs is named "staged bytes preserved" with assertions
that expect dead-lettered staged bytes to remain available, but this contradicts
the release_staged_blocks implementation which now drops staging bytes on every
abandonment. Rename the test section to reflect the actual drop behavior, update
the test's comment to match the current abandonment policy, and change the
assertions from expecting staged_bytes to exist to verifying that staged_bytes
is absent or properly dropped after abandonment occurs.
- Around line 1813-1823: The registered_by function should include
content_root_cid-derived retirement for every content-bearing operation, not
only OpKind::Create. Keep derive_write_name child retirement gated to Create,
while allowing UpdateContent and other applicable operations to return the
encoded content root so abandoned uploaded blocks can be released.
In `@crates/wasm/src/host.rs`:
- Around line 236-274: Validate size in begin_write before casting it to u64,
requiring it to be finite and non-negative like the existing
storage_headroom_bytes validation. Return a clear JsError for invalid values,
and preserve the existing write flow for valid sizes.
In `@packages/client/src/leaderRelay.ts`:
- Around line 218-220: Update the abortWrite arm in the relay step handler to
await this.transport.abortWrite(handle) before deleting handle from
this.writeOwners, matching the commitWrite policy so failed aborts leave the
handle owned and retryable. Preserve the existing successful abort return
behavior.
---
Outside diff comments:
In `@crates/fuse/src/error.rs`:
- Around line 172-192: Update the cause list in the
each_budget_keeps_its_own_cause test to include OverBudgetCause::TooManyWrites,
ensuring every OverBudgetCause variant is verified through the VfsError
conversion.
---
Nitpick comments:
In `@blueprint/core.md`:
- Around line 165-166: Update the content-key section in the manifest
documentation to describe its KAT vector families, content_key_accept and
content_key_reject, consistently with the neighboring tag entries.
In `@crates/core/examples/kat_gen.rs`:
- Around line 6437-6493: Add a new reject vector in build_content_key_reject
that uses reframe_content_key to set the enc field to a low-order value, similar
to how tampered_enc and wide_enc are constructed. This vector should mirror the
low-order enc vector pattern used in the settings-record family to pin the
contributory check behavior on the open_content_key path, alongside the existing
tampered_enc and wide_enc variants.
In `@crates/core/src/seal/mod.rs`:
- Line 15: Review the public API exposed by the seal module and make the
content_key module private if its helpers are already re-exported elsewhere and
no callers require the seal::content_key path. Update the module declaration in
seal and preserve the existing seal_* and open_* functions and constants as the
intended public API.
In `@crates/engine/examples/kat_gen.rs`:
- Around line 241-246: Reuse the existing leaf_cids variable from the assembly
setup for the later assertion and hexadecimal output in the kat generation flow,
removing the duplicate leaves.iter CID collection at those locations. Keep the
current assertion and encoding behavior unchanged while ensuring both consume
the same vector.
In `@crates/engine/src/content/chunk.rs`:
- Around line 16-20: Add a test function to verify that the SEALED_LEAF_OVERHEAD
constant matches the actual per-leaf overhead produced by seal_one_chunk. The
test should create a sample ContentKey and plaintext chunk, call seal_one_chunk
to produce a sealed leaf, and then assert that the sealed output length equals
the plaintext chunk length plus SEALED_LEAF_OVERHEAD. This pins the constant
against the real wire layout so that future changes to the sealing format are
caught before they silently break the staging admission ledger reservation
calculation.
In `@crates/engine/src/content/dag.rs`:
- Around line 572-599: The test root_block_len_matches_the_assembled_root does
not cover the CBOR head width boundary at 0x1_0000 where both the links-array
count and size uint widen from 3 bytes to 5 bytes. Extend the leaves array
(currently [0u64, 1, 23, 24, 255, 256, 300]) to include 65535 and 65536 so the
sizing arithmetic is pinned at this boundary transition and validates at every
leaf count where a CBOR head width changes.
In `@crates/engine/src/content/mod.rs`:
- Around line 12-22: The budget module declaration uses pub mod while all of its
exports (Refused, StagingLedger, sealed_total_bytes, ReservationId,
MAX_OPEN_WRITES) are pub(crate), which means the module exposes no public API.
Change pub mod budget to pub(crate) mod budget to align with the consistency
pattern already established by the limits module in the same visibility scope.
In `@crates/engine/src/content/write.rs`:
- Around line 241-268: Update the test the_pending_buffer_is_never_reallocated
to assert the expected push error when the input exceeds the declared size,
rather than breaking when remaining.len() equals rest.len(). Preserve the
capacity and observed_size assertions after validating the fail-closed behavior.
- Around line 74-92: Update ContentWriter::push to reject input when
self.observed plus bytes.len() exceeds the declared size, returning the
appropriate existing error before extending pending or updating observed.
Preserve normal chunk sealing and remainder handling for inputs within the
declared bound.
In `@crates/engine/src/sync/overlay.rs`:
- Around line 279-297: Extend the test function
overlay_renders_the_one_version_a_content_bearing_create_authors with a
NewNode::File { content: None } create operation, apply it alongside the
existing operations, and assert its rendered node has content_version set to
None. Keep the existing assertions for content-bearing files and folders
unchanged.
In `@crates/engine/src/sync/staging.rs`:
- Around line 128-157: The duplicated ContentWriter framing loop should be
centralized in a shared testkit helper. In crates/engine/src/sync/staging.rs
lines 128-157, update framed to call the helper while preserving its blocks,
root block, and StagedContent outputs; in crates/engine/tests/write_plane.rs
lines 899-921, replace frame_version’s duplicated loop with the same helper and
continue returning the root CID derived from finished.content.content_cid().
Expose one helper from crate::testkit for both call sites.
In `@crates/engine/tests/write_plane.rs`:
- Around line 1004-1119: Add assertions after tick in
an_evicted_prefix_of_the_block_set_is_loss_not_progress and
a_hole_in_the_staged_block_suffix_is_loss_and_fails_closed verifying the
remaining staged content is preserved via staged_keys() or staged_bytes_total(),
matching the documented unrecoverable-record behavior. Keep the existing
dead-letter and no-publish assertions, and optionally extract their shared
setup/assertion flow into a helper parameterized by the removed leaf index.
In `@packages/client/src/correlatedTransport.ts`:
- Around line 68-71: Introduce a named OpId type for durable operation
identifiers and update commitWrite to return Promise<OpId> instead of
Promise<bigint>. Propagate this alias through the worker protocol and all
implementations or consumers of commitWrite, while keeping WriteHandle for
beginWrite, pushChunk, and abortWrite.
In `@packages/client/src/worker/commandCodec.test.ts`:
- Around line 34-37: Update the test assertions around the recorded call to also
verify calls[0][0] equals the expected parent argument bytes, using the stubbed
{ bytes } value returned by NodeId.fromBytes. Keep the existing assertions for
argument count, filename, and node kind unchanged.
In `@packages/client/src/worker/engineHost.ts`:
- Around line 71-76: Refactor beginWrite so it narrows target once using a
single branch on the exclusive 'node' shape, deriving node, parent, and name
within the respective branches before calling this.handle.beginWrite. Preserve
the existing NodeId conversions and arguments for both target variants.
In `@packages/client/src/writeQueue.ts`:
- Around line 1-33: Add a direct unit test for the WriteQueue class covering
per-handle call-order serialization, independent progress for interleaved
handles, and continuation after a rejected step. Assert that only the failed run
rejects, the subsequent run executes, and the internal chains map is pruned
after each handle drains.
In `@packages/client/test/browser/journalEngine.worker.ts`:
- Around line 79-86: Update commitWrite in the journalEngine worker fake to
return a distinct durable operation id rather than the input handle, matching
the separate handle/op-id contract and the ReadHost values used by
serve.test.ts.
In `@packages/client/test/browser/leadership.ts`:
- Around line 104-119: Update the inner error handling in window.cbCreateFile so
an abortWrite failure cannot replace the original pushChunk or commitWrite
error; preserve and rethrow the initial failure while still attempting
abortWrite, allowing the outer settle(error) path to return the root cause.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b86fc20f-4b3e-4afd-bcfd-3e0511504eec
📒 Files selected for processing (64)
blueprint/core.mdcrates/contract/tests/contract.rscrates/core/examples/kat_gen.rscrates/core/kat/manifest.jsoncrates/core/kat/vectors/content_key/content_key_accept.jsoncrates/core/kat/vectors/content_key/content_key_reject.jsoncrates/core/src/seal/aad.rscrates/core/src/seal/content_key.rscrates/core/src/seal/mod.rscrates/core/tests/kat_manifest.rscrates/engine/examples/kat_gen.rscrates/engine/src/content/budget.rscrates/engine/src/content/chunk.rscrates/engine/src/content/dag.rscrates/engine/src/content/mod.rscrates/engine/src/content/write.rscrates/engine/src/entropy.rscrates/engine/src/facade.rscrates/engine/src/lib.rscrates/engine/src/net/author.rscrates/engine/src/storage_policy.rscrates/engine/src/sync/boot.rscrates/engine/src/sync/drain.rscrates/engine/src/sync/mod.rscrates/engine/src/sync/op.rscrates/engine/src/sync/overlay.rscrates/engine/src/sync/rebase.rscrates/engine/src/sync/record.rscrates/engine/src/sync/staging.rscrates/engine/tests/facade.rscrates/engine/tests/kat_content.rscrates/engine/tests/sync.rscrates/engine/tests/write_plane.rscrates/fuse/src/error.rscrates/fuse/src/ops.rscrates/fuse/tests/fuse_op_core.rscrates/wasm/src/host.rscrates/wasm/src/lib.rscrates/wasm/tests/boundary.rspackages/client/src/broadcast.tspackages/client/src/broadcastTransport.test.tspackages/client/src/broadcastTransport.tspackages/client/src/correlatedTransport.tspackages/client/src/engineClient.tspackages/client/src/facade.test.tspackages/client/src/facade.tspackages/client/src/index.tspackages/client/src/leaderRelay.tspackages/client/src/testkit.tspackages/client/src/transport.test.tspackages/client/src/transport.tspackages/client/src/worker/commandCodec.test.tspackages/client/src/worker/commandCodec.tspackages/client/src/worker/engineHost.tspackages/client/src/worker/engineWasm.tspackages/client/src/worker/protocol.tspackages/client/src/worker/serve.test.tspackages/client/src/worker/serve.tspackages/client/src/writeQueue.tspackages/client/test/browser/engineHarness.tspackages/client/test/browser/fakeEngine.worker.tspackages/client/test/browser/journalEngine.worker.tspackages/client/test/browser/leadership.spec.tspackages/client/test/browser/leadership.ts
💤 Files with no reviewable changes (1)
- crates/fuse/src/ops.rs
Retire the version root of any content-bearing op on abandonment, not just a create's: an abandoned updateContent had uploaded — and so charged — blocks that no record links and nothing ever retired. Validate a WriteTarget::Version before admission, so a version of an unknown or folder node is refused before it burns staging, entropy or budget rather than halting the drain after a whole upload. Classify a content-key seal failure as its own deterministic error instead of EngineError::Seam, which hosts retry as availability, and reject a non-finite or negative beginWrite size at the wasm boundary before the saturating cast turns it into a zero-byte reservation. Drop a relay write owner only once its abort settles, matching the commit arm, so a rejected abort leaves the handle retryable instead of stranding its reservation. Correct the release-on-abandonment docs to name the failure-valve paths they actually cover, add the low-order enc reject vector to the content-key KAT family, and document that family in the tag registry. Entire-Checkpoint: 37500a3daed1
|
All 26 review items worked in 67de2b0. The 7 inline threads carry their own replies and are resolved; the outside-diff comment and the 18 nitpicks have no threads, so their disposition is below. Outside diff range
Nitpicks
Split outThe Verification
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/sync/staging.rs (1)
185-214: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate every manifest leaf before queueing the content operation.
Root presence and root CID validation do not prove that every referenced leaf is staged. A valid root can reference a missing leaf. The queue can then retain an operation with no complete upload set.
Decode the root and require each referenced leaf to exist and address to its CID before
enqueue_op. Add a case that stages the valid root but omits one leaf, then assert that no operation is queued.As per path instructions,
blueprint/engine.mdrequires “CID verification on every block.”🤖 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 `@crates/engine/src/sync/staging.rs` around lines 185 - 214, Update the content-operation staging flow, including the logic exercised by a_content_op_enqueues_once_its_root_block_is_staged, to decode the staged root manifest and validate every referenced leaf before enqueue_op: each leaf must exist in staging and its contents must match its CID. Add a test alongside a_content_op_whose_root_is_not_staged_fails_closed_and_queues_nothing that stages the valid root while omitting one leaf, then verifies staging fails and queued_ops remains empty.Source: Path instructions
🤖 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 `@packages/client/src/writeQueue.test.ts`:
- Around line 1-69: Add named required CI status checks for both test suites:
configure a check that runs the Vitest suite containing WriteQueue tests in
packages/client/src/writeQueue.test.ts and another that runs the engine tests
covering the staging logic in crates/engine/src/sync/staging.rs. Ensure both
checks execute before merges and are enforced as required gates.
---
Outside diff comments:
In `@crates/engine/src/sync/staging.rs`:
- Around line 185-214: Update the content-operation staging flow, including the
logic exercised by a_content_op_enqueues_once_its_root_block_is_staged, to
decode the staged root manifest and validate every referenced leaf before
enqueue_op: each leaf must exist in staging and its contents must match its CID.
Add a test alongside
a_content_op_whose_root_is_not_staged_fails_closed_and_queues_nothing that
stages the valid root while omitting one leaf, then verifies staging fails and
queued_ops remains empty.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c312fc4-23d1-412e-9858-bc29b11a84ef
📒 Files selected for processing (26)
blueprint/core.mdcrates/core/examples/kat_gen.rscrates/core/kat/manifest.jsoncrates/core/kat/vectors/content_key/content_key_reject.jsoncrates/engine/examples/kat_gen.rscrates/engine/src/content/budget.rscrates/engine/src/content/chunk.rscrates/engine/src/content/dag.rscrates/engine/src/content/mod.rscrates/engine/src/content/write.rscrates/engine/src/facade.rscrates/engine/src/sync/drain.rscrates/engine/src/sync/overlay.rscrates/engine/src/sync/staging.rscrates/engine/src/testkit/content.rscrates/engine/src/testkit/mod.rscrates/engine/tests/sync.rscrates/engine/tests/write_plane.rscrates/fuse/src/error.rscrates/wasm/src/host.rspackages/client/src/leaderRelay.tspackages/client/src/worker/commandCodec.test.tspackages/client/src/worker/engineHost.tspackages/client/src/writeQueue.test.tspackages/client/test/browser/journalEngine.worker.tspackages/client/test/browser/leadership.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- crates/core/kat/manifest.json
- crates/engine/src/content/chunk.rs
- crates/fuse/src/error.rs
- packages/client/test/browser/leadership.ts
- packages/client/src/worker/commandCodec.test.ts
- crates/engine/src/sync/overlay.rs
- packages/client/src/worker/engineHost.ts
- crates/engine/src/content/dag.rs
- crates/engine/src/content/budget.rs
- crates/engine/tests/sync.rs
- crates/engine/examples/kat_gen.rs
- crates/engine/src/content/mod.rs
- crates/wasm/src/host.rs
- packages/client/src/leaderRelay.ts
- crates/engine/src/sync/drain.rs
- crates/engine/src/facade.rs
stage_op binds only the root: staging is mutable between the journal entry and the upload, so a leaf sweep here would pass and still leave the drain re-reading every block. Assert that a missing leaf enqueues rather than fails, so the division of responsibility is a test rather than prose.
|
Second pass worked. The actionable thread is replied to and resolved; the outside-diff-range item has no thread, so its disposition is here. Outside diff range —
|
1 participant
Content bytes flow end to end: sliced by the client, sealed and staged per block by the engine, uploaded and published by the drain, and downloaded and verified by a second device. Gives
ApiClient::uploadits first production caller and the contract suite its first live write coverage.What landed
Facade write handles.
Command::Create { content }andCommand::UpdateContentare gone;PlaintextContentis deleted. Content crosses asbeginWrite(target, size)→pushChunk(handle, bytes)*→commitWrite(handle), withabortWritefor the abandon path.ContentWritercopies only up to the chunk boundary and hands the rest back, so peak plaintext held is one chunk however much a caller pushes; the op is journaled once at commit.Budget admission.
beginWritereserves the exact sealed total —size + 40n + root_len, computed by assembling a root over fixed-width placeholder links — in an in-memory ledger of live handles, held frombeginWriteto release. Two handles opened before either stages a byte therefore contend for the budget instead of both being admitted against room only one can have.pushChunkenforces the declared shape and never re-checks the budget. The refusal quotesbudget - (staged + reserved)and names which of four actions applies: past the platform cap, cut below it by measured headroom, a transient backlog the drain frees, or an unmeasurable host.Per-block staging. A version stages as N+1 blocks, one staging key per block.
stage_oploses its budget and upload parameters and instead re-establishes the binding the drain depends on: the op's root CID must name bytes the store holds and that address to it. Orphan GC now expands a referenced root into the leaf keys its own manifest lists, and fails closed on a root it cannot produce or decode — collecting a live upload's leaves is unrecoverable.The per-version content key. A new
content-keystructure tag (0x0b) and acrates/coremodule sealing the key HPKE-to-self in auth mode under the ownerenc-subkey. The epoch rides the AAD as a value, never a key input, so a rotation between commit and drain leaves the blob openable — content bytes are never re-encrypted by any rotation path. ThecontentCidrides the sealed payload and is re-checked on open, so a blob cannot be moved onto another version's blocks. A key-open failure dead-letters the op asContentUnrecoverableand, uniquely among abandonments, releases its blocks: bytes no key opens are not recoverable work.Drain upload. File order, root last, each leaf removed on its confirmed
UploadResult, so the still-staged set is always a suffix. A block absent after a present one is loss, not progress, and fails closed. The root stays staged until the record publishes — it is the manifest every retry re-derives the plan from — then the whole set is released. Upload then publish; no CID pre-registration, sinceNameRegistrationhas no CID-only form.The remaining #823 guards, both structural.
new_childtakes aNewNodeBodycarrying exactly the body content its kind can hold, so a folder with a file version is unrepresentable;OpKind::Createmirrors it withNewNodeso the same shape cannot be journaled either.SealedContentcarriescontent_cid,size, andleaf_cidsfrom one decoded manifest and yields theVersion, soopen_content's manifest-vs-version size reject cannot fire on bytes this build authored. Both are release-active by construction and covered by tests that fire in a release build.content_cidsthreaded into held records. One list feeds bothRecordPublishRequestandHeldRecord, so a sub-EOL renewal re-registers exactly the content its record points at.The observed size. The
pushChunktotal is cross-checked against thebeginWritedeclaration at commit and against the staged root's own manifest at drain. A backing file truncated mid-read fails the commit rather than publishing a short version as a success.Coverage
crates/engine/tests/write_plane.rs: a file create round-trips its bytes to a second device that only ever saw the network; anupdateContentversion takes the head of the list and round-trips too; the whole block set is registered; a truncated file fails the commit and publishes nothing; a hole in the staged suffix fails closed; an unopenable key blob dead-letters and releases its blocks.crates/contract/tests/contract.rs: a live upload, register and retire leg over a version's whole block set, asserting per-block ingress, batch registration, quota accounting over the sum, and idempotent retirement.crates/engine/src/content/budget.rs: the reservation equals what staging actually holds for every framing shape, and concurrent handles cannot over-admit.Review gates
/simplify,/security-reviewand/crypto-privacy-reviewall ran on this diff. What they found and what changed:beginWriteallocated a full chunk per handle and sized its reservation by the sealed total, a 10,000× mis-accounting for a small declaration; andsealed_total_bytesmaterialised the whole leaf set before it could refuse, so an absurd declared size aborted the wasm instance instead of returningRootTooLarge. The buffer is now sized to the declaration, the root length is computed arithmetically (dag::root_block_len, held to the real encoder by a test), and open handles are capped.UPLOAD_MARK_KEY), and an absence past the mark is loss.orphan_staging_keysnow takes the live-handle set.{scope, epoch}, with negative tests for both transplant axes.contentKeysection with 2 accept and 13 reject vectors, plus a cross-structure separation test proving an op-record and a content-key blob never open as each other.pushChunks could apply out of order — scrambling a file while every integrity check still passed — and any tab could push into or abort another's handle, or strand its reservation by closing. The relay now serializes per handle, binds handles to their client, and aborts a departing client's writes.Also folded in: the flat-DAG ceiling is now its own error rather than a budget verdict quoting a meaningless figure; the admission refusal lost a dead arm;
commit_writereusesrecord_sealso the two seals to one recipient key cannot share an ephemeral; and speculative public exports were pulled back.Verification
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace,cargo check -p cipherbox-wasm --target wasm32-unknown-unknown --all-targets(which caught a match the workspace build cannot see), both KAT generators diff-clean,pnpm --filter @cipherbox/client test, rootpnpm typecheck,pnpm exec eslint .. The contract suite was run live against a real API, Postgres, and Kubo: 15 of 15 pass, before and after the review fixes.Surfaced, not fixed
#906 — the hosted ingress pins blocks under Kubo's own UnixFS address rather than the caller-computed content address, so a published record's
headCidand its registeredcontentCidsname bytes the accelerator does not hold. It predates this slice (the metadata publish path has the same dependency) but was unreachable untilApiClient::uploadgained a production caller. Confirmed empirically against the local stack. Fixing it needs an API surface change, so it is filed with dependency edges rather than folded in here.Deliberately left for their own slices, each noted rather than half-done:
orphan_staging_keysto a caller.LiveWrite/LiveWritessit onEnginetoday; the reviewer is right that they want to be acontent::writetype the facade merely drives. That is a refactor of working, tested code and is better done on its own.Closes #868
Closes #812
Closes #797
Part of #655
Unblocks #869, #871, #873, #896, #860 and #846.
Summary by CodeRabbit