feat: Browsa WebSocket serve + Uint8Array migration (v14.0.0)

[flyingrobots]

Summary

• git warp serve — New CLI command that opens a WebSocket server exposing WARP graphs to browser clients. Supports Node.js, Bun, and Deno via runtime-detected adapters (NodeWsAdapter, BunWsAdapter, DenoWsAdapter). Optional --static <dir> flag serves a built SPA on the same port.

• Browsa architecture pivot — Rewired the Vue demo app from in-memory WarpGraph instances to a live WebSocket connection via WarpSocket. The browser now views/edits a real Git-backed graph through git warp serve.

• Uint8Array migration (BREAKING) — Domain layer now uses Uint8Array exclusively; Buffer confined to infrastructure adapters. ESLint no-restricted-globals enforces the boundary. All port contracts, return types, and domain utilities updated.

• git-cas v5.2.4 — CDC chunking for seek cache (~98% chunk reuse), encrypted seek cache via AES-256-GCM, streaming restore, observability bridge.

• Graph encryption at rest (B164) — patchBlobStorage option on WarpGraph.open() encrypts/decrypts patch CBOR via CAS. Mixed encrypted/plain patches supported.

• Content attachments via CAS (B160) — BlobStoragePort and CasBlobAdapter for CDC-chunked, optionally encrypted content blob storage.

• Security hardening — Mutation op allowlist + per-op arg validation in WarpServeService, hexDecode input validation, protocol version checking, request timeouts.

Test plan

• 272 test files, 4857 tests passing
• ESLint: 0 errors, 0 warnings
• tsc --noEmit: 0 errors (strict mode)
• Consumer type surface test: passing
• Pre-push IRONCLAD gates: all passing
• CI: lint + unit + integration + BATS (Node 22, Bun, Deno)
• Manual: git warp serve → open browsa in browser → add/remove nodes → time-travel slider → inspect node

Summary by CodeRabbit

• New Features

  • New "serve" CLI with a browser-based Web Inspector (live graph view, mutations, time-travel) and runtime WebSocket adapters for Node/Bun/Deno.
  • Optional encrypted patch/blob storage and a CAS-backed blob adapter; browser entrypoint and sha1sync for browser hashing.

• Changed

  • Binary I/O unified to Uint8Array across APIs; portable byte/crypto utilities and broader browser compatibility.
  • WebSocket protocol tightened with payload validation and error handling.

• Upgraded

  • Package version bumped to 14.0.0.

• Tests

  • Extensive unit/integration tests added and updated for new features and Uint8Array behavior.

• Documentation

  • README and roadmap updated with serve/Inspector and browser compatibility notes.

[flyingrobots]

PR Feedback Round 5 — Resolved in 135a833

Fixed (10 issues)

1. WarpServeService.listen() — Added double-call guard (throws if already listening)
2. WarpServeService.close() — Now closes active WebSocket connections before clearing
3. WarpServeService._handleOpen() — Moved openGraphs.add() after materialization succeeds
4. WarpServeService._applyMutateOps() — Pre-validates all ops before createPatch()
5. bytes.js base64Decode — Rejects malformed input with RangeError
6. NodeWsAdapter — Resets state on failed listen()/close()
7. isLoopback() — Now handles full 127.0.0.0/8 range
8. materializeAdvanced — buildSeekCacheKey wrapped in try/catch (cache miss, not crash)
9. BunWsAdapter test — Saves/restores globalThis.Bun instead of deleting
10. WarpServeService test — Replaced 22 boolean vi.waitFor() callbacks with assertion-based

False Positives (12 threads)

• 5× demo/browsa/ — Files extracted to git-warp-web-inspector, no longer on this branch
• serve.js:55 port type — serveSchema uses z.coerce.number(), converts string→number before listen()
• eslint.config.js:19 ignore — Ignore list doesn't include demo/browsa/; nothing to narrow
• eslint.config.js:293 Buffer imports — Already have no-restricted-imports banning node:buffer/buffer in src/domain/**
• staticFileHandler.test.js:37 trailing slash — Test correctly asserts 404 for dir with no index.html
• InMemoryGraphAdapter logNodesStream — Lazy import('node:stream') is the standard pattern; function is in infrastructure, not domain
• defaultCrypto.js singleton — Intentional BULKHEAD pattern (matches defaultCodec/defaultClock); services accept crypto via constructor DI with defaultCrypto as fallback
• BunWsAdapter.test.js:45 fetch/upgrade — Tests mock Bun.serve() and simulate events directly; appropriate for adapter unit tests

[flyingrobots]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

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)

jsr.json (1)

11-24: ⚠️ Potential issue | 🔴 Critical

Publish the new ./browser entrypoint.

Line 11 adds ./browser, but Lines 15-24 never ship browser.js, so the JSR artifact will expose a broken public subpath import.

📦 Minimal manifest fix

   "publish": {
     "include": [
+      "browser.js",
       "index.js",
       "index.d.ts",
       "src/**/*.js",
       "src/**/*.d.ts",

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@jsr.json` around lines 11 - 24, The manifest exposes a new public subpath
"./browser" but the publish.include array doesn't list browser.js, causing
published artifacts to miss that entrypoint; update the publish.include array to
add browser.js so the "./browser" entrypoint is shipped (ensure the string
"browser.js" is included alongside other entries in the "publish.include" list
to match the new "./browser" mapping).

♻️ Duplicate comments (4)

eslint.config.js (1)

280-291: ⚠️ Potential issue | 🟠 Major

Also block dynamic Buffer loads in domain code.

Lines 280-291 only cover static import declarations. A domain module can still pull in Buffer via import("node:buffer") (and any require("buffer") path), so the Uint8Array boundary is still bypassable. Please extend the existing src/domain/**/*.js no-restricted-syntax rule at Lines 299-312 instead of adding a second no-restricted-syntax override.

Suggested hardening

   {
     files: ["src/domain/**/*.js"],
     rules: {
       "no-restricted-syntax": ["error",
+        {
+          "selector": "ImportExpression[source.value=/^(?:buffer|node:buffer)$/]",
+          "message": "Use Uint8Array + helpers from domain/utils/bytes.js. Buffer is confined to infrastructure adapters.",
+        },
+        {
+          "selector": "CallExpression[callee.name='require'][arguments.0.value=/^(?:buffer|node:buffer)$/]",
+          "message": "Use Uint8Array + helpers from domain/utils/bytes.js. Buffer is confined to infrastructure adapters.",
+        },
         {
           "selector": "CallExpression[callee.object.name='Date'][callee.property.name='now']",
           "message": "Date.now() is banned in domain code. Use ClockPort / ClockAdapter instead.",
         },

#!/bin/bash
# Verify the current config only restricts static imports, then look for any
# dynamic buffer loads in domain code.
sed -n '273,314p' eslint.config.js
rg -nP --type=js 'import\s*\(\s*["'"'"'](?:node:buffer|buffer)["'"'"']\s*\)|\brequire\s*\(\s*["'"'"'](?:node:buffer|buffer)["'"'"']\s*\)' src/domain

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@eslint.config.js` around lines 280 - 291, The current no-restricted-imports
entries block only static imports for "node:buffer" and "buffer" but dynamic
loads (import(...) / require(...)) are still allowed; modify the existing
no-restricted-syntax override that targets domain files (the override with
"files": ["src/domain/**/*.js"] and rule "no-restricted-syntax") to include AST
selectors that reject ImportExpression and require CallExpression for those same
module names (e.g., selectors targeting ImportExpression whose source value is
"node:buffer" or "buffer", and CallExpression where callee is require and the
first argument value is "node:buffer" or "buffer"), rather than adding a second
no-restricted-syntax override, so dynamic imports/require of Buffer are
forbidden in domain code just like static imports are.

src/infrastructure/adapters/CasBlobAdapter.js (1)

135-151: ⚠️ Potential issue | 🟠 Major

Only apply the legacy-blob fallback to readManifest() failures.

After readManifest() succeeds, oid is already known to be a CAS tree. With the current shared try/catch, a later restore() failure such as a missing chunk (bad object / does not exist) is treated as legacy content and falls back to readBlob(oid), which masks the real restore error.

Proposed fix

   async retrieve(oid) {
     const cas = await this._getCas();
-
-    try {
-      const manifest = await cas.readManifest({ treeOid: oid });
-      /** `@type` {{ manifest: *, encryptionKey?: Buffer|Uint8Array }} */
-      const restoreOpts = { manifest };
-      if (this._encryptionKey) {
-        restoreOpts.encryptionKey = this._encryptionKey;
-      }
-      const { buffer } = await cas.restore(restoreOpts);
-      return buffer;
-    } catch (err) {
-      // Fallback: OID may be a raw Git blob (pre-CAS content).
-      // Only fall through for "not a manifest" errors (missing tree, bad format).
-      // Rethrow corruption, decryption, and I/O errors.
-      if (!isLegacyBlobError(err)) {
-        throw err;
-      }
-      return await this._persistence.readBlob(oid);
-    }
+    let manifest;
+    try {
+      manifest = await cas.readManifest({ treeOid: oid });
+    } catch (err) {
+      if (!isLegacyBlobError(err)) {
+        throw err;
+      }
+      return await this._persistence.readBlob(oid);
+    }
+
+    /** `@type` {{ manifest: *, encryptionKey?: Buffer|Uint8Array }} */
+    const restoreOpts = { manifest };
+    if (this._encryptionKey) {
+      restoreOpts.encryptionKey = this._encryptionKey;
+    }
+    const { buffer } = await cas.restore(restoreOpts);
+    return buffer;
   }

🤖 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 135 - 151, The
fallback to legacy blobs is too broad because the try/catch wraps both
cas.readManifest and cas.restore; instead only catch errors from
cas.readManifest and apply isLegacyBlobError-based fallback there. Concretely,
call cas.readManifest({ treeOid: oid }) inside its own try/catch (using
isLegacyBlobError to decide to call this._persistence.readBlob(oid)), and if
readManifest succeeds then call cas.restore(restoreOpts) without that
legacy-error catch so restore errors (e.g., missing chunks, corruption,
decryption, I/O) propagate; adjust references to cas.readManifest, cas.restore,
isLegacyBlobError, and this._persistence.readBlob accordingly.

src/domain/services/WarpServeService.js (1)

244-258: ⚠️ Potential issue | 🟠 Major

Delay _server and subscriptions until startup succeeds.

The guard at Lines 245-247 only covers the already-started path. Line 248 stores this._server and Lines 250-256 register subscriptions before the bind completes, so a thrown listen() or subscribe() leaves the instance stuck in "Server is already listening" with leaked diff callbacks.

Possible fix

   async listen(port, host) {
     if (this._server) {
       throw new Error('Server is already listening');
     }
-    this._server = this._wsPort.createServer((conn) => this._onConnection(conn));
-
-    // Subscribe to each graph for live diff push
-    for (const [graphName, graph] of this._graphs) {
-      const sub = graph.subscribe({
-        onChange: (/** `@type` {unknown} */ diff) => this._broadcastDiff(graphName, diff),
-      });
-      this._subscriptions.set(graphName, sub);
-    }
-
-    return await this._server.listen(port, host);
+    const server = this._wsPort.createServer((conn) => this._onConnection(conn));
+    /** `@type` {Map<string, { unsubscribe: () => void }>} */
+    const subscriptions = new Map();
+    try {
+      const addr = await server.listen(port, host);
+      for (const [graphName, graph] of this._graphs) {
+        const sub = graph.subscribe({
+          onChange: (/** `@type` {unknown} */ diff) => this._broadcastDiff(graphName, diff),
+        });
+        subscriptions.set(graphName, sub);
+      }
+      this._server = server;
+      this._subscriptions = subscriptions;
+      return addr;
+    } catch (err) {
+      for (const [, sub] of subscriptions) {
+        sub.unsubscribe();
+      }
+      await server.close().catch(() => {});
+      throw err;
+    }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/domain/services/WarpServeService.js` around lines 244 - 258, The listen
method currently assigns this._server and registers subscriptions before the
async bind completes, causing leaked state if listen() or subscribe() throws;
change listen to create the server locally (e.g., const server =
this._wsPort.createServer(...)) and perform await server.listen(port, host)
first, then on success set this._server = server and move the subscription loop
(graph.subscribe with this._broadcastDiff) after the successful bind, storing
each subscription into this._subscriptions; also ensure you catch errors from
server.listen or graph.subscribe and clean up any partial subscriptions or close
the local server before rethrowing so the instance isn't left in a
half-initialized state.

bin/cli/commands/serve.js (1)

43-45: ⚠️ Potential issue | 🟠 Major

Prefix-matching 127. weakens the --expose safety check.

Line 44 now whitelists any hostname beginning with 127.. Values like 127.example.com are not IP literals and can still resolve off-box, so they bypass the non-loopback bind guard. Only treat parsed IP literals in 127.0.0.0/8 as loopback, and include mapped ::ffff:127.x.x.x forms.

🔒 Safer loopback check

 import process from 'node:process';
 import { resolve } from 'node:path';
 import { stat } from 'node:fs/promises';
+import { isIP } from 'node:net';
@@
 function isLoopback(h) {
-  return h === '127.0.0.1' || h === '::1' || h === 'localhost' || h.startsWith('127.');
+  if (h.toLowerCase() === 'localhost' || h === '::1') {
+    return true;
+  }
+  if (h.startsWith('::ffff:')) {
+    const mapped = h.slice('::ffff:'.length);
+    return isIP(mapped) === 4 && mapped.split('.')[0] === '127';
+  }
+  return isIP(h) === 4 && h.split('.')[0] === '127';
 }

Please add a regression test for a 127.* hostname that is not an IP literal.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bin/cli/commands/serve.js` around lines 43 - 45, The isLoopback function
incorrectly treats any hostname starting with "127." as loopback; update
isLoopback to parse the hostname as an IP literal (use Node's net.isIP or
equivalent) and then check that the parsed address is in 127.0.0.0/8 or the
mapped ::ffff:127.x.x.x form rather than prefix-matching strings, and ensure
"localhost" and "::1" still pass; also add a regression test that asserts a
non-literal hostname like "127.example.com" is NOT considered loopback by
isLoopback and that a literal "127.0.0.1" (and ::ffff:127.0.0.1) still are.

🤖 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/services/WarpServeService.js`:
- Around line 120-125: The props bag is created with a normal object literal
which allows graph-derived keys like "__proto__" or "constructor" to mutate the
prototype; when initializing the props entry in nodePropsMap for decoded.nodeId,
create a null-prototype object (e.g. via Object.create(null)) instead of {} so
decoded.propKey can be used safely, then continue assigning
props[decoded.propKey] = lwwValue(reg) as before; update the initialization
logic around nodePropsMap.get(decoded.nodeId) / nodePropsMap.set(...) to use the
null-prototype object.

In `@src/infrastructure/adapters/NodeWsAdapter.js`:
- Around line 18-28: wrapConnection currently registers message and close
listeners on ws but omits a socket-level 'error' listener; add ws.on('error',
...) inside wrapConnection so socket errors don't crash the process, and route
the error to an existing error handler if one exists (or at minimum log the
error) — reference ws, wrapConnection, messageHandler and closeHandler when
adding this handler so it mirrors the other event wiring and safely consumes or
delegates socket errors.

In `@src/infrastructure/adapters/sha1sync.js`:
- Around line 107-119: Add a TypeScript declaration for the public sha1sync
module so the new ./sha1sync entrypoint has types; create a file named
src/infrastructure/adapters/sha1sync.d.ts that declares the exported function
sha1sync with the correct signature (accepting a Uint8Array and returning a
string) so TypeScript consumers and Release Preflight stop warning about a
missing declaration for the sha1sync export.

In `@test/unit/domain/WarpGraph.encryption.test.js`:
- Around line 127-138: The test only captures a rejection from WarpGraph.open
but not a later rejection from graph.materialize, so it can miss the error if
the implementation defers the check; change the test to handle both cases by
awaiting openPromise and, if it resolves to a graph, call graph.materialize()
and capture any thrown error, otherwise capture the open() rejection — then
assert the captured error is an EncryptionError and its message matches
/encrypted patches/; reference WarpGraph.open, the openPromise variable, and the
graph.materialize() call when implementing this.
- Around line 25-41: The in-memory blob store is too permissive and returns
mutable buffers by reference; update the store(…) and retrieve(…) in this test
helper so store only accepts/operates on Uint8Array (reject or coerce strings
upstream, or throw if a string is passed) and always store a copied Uint8Array
(use a new Uint8Array(buf) copy) into this._blobs; change retrieve(oid) to
return a copy of the stored buffer (not the original reference) so callers
cannot mutate the backing buffer. Ensure references to store and retrieve in
tests use the Uint8Array-only contract and that stored buffers are
created/copied via new Uint8Array(...) before insertion into this._blobs.
- Around line 182-203: The test currently materializes the same WarpGraph
instance that performed the patch, which can hide regressions in the persistence
read path; after performing graph.patch and asserting patchStorage._blobs is
empty, close the current graph and reopen a fresh WarpGraph via WarpGraph.open
(using the same persistence, graphName 'plain-test', and writerId) and then call
materialize(), hasNode('user:normal'), and getNodeProps('user:normal') on the
reopened instance to verify the unencrypted persistence path actually persisted
and can be read by a new graph instance.

In `@test/unit/infrastructure/adapters/CasBlobAdapter.test.js`:
- Around line 9-20: The test currently relies on lastConstructorArgs to infer
reuse but that can be identical across two instantiations; modify
MockContentAddressableStore to track explicit constructor calls by adding a
counter (e.g., casConstructorCount) and incrementing it in the constructor
(inside class MockContentAddressableStore); in the reuse test assert
casConstructorCount === 1 (or appropriate expected count) in addition to any
existing checks; update other referenced sections (around lines noted) to use
the same counter check so tests verify actual reuse rather than identical
options.

---

Outside diff comments:
In `@jsr.json`:
- Around line 11-24: The manifest exposes a new public subpath "./browser" but
the publish.include array doesn't list browser.js, causing published artifacts
to miss that entrypoint; update the publish.include array to add browser.js so
the "./browser" entrypoint is shipped (ensure the string "browser.js" is
included alongside other entries in the "publish.include" list to match the new
"./browser" mapping).

---

Duplicate comments:
In `@bin/cli/commands/serve.js`:
- Around line 43-45: The isLoopback function incorrectly treats any hostname
starting with "127." as loopback; update isLoopback to parse the hostname as an
IP literal (use Node's net.isIP or equivalent) and then check that the parsed
address is in 127.0.0.0/8 or the mapped ::ffff:127.x.x.x form rather than
prefix-matching strings, and ensure "localhost" and "::1" still pass; also add a
regression test that asserts a non-literal hostname like "127.example.com" is
NOT considered loopback by isLoopback and that a literal "127.0.0.1" (and
::ffff:127.0.0.1) still are.

In `@eslint.config.js`:
- Around line 280-291: The current no-restricted-imports entries block only
static imports for "node:buffer" and "buffer" but dynamic loads (import(...) /
require(...)) are still allowed; modify the existing no-restricted-syntax
override that targets domain files (the override with "files":
["src/domain/**/*.js"] and rule "no-restricted-syntax") to include AST selectors
that reject ImportExpression and require CallExpression for those same module
names (e.g., selectors targeting ImportExpression whose source value is
"node:buffer" or "buffer", and CallExpression where callee is require and the
first argument value is "node:buffer" or "buffer"), rather than adding a second
no-restricted-syntax override, so dynamic imports/require of Buffer are
forbidden in domain code just like static imports are.

In `@src/domain/services/WarpServeService.js`:
- Around line 244-258: The listen method currently assigns this._server and
registers subscriptions before the async bind completes, causing leaked state if
listen() or subscribe() throws; change listen to create the server locally
(e.g., const server = this._wsPort.createServer(...)) and perform await
server.listen(port, host) first, then on success set this._server = server and
move the subscription loop (graph.subscribe with this._broadcastDiff) after the
successful bind, storing each subscription into this._subscriptions; also ensure
you catch errors from server.listen or graph.subscribe and clean up any partial
subscriptions or close the local server before rethrowing so the instance isn't
left in a half-initialized state.

In `@src/infrastructure/adapters/CasBlobAdapter.js`:
- Around line 135-151: The fallback to legacy blobs is too broad because the
try/catch wraps both cas.readManifest and cas.restore; instead only catch errors
from cas.readManifest and apply isLegacyBlobError-based fallback there.
Concretely, call cas.readManifest({ treeOid: oid }) inside its own try/catch
(using isLegacyBlobError to decide to call this._persistence.readBlob(oid)), and
if readManifest succeeds then call cas.restore(restoreOpts) without that
legacy-error catch so restore errors (e.g., missing chunks, corruption,
decryption, I/O) propagate; adjust references to cas.readManifest, cas.restore,
isLegacyBlobError, and this._persistence.readBlob accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ad100dd8-619e-45d0-befb-152954b78393

📥 Commits

Reviewing files that changed from the base of the PR and between 0478abf and 135a833.

📒 Files selected for processing (26)

• CHANGELOG.md
• bin/cli/commands/serve.js
• bin/cli/schemas.js
• bin/warp-graph.js
• eslint.config.js
• index.d.ts
• index.js
• jsr.json
• src/domain/services/WarpServeService.js
• src/domain/utils/bytes.js
• src/domain/utils/defaultCrypto.js
• src/domain/warp/materializeAdvanced.methods.js
• src/infrastructure/adapters/CasBlobAdapter.js
• src/infrastructure/adapters/NodeCryptoAdapter.js
• src/infrastructure/adapters/NodeWsAdapter.js
• src/infrastructure/adapters/TrustCryptoAdapter.js
• src/infrastructure/adapters/sha1sync.js
• test/unit/cli/commands/serve.test.js
• test/unit/cli/schemas.test.js
• test/unit/domain/WarpGraph.encryption.test.js
• test/unit/domain/services/WarpServeService.test.js
• test/unit/infrastructure/adapters/BunWsAdapter.test.js
• test/unit/infrastructure/adapters/CasBlobAdapter.test.js
• test/unit/infrastructure/adapters/httpAdapterUtils.test.js
• test/unit/infrastructure/adapters/lazyCasInit.test.js
• test/unit/infrastructure/adapters/sha1sync.test.js

✅ Files skipped from review due to trivial changes (1)

• test/unit/infrastructure/adapters/lazyCasInit.test.js

🚧 Files skipped from review as they are similar to previous changes (5)

• bin/warp-graph.js
• test/unit/infrastructure/adapters/BunWsAdapter.test.js
• test/unit/infrastructure/adapters/sha1sync.test.js
• src/domain/utils/bytes.js
• src/infrastructure/adapters/TrustCryptoAdapter.js

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use a null-prototype object for serialized node props.

Line 122 starts each props bag with {} even though decoded.propKey comes from graph data. Keys like __proto__ or constructor won’t round-trip as ordinary properties here, and they can mutate the payload object’s prototype instead.

Possible fix

     let props = nodePropsMap.get(decoded.nodeId);
     if (!props) {
-      props = {};
+      props = /** `@type` {Record<string, unknown>} */ (Object.create(null));
       nodePropsMap.set(decoded.nodeId, props);
     }
     props[decoded.propKey] = lwwValue(reg);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/domain/services/WarpServeService.js` around lines 120 - 125, The props
bag is created with a normal object literal which allows graph-derived keys like
"__proto__" or "constructor" to mutate the prototype; when initializing the
props entry in nodePropsMap for decoded.nodeId, create a null-prototype object
(e.g. via Object.create(null)) instead of {} so decoded.propKey can be used
safely, then continue assigning props[decoded.propKey] = lwwValue(reg) as
before; update the initialization logic around nodePropsMap.get(decoded.nodeId)
/ nodePropsMap.set(...) to use the null-prototype object.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "NodeWsAdapter.js" -type f

Repository: git-stunts/git-warp

Length of output: 109

---

🏁 Script executed:

cat -n src/infrastructure/adapters/NodeWsAdapter.js

Repository: git-stunts/git-warp

Length of output: 8364

---

🏁 Script executed:

cat -n package.json | grep -A 5 -B 5 '"ws"'

Repository: git-stunts/git-warp

Length of output: 416

---

🌐 Web query:

ws npm library error handling unhandled error event EventEmitter

💡 Result:

In Node.js, any EventEmitter that emits an 'error' event will crash the process if you didn’t register at least one 'error' listener—this is what produces throw er; // Unhandled 'error' event. [1]

With the ws library, the fix is to always attach error handlers on:

1. the server (WebSocketServer)
2. each connection (ws socket)
3. (optionally) the underlying HTTP server / upgrade path for bad handshakes

Minimal robust pattern (server)

import http from "http";
import WebSocket, { WebSocketServer } from "ws";

const server = http.createServer();

server.on("error", (err) => console.error("HTTP server error:", err));

const wss = new WebSocketServer({ server });

wss.on("error", (err) => console.error("WSS error:", err));

wss.on("connection", (ws, req) => {
  ws.on("error", (err) => console.error("WS client error:", err)); // prevents “Unhandled 'error' event”
  ws.on("close", (code, reason) => {
    // cleanup / logging
  });
  ws.on("message", (data) => {
    // handle message
  });
});

server.listen(8080);

ws itself shows this same idea in its docs (e.g., ws.on('error', console.error)). [2]

Typical cause of the error you’re seeing

A network/protocol issue (very commonly ECONNRESET, bad frames, etc.) triggers an 'error' on the WebSocket instance; if you didn’t attach ws.on('error', ...), Node treats it as unhandled and throws. [3]

Sources:
[1] Node.js EventEmitter error-event behavior (no listener ⇒ throw) (r2.nodejs.org)
[2] ws package docs example using ws.on('error', ...) (npmjs.com)
[3] Example stack trace showing ws emitting an 'error' leading to “Unhandled 'error' event” (stackoverflow.com)

Citations:

• 1: https://r2.nodejs.org/docs/v5.10.0/api/all.html?utm_source=openai
• 2: https://www.npmjs.com/package/ws?utm_source=openai
• 3: https://stackoverflow.com/questions/69886630/node-js-websocket-unhandeled-error-events-js-websocket-js?utm_source=openai

---

Add a socket-level error handler in wrapConnection.

Lines 18–28 register message and close listeners, but each ws connection is an EventEmitter that can emit error. Without a listener, any socket-level error (e.g., ECONNRESET, malformed frames) will crash the process. Server-level error handlers do not prevent individual socket errors.

Possible fix

   ws.on('message', (/** `@type` {import('ws').RawData} */ data) => {
     if (messageHandler) {
       messageHandler(messageToString(data));
     }
   });
+
+  ws.on('error', () => {
+    // Prevent unhandled socket-level errors; cleanup still happens via `close`.
+  });
 
   ws.on('close', (/** `@type` {number} */ code, /** `@type` {Buffer} */ reason) => {
     if (closeHandler) {
       closeHandler(code, reason?.toString());
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/infrastructure/adapters/NodeWsAdapter.js` around lines 18 - 28,
wrapConnection currently registers message and close listeners on ws but omits a
socket-level 'error' listener; add ws.on('error', ...) inside wrapConnection so
socket errors don't crash the process, and route the error to an existing error
handler if one exists (or at minimum log the error) — reference ws,
wrapConnection, messageHandler and closeHandler when adding this handler so it
mirrors the other event wiring and safely consumes or delegates socket errors.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't materialize every expanded block before hashing.

Lines 38-47 allocate and retain an 80-word schedule for each 64-byte chunk. That is ~320 bytes of schedule data per 64-byte block, so sha1sync() can hit multi-GB transient memory long before the 512 MB guard on Line 108.

♻️ Stream the schedules instead of collecting them

-function preprocess(msg) {
+function* preprocess(msg) {
   const bitLen = msg.length * 8;
   const totalBytes = msg.length + 1 + ((119 - (msg.length % 64)) % 64) + 8;
   const padded = new Uint8Array(totalBytes);
   padded.set(msg);
   padded[msg.length] = 0x80;
   const dv = new DataView(padded.buffer);
   dv.setUint32(totalBytes - 4, bitLen, false);

-  const blocks = [];
   for (let i = 0; i < totalBytes; i += 64) {
     const block = new Uint32Array(80);
     for (let j = 0; j < 16; j++) {
       block[j] = dv.getUint32(i + j * 4, false);
     }
     for (let j = 16; j < 80; j++) {
       block[j] = rotl(block[j - 3] ^ block[j - 8] ^ block[j - 14] ^ block[j - 16], 1);
     }
-    blocks.push(block);
+    yield block;
   }
-  return blocks;
 }
@@
-  const blocks = preprocess(data);
   const state = [0x67452301, 0xEFCDAB89, 0x98BADCFE, 0x10325476, 0xC3D2E1F0];

-  for (const w of blocks) {
+  for (const w of preprocess(data)) {
     processBlock(state, w);
   }

Also applies to: 111-116

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Keep the blob-storage double strict and immutable.

store() still accepts string, and retrieve() returns the backing Uint8Array by reference. That makes this double looser than the new Uint8Array-only port boundary and can hide in-place mutation bugs on repeated reads.

Suggested fix

-  async store(/** `@type` {string|Uint8Array} */ content) {
+  async store(/** `@type` {Uint8Array} */ content) {
     this._counter++;
     // Generate a fake OID (40-char hex)
     const oid = this._counter.toString(16).padStart(40, '0');
-    const buf = typeof content === 'string'
-      ? new TextEncoder().encode(content)
-      : new Uint8Array(content);
+    const buf = new Uint8Array(content);
     this._blobs.set(oid, buf);
     return oid;
   }

   async retrieve(/** `@type` {string} */ oid) {
     const buf = this._blobs.get(oid);
     if (!buf) {
       throw new Error(`InMemoryBlobStorage: OID not found: ${oid}`);
     }
-    return buf;
+    return new Uint8Array(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

	async store(/** @type {string|Uint8Array} */ content) {
	this._counter++;
	// Generate a fake OID (40-char hex)
	const oid = this._counter.toString(16).padStart(40, '0');
	const buf = typeof content === 'string'
	? new TextEncoder().encode(content)
	: new Uint8Array(content);
	this._blobs.set(oid, buf);
	return oid;
	}
	async retrieve(/** @type {string} */ oid) {
	const buf = this._blobs.get(oid);
	if (!buf) {
	throw new Error(`InMemoryBlobStorage: OID not found: ${oid}`);
	}
	return buf;
	async store(/** `@type` {Uint8Array} */ content) {
	this._counter++;
	// Generate a fake OID (40-char hex)
	const oid = this._counter.toString(16).padStart(40, '0');
	const buf = new Uint8Array(content);
	this._blobs.set(oid, buf);
	return oid;
	}
	async retrieve(/** `@type` {string} */ oid) {
	const buf = this._blobs.get(oid);
	if (!buf) {
	throw new Error(`InMemoryBlobStorage: OID not found: ${oid}`);
	}
	return new Uint8Array(buf);
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/domain/WarpGraph.encryption.test.js` around lines 25 - 41, The
in-memory blob store is too permissive and returns mutable buffers by reference;
update the store(…) and retrieve(…) in this test helper so store only
accepts/operates on Uint8Array (reject or coerce strings upstream, or throw if a
string is passed) and always store a copied Uint8Array (use a new
Uint8Array(buf) copy) into this._blobs; change retrieve(oid) to return a copy of
the stored buffer (not the original reference) so callers cannot mutate the
backing buffer. Ensure references to store and retrieve in tests use the
Uint8Array-only contract and that stored buffers are created/copied via new
Uint8Array(...) before insertion into this._blobs.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Exercise both failure points this test claims to cover.

The comment says the missing patchBlobStorage check may fail in open() or materialize(), but the assertion only accepts an open() rejection. If the implementation defers the error until materialization, this becomes a false negative.

Suggested fix

-    const openPromise = WarpGraph.open({
+    const openedOrErr = await WarpGraph.open({
       persistence: repo.persistence,
       graphName: 'enc-test',
       writerId: 'writer-2',
-    });
-
-    /** `@type` {unknown} */
-    const err = await openPromise.then(() => null, (e) => e);
+    }).then((graph) => graph, (e) => e);
+
+    /** `@type` {unknown} */
+    const err = openedOrErr instanceof Error
+      ? openedOrErr
+      : await openedOrErr.materialize().then(() => null, (e) => e);
     expect(err).toBeInstanceOf(EncryptionError);
     expect(/** `@type` {Error} */ (err).message).toMatch(/encrypted patches/);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/domain/WarpGraph.encryption.test.js` around lines 127 - 138, The
test only captures a rejection from WarpGraph.open but not a later rejection
from graph.materialize, so it can miss the error if the implementation defers
the check; change the test to handle both cases by awaiting openPromise and, if
it resolves to a graph, call graph.materialize() and capture any thrown error,
otherwise capture the open() rejection — then assert the captured error is an
EncryptionError and its message matches /encrypted patches/; reference
WarpGraph.open, the openPromise variable, and the graph.materialize() call when
implementing this.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Reopen before asserting the plain persistence path.

This test materializes the same graph instance that wrote the patch. That can still pass from in-memory state even if the unencrypted read/write path regressed. Reopening a fresh WarpGraph is needed to verify the “no behavior change” claim.

Suggested fix

   it('no behavior change when patchBlobStorage is not provided', async () => {
     const graph = await WarpGraph.open({
       persistence: repo.persistence,
       graphName: 'plain-test',
       writerId: 'writer-1',
     });

     await graph.patch(p => {
       p.addNode('user:normal');
       p.setProperty('user:normal', 'status', 'active');
     });

     // patchBlobStorage should be empty — patches went to persistence directly
     expect(patchStorage._blobs.size).toBe(0);

-    const state = await graph.materialize();
+    const reopened = await WarpGraph.open({
+      persistence: repo.persistence,
+      graphName: 'plain-test',
+      writerId: 'writer-2',
+    });
+    const state = await reopened.materialize();
     expect(state).toBeTruthy();

-    expect(await graph.hasNode('user:normal')).toBe(true);
-    const props = /** `@type` {any} */ (await graph.getNodeProps('user:normal'));
+    expect(await reopened.hasNode('user:normal')).toBe(true);
+    const props = /** `@type` {any} */ (await reopened.getNodeProps('user:normal'));
     expect(props.status).toBe('active');
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/domain/WarpGraph.encryption.test.js` around lines 182 - 203, The
test currently materializes the same WarpGraph instance that performed the
patch, which can hide regressions in the persistence read path; after performing
graph.patch and asserting patchStorage._blobs is empty, close the current graph
and reopen a fresh WarpGraph via WarpGraph.open (using the same persistence,
graphName 'plain-test', and writerId) and then call materialize(),
hasNode('user:normal'), and getNodeProps('user:normal') on the reopened instance
to verify the unencrypted persistence path actually persisted and can be read by
a new graph instance.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Count CAS constructor invocations explicitly in the reuse test.

lastConstructorArgs staying equal after two store() calls does not prove the CAS instance was reused; a second initialization with the same options would still pass this test.

Proposed fix

 /** Captures constructor args for assertion. `@type` {any} */
 let lastConstructorArgs = {};
+let constructorCallCount = 0;
 
 class MockContentAddressableStore {
   constructor(/** `@type` {any} */ opts) {
+    constructorCallCount += 1;
     lastConstructorArgs = opts;
     this.readManifest = mockReadManifest;
     this.restore = mockRestore;
     this.store = mockStore;
     this.createTree = mockCreateTree;
@@
   beforeEach(() => {
     vi.clearAllMocks();
     lastConstructorArgs = {};
+    constructorCallCount = 0;
   });
@@
     it('reuses CAS instance across multiple calls', async () => {
       mockStore.mockResolvedValue({});
       mockCreateTree.mockResolvedValue('tree-oid');
@@
       await adapter.store('data1');
       const firstArgs = { ...lastConstructorArgs };
       await adapter.store('data2');
 
       // Same instance (constructor called only once)
       expect(lastConstructorArgs).toEqual(firstArgs);
+      expect(constructorCallCount).toBe(1);
     });

Also applies to: 67-70, 321-335

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/infrastructure/adapters/CasBlobAdapter.test.js` around lines 9 -
20, The test currently relies on lastConstructorArgs to infer reuse but that can
be identical across two instantiations; modify MockContentAddressableStore to
track explicit constructor calls by adding a counter (e.g., casConstructorCount)
and incrementing it in the constructor (inside class
MockContentAddressableStore); in the reuse test assert casConstructorCount === 1
(or appropriate expected count) in addition to any existing checks; update other
referenced sections (around lines noted) to use the same counter check so tests
verify actual reuse rather than identical options.

[github-actions]

Release Preflight

• package version: 14.0.0
• prerelease: false
• npm dist-tag on release: latest
• npm pack dry-run: passed
• jsr publish dry-run: passed

If you tag this commit as v14.0.0, release workflow will publish.

[github-actions]

Release Preflight

• package version: 14.0.0
• prerelease: false
• npm dist-tag on release: latest
• npm pack dry-run: passed
• jsr publish dry-run: passed

If you tag this commit as v14.0.0, release workflow will publish.

[flyingrobots]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

Release Preflight

• package version: 14.0.0
• prerelease: false
• npm dist-tag on release: latest
• npm pack dry-run: passed
• jsr publish dry-run: passed

If you tag this commit as v14.0.0, release workflow will publish.