feat: rotation soundness — content-key, inner-grant, concurrent-add, crash-safe resume#582
Merged
Merged
Conversation
Entire-Checkpoint: e93088839ad6
Entire-Checkpoint: 500072385b16
… hardening Entire-Checkpoint: e21e9fa3c13d
Entire-Checkpoint: 7fdbefa471e2
Entire-Checkpoint: a7f5d10d27cf
Entire-Checkpoint: 1e8ed6c455d4
Entire-Checkpoint: cc92facc65bc
Entire-Checkpoint: d727bfa43d77
- RED: 'throws when nodeId not provided' fails (function resolves, no guard yet) - RED: 'throws when nodeGeneration not provided' fails (function resolves, no guard yet) - Behavioral round-trip tests pass to document correct identity-preservation behavior - Uses real sealNode/unsealNode; mocks only IPFS/IPNS I/O layers Entire-Checkpoint: 3615bef8dac1
…ataAndPublish - Remove ?? crypto.randomUUID() and ?? 0 fallbacks (D-06 fix) - Add required nodeId: string and nodeGeneration: number to params type - Add runtime guards with descriptive error messages (AAD-stability enforcement) - Add nodeId/nodeGeneration as required fields on FolderState (sdk/types.ts) - Update existing folder.test.ts calls to supply the required fields - GREEN: all 5 registration.test.ts tests pass Entire-Checkpoint: 84bea983f14c
…RUD call sites - Add nodeId/nodeGeneration optional params to registerFolder (D-06 bridge) - Set nodeId/nodeGeneration in loadFolder from result.metadata.id/.generation - Pass folder.nodeId/folder.nodeGeneration to all six updateFolderMetadataAndPublish call sites: renameItem, moveItem (src+dst), deleteItem, uploadFile, uploadFiles - Fix FolderState test fixtures in 7 test files (Rule 1: caused by required fields) Entire-Checkpoint: 177ccaf415e2
…l (FLAG-63-U2) Entire-Checkpoint: 1f1ced14e87c
… moveItem (FLAG-63-U2) - Import sealChildReadKey, unsealChildReadKey, PublishedNode from @cipherbox/core - After sdkCore.moveItem() link rewrite, resolve child IPNS to get plaintext id/kind - Unseal movedRef.readKeySealed under source parent folderKey - Re-seal under dest parent folderKey using unchanged id/kind/generation - Zero recovered childReadKey (engine-derived, terminal-owned — D-09) - Do NOT zero sourceFolder.folderKey or destFolder.folderKey (caller-owned) Entire-Checkpoint: 8d37fe2fc8c8
…al plan Entire-Checkpoint: 9721d0ffb633
Entire-Checkpoint: 6cf80e49e9b2
- Union by ipnsName: local inserted first, remote overwrites on conflict - Remote wins on conflict so concurrent adds are never silently dropped - Intentional delete: base entry absent from both local and remote is pruned - One-sided delete: entry kept when at least one side still holds it - Pure structural merge: no crypto, no mutation of sealed bytes - Return type changed from never to SealedChildRef[] Entire-Checkpoint: 70a7d11743f2
Entire-Checkpoint: 6d39de02c928
- RED: mintFileKeyOnRotate assigns fresh 32-byte fileKey distinct from old key - RED: mintFileKeyOnRotate is no-op for folder nodes (no content field added) - RED: rotateOne sealNode integration — sealNode receives new fileKey after mint Entire-Checkpoint: 9b10bf0c5309
- Fill mintFileKeyOnRotate seam (ROT-03/CRIT-1): mint fileKey' = generateRandomBytes(32) and assign to node.content.fileKey when content present; folder nodes are no-op - Import generateRandomBytes from @cipherbox/crypto for cryptographic randomness - Remove obsolete 'mintFileKeyOnRotate throws phase 64' tests from Phase-63 seam suite - Green: all 19 engine tests pass; old readKey/fileKey holder cannot decrypt next version Entire-Checkpoint: 503247cc33ff
Entire-Checkpoint: 95e1a7552608
Entire-Checkpoint: 9003f3740525
- Assert rotateOne throws when nodeIpnsPrivateKey is absent - Assert publishWithCas not called with all-zero placeholder key - Assert BFS threads nodeKeySource key to child publishWithCas - Assert BFS throws when nodeKeySource returns undefined for a child - Add nodeKeySource? to RotationParams type (type-only seam, no threading yet) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUSYqMyCLbNGDLcn1cp1eh Entire-Checkpoint: 068bec968119
…eading - Add runtime guard in rotateOne: throws when nodeIpnsPrivateKey absent - Remove PLACEHOLDER_WRITE_KEY fallback from publishWithCas ipnsPrivateKey arg - Destructure nodeKeySource from RotationParams in rotateReadFromNode - Thread nodeKeySource keys to child and grandchild BFS queue items - Update all existing rotateOne tests to supply nodeIpnsPrivateKey - Update all rotateReadFromNode tests with children to supply rootIpnsPrivateKey and nodeKeySource; add unsealChildReadKey mock where missing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUSYqMyCLbNGDLcn1cp1eh Entire-Checkpoint: 5144d4bc942a
…t publish - Assert sealChildReadKey called with parent new readKey' for child re-seal - Assert publishWithCas called 3 times for root->child walk - Assert sealChildReadKey total call count is 3 for root->child walk Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUSYqMyCLbNGDLcn1cp1eh Entire-Checkpoint: 412974d990b3
- Add newSequenceNumber to RotateOneDone; capture publishWithCas return in rotateOne - Add ParentTrackingState and parentTracking Map to rotateReadFromNode BFS walker - After each child rotates, call sealChildReadKey with parent's NEW readKey' (D-02 fix) - Update parent's mutable SealedChildRef copy with re-sealed key and new generation - Call updateFolderMetadataAndPublish once when all children done (D-09 batched publish) - Add childPubId/childPubKind to BFS queue items for AAD binding in D-02 re-seal - Import updateFolderMetadataAndPublish from folder/registration - Fix TypeScript tuple destructuring in sealChildReadKey mock assertion Entire-Checkpoint: 91655dc8a1c4
Entire-Checkpoint: d29b43cfd1a3
- 4 RED tests for reMintGrantsRootedAt (ROT-04/HIGH-3/D-04) - Test 1: non-revoked grant → wrapKey + updateGrantFn; deleteGrantFn not called - Test 2: revoked grant → deleteGrantFn only; wrapKey + updateGrantFn not called - Test 3: mixed set → exactly one update and one delete with correct shareIds - Test 4: no callbacks supplied → clean no-op (no throw) - Extend reMintGrantsRootedAt signature with optional GrantRemintCallbacks param - Add GrantRemintCallbacks type (D-04 transport seam) - Add grantCallbacks field to RotateOneParams for call-site threading Entire-Checkpoint: ef5d06556ecc
- Fill reMintGrantsRootedAt body (ROT-04/HIGH-3/D-04) - Add wrapKey import from @cipherbox/crypto for ECIES descriptor minting - Add local bytesToBase64 helper for readDescriptorRef encoding - Enumerate grants via queryGrantsFn; delete revoked, re-mint non-revoked - Never zero newReadKey (caller is terminal owner per D-09) - Add grantCallbacks field to RotateOneParams; thread through call site - Update engine.test.ts seam test to reflect filled reMintGrantsRootedAt Entire-Checkpoint: 2f84097cb689
Entire-Checkpoint: cd15a6063aa9
Entire-Checkpoint: 30192904c769
Entire-Checkpoint: 7e9f7f2c3a63
…ergence guard
- Replace old throws-phase-64 test with 5 new ROT-06 tests (Tests 1-5)
- Test 1/2: verifySubtreeClean { isDirty, frontier } return shape (BFS dirty-edge)
- Test 3: resume guard with dirty child triggers D-09 re-publish not short-circuit
- Test 4: clean resume marks complete and persists (passes in RED too, regression gate)
- Test 5: fresh-job no-double-bump convergence guard skips child already at baseline+1
- Update verifySubtreeClean signature to accept rootIpnsName, rootReadKey, ctx
Entire-Checkpoint: 51f009ac09cf
…nvergence guard - Implement verifySubtreeClean: BFS read-only pass comparing SealedChildRef.generation (parent mirror) vs childPub.generation (plaintext) to rebuild dirty-edge frontier - Rewrite resume guard: call verifySubtreeClean before marking complete (Pitfall 5); dirty resume seeds BFS queue from frontier instead of short-circuiting to complete - Add enqueuedGeneration field to BFS queue items (parent mirror at enqueue time) - Add convergence guard before each rotateOne in BFS: skip if current published generation exceeds enqueued baseline (ROT-06 no-double-bump); still handle D-09 Entire-Checkpoint: 4749801642b4
… queue-key zeroization - Test 1: reMintGrantsRootedAt throws → nodeId must NOT be in completedNodeIds (D-07) - Test 2: terminal status=complete must be persisted via persistCallback (terminal persist) - Test 3: queue-derived child nodeReadKey must be zeroed after grandchildren enqueued; rootReadKey must NOT be zeroed (regression gate — caller is terminal owner per D-09) Entire-Checkpoint: 6d7dbbd5806f
…tion - Move completedNodeIds.add(nodeId) to AFTER reMintGrantsRootedAt succeeds (D-07): a failed re-mint no longer silently skips the node on resume - Add persistCallback call at terminal status=complete (Pitfall 5: advisory job record must be persisted at completion so the resumable walk gate is accurate) - Zero item.nodeReadKey after grandchildren enqueued (D-09 queue-key ownership): engine-derived BFS readKeys are zeroed once their consumers are enqueued; caller-supplied rootReadKey is never zeroed (caller is terminal owner) Entire-Checkpoint: 1dd05f2fedc4
Entire-Checkpoint: 086888ac926c
- Task 1: depth-2 tree (root→subfolder→file) happy-path rotation; all nodes advance to gen=1; post-rotation read-chain navigation under new keys proves multi-level D-02 re-seal; pre-rotation grant returns behind-retry - Task 2: crash at final persistCallback (call 4, after all D-09s complete); fresh resume with seeded completedNodeIds and readKeyPrime converges with isDirty=false, no double-bump, zero getRandomValues calls on resume - Task 3: concurrent IPNS write to parent between root commit and D-09 batched republish forces CAS-409; mergeChildren (ROT-05/HIGH-4) absorbs the conflict and the concurrently-added child survives in the final parent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUSYqMyCLbNGDLcn1cp1eh Entire-Checkpoint: c7801eb52d88
Entire-Checkpoint: e69fa8406d74
…esh-record resume) Entire-Checkpoint: 7ed06cb1ff18
Entire-Checkpoint: 36441ac5f1c9
Entire-Checkpoint: 4b277c878d83
Entire-Checkpoint: 1226147f936f
github-actions
Bot
added
release:sdk-core:feat
Contributor
Release Preview
Cascade Details
|
|
Warning Review limit reached
Next review available in: 24 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 Run ID: 📒 Files selected for processing (8)
WalkthroughPhase 64 "Rotation Soundness — Revocation Guarantees" fills four previously stubbed rotation engine seams ( ChangesPhase 64 Rotation Soundness
Sequence DiagramsequenceDiagram
participant caller as rotateReadFromNode caller
participant BFS as BFS loop
participant rotateOne
participant engine as engine seams
participant ipns as IPNS / publishWithCas
participant parent as Parent re-seal + republish
caller->>BFS: rotateReadFromNode(root, rootReadKey, jobRecord, ...)
alt root already in completedNodeIds
BFS->>engine: verifySubtreeClean(rootIpnsName, rootReadKey, ctx)
engine-->>BFS: {isDirty, frontier}
alt dirty
BFS->>BFS: seed queue from frontier
else clean
BFS-->>caller: status=complete + persistCallback
end
end
loop each queued node
alt generation already converged
BFS->>BFS: skip rotateOne, decrement parent pendingChildren
else
BFS->>rotateOne: rotateOne(node, parentReadKey, nodeIpnsPrivateKey, ...)
rotateOne->>rotateOne: fail-closed guard (throw if no nodeIpnsPrivateKey)
rotateOne->>engine: mintFileKeyOnRotate(node)
rotateOne->>ipns: publishWithCas(sealedNode)
ipns-->>rotateOne: casResult (or 409 conflict)
opt 409 conflict
rotateOne->>engine: mergeConcurrentChildren(base, remote, oldKey, local, newKey)
engine-->>rotateOne: merged PublishedNode
end
rotateOne->>engine: reMintGrantsRootedAt(nodeId, newReadKey, gen, callbacks)
rotateOne->>BFS: completedNodeIds.add(nodeId)
end
BFS->>parent: sealChildReadKey under parent newReadKey
opt all children processed
BFS->>ipns: batched parent republish
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR implements Phase 64's rotation soundness guarantees, filling four previously-stubbed seams: content-key rotation (
Confidence Score: 5/5The rotation walk is safe to merge: revocation cuts at the root, merged parent bodies are under readKey prime, and the D-02 re-seal fix closes the Phase-63 read-chain bug. The four previously-stubbed seams are fully implemented with correct key zeroization, D-01 fail-closed enforcement, and D-09 batched republish that correctly re-seals child readKeys under the parent's new key for all normally-exercised paths. Accepted deferrals do not create confidentiality bypasses. The convergence guard readKeySealed gap requires an unlikely concurrent external rotation during the walk to trigger and is not reachable in the tested crash scenario. The BFS convergence guard path in engine.ts (lines 914-944) merits a follow-up todo before the Phase-68 dirty-resume wiring lands; the e2e test capturedReadKeys index documentation should be updated to list fileKeyPrime entries for file nodes. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant rotateReadFromNode
participant rotateOne
participant reMintGrantsRootedAt
participant updateFolderMetadataAndPublish
participant verifySubtreeClean
Caller->>rotateReadFromNode: rootReadKey, jobRecord, nodeKeySource
rotateReadFromNode->>rotateOne: root node (D-01 key guard)
rotateOne-->>rotateReadFromNode: readKeyPrime, children, newSequenceNumber
alt rootResult.skipped (resume path)
rotateReadFromNode->>verifySubtreeClean: rootIpnsName, rootReadKey
verifySubtreeClean-->>rotateReadFromNode: isDirty, frontier
note over rotateReadFromNode: Seed BFS from dirty frontier
else normal path
note over rotateReadFromNode: Set up parentTracking for root
end
loop BFS frontier
rotateReadFromNode->>rotateReadFromNode: convergence guard (generation check)
alt child already rotated
rotateReadFromNode->>rotateReadFromNode: update generation only, decrement pendingChildCount
else
rotateReadFromNode->>rotateOne: child node (per-node IPNS key)
rotateOne->>reMintGrantsRootedAt: nodeId, readKeyPrime, grantCallbacks
rotateOne-->>rotateReadFromNode: childReadKey prime, newGeneration, newSequenceNumber
note over rotateReadFromNode: D-02 sealChildReadKey under parent readKey prime
note over rotateReadFromNode: Update parentState.children in-place
end
alt "pendingChildCount == 0"
rotateReadFromNode->>updateFolderMetadataAndPublish: D-09 batched parent republish
end
end
rotateReadFromNode->>Caller: "status = complete, persist"
%%{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 rotateReadFromNode
participant rotateOne
participant reMintGrantsRootedAt
participant updateFolderMetadataAndPublish
participant verifySubtreeClean
Caller->>rotateReadFromNode: rootReadKey, jobRecord, nodeKeySource
rotateReadFromNode->>rotateOne: root node (D-01 key guard)
rotateOne-->>rotateReadFromNode: readKeyPrime, children, newSequenceNumber
alt rootResult.skipped (resume path)
rotateReadFromNode->>verifySubtreeClean: rootIpnsName, rootReadKey
verifySubtreeClean-->>rotateReadFromNode: isDirty, frontier
note over rotateReadFromNode: Seed BFS from dirty frontier
else normal path
note over rotateReadFromNode: Set up parentTracking for root
end
loop BFS frontier
rotateReadFromNode->>rotateReadFromNode: convergence guard (generation check)
alt child already rotated
rotateReadFromNode->>rotateReadFromNode: update generation only, decrement pendingChildCount
else
rotateReadFromNode->>rotateOne: child node (per-node IPNS key)
rotateOne->>reMintGrantsRootedAt: nodeId, readKeyPrime, grantCallbacks
rotateOne-->>rotateReadFromNode: childReadKey prime, newGeneration, newSequenceNumber
note over rotateReadFromNode: D-02 sealChildReadKey under parent readKey prime
note over rotateReadFromNode: Update parentState.children in-place
end
alt "pendingChildCount == 0"
rotateReadFromNode->>updateFolderMetadataAndPublish: D-09 batched parent republish
end
end
rotateReadFromNode->>Caller: "status = complete, persist"
Reviews (3): Last reviewed commit: "fix(64): harden rotation D-01/D-09 guard..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-core/src/rotation/engine.ts (1)
208-233: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake grant re-minting reachable from full-tree rotation.
reMintGrantsRootedAtonly runs whenrotateOnereceivesinnerGrants, butRotationParamshas no grant callbacks/grant signal androtateReadFromNodenever forwards them for root or BFS children. Full-tree rotations can therefore mark nodes complete without updating non-revoked grants or deleting revoked grant rows.Also applies to: 679-689, 924-934, 599-614
🤖 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/sdk-core/src/rotation/engine.ts` around lines 208 - 233, Full-tree rotation currently cannot reach grant re-minting because RotationParams has no grant-related inputs and rotateReadFromNode does not pass any grant signal into rotateOne, so reMintGrantsRootedAt never runs for root or BFS children. Extend RotationParams with the needed grant callback/signal and thread it through rotateReadFromNode into rotateOne for every node in the tree. Ensure the full rotation path can invoke reMintGrantsRootedAt so non-revoked grants are updated and revoked grant rows are deleted before nodes are marked complete.
🧹 Nitpick comments (4)
tests/sdk-e2e/src/suites/rotation-crash-safety.test.ts (1)
698-717: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the concurrent child is actually readable, not just present.
The test only checks
ipnsNamemembership. It would still pass if the CAS merge preserved a stalereadKeySealedthat cannot be unsealed withreadKeyPrimeRoot3. Add a navigation/unseal assertion forconcurrentIpnsName.As per path instructions,
**/*.test.ts: “Focus on test coverage, edge cases, and test quality. Ensure tests are meaningful and not just for coverage metrics.”🤖 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 `@tests/sdk-e2e/src/suites/rotation-crash-safety.test.ts` around lines 698 - 717, The assertion in the root3 merge test only verifies that concurrentIpnsName is listed in children, which can miss a stale readKeySealed that cannot actually be opened. Update the checks around root3FinalNode in the rotation-crash-safety test to navigate to the concurrent child and unseal it using readKeyPrimeRoot3, alongside the existing ipnsName membership assertions. Use the existing helpers unsealNode, fetchFromIpfs, resolveIpnsRecord, and the child traversal from root3FinalNode.children to confirm the concurrent child is truly readable after the CAS-409 merge.Source: Path instructions
packages/sdk/src/__tests__/client-load-reconcile.test.ts (1)
78-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a same-sequence backfill regression here.
These fixtures now seed the placeholder
nodeId/nodeGeneration, but the suite still doesn't assert that an equal-sequenceloadFolder()replaces them withmetadata.id/metadata.generation. That is the bridge contractregisterFolder()now depends on, so this regression currently slips through. As per path instructions,**/*.test.ts: Focus on test coverage, edge cases, and test quality. Ensure tests are meaningful and not just for coverage metrics.Also applies to: 142-143
🤖 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/sdk/src/__tests__/client-load-reconcile.test.ts` around lines 78 - 79, Add a regression test in client-load-reconcile.test.ts around the loadFolder()/registerFolder flow that starts with placeholder nodeId and nodeGeneration, then asserts an equal-sequence loadFolder() backfills metadata.id and metadata.generation onto the existing record. Use the relevant loadFolder and registerFolder test setup/fixtures already in the suite, and verify the placeholder values are replaced rather than left unchanged, since this is the bridge contract the reconciliation logic depends on.Source: Path instructions
packages/sdk-core/src/__tests__/folder-merge.test.ts (1)
119-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid asserting referential identity here.
remote winsis a data-semantic guarantee, not an aliasing guarantee.expect(result[0]).toBe(remote)will fail on valid implementations that clone or normalize the chosen entry before returning it. Assert the selected fields instead.Suggested fix
expect(result).toHaveLength(1); expect(result[0].readKeySealed).toBe('remote-sealed'); - expect(result[0]).toBe(remote); + expect(result[0]).toMatchObject({ + ipnsName: remote.ipnsName, + generation: remote.generation, + readKeySealed: remote.readKeySealed, + });As per path instructions,
**/*.test.ts: “Focus on test coverage, edge cases, and test quality. Ensure tests are meaningful and not just for coverage metrics.”🤖 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/sdk-core/src/__tests__/folder-merge.test.ts` around lines 119 - 127, The test in mergeChildren is over-specifying implementation details by asserting referential identity with expect(result[0]).toBe(remote), which can fail even when remote wins correctly. Update the assertion to verify the data-semantic outcome only by checking the selected fields on result[0] in folder-merge.test.ts, keeping the existing mergeChildren scenario and symbols like makeChild and mergeChildren intact.Source: Path instructions
packages/sdk-core/src/__tests__/folder/move-reseal.test.ts (1)
139-202: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThis test only proves generation binding, not full identity binding.
The title and comments say id/kind/generation stay bound, but the only negative case here is
generation + 1. A regression that stopped includingidorkindin the AAD would still pass this suite. Please add rejects for wrongidand wrongkindas well.As per path instructions,
**/*.test.ts: “Focus on test coverage, edge cases, and test quality. Ensure tests are meaningful and not just for coverage metrics.”🤖 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/sdk-core/src/__tests__/folder/move-reseal.test.ts` around lines 139 - 202, The re-seal test in `move-reseal.test.ts` only verifies the generation binding, so it would miss regressions where `sealChildReadKey` or `unsealChildReadKey` stop binding `id` or `kind` into the AAD. Update the `re-seal preserves child node id, kind, and generation` test to add negative cases that attempt to unseal the sealed blob with an incorrect child `id` and an incorrect `kind`, and assert both reject with `CryptoError`, alongside the existing wrong-generation check.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/sdk-core/src/rotation/engine.ts`:
- Around line 891-917: The skip path in rotation/engine.ts only updates the
child’s generation and then republishes the parent, but it leaves the child’s
sealed read key stale. Update the convergence-skip branch around currentPub,
parentTracking, and updateFolderMetadataAndPublish so that a skipped child is
re-sealed under the parent’s new read key before publishing, and ensure any
descendant rotation state for that child is also propagated instead of being
bypassed.
- Around line 426-439: verifySubtreeClean currently only checks
rootNode.children and misses stale deeper descendants, so make the traversal
recursive through the subtree instead of stopping at immediate children. Update
verifySubtreeClean to descend into each child PublishedNode (using
resolveIpnsRecord and fetchFromIpfs as needed), compare generations at every
edge, and accumulate any dirty nodes into frontier so isDirty reflects stale
grandchild and deeper links too. Keep the existing childRef, childResolved,
childPub, and frontier flow, but extend it so the function walks all descendants
before returning.
- Around line 789-803: The skipped-root dirty-resume path in engine.ts can
continue even when the root IPNS private key is missing, which later causes the
non-null assertion on parentState.parentIpnsPrivateKey to reach
updateFolderMetadataAndPublish unsafely. Add an explicit fail-closed guard in
the dirty-resume flow before seeding parentTracking for rootNodeIpnsName so the
republish path is aborted or surfaced as an error when rootIpnsPrivateKey is
undefined. Make the check in the same area as the parentTracking.set logic and
ensure the later parentState.parentIpnsPrivateKey usage is only reachable when
the key is present.
- Around line 383-392: In mergeConcurrentChildren, remote-only SealedChildRef
entries are being carried forward without re-sealing them under the new parent
read key, and returning node.children prevents the BFS from rotating or D-02
re-sealing those children. Update the merge flow to rebuild or rewrap merged
child refs for the new chain before calling sealNode on the merged node, and
ensure the child traversal path does not just reuse node.children but processes
the merged children so remote-only refs are re-sealed under
readKeyPrime/newReadKey.
- Around line 504-512: The guard in rotateOne only rejects missing IPNS keys, so
placeholder or malformed key material can still pass through. Tighten the check
around nodeIpnsPrivateKey in engine.ts to validate that the provided key is a
real IPNS signing key, not just a non-empty Uint8Array, before publishWithCas is
reached. Use the rotateOne and nodeIpnsPrivateKey symbols to locate the guard
and replace the current undefined-only condition with stricter validation plus
the same fail-closed error path.
In `@packages/sdk/src/__tests__/client-move-reencrypt.test.ts`:
- Around line 102-103: The move fixtures in client-move-reencrypt.test.ts use an
empty nodeId, but moveItem() now passes nodeId through
updateFolderMetadataAndPublish() where falsy values throw, so update the
affected fixture objects to use a valid non-empty nodeId while keeping
nodeGeneration unchanged. Make this change for both of the move cases referenced
in the test so the setup exercises the re-encryption path instead of failing
during fixture validation.
In `@packages/sdk/src/__tests__/client.test.ts`:
- Around line 295-296: The deleteItem test fixture is using an invalid folder
state by seeding a falsy nodeId, which no longer matches the metadata-publish
contract. Update the test data in client.test.ts for the deleteItem case to use
a valid non-empty nodeId so the test exercises the actual delete flow; keep the
rest of the fixture aligned with the delete path and use the deleteItem-related
test setup as the locator.
In `@packages/sdk/src/__tests__/helpers.ts`:
- Around line 58-59: The shared test helper is seeding an invalid folder fixture
because setupFolder() uses an empty nodeId, which now violates the publish
contract. Update the helper in helpers.ts to initialize nodeId with a stable
non-empty placeholder so tests that rely on this fixture can reach the publish
path; use setupFolder() and the folder seed fields as the main symbols to locate
the change.
In `@packages/sdk/src/client.ts`:
- Around line 286-299: The cached-folder path in loadFolder() leaves the
placeholder node identity from registerFolder() unchanged when
existing.sequenceNumber matches the loaded record, so later CRUD calls still see
nodeId missing. Update the equal-sequence branch in loadFolder() to merge
result.metadata.id and result.metadata.generation into the cached folder entry
before returning it, keeping the cache consistent for renameItem, deleteItem,
and upload* flows.
- Around line 577-594: The re-seal lookup in moveItem is using the caller-facing
childId instead of the moved node’s ipnsName, so the ref match and IPNS
resolution can fail after the move. Update the moveItem flow in client.ts to
capture the matched updatedDest entry from the ipnsName search and use that node
identifier for both the resolveIpnsRecord call and the subsequent fetch/re-seal
path, or have sdkCore.moveItem() return the moved ref’s ipnsName and thread that
through consistently.
---
Outside diff comments:
In `@packages/sdk-core/src/rotation/engine.ts`:
- Around line 208-233: Full-tree rotation currently cannot reach grant
re-minting because RotationParams has no grant-related inputs and
rotateReadFromNode does not pass any grant signal into rotateOne, so
reMintGrantsRootedAt never runs for root or BFS children. Extend RotationParams
with the needed grant callback/signal and thread it through rotateReadFromNode
into rotateOne for every node in the tree. Ensure the full rotation path can
invoke reMintGrantsRootedAt so non-revoked grants are updated and revoked grant
rows are deleted before nodes are marked complete.
---
Nitpick comments:
In `@packages/sdk-core/src/__tests__/folder-merge.test.ts`:
- Around line 119-127: The test in mergeChildren is over-specifying
implementation details by asserting referential identity with
expect(result[0]).toBe(remote), which can fail even when remote wins correctly.
Update the assertion to verify the data-semantic outcome only by checking the
selected fields on result[0] in folder-merge.test.ts, keeping the existing
mergeChildren scenario and symbols like makeChild and mergeChildren intact.
In `@packages/sdk-core/src/__tests__/folder/move-reseal.test.ts`:
- Around line 139-202: The re-seal test in `move-reseal.test.ts` only verifies
the generation binding, so it would miss regressions where `sealChildReadKey` or
`unsealChildReadKey` stop binding `id` or `kind` into the AAD. Update the
`re-seal preserves child node id, kind, and generation` test to add negative
cases that attempt to unseal the sealed blob with an incorrect child `id` and an
incorrect `kind`, and assert both reject with `CryptoError`, alongside the
existing wrong-generation check.
In `@packages/sdk/src/__tests__/client-load-reconcile.test.ts`:
- Around line 78-79: Add a regression test in client-load-reconcile.test.ts
around the loadFolder()/registerFolder flow that starts with placeholder nodeId
and nodeGeneration, then asserts an equal-sequence loadFolder() backfills
metadata.id and metadata.generation onto the existing record. Use the relevant
loadFolder and registerFolder test setup/fixtures already in the suite, and
verify the placeholder values are replaced rather than left unchanged, since
this is the bridge contract the reconciliation logic depends on.
In `@tests/sdk-e2e/src/suites/rotation-crash-safety.test.ts`:
- Around line 698-717: The assertion in the root3 merge test only verifies that
concurrentIpnsName is listed in children, which can miss a stale readKeySealed
that cannot actually be opened. Update the checks around root3FinalNode in the
rotation-crash-safety test to navigate to the concurrent child and unseal it
using readKeyPrimeRoot3, alongside the existing ipnsName membership assertions.
Use the existing helpers unsealNode, fetchFromIpfs, resolveIpnsRecord, and the
child traversal from root3FinalNode.children to confirm the concurrent child is
truly readable after the CAS-409 merge.
🪄 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
Run ID: 6d28dba0-3109-4244-9fcc-a5c6651e23dc
📒 Files selected for processing (49)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-01-PLAN.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-01-SUMMARY.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-02-PLAN.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-02-SUMMARY.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-03-PLAN.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-03-SUMMARY.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-04-PLAN.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-04-SUMMARY.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-05-PLAN.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-05-SUMMARY.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-06-PLAN.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-06-SUMMARY.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-07-PLAN.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-07-SUMMARY.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-08-PLAN.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-08-SUMMARY.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-CONTEXT.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-DISCUSSION-LOG.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-PATTERNS.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-RESEARCH.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-SECURITY.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-VALIDATION.md.planning/phases/64-rotation-soundness-revocation-guarantees/64-VERIFICATION.md.planning/todos/pending/2026-06-29-rotation-coderabbit-followups-deferred.md.planning/todos/pending/2026-06-29-rotation-concurrent-add-merge-downgrades-rotated-child-readkey.md.planning/todos/pending/2026-06-29-rotation-fresh-record-resume-and-sc4-double-bump.mdpackages/sdk-core/src/__tests__/folder-merge.test.tspackages/sdk-core/src/__tests__/folder.test.tspackages/sdk-core/src/__tests__/folder/move-reseal.test.tspackages/sdk-core/src/__tests__/folder/registration.test.tspackages/sdk-core/src/__tests__/rotation/engine.test.tspackages/sdk-core/src/__tests__/rotation/grant-remint.test.tspackages/sdk-core/src/cas.tspackages/sdk-core/src/folder/merge.tspackages/sdk-core/src/folder/registration.tspackages/sdk-core/src/rotation/engine.tspackages/sdk/src/__tests__/bin.test.tspackages/sdk/src/__tests__/client-load-reconcile.test.tspackages/sdk/src/__tests__/client-move-reencrypt.test.tspackages/sdk/src/__tests__/client.test.tspackages/sdk/src/__tests__/collect-subtree-ipns-names.test.tspackages/sdk/src/__tests__/ensure-folder-loaded.test.tspackages/sdk/src/__tests__/helpers.tspackages/sdk/src/client.tspackages/sdk/src/types.tstests/sdk-e2e/src/suites/rotation-crash-safety.test.ts
The Phase-64 moveItem destination re-seal (FLAG-63-U2) looked up the moved child in updatedDest by childId (a UUID handle), but SealedChildRef carries no id (NODE-03) — it is keyed by ipnsName. The lookup always missed and threw "moved child <id> not found in dest after link rewrite", breaking the active client-extended.test.ts > moveItem CI test (the Test job). Use the movedRef that sdkCore.moveItem() already returns, locate the real entry in updatedDest by movedRef.ipnsName, and resolve the child IPNS by ipnsName. Strengthen the unit test to mock the IPNS resolve/fetch + seal/unseal and assert the published dest entry carries the re-sealed key (CodeRabbit #2 fixed, #3 addressed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUSYqMyCLbNGDLcn1cp1eh Entire-Checkpoint: db39705f39f0
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #582 +/- ##
===========================================
+ Coverage 63.76% 83.65% +19.89%
===========================================
Files 152 116 -36
Lines 11246 7367 -3879
Branches 1252 1314 +62
===========================================
- Hits 7171 6163 -1008
+ Misses 3832 961 -2871
Partials 243 243
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Apply the low-risk quick-win findings from the CodeRabbit + greptile PR reviews; defer the heavy rotation-soundness rework (RR-01 merge re-enqueue, recursive verifySubtreeClean, convergence-skip re-seal) to its existing Phase 66/68 todos. - D-09: mintFileKeyOnRotate now zeroes the pre-rotation node.content.fileKey before overwriting (safe — node is a fresh unsealNode output, engine-owned). - D-01: rotateOne's fail-closed guard rejects malformed/all-zero/wrong-length IPNS keys, not just undefined (keys are the 32-byte Ed25519 seed). - D-01: dirty-resume root republish fails closed when rootIpnsPrivateKey is missing, before seeding parentTracking (no more parentIpnsPrivateKey! leaking undefined into updateFolderMetadataAndPublish). - Test fixtures (helpers.setupFolder, client.test, client-move-reencrypt.test) seed stable non-empty nodeId placeholders per the folder publish contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUSYqMyCLbNGDLcn1cp1eh Entire-Checkpoint: c85e68fa1b12
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.
Phase 64 — Rotation Soundness: Revocation Guarantees
Makes the Phase-63 rotation engine cryptographically sound by filling its four stubbed seams and hardening the multi-node BFS walk, then proving it against a live local API stack. Read-revocation now closes all three crypto gaps and survives a crash mid-walk (within the achievable mid-milestone scope).
What changed
mintFileKeyOnRotatemints a freshfileKey'on file rotation so a holder of the oldreadKey/fileKeycannot decrypt the next published version (lazycontentRekeyPendingwiring deferred to Phase 65 — thenode/v3schema is frozen this phase).reMintGrantsRootedAtre-mintsreadDescriptorRef(ECIES) for every non-revoked recipient incl. inner grants and deletes the revoked recipient's row, behind a transport-decoupled callback seam (livesharesschema is Phase 66).mergeChildrenthree-way merge +mergeConcurrentChildrenon a CAS-409 re-fetch the parent, merge concurrently-addedSealedChildRefs, and re-seal — a concurrent add is never dropped.verifySubtreeCleanrebuilds the resume frontier from published IPNS truth; the D-07 walk hardening lands (completedNodeIdsordering, resume guard, terminal-status persist, queue-key zeroization, per-node convergence guard).nodeKeySource).SealedChildRef[N].readKeySealedis re-sealed under the parent's NEWreadKey'out-of-band in the caller and republished (the Phase-63 bug: it was sealed under the child's own key and never written back).updateFolderMetadataAndPublishrequires stablenodeId/nodeGeneration(threaded through all sixclient.tscall sites);moveItemre-seals the moved child under the destination parent'sreadKey.tests/sdk-e2e/src/suites/rotation-crash-safety.test.tsabort-and-resume + concurrent-add suite (3/3 green against the live stack).Verification
readKey').describe.skip-quarantined mid-milestone ("SDK runtime stubbed, re-enable at phase 63-65 consumer re-wire").Accepted deferrals (user-decided, todos captured)
2026-06-29-rotation-concurrent-add-merge-downgrades-rotated-child-readkey— HIGH-4 re-merge uses remote-wins, which downgrades an already-rotated child'sreadKeySealedon concurrent-add (liveness regression, not a revocation bypass). The minimal "never dropped" property is met; the local-wins fix is deferred.2026-06-29-rotation-fresh-record-resume-and-sc4-double-bump— true fresh-record crash-resume of an already-rotated root needs the Phase-68 durable client floor; SC#4's "without double-bumping" wording also contradicts design §4.5 (double-rotation is the safe recovery). Both → Phase 68.Notes
cas.test.ts(12) andshare/grant.test.ts(11) predate this phase and are invisible to CI (tsconfig.build.jsonexcludessrc/**/*.test.ts); not a Phase-64 regression.CodeRabbit review (local CLI, pre-merge)
17 findings triaged. 7 low-risk fixed in
d65cf5d2b(doc sync,nodeGenerationfinite-integer validation, zeroization completeness inmoveItem/BFS-queue/mintFileKeyOnRotate-failure-path, e2e key hygiene). 1 false positive skipped (a block-scopedpublishCallsconst flagged as a duplicate). The remaining ~9 are in the rotation-soundness domain already deferred above — captured in2026-06-29-rotation-coderabbit-followups-deferred(merge re-enqueue + e2e assertion → RR-01;verifySubtreeCleandepth + frontierpendingChildCount+ resume fail-closed → RR-02;grantCallbacksthreading → Phase 66;client.tsmove/FolderState items → the SDK consumer re-wire).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes