feat!: mandatory CAS blob storage with streaming I/O (OG-014)#72
Merged
flyingrobots merged 16 commits intomainfrom Mar 29, 2026
Merged
feat!: mandatory CAS blob storage with streaming I/O (OG-014)#72flyingrobots merged 16 commits intomainfrom
flyingrobots merged 16 commits intomainfrom
Conversation
Expand OG-014 scope from streaming reads only to three requirements: mandatory CAS blob storage, streaming writes, and streaming reads. Add design doc at docs/design/streaming-cas-blob-storage.md with IBM Design Thinking framing, hills, invariants, checkpoint gates, and affected files inventory.
Checkpoint 2 (spec) for the streaming CAS blob storage design. New and expanded test coverage: - BlobStoragePort: storeStream() and retrieveStream() contract stubs - InMemoryBlobStorageAdapter: full adapter test suite (14 tests) - CasBlobAdapter: storeStream(), retrieveStream(), legacy fallback (6 tests) - PatchBuilderV2: streaming input (AsyncIterable, ReadableStream), no-raw-fallback assertion (4 tests) - WarpGraph: getContentStream(), getEdgeContentStream() (5 tests) - WarpRuntime: blob storage auto-construction (4 tests) All 35 new tests fail against current code (red phase). All 5171 existing tests pass — zero regressions.
… propagation PatchBuilderV2.attachContent() and attachEdgeContent() now require blobStorage (E_NO_BLOB_STORAGE). Updated all tests that constructed builders without blobStorage to inject createMockBlobStorage(). Also fixed a bug where setProperty for _content.size and _content.mime used raw metadata instead of normalizedMeta for non-streaming inputs, causing size to be null when not explicitly provided. Fixed pre-existing lint errors in defaultBlobStorage.js (require-await), streamUtils.js (require-await), CasBlobAdapter.js (return-await), and WarpRuntime.js (bare wildcard JSDoc type).
Checkpoint 3 (semantic) for the streaming CAS blob storage design. BREAKING CHANGES: - attachContent() and attachEdgeContent() now require blob storage. Calling without blobStorage throws E_NO_BLOB_STORAGE instead of falling back to raw persistence.writeBlob(). - WarpRuntime.open() auto-constructs blob storage when none is provided: CasBlobAdapter for Git-backed persistence (has plumbing), InMemoryBlobStorageAdapter for in-memory/browser paths. - Content blob tree entries now use 040000 tree mode (CAS trees) instead of 100644 blob mode (raw Git blobs). Implementation: - BlobStoragePort: added storeStream() and retrieveStream() abstract methods for streaming I/O - InMemoryBlobStorageAdapter: new domain-local adapter (defaultBlobStorage.js) with content-addressed Map storage, streaming round-trip support - CasBlobAdapter: added storeStream() piping through git-cas store(), retrieveStream() using git-cas restoreStream() with legacy blob fallback, Buffer→Uint8Array normalization on retrieve() - PatchBuilderV2: accepts AsyncIterable/ReadableStream content input, routes streaming input to storeStream(), removes raw writeBlob fallback - query.methods.js: added getContentStream() and getEdgeContentStream() returning AsyncIterable<Uint8Array> for incremental consumption - streamUtils.js: normalizeToAsyncIterable() and isStreamingInput() domain utilities for input normalization - WarpRuntime: auto-constructs blob storage in open(), new streaming read methods auto-wired via wireWarpMethods All 5203 tests pass across 322 files.
…int 4) Complete the OG-014 surface checkpoint: - WarpApp: getContent, getContentStream, getContentOid, getContentMeta and edge equivalents — all 8 content read methods now on the product surface - WarpCore: same 8 methods via WarpRuntime.prototype delegation - index.js: export InMemoryBlobStorageAdapter - index.d.ts: streaming types on BlobStoragePort (storeStream, retrieveStream), InMemoryBlobStorageAdapter class, streaming input union on attachContent/attachEdgeContent, getContentStream and getEdgeContentStream on WarpCoreBase and WarpApp - CHANGELOG: [Unreleased] section with OG-014 additions and breaking changes - ROADMAP: updated to v15.0.1 current / v16.0.0 next - OG-014 and design doc marked DONE / IMPLEMENTED BREAKING CHANGE: CAS blob storage is mandatory for content attachments. attachContent() without blobStorage now throws E_NO_BLOB_STORAGE. WarpRuntime.open() auto-constructs blob storage when none is provided.
Design alignment audit: 6/6 design decisions aligned (2 partially — adapter placement and dynamic import, both accepted as deliberate trade-offs). Two unanticipated drift items documented: content anchor tree mode change and retrieve() independence from retrieveStream().
The WarpApp/WarpCore split, Observer/Lens public noun cuts, Strand rename, README rewrite, and cost-signaling work all landed in v15.0.0. No remaining open items under this cycle.
Document 29 visible state, coordinate comparison, and transfer plan type exports that were missing JSDoc comments (lines ~3155-3484).
Add JSDoc comments to all 80 previously undocumented exported symbols in index.d.ts: - 14 traversal, observer, content types (L248-L1629) - 18 conflict analysis and strand types (L2819-L2994) - 9 op interfaces + 9 factory functions + 1 migration function (L3039-L3131) - 29 visible state, comparison, and transfer plan types (L3159-L3478) - 1 default export Symbol documentation coverage: 65% → 100% (227/227 exports).
All 227 exported symbols in index.d.ts now have JSDoc comments. Coverage raised from 65% to 100%.
- Fix tsc errors: import content query functions directly in WarpApp and WarpCore instead of accessing dynamically-wired prototype methods, narrow streaming union type in PatchBuilderV2, fix crypto.subtle.digest typing in defaultBlobStorage, add _asRuntime() helper in WarpCore for type-safe delegation - Add InMemoryBlobStorageAdapter to type-surface.m8.json manifest - Fix markdown code sample lint: use text blocks for pseudocode signatures in design doc - Clean up CasBlobAdapter test mock: declare restoreStream in constructor instead of prototype patching - Add JSDoc types to integration test helper openGraph params - Restore README.md (reverted unintended agent modification) All gates pass: tsc, surface validator, policy ratchet, markdown lint, 5203 tests green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds mandatory content-addressed blob storage with streaming I/O: new streaming methods on BlobStoragePort, an InMemoryBlobStorageAdapter, streaming attach/read APIs on app/core, PatchBuilderV2 enforces CAS and streaming, WarpRuntime auto-constructs blob storage, and content anchors changed to tree-mode entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WarpApp
participant PatchBuilderV2
participant BlobStorage
participant Persistence_CAS
Client->>WarpApp: attachContent(nodeId, asyncIterable)
WarpApp->>PatchBuilderV2: attachContent(nodeId, asyncIterable, metadata?)
PatchBuilderV2->>PatchBuilderV2: isStreamingInput? / normalizeToAsyncIterable
PatchBuilderV2->>BlobStorage: storeStream(normalized, options)
BlobStorage->>Persistence_CAS: stream chunks to git-cas
Persistence_CAS-->>BlobStorage: return tree OID
BlobStorage-->>PatchBuilderV2: return OID
PatchBuilderV2->>PatchBuilderV2: write content metadata + anchor (tree-mode)
PatchBuilderV2-->>WarpApp: patch queued
WarpApp-->>Client: success
sequenceDiagram
participant Client
participant WarpApp
participant WarpRuntime
participant BlobStorage
participant Persistence_CAS
Client->>WarpApp: getContentStream(nodeId)
WarpApp->>WarpRuntime: getContentStream(nodeId)
WarpRuntime->>WarpRuntime: locate content OID
WarpRuntime->>BlobStorage: retrieveStream(oid)
alt BlobStorage supports streaming
BlobStorage->>Persistence_CAS: retrieve via git-cas stream
Persistence_CAS-->>BlobStorage: AsyncIterable<Uint8Array>
else Fallback to buffered read
BlobStorage->>Persistence_CAS: readBlob(oid)
Persistence_CAS-->>BlobStorage: Uint8Array
BlobStorage->>BlobStorage: wrap in single-chunk AsyncIterable
end
BlobStorage-->>WarpRuntime: AsyncIterable<Uint8Array>
WarpRuntime-->>WarpApp: AsyncIterable<Uint8Array>
WarpApp-->>Client: AsyncIterable<Uint8Array>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Release Preflight
If you tag this commit as |
![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 (2)
src/infrastructure/adapters/CasBlobAdapter.js (1)
157-165:⚠️ Potential issue | 🟡 MinorNormalize the legacy raw-blob fallback too.
The CAS path now converts
Bufferto plainUint8Array, but the raw-blob fallback still returnspersistence.readBlob()verbatim. That makes theBlobStoragePort.retrieve()contract insrc/ports/BlobStoragePort.js:23-32depend on how the attachment was originally stored.🩹 Normalize the fallback path too
const blob = await this._persistence.readBlob(oid); if (blob === null || blob === undefined) { throw new PersistenceError( `Missing Git object: ${oid}`, PersistenceError.E_MISSING_OBJECT, { context: { oid } }, ); } - return blob; + return blob instanceof Uint8Array && blob.constructor !== Uint8Array + ? new Uint8Array(blob.buffer, blob.byteOffset, blob.byteLength) + : blob;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/CasBlobAdapter.js` around lines 157 - 165, The legacy fallback in CasBlobAdapter (where you call this._persistence.readBlob(oid)) must normalize returned Buffer to a plain Uint8Array to match the CAS path and the BlobStoragePort.retrieve contract; update the retrieval path in the CasBlobAdapter method that returns blob (the same block that throws PersistenceError on missing object) to detect Buffer/Node buffer-like values and convert them to Uint8Array before returning so callers always receive a Uint8Array.CHANGELOG.md (1)
22-33:⚠️ Potential issue | 🟡 MinorRemove the duplicate empty
15.0.0header to keep version chronology unambiguous.
## [15.0.0]appears twice, and the first instance is empty. This can break changelog consumers/parsers and makes release history unclear.✂️ Proposed fix
-## [15.0.0] — 2026-03-28 - ## [15.0.1] — 2026-03-28Based on learnings: Follow Keep a Changelog conventions; duplicate subheadings across versions are acceptable, but version entries should remain structurally clear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 22 - 33, Remove the duplicate empty "## [15.0.0] — 2026-03-28" heading so the changelog has a single clear entry for 15.0.0; locate the first occurrence of the header string "## [15.0.0] — 2026-03-28" (the empty block above the 15.0.1 section) and delete that header line (and any immediately adjacent blank lines) so only the intended 15.0.0 entry remains and chronology/parsing is unambiguous.
🧹 Nitpick comments (3)
src/domain/utils/streamUtils.js (1)
108-128: Consider addingthrow()for complete iterator cleanup.The async iterator returned by
readableStreamToAsyncIterableimplementsnext()andreturn()but omitsthrow(). Whilethrow()is optional per the async iteration protocol, implementing it would ensure the reader lock is released if an error is thrown into the iterator.This is a minor concern since
return()handles most cleanup scenarios (break, return, normal completion).♻️ Optional: Add throw() method
return() { reader.releaseLock(); return Promise.resolve({ value: undefined, done: true }); }, + throw(err) { + reader.releaseLock(); + return Promise.reject(err); + }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/utils/streamUtils.js` around lines 108 - 128, The async iterator produced by readableStreamToAsyncIterable lacks a throw() method, so if an error is thrown into the iterator the reader lock may not be released; add an async throw(err) method to the iterator returned from [Symbol.asyncIterator]() (alongside next() and return()) that calls reader.releaseLock() and then rethrows or returns a rejected promise with the provided error to ensure proper cleanup of the stream reader in error scenarios.test/unit/domain/WarpRuntime.blobAutoConstruct.test.js (1)
11-13: Stale comment: tests are no longer in "red phase".The comment states tests should "all FAIL against the current code (red phase)", but per the PR description, the implementation is complete and tests are green. Consider updating or removing this comment.
📝 Suggested update
-/** - * Spec tests for OG-014: auto-construction of blob storage. - * - * When no explicit `blobStorage` is provided to `WarpRuntime.open()`, - * the runtime should auto-construct the appropriate adapter: - * - `CasBlobAdapter` when persistence has plumbing (Git-backed) - * - `InMemoryBlobStorageAdapter` when persistence lacks plumbing (in-memory) - * - * These tests should all FAIL against the current code (red phase). - */ +/** + * Spec tests for OG-014: auto-construction of blob storage. + * + * When no explicit `blobStorage` is provided to `WarpRuntime.open()`, + * the runtime should auto-construct the appropriate adapter: + * - `CasBlobAdapter` when persistence has plumbing (Git-backed) + * - `InMemoryBlobStorageAdapter` when persistence lacks plumbing (in-memory) + */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/WarpRuntime.blobAutoConstruct.test.js` around lines 11 - 13, The block comment in test/unit/domain/WarpRuntime.blobAutoConstruct.test.js that reads "These tests should all FAIL against the current code (red phase)" is stale; remove or update it to reflect the current state (tests are implemented and passing). Locate the comment text in the file (search for "These tests should all FAIL against the current code (red phase)") and either delete the line or replace it with a short, accurate note (e.g., "Tests validate blob auto-construction; implementation complete") so the comment matches the current behavior of the WarpRuntime.blobAutoConstruct tests.test/unit/domain/services/PatchBuilderV2.content.test.js (1)
492-503: Assert the streamed metadata path, not just the call count.These cases only prove that
storeStream()was reached. They do not verify{ slug, mime, size }forwarding or the_content.size/_content.mimepatch entries, so a regression in the streamed metadata path would still pass.Also applies to: 532-535, 563-566
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/PatchBuilderV2.content.test.js` around lines 492 - 503, The tests currently only assert that blobStorage.storeStream was called; update the assertions around builder.attachContent and builder.build to also verify the streamed metadata and resulting patch entries: assert blobStorage.storeStream was called with the expected metadata object including slug, mime and size (from the source()), and assert the built patch.ops contains PropSet entries for '_content.size' and '_content.mime' (and the forwarded slug if applicable) alongside the existing '_content' entry; reference the existing calls to builder.attachContent, blobStorage.storeStream and builder.build/patch.ops to locate where to add these extra assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BACKLOG/OG-014-streaming-content-attachments.md`:
- Around line 87-88: The buffered helpers getContent() and getEdgeContent()
currently call retrieve() directly (see retrieve()), which contradicts the
stated design that they should be implemented on top of the stream primitive;
update those functions so they route through the stream-based path instead of
calling retrieve() directly — e.g., have getContent()/getEdgeContent() call the
stream primitive (the existing streaming retrieval function) and then collect
the stream into a buffer before returning, or alternatively relax the spec text
to reflect current implementation if you don't want to change code; ensure you
update references to retrieve(), getContent(), and getEdgeContent() so the
buffered getters either wrap the stream primitive or the doc is revised.
In `@index.d.ts`:
- Line 256: The deprecation comment for LogicalTraversal incorrectly tells users
to "Use GraphTraversal directly" even though GraphTraversal isn't exported;
either export GraphTraversal or (recommended) update the deprecation text on
LogicalTraversal to point to the actual public replacement — e.g., "Use the
traverse APIs exposed on WarpAppObserver, Worldline, or WarpCoreBase" — so
change the comment attached to LogicalTraversal to reference those public
traverse properties (or export GraphTraversal if you prefer that path).
In `@src/domain/utils/defaultBlobStorage.js`:
- Around line 24-40: The contentHash function may pass the entire underlying
ArrayBuffer via bytes.buffer to crypto.subtle.digest, which can include extra
data if bytes is a view; update the crypto path in contentHash to pass the
Uint8Array (bytes) directly as the BufferSource to crypto.subtle.digest (leave
hexEncode usage and the FNV-1a fallback intact) so only the intended slice is
hashed.
In `@src/domain/warp/query.methods.js`:
- Around line 866-868: The current logic calls this._persistence.readBlob(oid)
and immediately wraps the result with singleChunkAsyncIterable(buf), which will
yield a null chunk if readBlob returns null; change the flow in the method that
calls readBlob so it checks the result (buf) and if buf is null return null (or
throw a clear error) instead of wrapping it in singleChunkAsyncIterable; update
the branch around this._persistence.readBlob(oid) to return null when buf ===
null and only call singleChunkAsyncIterable(buf) when buf is non-null so callers
of this method (which expect either a null or an async iterable) get the correct
behavior.
- Around line 838-841: In the fallback path where _blobStorage is missing,
ensure you handle a null return from _persistence.readBlob(oid) — call readBlob,
and if buf === null return null immediately (rather than wrapping it with
singleChunkAsyncIterable), so getContentStream/getContent behavior matches the
buffered getContent(); reference symbols: _blobStorage, _persistence.readBlob,
singleChunkAsyncIterable, getContent, and consider consistency with
CasBlobAdapter / PersistenceError.E_MISSING_OBJECT if you later decide to throw
instead.
In `@src/domain/WarpRuntime.js`:
- Around line 53-62: autoConstructBlobStorage currently returns a new
InMemoryBlobStorageAdapter() every time (in autoConstructBlobStorage), which
loses prior in-memory blobs across open() calls; change it to cache and reuse a
single InMemoryBlobStorageAdapter instance per persistence object (keyed by the
persistence reference) so subsequent calls return the same adapter for that
persistence. Implement similar caching wherever the same one-off creation occurs
(the other spot around lines 415-418), ensuring you return the cached adapter
when p.plumbing is absent and only create a new InMemoryBlobStorageAdapter once
per persistence object.
In `@src/infrastructure/adapters/CasBlobAdapter.js`:
- Around line 210-268: The current retrieveStream wrapper (retrieveStream) only
implements next() so return()/throw() never propagate to the underlying iterator
returned by _resolveStreamIterator, and streamed chunks are not normalized to
true Uint8Array like retrieve() does; convert retrieveStream into an async
generator that awaits the iterator from _resolveStreamIterator (or uses the
iterator returned by cas.restoreStream/singleChunkIterator) and uses a
normalization helper (same logic as retrieve() that converts Buffer subclasses
to Uint8Array) before yielding each chunk so both buffered and streamed paths
behave identically, and rely on generator semantics to delegate return()/throw()
to the inner iterator to ensure cleanup; add a unit test that consumes the async
iterable and breaks early (calls return on the iterator or uses break in a
for-await loop) to assert the underlying cas.restoreStream iterator saw
cancellation/cleanup.
In `@test/unit/domain/services/PatchBuilderV2.content.test.js`:
- Around line 618-620: Replace the generic error message assertion with an
explicit check for the error code E_NO_BLOB_STORAGE when calling
builder.attachContent('node:1', 'hello') — e.g. use await
expect(builder.attachContent(...)).rejects.toMatchObject({code:
'E_NO_BLOB_STORAGE'}) or await expect(...).rejects.toHaveProperty('code',
'E_NO_BLOB_STORAGE') and keep the persistence.writeBlob non-call assertion;
apply the same change for the other case referenced (OG-014 at the later
assertion).
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 22-33: Remove the duplicate empty "## [15.0.0] — 2026-03-28"
heading so the changelog has a single clear entry for 15.0.0; locate the first
occurrence of the header string "## [15.0.0] — 2026-03-28" (the empty block
above the 15.0.1 section) and delete that header line (and any immediately
adjacent blank lines) so only the intended 15.0.0 entry remains and
chronology/parsing is unambiguous.
In `@src/infrastructure/adapters/CasBlobAdapter.js`:
- Around line 157-165: The legacy fallback in CasBlobAdapter (where you call
this._persistence.readBlob(oid)) must normalize returned Buffer to a plain
Uint8Array to match the CAS path and the BlobStoragePort.retrieve contract;
update the retrieval path in the CasBlobAdapter method that returns blob (the
same block that throws PersistenceError on missing object) to detect Buffer/Node
buffer-like values and convert them to Uint8Array before returning so callers
always receive a Uint8Array.
---
Nitpick comments:
In `@src/domain/utils/streamUtils.js`:
- Around line 108-128: The async iterator produced by
readableStreamToAsyncIterable lacks a throw() method, so if an error is thrown
into the iterator the reader lock may not be released; add an async throw(err)
method to the iterator returned from [Symbol.asyncIterator]() (alongside next()
and return()) that calls reader.releaseLock() and then rethrows or returns a
rejected promise with the provided error to ensure proper cleanup of the stream
reader in error scenarios.
In `@test/unit/domain/services/PatchBuilderV2.content.test.js`:
- Around line 492-503: The tests currently only assert that
blobStorage.storeStream was called; update the assertions around
builder.attachContent and builder.build to also verify the streamed metadata and
resulting patch entries: assert blobStorage.storeStream was called with the
expected metadata object including slug, mime and size (from the source()), and
assert the built patch.ops contains PropSet entries for '_content.size' and
'_content.mime' (and the forwarded slug if applicable) alongside the existing
'_content' entry; reference the existing calls to builder.attachContent,
blobStorage.storeStream and builder.build/patch.ops to locate where to add these
extra assertions.
In `@test/unit/domain/WarpRuntime.blobAutoConstruct.test.js`:
- Around line 11-13: The block comment in
test/unit/domain/WarpRuntime.blobAutoConstruct.test.js that reads "These tests
should all FAIL against the current code (red phase)" is stale; remove or update
it to reflect the current state (tests are implemented and passing). Locate the
comment text in the file (search for "These tests should all FAIL against the
current code (red phase)") and either delete the line or replace it with a
short, accurate note (e.g., "Tests validate blob auto-construction;
implementation complete") so the comment matches the current behavior of the
WarpRuntime.blobAutoConstruct tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 618511ac-4c40-4485-a0ca-09bffdfb570e
⛔ Files ignored due to path filters (1)
test/unit/domain/__snapshots__/WarpRuntime.apiSurface.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (33)
BACKLOG/OG-010-public-api-design-thinking.mdBACKLOG/OG-014-streaming-content-attachments.mdBACKLOG/OG-015-jsr-documentation-quality.mdBACKLOG/README.mdCHANGELOG.mdREADME.mdcontracts/type-surface.m8.jsondocs/ROADMAP.mddocs/design/streaming-cas-blob-storage.mddocs/design/streaming-cas-blob-storage.retro.mdindex.d.tsindex.jssrc/domain/WarpApp.jssrc/domain/WarpCore.jssrc/domain/WarpRuntime.jssrc/domain/errors/WriterError.jssrc/domain/services/CheckpointService.jssrc/domain/services/PatchBuilderV2.jssrc/domain/utils/defaultBlobStorage.jssrc/domain/utils/streamUtils.jssrc/domain/warp/query.methods.jssrc/infrastructure/adapters/CasBlobAdapter.jssrc/ports/BlobStoragePort.jstest/integration/api/helpers/setup.jstest/unit/domain/WarpGraph.content.test.jstest/unit/domain/WarpRuntime.blobAutoConstruct.test.jstest/unit/domain/services/CheckpointService.test.jstest/unit/domain/services/PatchBuilderV2.content.test.jstest/unit/domain/services/TreeConstruction.determinism.test.jstest/unit/infrastructure/adapters/CasBlobAdapter.test.jstest/unit/infrastructure/adapters/InMemoryBlobStorageAdapter.test.jstest/unit/ports/BlobStoragePort.test.jstest/unit/scripts/release-policy-shape.test.js
Comment on lines
+87
to
+88
| 4. Existing `getContent()` / `getEdgeContent()` remain as buffered convenience, | ||
| implemented on top of the stream primitive. |
There was a problem hiding this comment.
Criterion 4 does not match the current implementation.
src/domain/warp/query.methods.js:737-760 and src/domain/warp/query.methods.js:803-826 still call retrieve() directly for the buffered getters, so they are not yet “implemented on top of the stream primitive” as written. Please either relax this wording or route the buffered helpers through the stream path before closing OG-014.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BACKLOG/OG-014-streaming-content-attachments.md` around lines 87 - 88, The
buffered helpers getContent() and getEdgeContent() currently call retrieve()
directly (see retrieve()), which contradicts the stated design that they should
be implemented on top of the stream primitive; update those functions so they
route through the stream-based path instead of calling retrieve() directly —
e.g., have getContent()/getEdgeContent() call the stream primitive (the existing
streaming retrieval function) and then collect the stream into a buffer before
returning, or alternatively relax the spec text to reflect current
implementation if you don't want to change code; ensure you update references to
retrieve(), getContent(), and getEdgeContent() so the buffered getters either
wrap the stream primitive or the doc is revised.
Comment on lines
+838
to
+841
| // Fallback: wrap buffered read as single-chunk async iterable | ||
| const buf = await this._persistence.readBlob(oid); | ||
| return singleChunkAsyncIterable(buf); | ||
| } |
There was a problem hiding this comment.
Potential null propagation in fallback path.
When _blobStorage is unavailable and persistence.readBlob(oid) returns null (e.g., missing Git object), singleChunkAsyncIterable(buf) will yield null as a chunk. This violates the AsyncIterable<Uint8Array> return type contract.
The buffered getContent() at line 748 returns the raw null directly, which is semantically different (return null vs. yield null as a chunk).
Consider either:
- Checking for
nulland returningnullbefore wrapping (matching buffered behavior) - Throwing
PersistenceError.E_MISSING_OBJECT(matchingCasBlobAdapterbehavior)
🛡️ Suggested fix (option 1 - match buffered API)
// Fallback: wrap buffered read as single-chunk async iterable
const buf = await this._persistence.readBlob(oid);
+ if (buf === null) {
+ return null;
+ }
return singleChunkAsyncIterable(buf);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fallback: wrap buffered read as single-chunk async iterable | |
| const buf = await this._persistence.readBlob(oid); | |
| return singleChunkAsyncIterable(buf); | |
| } | |
| // Fallback: wrap buffered read as single-chunk async iterable | |
| const buf = await this._persistence.readBlob(oid); | |
| if (buf === null) { | |
| return null; | |
| } | |
| return singleChunkAsyncIterable(buf); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/warp/query.methods.js` around lines 838 - 841, In the fallback
path where _blobStorage is missing, ensure you handle a null return from
_persistence.readBlob(oid) — call readBlob, and if buf === null return null
immediately (rather than wrapping it with singleChunkAsyncIterable), so
getContentStream/getContent behavior matches the buffered getContent();
reference symbols: _blobStorage, _persistence.readBlob,
singleChunkAsyncIterable, getContent, and consider consistency with
CasBlobAdapter / PersistenceError.E_MISSING_OBJECT if you later decide to throw
instead.
Comment on lines
+866
to
+868
| const buf = await this._persistence.readBlob(oid); | ||
| return singleChunkAsyncIterable(buf); | ||
| } |
There was a problem hiding this comment.
Same null handling issue in edge content fallback.
The same concern applies here: if persistence.readBlob(oid) returns null, the streaming API will yield null as a chunk rather than returning null or throwing.
🛡️ Suggested fix
const buf = await this._persistence.readBlob(oid);
+ if (buf === null) {
+ return null;
+ }
return singleChunkAsyncIterable(buf);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const buf = await this._persistence.readBlob(oid); | |
| return singleChunkAsyncIterable(buf); | |
| } | |
| const buf = await this._persistence.readBlob(oid); | |
| if (buf === null) { | |
| return null; | |
| } | |
| return singleChunkAsyncIterable(buf); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/warp/query.methods.js` around lines 866 - 868, The current logic
calls this._persistence.readBlob(oid) and immediately wraps the result with
singleChunkAsyncIterable(buf), which will yield a null chunk if readBlob returns
null; change the flow in the method that calls readBlob so it checks the result
(buf) and if buf is null return null (or throw a clear error) instead of
wrapping it in singleChunkAsyncIterable; update the branch around
this._persistence.readBlob(oid) to return null when buf === null and only call
singleChunkAsyncIterable(buf) when buf is non-null so callers of this method
(which expect either a null or an async iterable) get the correct behavior.
Comment on lines
+53
to
+62
| async function autoConstructBlobStorage(persistence) { | ||
| const p = /** @type {{ plumbing?: unknown }} */ (persistence); | ||
| if (p.plumbing) { | ||
| const { default: CasBlobAdapter } = await import( | ||
| /* webpackIgnore: true */ '../infrastructure/adapters/CasBlobAdapter.js' | ||
| ); | ||
| return new CasBlobAdapter({ plumbing: p.plumbing, persistence }); | ||
| } | ||
| return new InMemoryBlobStorageAdapter(); | ||
| } |
There was a problem hiding this comment.
Persist auto-constructed in-memory blob storage across open() calls.
Line 61 creates a brand-new InMemoryBlobStorageAdapter on every open(). With mandatory blob storage, that can make prior content OIDs unreadable after reopening against the same in-memory persistence instance.
💡 Proposed fix (cache auto-constructed in-memory adapter per persistence object)
+const _autoInMemoryBlobStorageByPersistence = new WeakMap();
async function autoConstructBlobStorage(persistence) {
const p = /** `@type` {{ plumbing?: unknown }} */ (persistence);
if (p.plumbing) {
const { default: CasBlobAdapter } = await import(
/* webpackIgnore: true */ '../infrastructure/adapters/CasBlobAdapter.js'
);
return new CasBlobAdapter({ plumbing: p.plumbing, persistence });
}
- return new InMemoryBlobStorageAdapter();
+ if (typeof persistence === 'object' && persistence !== null) {
+ const key = /** `@type` {object} */ (persistence);
+ const existing = _autoInMemoryBlobStorageByPersistence.get(key);
+ if (existing) {
+ return existing;
+ }
+ const storage = new InMemoryBlobStorageAdapter();
+ _autoInMemoryBlobStorageByPersistence.set(key, storage);
+ return storage;
+ }
+ return new InMemoryBlobStorageAdapter();
}Also applies to: 415-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/WarpRuntime.js` around lines 53 - 62, autoConstructBlobStorage
currently returns a new InMemoryBlobStorageAdapter() every time (in
autoConstructBlobStorage), which loses prior in-memory blobs across open()
calls; change it to cache and reuse a single InMemoryBlobStorageAdapter instance
per persistence object (keyed by the persistence reference) so subsequent calls
return the same adapter for that persistence. Implement similar caching wherever
the same one-off creation occurs (the other spot around lines 415-418), ensuring
you return the cached adapter when p.plumbing is absent and only create a new
InMemoryBlobStorageAdapter once per persistence object.
Comment on lines
+210
to
+268
| retrieveStream(oid) { | ||
| const self = this; | ||
| return /** @type {AsyncIterable<Uint8Array>} */ ({ | ||
| [Symbol.asyncIterator]() { | ||
| /** @type {AsyncIterator<Uint8Array>|null} */ | ||
| let inner = null; | ||
| let initialized = false; | ||
| return { | ||
| async next() { | ||
| if (!initialized) { | ||
| initialized = true; | ||
| inner = await self._resolveStreamIterator(oid); | ||
| } | ||
| return await /** @type {AsyncIterator<Uint8Array>} */ (inner).next(); | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Resolves the inner async iterator for retrieveStream(). | ||
| * | ||
| * @private | ||
| * @param {string} oid | ||
| * @returns {Promise<AsyncIterator<Uint8Array>>} | ||
| */ | ||
| async _resolveStreamIterator(oid) { | ||
| const cas = await this._getCas(); | ||
|
|
||
| try { | ||
| const manifest = await cas.readManifest({ treeOid: oid }); | ||
| /** @type {{ manifest: *, encryptionKey?: Uint8Array }} */ | ||
| const restoreOpts = { manifest }; | ||
| if (this._encryptionKey) { | ||
| restoreOpts.encryptionKey = this._encryptionKey; | ||
| } | ||
|
|
||
| if (typeof cas.restoreStream === 'function') { | ||
| const stream = cas.restoreStream(restoreOpts); | ||
| return stream[Symbol.asyncIterator](); | ||
| } | ||
|
|
||
| // Fallback: buffered restore as single chunk | ||
| const { buffer } = await cas.restore(restoreOpts); | ||
| return singleChunkIterator(buffer); | ||
| } catch (err) { | ||
| if (!isLegacyBlobError(err)) { | ||
| throw err; | ||
| } | ||
| const blob = await this._persistence.readBlob(oid); | ||
| if (blob === null || blob === undefined) { | ||
| throw new PersistenceError( | ||
| `Missing Git object: ${oid}`, | ||
| PersistenceError.E_MISSING_OBJECT, | ||
| { context: { oid } }, | ||
| ); | ||
| } | ||
| return singleChunkIterator(blob); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full file to understand the context
wc -l src/infrastructure/adapters/CasBlobAdapter.jsRepository: git-stunts/git-warp
Length of output: 112
🏁 Script executed:
# Check the retrieve() method to see the Uint8Array normalization
rg -A 30 "retrieve\(oid\)" src/infrastructure/adapters/CasBlobAdapter.js | head -50Repository: git-stunts/git-warp
Length of output: 1292
🏁 Script executed:
# Look for any return() method handling in the file
rg "return\(\)" src/infrastructure/adapters/CasBlobAdapter.jsRepository: git-stunts/git-warp
Length of output: 45
🏁 Script executed:
# Check if there's any async generator or similar pattern in the file
rg "async\s*\*|Symbol\.asyncIterator" src/infrastructure/adapters/CasBlobAdapter.js -A 3Repository: git-stunts/git-warp
Length of output: 319
🏁 Script executed:
# Get the full retrieveStream() method implementation
sed -n '210,230p' src/infrastructure/adapters/CasBlobAdapter.jsRepository: git-stunts/git-warp
Length of output: 628
🏁 Script executed:
# Look for tests related to retrieveStream or stream cleanup
find . -type f -name "*.test.js" -o -name "*.spec.js" | xargs grep -l "retrieveStream" 2>/dev/null | head -5Repository: git-stunts/git-warp
Length of output: 353
🏁 Script executed:
# Check if there's a return() method implementation anywhere in the file
sed -n '1,290p' src/infrastructure/adapters/CasBlobAdapter.js | grep -n "return\s*(\|throw\s*(" | grep -v "//"Repository: git-stunts/git-warp
Length of output: 45
🏁 Script executed:
# Look at the complete _resolveStreamIterator method to see the full logic
sed -n '228,268p' src/infrastructure/adapters/CasBlobAdapter.jsRepository: git-stunts/git-warp
Length of output: 1309
🏁 Script executed:
# Check what singleChunkIterator does
rg -B 3 -A 10 "singleChunkIterator" src/infrastructure/adapters/CasBlobAdapter.js | head -30Repository: git-stunts/git-warp
Length of output: 833
🏁 Script executed:
# Look at the test file to see if early-break tests exist
rg "retrieveStream|early.*break|return\(\)" test/unit/infrastructure/adapters/CasBlobAdapter.test.js -A 5 -B 2 | head -50Repository: git-stunts/git-warp
Length of output: 1220
🏁 Script executed:
# Check the full singleChunkIterator implementation to see if it handles cleanup
sed -n '273,290p' src/infrastructure/adapters/CasBlobAdapter.jsRepository: git-stunts/git-warp
Length of output: 462
🏁 Script executed:
# Search for any async-generator patterns or return() handling elsewhere in the file
rg "async\s*\*.*{|\.return\(|\.throw\(" src/infrastructure/adapters/CasBlobAdapter.jsRepository: git-stunts/git-warp
Length of output: 192
🏁 Script executed:
# Let's verify if there are any tests that break early from retrieveStream
rg -A 20 "retrieveStream" test/unit/infrastructure/adapters/CasBlobAdapter.test.js | grep -A 20 "break\|return\|early"Repository: git-stunts/git-warp
Length of output: 803
🏁 Script executed:
# Check if there's Uint8Array normalization applied to the chunks in any way
rg -B 5 -A 5 "Uint8Array" src/infrastructure/adapters/CasBlobAdapter.js | grep -A 5 -B 5 "normali\|constructor"Repository: git-stunts/git-warp
Length of output: 995
Add return() and throw() delegation to the async iterator wrapper, and normalize Uint8Array chunks in retrieveStream().
The wrapper at lines 210–220 only implements next(), missing the return() and throw() methods needed for cleanup. If a consumer breaks early from the stream, the underlying cas.restoreStream() iterator never receives the cancellation signal, leaking resources. Additionally, the retrieve() method normalizes Buffer subclasses to true Uint8Array instances (lines 168–170), but retrieveStream() skips this normalization entirely—the streamed chunks may not match the buffered path's behavior.
Convert the wrapper to an async generator, which automatically delegates cleanup methods and provides a clearer control flow for applying the normalization. Add a test that breaks early from the stream to verify cleanup is properly invoked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/infrastructure/adapters/CasBlobAdapter.js` around lines 210 - 268, The
current retrieveStream wrapper (retrieveStream) only implements next() so
return()/throw() never propagate to the underlying iterator returned by
_resolveStreamIterator, and streamed chunks are not normalized to true
Uint8Array like retrieve() does; convert retrieveStream into an async generator
that awaits the iterator from _resolveStreamIterator (or uses the iterator
returned by cas.restoreStream/singleChunkIterator) and uses a normalization
helper (same logic as retrieve() that converts Buffer subclasses to Uint8Array)
before yielding each chunk so both buffered and streamed paths behave
identically, and rely on generator semantics to delegate return()/throw() to the
inner iterator to ensure cleanup; add a unit test that consumes the async
iterable and breaks early (calls return on the iterator or uses break in a
for-await loop) to assert the underlying cas.restoreStream iterator saw
cancellation/cleanup.
Comment on lines
+618
to
+620
| await expect(builder.attachContent('node:1', 'hello')) | ||
| .rejects.toThrow('Cannot attach content without blob storage'); | ||
| expect(persistence.writeBlob).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Check E_NO_BLOB_STORAGE explicitly.
This breaking change is the error code, not just the message. These expectations still pass if the implementation regresses to a generic Error, and the OG-014 case below duplicates the same behavior without tightening the contract.
Also applies to: 642-644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/services/PatchBuilderV2.content.test.js` around lines 618 -
620, Replace the generic error message assertion with an explicit check for the
error code E_NO_BLOB_STORAGE when calling builder.attachContent('node:1',
'hello') — e.g. use await
expect(builder.attachContent(...)).rejects.toMatchObject({code:
'E_NO_BLOB_STORAGE'}) or await expect(...).rejects.toHaveProperty('code',
'E_NO_BLOB_STORAGE') and keep the persistence.writeBlob non-call assertion;
apply the same change for the other case referenced (OG-014 at the later
assertion).
Critical: - Fix contentHash to slice ArrayBuffer view correctly (was hashing entire backing buffer for sub-array inputs) High: - Remove dead legacy blob-mode branch in PatchBuilderV2 tree construction - Guard isStreamingInput against Uint8Array/string before async checks Medium: - Add return() to CasBlobAdapter retrieveStream iterator and singleChunkIterator for proper early-break cleanup - Fully revert README.md (unintended agent modification) Low: - Move collectAsyncIterable to streamUtils.js (eliminate duplication) - Rename error code E_NO_BLOB_STORAGE → NO_BLOB_STORAGE (convention) - Add FNV-1a OID length documentation comment Nit: - Fix stale comment in WarpApp content section - Collapse multi-line JSDoc in WarpCore getEdgeContentMeta
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/infrastructure/adapters/CasBlobAdapter.js (1)
210-231:⚠️ Potential issue | 🟠 Major
return()does not delegate to inner iterator; streamed chunks lack normalization.Two issues remain from the prior review feedback:
Incomplete cleanup: Line 225-227 returns immediately without calling
inner.return(). If a consumer breaks early from acas.restoreStream()iterator, the underlying stream never receives cancellation, potentially leaking resources.Missing normalization: Unlike
retrieve()(lines 146-149),retrieveStream()yields chunks without normalizingBuffersubclasses toUint8Array. This inconsistency could causeinstanceof Uint8Arraychecks to fail downstream (seecomparison.methods.js:874).🔒 Proposed fix: delegate return() and normalize chunks
retrieveStream(oid) { const self = this; return /** `@type` {AsyncIterable<Uint8Array>} */ ({ [Symbol.asyncIterator]() { /** `@type` {AsyncIterator<Uint8Array>|null} */ let inner = null; let initialized = false; return { async next() { if (!initialized) { initialized = true; inner = await self._resolveStreamIterator(oid); } - return await /** `@type` {AsyncIterator<Uint8Array>} */ (inner).next(); + const result = await /** `@type` {AsyncIterator<Uint8Array>} */ (inner).next(); + if (result.done) { + return result; + } + // Normalize Buffer to Uint8Array for domain boundary compliance + const chunk = result.value; + const normalized = chunk instanceof Uint8Array && chunk.constructor !== Uint8Array + ? new Uint8Array(chunk.buffer, chunk.byteOffset, chunk.byteLength) + : chunk; + return { value: normalized, done: false }; }, - return() { - return Promise.resolve({ value: undefined, done: true }); + async return() { + if (inner && typeof inner.return === 'function') { + await inner.return(); + } + return { value: undefined, done: true }; }, }; }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/CasBlobAdapter.js` around lines 210 - 231, In retrieveStream(), ensure early cancellation delegates to the underlying iterator and that yielded chunks are normalized to Uint8Array: when implementing the iterator returned by retrieveStream(), call await inner.return() (if present) inside the return() method rather than immediately resolving, and when pulling values from inner (in next()) coerce any Buffer or Buffer subclass to a Uint8Array (matching retrieve()'s normalization) before returning; locate symbols retrieveStream, _resolveStreamIterator, inner, and return() to apply these changes.
🧹 Nitpick comments (2)
src/domain/utils/streamUtils.js (1)
89-104: Missingreturn()handler for iterator cleanup consistency.The
readableStreamToAsyncIterablefunction (lines 125-128) includes areturn()handler for cleanup, butsingleValueAsyncIterabledoes not. While there's nothing to clean up for a single value, adding a no-opreturn()maintains consistency and ensures proper protocol compliance when consumers callbreakorreturnearly in afor-awaitloop.♻️ Suggested addition for consistency
next() { if (done) { return Promise.resolve({ value: undefined, done: true }); } done = true; return Promise.resolve({ value, done: false }); }, + return() { + done = true; + return Promise.resolve({ value: undefined, done: true }); + }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/utils/streamUtils.js` around lines 89 - 104, Add a no-op return() method to the iterator returned by singleValueAsyncIterable so it implements the async iterator cleanup protocol like readableStreamToAsyncIterable; update the object returned by [Symbol.asyncIterator]() (the iterator with next()) to also include a return() that sets done = true and returns Promise.resolve({ value: undefined, done: true }) to gracefully handle early termination in for-await loops.src/domain/utils/defaultBlobStorage.js (1)
97-117: Consider addingreturn()handler for iterator protocol completeness.The async iterator lacks a
return()method. While cleanup isn't strictly needed for in-memory reads, adding it would make the implementation consistent withCasBlobAdapter.singleChunkIteratorand fully conform to the async iterator protocol.🔧 Optional: Add return() handler
return { next() { if (done) { return Promise.resolve({ value: undefined, done: true }); } done = true; return Promise.resolve({ value: chunk, done: false }); }, + return() { + done = true; + return Promise.resolve({ value: undefined, done: true }); + }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/utils/defaultBlobStorage.js` around lines 97 - 117, The async iterator returned by retrieveStream(oid) is missing a return() method; add a return() handler on the iterator object returned by [Symbol.asyncIterator]() that sets the internal done flag to true and returns a resolved Promise of { value: undefined, done: true } so the iterator conforms to the async iterator protocol (mirror the behavior used by CasBlobAdapter.singleChunkIterator), ensuring proper cleanup/early termination support.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/utils/streamUtils.js`:
- Around line 112-132: The iterator returned by readableStreamToAsyncIterable
can leak the reader lock if reader.read() throws; modify the iterator so
reader.releaseLock() is always called on error by wrapping the await
reader.read() in try/catch/finally (or implement a throw(error) method on the
iterator that releases the lock before rethrowing), ensure next() uses
try/finally to release on thrown errors, and keep the existing return() behavior
to release the lock when iteration finishes; reference the
readableStreamToAsyncIterable function, its inner [Symbol.asyncIterator]()
returning next() and return(), and the reader.read()/reader.releaseLock() calls
when making the change.
---
Duplicate comments:
In `@src/infrastructure/adapters/CasBlobAdapter.js`:
- Around line 210-231: In retrieveStream(), ensure early cancellation delegates
to the underlying iterator and that yielded chunks are normalized to Uint8Array:
when implementing the iterator returned by retrieveStream(), call await
inner.return() (if present) inside the return() method rather than immediately
resolving, and when pulling values from inner (in next()) coerce any Buffer or
Buffer subclass to a Uint8Array (matching retrieve()'s normalization) before
returning; locate symbols retrieveStream, _resolveStreamIterator, inner, and
return() to apply these changes.
---
Nitpick comments:
In `@src/domain/utils/defaultBlobStorage.js`:
- Around line 97-117: The async iterator returned by retrieveStream(oid) is
missing a return() method; add a return() handler on the iterator object
returned by [Symbol.asyncIterator]() that sets the internal done flag to true
and returns a resolved Promise of { value: undefined, done: true } so the
iterator conforms to the async iterator protocol (mirror the behavior used by
CasBlobAdapter.singleChunkIterator), ensuring proper cleanup/early termination
support.
In `@src/domain/utils/streamUtils.js`:
- Around line 89-104: Add a no-op return() method to the iterator returned by
singleValueAsyncIterable so it implements the async iterator cleanup protocol
like readableStreamToAsyncIterable; update the object returned by
[Symbol.asyncIterator]() (the iterator with next()) to also include a return()
that sets done = true and returns Promise.resolve({ value: undefined, done: true
}) to gracefully handle early termination in for-await loops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62637179-3ec1-4f0b-a491-6d2d4d591612
📒 Files selected for processing (8)
src/domain/WarpApp.jssrc/domain/WarpCore.jssrc/domain/errors/WriterError.jssrc/domain/services/PatchBuilderV2.jssrc/domain/utils/defaultBlobStorage.jssrc/domain/utils/streamUtils.jssrc/infrastructure/adapters/CasBlobAdapter.jstest/unit/domain/services/PatchBuilderV2.content.test.js
✅ Files skipped from review due to trivial changes (2)
- src/domain/errors/WriterError.js
- src/domain/services/PatchBuilderV2.js
Comment on lines
+112
to
+132
| function readableStreamToAsyncIterable(stream) { | ||
| return { | ||
| [Symbol.asyncIterator]() { | ||
| const reader = stream.getReader(); | ||
| return { | ||
| async next() { | ||
| const { value, done } = await reader.read(); | ||
| if (done) { | ||
| reader.releaseLock(); | ||
| return { value: undefined, done: true }; | ||
| } | ||
| return { value, done: false }; | ||
| }, | ||
| return() { | ||
| reader.releaseLock(); | ||
| return Promise.resolve({ value: undefined, done: true }); | ||
| }, | ||
| }; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Potential resource leak if reader.read() throws.
If reader.read() throws an error, releaseLock() is never called, leaving the stream locked. Consider wrapping the read in try-catch or adding a throw() method to the iterator.
🛡️ Proposed fix to ensure lock release on error
function readableStreamToAsyncIterable(stream) {
return {
[Symbol.asyncIterator]() {
const reader = stream.getReader();
return {
async next() {
- const { value, done } = await reader.read();
- if (done) {
+ try {
+ const { value, done } = await reader.read();
+ if (done) {
+ reader.releaseLock();
+ return { value: undefined, done: true };
+ }
+ return { value, done: false };
+ } catch (err) {
reader.releaseLock();
- return { value: undefined, done: true };
+ throw err;
}
- return { value, done: false };
},
return() {
reader.releaseLock();
return Promise.resolve({ value: undefined, done: true });
},
};
},
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/utils/streamUtils.js` around lines 112 - 132, The iterator
returned by readableStreamToAsyncIterable can leak the reader lock if
reader.read() throws; modify the iterator so reader.releaseLock() is always
called on error by wrapping the await reader.read() in try/catch/finally (or
implement a throw(error) method on the iterator that releases the lock before
rethrowing), ensure next() uses try/finally to release on thrown errors, and
keep the existing return() behavior to release the lock when iteration finishes;
reference the readableStreamToAsyncIterable function, its inner
[Symbol.asyncIterator]() returning next() and return(), and the
reader.read()/reader.releaseLock() calls when making the change.
BREAKING CHANGE: TraversalService alias and createWriter() method removed. - TraversalService: deleted source file and test, removed from index.js, index.d.ts, type-surface manifest, ESLint config, and export tests. Use CommitDagTraversalService directly. - createWriter(): removed from patch.methods.js, WarpApp, index.d.ts, type-surface manifest, and all test files. Use writer() or writer(id). LogicalTraversal retained as internal implementation — traverse property still works on WarpRuntime, Observer, and Worldline. 5192 tests pass across 321 files.
Bump package.json and jsr.json to 16.0.0. Date CHANGELOG [Unreleased] section to [16.0.0] — 2026-03-29. Add MIGRATING.md with upgrade guide for all breaking changes. Update ROADMAP header to v16.0.0 current. BREAKING CHANGE: see MIGRATING.md for full upgrade guide.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
index.d.ts (1)
366-367: Narrow theGraphPersistencePortsummary to match declared methods.The description currently mentions refs/blobs/trees, but this interface only declares commit/node/log/ping/count operations.
Doc wording tweak
-/** Abstract port for Git persistence operations (commits, refs, blobs, trees). */ +/** Abstract port for Git commit persistence and graph-walk operations. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.d.ts` around lines 366 - 367, Update the JSDoc summary for class GraphPersistencePort to accurately reflect only the operations declared (commit, node, log, ping, count) instead of mentioning refs/blobs/trees; locate the GraphPersistencePort class and edit its top comment to describe it as an abstract port for commit/node/log/ping/count persistence operations and remove references to refs, blobs, and trees so the doc matches the implemented methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.d.ts`:
- Around line 251-254: The union type WeightedCostSelector currently allows `{}`
because both properties are optional; change the union so each branch requires
its respective provider: make the first branch require weightFn (and keep
nodeWeightFn as never/optional) and make the second branch require nodeWeightFn
(and keep weightFn as never/optional) so consumers must supply exactly one of
EdgeWeightFn or NodeWeightFn; refer to WeightedCostSelector, EdgeWeightFn and
NodeWeightFn when updating the type.
---
Nitpick comments:
In `@index.d.ts`:
- Around line 366-367: Update the JSDoc summary for class GraphPersistencePort
to accurately reflect only the operations declared (commit, node, log, ping,
count) instead of mentioning refs/blobs/trees; locate the GraphPersistencePort
class and edit its top comment to describe it as an abstract port for
commit/node/log/ping/count persistence operations and remove references to refs,
blobs, and trees so the doc matches the implemented methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a24a88d2-7191-49db-8f34-63b4d36dc9c7
⛔ Files ignored due to path filters (1)
test/unit/domain/__snapshots__/WarpRuntime.apiSurface.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
.gitignoreCHANGELOG.mdMIGRATING.mdcontracts/type-surface.m8.jsondocs/ROADMAP.mdeslint.config.jsindex.d.tsindex.jsjsr.jsonpackage.jsonsrc/domain/WarpApp.jssrc/domain/services/TraversalService.jssrc/domain/warp/_wiredMethods.d.tssrc/domain/warp/patch.methods.jstest/type-check/consumer.tstest/unit/domain/WarpGraph.prototypeWiring.test.jstest/unit/domain/WarpGraph.test.jstest/unit/domain/WarpGraph.writerApi.test.jstest/unit/domain/WarpGraph.writerInvalidation.test.jstest/unit/domain/index.exports.test.jstest/unit/domain/services/TraversalService.test.jstest/unit/scripts/release-policy-shape.test.js
💤 Files with no reviewable changes (8)
- src/domain/services/TraversalService.js
- test/type-check/consumer.ts
- test/unit/domain/services/TraversalService.test.js
- eslint.config.js
- src/domain/warp/_wiredMethods.d.ts
- test/unit/domain/WarpGraph.test.js
- test/unit/domain/index.exports.test.js
- test/unit/domain/WarpGraph.writerApi.test.js
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- package.json
- jsr.json
- test/unit/scripts/release-policy-shape.test.js
- MIGRATING.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/ROADMAP.md
- index.js
- contracts/type-surface.m8.json
Comment on lines
+251
to
254
| /** Selector for weighted cost traversal mode — supply either an edge or node weight function, not both. */ | ||
| export type WeightedCostSelector = | ||
| | { weightFn?: EdgeWeightFn; nodeWeightFn?: never } | ||
| | { nodeWeightFn?: NodeWeightFn; weightFn?: never }; |
There was a problem hiding this comment.
Require exactly one provider in WeightedCostSelector.
The current union still accepts {} because both branches are optional, which conflicts with the declared contract (“either edge or node weight function”).
Suggested type fix
export type WeightedCostSelector =
- | { weightFn?: EdgeWeightFn; nodeWeightFn?: never }
- | { nodeWeightFn?: NodeWeightFn; weightFn?: never };
+ | { weightFn: EdgeWeightFn; nodeWeightFn?: never }
+ | { nodeWeightFn: NodeWeightFn; weightFn?: never };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Selector for weighted cost traversal mode — supply either an edge or node weight function, not both. */ | |
| export type WeightedCostSelector = | |
| | { weightFn?: EdgeWeightFn; nodeWeightFn?: never } | |
| | { nodeWeightFn?: NodeWeightFn; weightFn?: never }; | |
| /** Selector for weighted cost traversal mode — supply either an edge or node weight function, not both. */ | |
| export type WeightedCostSelector = | |
| | { weightFn: EdgeWeightFn; nodeWeightFn?: never } | |
| | { nodeWeightFn: NodeWeightFn; weightFn?: never }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.d.ts` around lines 251 - 254, The union type WeightedCostSelector
currently allows `{}` because both properties are optional; change the union so
each branch requires its respective provider: make the first branch require
weightFn (and keep nodeWeightFn as never/optional) and make the second branch
require nodeWeightFn (and keep weightFn as never/optional) so consumers must
supply exactly one of EdgeWeightFn or NodeWeightFn; refer to
WeightedCostSelector, EdgeWeightFn and NodeWeightFn when updating the type.
Keep LogicalTraversal interface and traverse property in index.d.ts and type-surface manifest — the runtime traverse property is still used by consumers via graph.traverse.bfs() etc. Only TraversalService alias and createWriter() are removed as breaking changes.
Release Preflight
If you tag this commit as |
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.
Summary
attachContent()/attachEdgeContent()always route throughBlobStoragePort. The rawpersistence.writeBlob()fallback is removed.WarpRuntime.open()auto-constructsCasBlobAdapterfor Git-backed persistence orInMemoryBlobStorageAdapterfor browser/test paths.attachContent()andattachEdgeContent()now acceptAsyncIterable<Uint8Array>,ReadableStream<Uint8Array>,Uint8Array, orstring. Streaming inputs pipe directly to blob storage without intermediate buffering.getContentStream()/getEdgeContentStream()returnAsyncIterable<Uint8Array>for incremental consumption. BufferedgetContent()/getEdgeContent()remain as convenience.index.d.tsnow have JSDoc comments (up from 65%).Breaking Changes
attachContent()/attachEdgeContent()without blob storage now throwE_NO_BLOB_STORAGE040000 treemode (CAS trees) instead of100644 blobWarpRuntime.open()auto-constructs blob storage when none providedNew Public API
BlobStoragePort.storeStream()/retrieveStream()InMemoryBlobStorageAdapter(exported)WarpApp.getContentStream()/getEdgeContentStream()WarpCore.getContentStream()/getEdgeContentStream()Backlog Items Closed
Test Plan
Summary by CodeRabbit
New Features
Documentation
Breaking Changes
Removals