Add structured bundle and page storage#63
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds immutable page blobs and deterministic structured bundles to the CAS, with typed handles, bounded fanout construction, targeted member reads, full graph validation, retention/publication support, Git object-size queries, public API updates, and comprehensive tests and documentation. ChangesStructured storage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Application
participant ContentAddressableStore
participant PageService
participant BundleService
participant GitPersistenceAdapter
Application->>ContentAddressableStore: pages.put(source)
ContentAddressableStore->>PageService: store bounded page
PageService->>GitPersistenceAdapter: writeBlob(bytes)
PageService-->>ContentAddressableStore: PageHandle
Application->>ContentAddressableStore: bundles.putOrdered(members)
ContentAddressableStore->>BundleService: build fanout tree
BundleService->>GitPersistenceAdapter: write descriptors and tree nodes
BundleService-->>ContentAddressableStore: BundleHandle
Application->>ContentAddressableStore: bundles.openMember(bundle, path)
ContentAddressableStore->>BundleService: resolve selected path
BundleService->>GitPersistenceAdapter: read selected descriptor and payload
BundleService-->>Application: member byte stream
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/infrastructure/adapters/GitPersistenceAdapter.js (1)
189-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate missing-object error-mapping logic.
readObjectSizerepeats the exact try/catch →isMissingGitObjectError→CasError(GIT_OBJECT_NOT_FOUND)pattern already used inreadObjectType. Extracting a shared private helper would keep both methods in sync if the mapping ever changes.♻️ Proposed refactor
+ async `#executeMappingMissingObject`(args, oid) { + try { + return await this.policy.execute(() => this.plumbing.execute({ args })); + } catch (err) { + if (isMissingGitObjectError(err)) { + throw new CasError(`Git object not found: ${oid}`, ErrorCodes.GIT_OBJECT_NOT_FOUND, { + oid, + originalError: err, + }); + } + throw err; + } + } + async readObjectType(oid) { - try { - return await this.policy.execute(() => - this.plumbing.execute({ args: ['cat-file', '-t', oid] }), - ); - } catch (err) { - if (isMissingGitObjectError(err)) { - throw new CasError(`Git object not found: ${oid}`, ErrorCodes.GIT_OBJECT_NOT_FOUND, { - oid, - originalError: err, - }); - } - throw err; - } + return this.#executeMappingMissingObject(['cat-file', '-t', oid], oid); } async readObjectSize(oid) { - let output; - try { - output = await this.policy.execute(() => - this.plumbing.execute({ args: ['cat-file', '-s', oid] }), - ); - } catch (err) { - if (isMissingGitObjectError(err)) { - throw new CasError(`Git object not found: ${oid}`, ErrorCodes.GIT_OBJECT_NOT_FOUND, { - oid, - originalError: err, - }); - } - throw err; - } + const output = await this.#executeMappingMissingObject(['cat-file', '-s', oid], oid); const size = Number(output);🤖 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 `@src/infrastructure/adapters/GitPersistenceAdapter.js` around lines 189 - 233, Extract the repeated missing-object error mapping from readObjectType and readObjectSize into a shared private helper on GitPersistenceAdapter. Have both methods use the helper while preserving propagation of non-missing-object errors and the existing readObjectSize validation behavior.
🤖 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 `@src/domain/helpers/handleTarget.js`:
- Around line 32-47: Update mapHandleTargetError’s missing-error message
extraction to read a string `.message` from any thrown value, not only Error
instances, before falling back to String(error). Preserve the existing
object/blob/tree “not found” matching and HANDLE_TARGET_MISSING mapping
behavior.
In `@src/domain/services/BundleDescriptorCodec.js`:
- Around line 231-246: Update assertCanonicalPath so normalizeBundlePath
failures and non-canonical round-trip results are handled separately: wrap only
the normalizeBundlePath call in the corrupt error handling with its thrown error
as originalError, then perform the value comparison afterward and report a
distinct corruption error for a differing normalized path without attributing
the manually detected mismatch to normalizeBundlePath.
In `@src/domain/services/BundleService.js`:
- Around line 261-266: Update `#readRoot`’s call to `#readDescriptorBlob` for the
root descriptor to pass this.#limits.maxDescriptorBytes as the read limit,
matching the bounded read behavior used by `#readNode` while preserving the
existing handle and kind options.
In `@src/domain/services/CasService.d.ts`:
- Around line 82-83: Make readObjectSize mandatory in the GitPersistencePort
declaration by removing its optional marker in
src/domain/services/CasService.d.ts lines 82-83, matching PageService’s runtime
requirement. No direct change is needed in src/domain/services/PageService.js
lines 166-174 because the corrected type aligns with its existing validation.
In `@src/domain/value-objects/BundlePath.js`:
- Around line 57-71: Update hasUnpairedSurrogate so a high surrogate at the end
of value is treated as unpaired; explicitly handle the missing next code unit
before validating the low-surrogate range, while preserving existing handling
for valid pairs and isolated low surrogates.
In `@test/unit/domain/services/PageService.test.js`:
- Around line 57-71: Add a test in the existing page-limit suite that calls
pages.put with a non-byte, non-iterable source and asserts rejection with code
INVALID_OPTIONS and the message "Page source must be bytes or an iterable of
byte chunks", covering the dedicated PageService.#collect validation branch.
In `@test/unit/domain/value-objects/ApplicationStorageResults.test.js`:
- Around line 129-150: The “accepts every application handle kind” test only
covers PageHandle; parameterize it to exercise AssetHandle, PageHandle, and
BundleHandle instances, reusing the same RetentionWitness assertions for each
case.
In `@test/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.js`:
- Around line 124-133: Strengthen the readObjectSize() test to verify the size
lookup does not materialize the Git object: after the existing execute
assertion, assert plumbing.executeStream has not been called, and optionally
verify the expected plumbing call count.
---
Outside diff comments:
In `@src/infrastructure/adapters/GitPersistenceAdapter.js`:
- Around line 189-233: Extract the repeated missing-object error mapping from
readObjectType and readObjectSize into a shared private helper on
GitPersistenceAdapter. Have both methods use the helper while preserving
propagation of non-missing-object errors and the existing readObjectSize
validation behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6a642e0-1164-46c7-b870-67ac9f52de0c
📒 Files selected for processing (39)
CHANGELOG.mdGUIDE.mdREADME.mddocs/API.mddocs/design/0047-application-storage-cache-boundary/application-storage-cache-boundary.mddocs/design/0047-application-storage-cache-boundary/witness/bundle-pages.mdindex.d.tsindex.jssrc/domain/errors/Codes.jssrc/domain/helpers/handleTarget.jssrc/domain/services/BundleDescriptorCodec.jssrc/domain/services/BundleFanoutBuilder.jssrc/domain/services/BundleService.jssrc/domain/services/CasService.d.tssrc/domain/services/PageService.jssrc/domain/services/StagingEvidence.jssrc/domain/value-objects/ApplicationHandle.jssrc/domain/value-objects/BundleHandle.jssrc/domain/value-objects/BundleLimits.jssrc/domain/value-objects/BundlePath.jssrc/domain/value-objects/OpaqueHandle.jssrc/domain/value-objects/PageHandle.jssrc/domain/value-objects/RetentionWitness.jssrc/domain/value-objects/StagedBundle.jssrc/domain/value-objects/StagedPage.jssrc/infrastructure/adapters/GitPersistenceAdapter.jssrc/ports/GitPersistencePort.jstest/helpers/MemoryPersistenceAdapter.jstest/integration/application-storage.test.jstest/unit/domain/services/BundleAdmission.test.jstest/unit/domain/services/BundleService.test.jstest/unit/domain/services/PageService.test.jstest/unit/domain/services/StagingEvidence.test.jstest/unit/domain/value-objects/ApplicationStorageResults.test.jstest/unit/domain/value-objects/StructuredHandle.test.jstest/unit/facade/ContentAddressableStore.application-storage.test.jstest/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.jstest/unit/ports/GitPersistencePort.test.jstest/unit/types/declaration-accuracy.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test-docker (bun)
- GitHub Check: test-docker (deno)
- GitHub Check: test-docker (node)
🧰 Additional context used
📓 Path-based instructions (4)
docs/design/**
📄 CodeRabbit inference engine (AGENTS.md)
Use
docs/design/directory for durable design contracts and proof plans
Files:
docs/design/0047-application-storage-cache-boundary/application-storage-cache-boundary.mddocs/design/0047-application-storage-cache-boundary/witness/bundle-pages.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
CHANGELOG.mdto record the historical truth of merged behavior
Files:
CHANGELOG.md
README.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
README.mdas the public front door, core value prop, and quick start documentation
Files:
README.md
GUIDE.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
GUIDE.mdfor orientation and productive-fast path documentation
Files:
GUIDE.md
🧠 Learnings (2)
📚 Learning: 2026-02-28T19:21:13.982Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 15
File: test/unit/domain/services/CasService.envelope.test.js:326-330
Timestamp: 2026-02-28T19:21:13.982Z
Learning: In fuzz tests for cryptographic operations, use a seeded PRNG (e.g., xorshift32) for control-flow variables such as plaintext size selection and recipient index selection to ensure reproducibility. Continue to use randomBytes() for cryptographic keys and nonces to preserve realistic randomness and avoid security anti-patterns. This guideline applies to test files that perform fuzz testing of crypto logic; implement a consistent seed setup (e.g., fixed seed in test initialization) and document the rationale to enable deterministic replays across runs.
Applied to files:
test/unit/domain/value-objects/StructuredHandle.test.jstest/unit/domain/services/StagingEvidence.test.jstest/unit/ports/GitPersistencePort.test.jstest/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.jstest/unit/domain/services/BundleAdmission.test.jstest/unit/domain/services/PageService.test.jstest/integration/application-storage.test.jstest/unit/types/declaration-accuracy.test.jstest/unit/domain/services/BundleService.test.jstest/unit/facade/ContentAddressableStore.application-storage.test.jstest/unit/domain/value-objects/ApplicationStorageResults.test.js
📚 Learning: 2026-03-30T19:53:48.000Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 29
File: docs/design/TR-010-planning-index-consistency-review.md:121-131
Timestamp: 2026-03-30T19:53:48.000Z
Learning: In this repo, CHANGELOG.md entries should be user-facing and descriptive (bullet points) rather than cycle-ID headings (e.g., use a phrase like “Planning-index consistency review” instead of a “TR-010 — …” heading). When searching the changelog, don’t rely on grepping for cycle IDs (e.g., “TR-010”) because it may cause false negatives; search for the descriptive keywords/phrases from the bullet entries instead.
Applied to files:
CHANGELOG.md
🔇 Additional comments (39)
test/unit/domain/value-objects/StructuredHandle.test.js (1)
1-26: LGTM!test/unit/domain/services/StagingEvidence.test.js (1)
1-28: LGTM!src/domain/errors/Codes.js (1)
2-13: LGTM!Also applies to: 43-43
test/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.js (1)
135-140: LGTM!docs/design/0047-application-storage-cache-boundary/application-storage-cache-boundary.md (1)
800-802: LGTM!src/domain/services/StagingEvidence.js (1)
1-39: LGTM!docs/design/0047-application-storage-cache-boundary/witness/bundle-pages.md (1)
1-82: LGTM!src/domain/value-objects/RetentionWitness.js (1)
4-4: LGTM!Also applies to: 17-17, 41-41
test/unit/ports/GitPersistencePort.test.js (1)
43-45: LGTM!src/ports/GitPersistencePort.js (1)
27-32: LGTM!Also applies to: 81-89
test/unit/domain/services/BundleAdmission.test.js (1)
7-55: LGTM!test/integration/application-storage.test.js (1)
174-296: LGTM!test/unit/domain/services/BundleService.test.js (1)
14-300: LGTM!src/domain/value-objects/ApplicationHandle.js (1)
1-40: LGTM!test/helpers/MemoryPersistenceAdapter.js (1)
65-134: LGTM!src/domain/services/BundleService.js (1)
70-260: LGTM!Also applies to: 278-663
src/domain/value-objects/StagedBundle.js (1)
10-66: LGTM!src/domain/value-objects/BundleLimits.js (1)
23-80: LGTM!test/unit/facade/ContentAddressableStore.application-storage.test.js (1)
4-9: LGTM!Also applies to: 27-36, 38-44
test/unit/domain/value-objects/ApplicationStorageResults.test.js (1)
3-8: LGTM!Also applies to: 65-91
index.d.ts (1)
80-152: LGTM!Also applies to: 188-266, 291-297, 402-402, 415-415, 530-535, 825-877, 887-893, 918-919
index.js (2)
14-38: LGTM!Also applies to: 82-86, 118-164, 170-214, 275-304, 332-372, 420-446, 908-918
374-418: 🎯 Functional CorrectnessNo issue: sequential traversal prevents sibling false-positives. Bundle validation resolves children and leaf targets with plain
for...of+await, not concurrent fan-out, so the sharedvalidation.activeset only tracks the current path. Shared members can still be reused via the cache once the first resolution completes.> Likely an incorrect or invalid review comment.docs/API.md (1)
78-80: LGTM!Also applies to: 1001-1104, 1136-1137, 1176-1177, 1193-1193, 1207-1207, 2653-2670
src/domain/services/CasService.d.ts (1)
69-69: LGTM!src/domain/value-objects/PageHandle.js (1)
1-34: LGTM!src/domain/helpers/handleTarget.js (1)
11-25: LGTM!src/domain/value-objects/StagedPage.js (1)
1-47: LGTM!test/unit/domain/services/PageService.test.js (1)
1-114: LGTM!test/unit/types/declaration-accuracy.test.js (1)
82-90: LGTM!src/domain/services/PageService.js (1)
1-164: LGTM!src/domain/services/BundleDescriptorCodec.js (2)
1-165: LGTM!Also applies to: 180-284
167-178: 🗄️ Data Integrity & IntegrationConfirm the leaf kind→type mapping covers every blob-backed handle kind.
expectedType = handle.kind === 'page' ? 'blob' : 'tree'assumes onlypageis blob-backed; if any other leaf handle kind can also carry blobs, this will reject valid entries as corrupt.CHANGELOG.md (1)
23-46: LGTM!README.md (1)
26-117: LGTM!src/domain/value-objects/BundleHandle.js (1)
1-32: LGTM!GUIDE.md (1)
54-165: LGTM!src/domain/services/BundleFanoutBuilder.js (1)
38-227: LGTM!src/domain/value-objects/OpaqueHandle.js (1)
16-171: LGTM!
|
Review follow-up: the outside-diff duplicate missing-object mapping is fixed in |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/services/BundleService.js (1)
98-109: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate summary metadata along targeted traversal.
getMember()followsroot.indexand branchchildranges without checking each loaded node against its declared OID/depth/count/path summary. A corrupted descriptor can therefore return a member on this path, whileresolveRoot()correctly rejects the same graph viaassertSummary()/assertChildSummary().Carry the expected root/child summary through the loop and validate it immediately after each
#readNode(); add corruption tests for mismatched root-index and branch-child summaries.🤖 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 `@src/domain/services/BundleService.js` around lines 98 - 109, Update getMember()’s targeted traversal to carry the expected root and branch-child summary metadata, validating each loaded node immediately after `#readNode`() with the same assertSummary()/assertChildSummary() checks used by resolveRoot(). Ensure root.index and each selected child range are validated for declared OID, depth, count, and path before continuing, and add corruption tests covering mismatched root-index and branch-child summaries.
🤖 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.
Outside diff comments:
In `@src/domain/services/BundleService.js`:
- Around line 98-109: Update getMember()’s targeted traversal to carry the
expected root and branch-child summary metadata, validating each loaded node
immediately after `#readNode`() with the same assertSummary()/assertChildSummary()
checks used by resolveRoot(). Ensure root.index and each selected child range
are validated for declared OID, depth, count, and path before continuing, and
add corruption tests covering mismatched root-index and branch-child summaries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27c23417-fbfb-41d9-aea9-ce3d86bcdfd2
📒 Files selected for processing (13)
docs/design/0047-application-storage-cache-boundary/witness/bundle-pages.mdsrc/domain/helpers/handleTarget.jssrc/domain/services/BundleDescriptorCodec.jssrc/domain/services/BundleService.jssrc/domain/services/CasService.d.tssrc/domain/value-objects/BundlePath.jssrc/infrastructure/adapters/GitPersistenceAdapter.jstest/unit/domain/helpers/handleTarget.test.jstest/unit/domain/services/BundleAdmission.test.jstest/unit/domain/services/BundleService.test.jstest/unit/domain/services/PageService.test.jstest/unit/domain/value-objects/ApplicationStorageResults.test.jstest/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test-unit
🧰 Additional context used
📓 Path-based instructions (1)
docs/design/**
📄 CodeRabbit inference engine (AGENTS.md)
Use
docs/design/directory for durable design contracts and proof plans
Files:
docs/design/0047-application-storage-cache-boundary/witness/bundle-pages.md
🧠 Learnings (1)
📚 Learning: 2026-02-28T19:21:13.982Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 15
File: test/unit/domain/services/CasService.envelope.test.js:326-330
Timestamp: 2026-02-28T19:21:13.982Z
Learning: In fuzz tests for cryptographic operations, use a seeded PRNG (e.g., xorshift32) for control-flow variables such as plaintext size selection and recipient index selection to ensure reproducibility. Continue to use randomBytes() for cryptographic keys and nonces to preserve realistic randomness and avoid security anti-patterns. This guideline applies to test files that perform fuzz testing of crypto logic; implement a consistent seed setup (e.g., fixed seed in test initialization) and document the rationale to enable deterministic replays across runs.
Applied to files:
test/unit/domain/helpers/handleTarget.test.jstest/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.jstest/unit/domain/services/BundleAdmission.test.jstest/unit/domain/services/PageService.test.jstest/unit/domain/value-objects/ApplicationStorageResults.test.jstest/unit/domain/services/BundleService.test.js
🔇 Additional comments (12)
test/unit/domain/helpers/handleTarget.test.js (1)
1-19: LGTM!test/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.js (1)
133-133: LGTM!src/domain/services/CasService.d.ts (1)
69-82: LGTM!src/domain/value-objects/BundlePath.js (1)
57-74: LGTM!test/unit/domain/services/BundleAdmission.test.js (1)
27-27: LGTM!docs/design/0047-application-storage-cache-boundary/witness/bundle-pages.md (1)
7-8: LGTM!Also applies to: 168-216, 235-238
src/domain/helpers/handleTarget.js (1)
32-48: LGTM!test/unit/domain/services/PageService.test.js (1)
74-82: LGTM!test/unit/domain/value-objects/ApplicationStorageResults.test.js (1)
130-150: LGTM!src/domain/services/BundleDescriptorCodec.js (1)
238-247: LGTM!test/unit/domain/services/BundleService.test.js (1)
267-294: LGTM!src/infrastructure/adapters/GitPersistenceAdapter.js (1)
199-207: 🗄️ Data Integrity & IntegrationReject empty or non-decimal size output before coercion.
Number(output)treats blank strings as0, so malformedcat-file -soutput can be accepted as a valid zero-byte size.
1 participant
Summary
PageHandlevaluesLinked Issue
Design / Proof
docs/design/0047-application-storage-cache-boundary/application-storage-cache-boundary.mddocs/design/0047-application-storage-cache-boundary/witness/bundle-pages.mdSelf-review
Reviewed the complete delta for object ownership, bounded residency, deterministic identity, imported-handle admission, transitive Git reachability, failure evidence, runtime portability, and additive semver posture. Graft reports 40 changed files, no breaking structural changes, and no removed or changed exports.
Code Lawyer
The adversarial review found and corrected:
StagedBundleThe complete CL-001 through CL-017 record is in the witness.
Validation
pnpm lintpnpm test(204 files, 1,789 passed, 2 skipped)pnpm test:integration:node(7 files, 165 passed)pnpm test:integration:bun(7 files, 165 passed)pnpm test:integration:deno(7 files, 165 passed)Release Impact