You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99331795-3999-4e67-9f1a-3060e54f8963
📥 Commits
Reviewing files that changed from the base of the PR and between 1ce95ac and 9b62c90.
⛔ Files ignored due to path filters (1)
package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
types/server.js
🚧 Files skipped from review as they are similar to previous changes (1)
types/server.js
📝 Walkthrough
Walkthrough
Adds transport helper modules (messageTransport, jsonRpcTransport, fabricWebRtcInterop), wires them into server WebSocket and HTTP JSON‑RPC flows, exposes protocol helpers under module.exports.protocol, removes a legacy build wrapper and some re-exports, adds MESSAGE_SPEC and docs updates, and adds tests for the new helpers and payment 402 behavior.
Adds MESSAGE_SPEC, README badges and Programmatic API example, Resources table, and documents export changes.
Build Cleanup builds/esm/fabric.http.js
Removes the legacy @fabric/http ESM wrapper export (FabricIdentity components and wrapper).
Transport Helpers functions/fabricMessageTransport.js, functions/fabricJsonRpcTransport.js, functions/fabricWebRtcInterop.js
New modules providing canonical transport/type constants and normalization, JSON‑RPC envelope and hash helpers, and WebRTC Hub parsing/mesh message utilities.
Package Exports package.json
Adds ./functions/fabricWebRtcInterop, ./functions/fabricMessageTransport, and ./functions/fabricJsonRpcTransport to the exports map.
Public API Surface types/web.js
Adds module.exports.protocol grouping protocol helpers and removes prior Server re-exports (resolveFabricHttpPackageAssetsDir, acceptFirstHtmlNavigation).
Server Integration types/server.js
Wires WebSocket/HTTP JSON‑RPC flows through messageTransport and jsonRpcTransport, normalizes transport types, emits standardized JSON‑RPC envelopes for errors/success, and narrows export to FabricHTTPServer.
Adds tests for WebRTC/message/json‑rpc helpers and extends payment 402 tests (www-authenticate header, macaroon attribute, sanitized invoice payloads, and bounded invoice cache stress test).
Sequence Diagram
sequenceDiagram
participant Client as Client
participant WS as WebSocket
participant MsgTx as MessageTransport
participant JRpcTx as JsonRpcTransport
participant Server as Server
Client->>WS: Send JSON-RPC request (text frame)
WS->>MsgTx: extractTransportControlType()/normalizeTransportType()
MsgTx->>Server: Dispatch canonical JSONCall type
Server->>JRpcTx: parseWebSocketJsonCallBody()/computeWebSocketJsonCallHashPair()
alt Authorized
Server->>Server: Execute handler
Server->>JRpcTx: buildWebSocketJsonCallResultBody()
else Unauthorized
Server->>JRpcTx: buildWebSocketJsonCallErrorBody()
end
JRpcTx->>WS: Return response payload (method, params[hash, result/null])
WS->>Client: Send JSON-RPC response
Loading
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
FabricLabs/fabric-http#54: Both PRs make closely related code-level changes to WebSocket/JSON-RPC handling and WebRTC/peering utilities (types/server.js, message/JSON-RPC transport and WebRTC interop).
FabricLabs/fabric-http#56: Main PR and retrieved PR #56 touch the same protocol pieces (402 payment handler, WebRTC registry/interop, JSON-RPC and message-transport helpers, exports and README/docs changes).
Suggested labels
enhancement
Poem
🐰 Three helpers hop into the fold,
To route WebSocket tales and JSON‑RPC untold.
The server now speaks in protocol tongue,
With transport-shaped tools, a new song is sung!
Hashes hum softly, messages take flight.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name
Status
Explanation
Resolution
Title check
❓ Inconclusive
The title 'RC1 Preparation' is vague and generic, using non-descriptive language that doesn't convey specific information about the extensive changes in this pull request.
Consider a more descriptive title that reflects key changes, such as 'Refactor JSON-RPC/WebSocket transport layer and remove deprecated server exports for RC1' or 'Consolidate transport helpers and update server module exports for 0.1.0-RC1'.
✅ Passed checks (4 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage
✅ Passed
Docstring coverage is 92.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches📝 Generate docstrings
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch feature/v0.1.0-RC1
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.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer TIP This summary will be updated as you push new changes.
Add CHANGELOG and README entries documenting the breaking API change.
The exports of resolveFabricHttpPackageAssetsDir and acceptFirstHtmlNavigation have been removed from types/server.js. While the codebase contains no remaining imports of these functions, downstream consumers relying on these exports will break. This breaking change must be documented in CHANGELOG and README (migration guide or deprecation notes) so users are aware and can update their code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@types/server.js` at line 2548, Update project documentation to record the
breaking API change: add a clear CHANGELOG entry and a README (or migration
guide/deprecation) note that the named exports resolveFabricHttpPackageAssetsDir
and acceptFirstHtmlNavigation were removed from the server module (previously
exported from types/server.js). In the CHANGELOG entry, include the version,
"breaking change" label, and a short migration snippet advising users to remove
imports or replace them with the new recommended approach (or point to
alternative APIs). In the README/migration guide, add a brief example showing
how to refactor code that imported resolveFabricHttpPackageAssetsDir and
acceptFirstHtmlNavigation and link to any upstream replacement or guidance for
maintaining equivalent behavior.
🧹 Nitpick comments (2)
types/server.js (2)
1162-1167: 💤 Low value
Minor: redundant authorization recompute and shadowed identifier.
server._isJsonRpcTransportAuthorized(request) is recomputed here even though socket._fabricTransportAuthorized was set from the same call at line 996/997 during connection setup, and the new local transportAuthorized shadows the outer-scope transportAuthorized from line 996. The same pattern repeats at lines 1272 and 1300.
If the policy decision is intended to be re-evaluated per-message (e.g., to pick up token rotation on request.headers), the duplication is fine but worth a comment. Otherwise, prefer reusing socket._fabricTransportAuthorized to avoid divergent results and the shadow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@types/server.js` around lines 1162 - 1167, The code recomputes transport
authorization into a new local transportAuthorized variable using
server._isJsonRpcTransportAuthorized(request) which shadows the outer
transportAuthorized set earlier and risks divergence; change the logic in the
block that checks server._isWebRtcRegistryMethod(jsonCallPayload.method) &&
wrtcCfg.requireTransportAuth === true to reuse socket._fabricTransportAuthorized
instead of calling server._isJsonRpcTransportAuthorized again (or, if you
intentionally want per-message re-evaluation for token rotation, add an explicit
comment explaining that server._isJsonRpcTransportAuthorized(request) must be
re-run). Update the occurrences at the shown snippet and the similar spots noted
(around the uses at lines corresponding to 1272 and 1300) to be consistent.
1095-1108: 💤 Low value
Use messageTransport.HEARTBEAT_TYPE instead of hardcoded 'HEARTBEAT' at line 1097.
Lines 1090 and 1097 both duplicate the extraction parsed.type || parsed['@type']. While consolidating this extraction would improve DRY, note that ctrl (from line 1080) is out of scope here. A simpler win is replacing the hardcoded literal with the exported constant, keeping the code consistent with the abstraction layer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@types/server.js` around lines 1095 - 1108, Replace the hardcoded literal
'HEARTBEAT' with the exported constant messageTransport.HEARTBEAT_TYPE in the
branch that builds a HEARTBEAT Message (the conditional comparing rawType at the
parsed object and the Message.fromVector call); keep the rest of the logic the
same (use parsed.type || parsed['@type'] to compute rawType and use parsed.body
?? parsed.data ?? parsed.content ?? parsed['@data'] ?? '' for payload), ensuring
you reference messageTransport.HEARTBEAT_TYPE instead of the string so the check
and the Message.fromVector payload use the constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@functions/fabricJsonRpcTransport.js`:
- Around line 60-62: The parseWebSocketJsonCallBody function currently blindly
returns JSON.parse(body); update it to parse the body then validate the result
is a non-null object and not an array (e.g., result !== null && typeof result
=== 'object' && !Array.isArray(result')); if the check fails, throw a TypeError
with a clear message indicating an unexpected JSON payload type so downstream
consumers expecting an object-like call body (parseWebSocketJsonCallBody) cannot
receive primitives or arrays.
In `@README.md`:
- Around line 123-124: Add a blank line between the heading "## Resources" and
the table row starting with "| Doc | Purpose |" so the table is separated from
the heading (satisfies markdownlint MD058); update the README.md by inserting an
empty line immediately after the "## Resources" heading.
In `@tests/web.httpHelpers.test.js`:
- Around line 58-67: The test currently compares identical literal arrays
(self-referential) so it never fails; update the test to import the module under
test (the httpHelpers/web module) and assert that its exported registry method
names match the expected array
['RegisterWebRTCPeer','UnregisterWebRTCPeer','ListWebRTCPeers'] — e.g., require
the module, derive the actual names (from exported object keys or the specific
export like registryMethods/allowedMethods), then replace the right-hand literal
with that actual value in the assert.deepStrictEqual call so the assertion
verifies runtime exports instead of two identical literals.
In `@types/server.js`:
- Around line 1257-1262: The GENERIC_MESSAGE branch builds a signed receipt in
the local variable (via Message.fromVector and later signing) but never sends
it; either send that receipt on the current socket or remove the dead
construction. Update the case handling for messageTransport.GENERIC_MESSAGE_TYPE
so that after building/signing local (the GENERIC_MESSAGE_RECEIPT_TYPE message)
you call socket.send(local.toBuffer()) to transmit the receipt (mirroring how
P2P_MESSAGE_RECEIPT is delivered), or if the receipt is no longer required,
delete the local assignment and related signing code to avoid dead code.
- Around line 1147-1149: The code currently calls
jsonRpcTransport.parseWebSocketJsonCallBody(message.body) and
jsonRpcTransport.computeWebSocketJsonCallHashPair(message.body) before checking
socket._fabricJsonRpcTransportAuthorized; move the unauthenticated auth gate to
run immediately after extracting message.body and before invoking
parseWebSocketJsonCallBody or computeWebSocketJsonCallHashPair so
unauthenticated sockets are rejected quickly and never trigger JSON.parse or
SHA256 work; update the control flow around parseWebSocketJsonCallBody and
computeWebSocketJsonCallHashPair to only run for authorized sockets and ensure
unauthenticated branches use the existing error envelope handling (the same
rejection path used for other malformed/unauthorized messages).
---
Outside diff comments:
In `@types/server.js`:
- Line 2548: Update project documentation to record the breaking API change: add
a clear CHANGELOG entry and a README (or migration guide/deprecation) note that
the named exports resolveFabricHttpPackageAssetsDir and
acceptFirstHtmlNavigation were removed from the server module (previously
exported from types/server.js). In the CHANGELOG entry, include the version,
"breaking change" label, and a short migration snippet advising users to remove
imports or replace them with the new recommended approach (or point to
alternative APIs). In the README/migration guide, add a brief example showing
how to refactor code that imported resolveFabricHttpPackageAssetsDir and
acceptFirstHtmlNavigation and link to any upstream replacement or guidance for
maintaining equivalent behavior.
---
Nitpick comments:
In `@types/server.js`:
- Around line 1162-1167: The code recomputes transport authorization into a new
local transportAuthorized variable using
server._isJsonRpcTransportAuthorized(request) which shadows the outer
transportAuthorized set earlier and risks divergence; change the logic in the
block that checks server._isWebRtcRegistryMethod(jsonCallPayload.method) &&
wrtcCfg.requireTransportAuth === true to reuse socket._fabricTransportAuthorized
instead of calling server._isJsonRpcTransportAuthorized again (or, if you
intentionally want per-message re-evaluation for token rotation, add an explicit
comment explaining that server._isJsonRpcTransportAuthorized(request) must be
re-run). Update the occurrences at the shown snippet and the similar spots noted
(around the uses at lines corresponding to 1272 and 1300) to be consistent.
- Around line 1095-1108: Replace the hardcoded literal 'HEARTBEAT' with the
exported constant messageTransport.HEARTBEAT_TYPE in the branch that builds a
HEARTBEAT Message (the conditional comparing rawType at the parsed object and
the Message.fromVector call); keep the rest of the logic the same (use
parsed.type || parsed['@type'] to compute rawType and use parsed.body ??
parsed.data ?? parsed.content ?? parsed['@data'] ?? '' for payload), ensuring
you reference messageTransport.HEARTBEAT_TYPE instead of the string so the check
and the Message.fromVector payload use the constant.
🪄 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: a8c12cad-d5e7-4784-9cb4-9b7551356ad9
📥 Commits
Reviewing files that changed from the base of the PR and between 4779a31 and 1b1ee22.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review complete. I found no high-confidence medium, high, or critical vulnerabilities introduced by this PR.
Validated areas:
Prior Cursor automation assessment had no findings; no prior Cursor security-review threads needed re-reporting.
WebSocket JSONCall and HTTP JSON-RPC auth paths still gate WebRTC registry methods via _fabricTransportAuthorized when webrtc.requireTransportAuth is enabled.
New WebRTC registry inputs are bounded/validated, unregister uses the returned secret, and package changes add helper exports without new dependencies.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review complete. I found no high-confidence medium, high, or critical vulnerabilities introduced by this PR.
Validated areas:
Rechecked previous automation context; prior Cursor security reviews had no inline findings to re-report, and cleanup was requested for stale automation output.
Validated the unresolved auth-before-parse DoS concern against current and base flow; the unauthenticated JSONCall parse/hash work was already present in the reviewed path and does not rise to a new PR-introduced medium+ finding.
Reviewed WebSocket JSONCall alias normalization, HTTP JSON-RPC envelope refactor, WebRTC registry auth/limits/secret handling, public protocol exports, and package-lock changes.
No inline security findings were left. Note: targeted tests could not be run because npm is not available in this environment.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review complete. I found no high-confidence medium, high, or critical vulnerabilities introduced by this PR.
Validated areas:
Rechecked prior automation context and the currently unresolved auth-before-parse DoS concern; the parse/hash work is not newly introduced by this PR and does not meet the medium+ reporting bar here.
Traced the changed WebSocket JSONCall flow, HTTP JSON-RPC envelopes, WebRTC registry auth/limits/secret handling, protocol helper exports, and package/dependency changes.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review complete. I found no high-confidence medium, high, or critical vulnerabilities introduced by this PR.
Validated areas:
Rechecked prior automation context and the currently unresolved auth-before-parse DoS concern; current code parses JSONCall bodies after the auth gate, and the remaining pre-auth hash is bounded correlation work for the denial response rather than a reportable medium+ issue.
Traced the changed WebSocket JSONCall and HTTP JSON-RPC paths through _handleCall, including WebRTC registry auth, peer metadata bounds, and secret-based update/unregister handling.
Reviewed the new protocol helper exports and dependency changes; npm audit --omit=dev reported 0 vulnerabilities, and standalone helper smoke checks passed.
No inline security findings were left. Targeted Mocha tests could not run because this checkout is missing @fabric/core/constants.
No inline security finding comments were left. Targeted npx mocha tests/web.httpHelpers.test.js --exit could not run in this checkout because @fabric/core/constants is missing.
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
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Streamline the HTTP library for the release of Fabric
0.1.0.Summary by CodeRabbit
New Features
Documentation
Tests
Chores