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
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
Adds Lightning/BOLT12 and Fabric bech32 helpers and Lightning RPC wrapper methods; introduces multiple native C crypto/security translation units and headers; fixes Bitcoin transaction/Chain behavior (tx hashing, proposal dedupe); normalizes Fabric document-offer envelopes and updates peer inventory handling; expands tests and unignores native sources in .gitignore.
Changes
Lightning & Fabric helpers + Lightning service wiring
Compilation Units / Aggregators src/crypto.c, src/protocol.c, src/p2p.c
Add translation units that #include multiple implementation .c files into single compilation units.
Build Tracking .gitignore
Negate src/* ignores to unignore specific native sources/headers (src/crypto.c, src/security.c, src/sha2.c, src/sha2.h, src/p2p.c, src/protocol.c, src/memory.h, src/random.h).
Types, peers, environment, and miscellaneous behavior changes
Layer / File(s)
Summary
Data Shape / Accessors types/bitcoin/transaction.js, types/chain.js, types/environment.js
Canonicalize transaction raw bytes (_state.content.raw), compute hash/txid/id from raw bytes, Chain height from blocks array, refine Environment seed selection and wallet load behavior (treat empty/corrupt wallet as warning).
Update signAsHolder to sign double-SHA256, deduplicate proposeTransaction by actor id, normalize document-offer envelopes before dispatch, remove try/catch in Codec encode/decode, Actor-based _REGISTER persistence, minor constructor/throw/log changes.
Add jayson uuid override, minor example comments, TODO/report updates, and seed/seed-shape adjustments in scripts (chat/keygen).
Sequence Diagram(s)
sequenceDiagram
participant Client
participant Lightning
participant RPCBackend
Client->>Lightning: callRpc(method, params, timeout)
Lightning->>Lightning: validate & normalize args
Lightning->>RPCBackend: _makeRPCRequest(method, params..., timeout)
RPCBackend-->>Lightning: rpc response or error
Lightning-->>Client: return value or throw
Loading
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
FabricLabs/fabric#176: Overlapping .gitignore edits to unignore native C sources/headers.
FabricLabs/fabric#177: Overlapping additions of native C translation units and crypto/security sources.
Suggested reviewers
naterchrdsn
ChronicSmoke
Poem
🐇 I nibble bech32, hum a SHA tune bright,
I hide seed crumbs safe through the long compile night.
TLVs in order, peers hop in a row,
Secure-zero the secrets so they softly go.
Hop—tests pass, build hums, and the network takes flight.
🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check
❓ Inconclusive
The PR title 'RC1 Push' is vague and generic, using a non-descriptive term that does not convey meaningful information about the actual changeset.
Consider a more specific title that highlights the primary changes, such as 'Add Lightning/BOLT12 and Fabric payment encoding support with crypto primitives' or 'Implement BOLT12, Fabric payments, and secure crypto library'
✅ Passed checks (3 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
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 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.
❌ Patch coverage is 94.15385% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.51%. Comparing base (8d4426b) to head (612180e).
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 14
🧹 Nitpick comments (4)
tests/fabric.chain.js (1)
90-104: LGTM — dedup coverage is appropriate, consider one extra assertion.
The test correctly pins down the new de-dup behavior: same payload → same tx.id, single mempool entry. Optionally also assert tx1 === tx2 (same instance) to lock in that proposeTransaction returns the existing Transaction object rather than a freshly wrapped one — this is the cheaper path your chain.js changes now take.
♻️ Optional extra assertion
assert.strictEqual(tx1.id, tx2.id);
+ assert.strictEqual(tx1, tx2, 'duplicate proposal should return the same Transaction instance');
assert.strictEqual(chain.mempool.filter((id) => id === tx1.id).length, 1);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/fabric.chain.js` around lines 90 - 104, Add an extra assertion to the
test to ensure proposeTransaction returns the same Transaction instance for
duplicate payloads: after creating tx1 and tx2 via
Chain.prototype.proposeTransaction (and existing assertions), assert that tx1
=== tx2 so the test verifies both identical ids and identical object identity
(same instance) rather than two separate wrappers.
types/chain.js (1)
138-141: Dead new Transaction(prior) branch.
The constructor (lines 56–59) already normalizes every entry in _state.transactions to a Transaction instance, and proposeTransaction below only ever stores actor (also a Transaction). The prior instanceof Transaction ? prior : new Transaction(prior) ternary's false branch is therefore unreachable in practice — you can simplify to return prior;.
♻️ Proposed simplification
- const prior = this._state.transactions[actor.id];- if (prior) {- return prior instanceof Transaction ? prior : new Transaction(prior);- }+ const prior = this._state.transactions[actor.id];+ if (prior) return prior;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/chain.js` around lines 138 - 141, The branch creating new Transaction
instances is dead because the constructor and proposeTransaction already ensure
every entry in _state.transactions is a Transaction; replace the ternary return
(prior instanceof Transaction ? prior : new Transaction(prior)) with a simple
return prior in the accessor so it always returns the stored Transaction
instance (update the code around references to _state.transactions and the
Transaction type accordingly).
types/bitcoin/transaction.js (1)
14-19: Remove unreachable catch block. Node's Buffer.from(string, 'hex') does not throw on malformed hex—it silently truncates at the first invalid nibble and returns the partial result. The try/catch here is dead code.
For stricter validation, replace the catch block with an additional guard: !/^[0-9a-fA-F]+$/.test(s) to reject strings with non-hex characters before decoding.
Verify each finding against the current code and only fix it if needed.
In `@types/bitcoin/transaction.js` around lines 14 - 19, The try/catch around
Buffer.from(s, 'hex') is unreachable; remove the catch and instead validate the
input string first—if s is falsy or fails the hex regex /^[0-9a-fA-F]+$/ then
return Buffer.alloc(0), otherwise call Buffer.from(s, 'hex') to decode; update
the code that currently uses Buffer.from(s, 'hex') and Buffer.alloc(0) so the
non-hex guard is applied before decoding.
src/sha2.h (1)
1-17: Consider adding extern "C" guards to the header for portability.
While binding.cc wraps its C header includes in an extern "C" block (lines 4–12), including sha2.h, the header itself lacks these guards. This is a best-practice portability measure: if another C++ file includes sha2.h without wrapping it, function declarations would be name-mangled, causing link failures. Adding guards to the header makes it reusable without requiring consumers to manage linkage externally.
Verify each finding against the current code and only fix it if needed.
In `@src/sha2.h` around lines 1 - 17, The header lacks C++ linkage guards which
can cause name-mangling when included from C++ directly; wrap the declarations
in an extern "C" block guarded by `#ifdef` __cplusplus to preserve C linkage.
Modify src/sha2.h to add the standard pattern (e.g., `#ifdef __cplusplus extern
"C" {` before the declarations and `} `#endif`` after) so all functions like
fabric_sha256, fabric_sha512, fabric_hmac_sha512, and fabric_pbkdf2_hmac_sha512
retain C linkage for C++ consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@functions/bolt12Semantics.js`:
- Around line 23-57: Rename the exported constant BOLT12_TLV to CLN_BOLT12_TLV
to explicitly mark Core Lightning extensions (keep all existing recurrence
entries so tests and summarizeBolt12RecurrenceFromDecode() continue to work),
and augment the constant with the missing BOLT `#12` spec TLV ids: INVREQ_FEATURES
(84), INVREQ_PAYER_NOTE (89), INVREQ_PATHS (90), INVREQ_BIP353_NAME (91),
INVOICE_RELATIVE_EXPIRY (166), INVOICE_FALLBACKS (172), and INVOICE_FEATURES
(174); then update any imports/usages in tests and code (e.g., change
bs.BOLT12_TLV to bs.CLN_BOLT12_TLV) to match the new name.
In `@functions/fabricPaymentBech32.js`:
- Around line 53-56: The array-to-buffer conversion for o.hash32 can silently
accept invalid numbers; update the Array.isArray(o.hash32) branch to validate
each element before calling Buffer.from: ensure every element is a finite
integer and between 0 and 255 (reject negatives, >255, NaN, and non-integers)
and throw a TypeError if any element fails; keep existing handling for Buffer
and Uint8Array as-is but consider throwing if their lengths are unexpected
elsewhere in the function (use the unique symbol o.hash32 to locate the checks).
In `@services/lightning.js`:
- Around line 999-1001: The static exports Lightning.Bolt12 and
Lightning.FabricPayment reference undefined symbols (lightningBolt12,
fabricPaymentBech32); import or require the bridge modules that define those
symbols before assigning the exports. Locate the assignments to Lightning.Bolt12
and Lightning.FabricPayment and add explicit imports/require calls that produce
lightningBolt12 and fabricPaymentBech32 (or ensure those variables are declared
earlier), then assign the imported values to Lightning.Bolt12 and
Lightning.FabricPayment so requiring the module does not throw.
- Around line 290-351: The RPC methods (createOffer, fetchInvoice, pay,
listOffers, disableOffer, createInvoiceRequest, listInvoiceRequests,
disableInvoiceRequest, sendInvoice) are wrapping parameter objects in arrays
which sends them as positional params; change calls to _makeRPCRequest to pass
named-parameter objects directly (e.g. _makeRPCRequest('offer', params) or
_makeRPCRequest('listoffers', filter) instead of [params]) and for variants like
fetchInvoice that accept either an offer string or an object, pass the single
argument directly (or merge invoiceParams into an object when both are provided)
so Core Lightning receives named params; keep using [] or null only for RPCs
that truly take no params and ensure method signatures still validate shapes
before calling _makeRPCRequest.
In `@src/security.c`:
- Around line 36-37: The allocation tracking logic can fail-open: when creating
an allocation_record_t (see allocations and memory_initialized) inside
fabric_secure_malloc (and the other record-creation sites), the code currently
returns the secure buffer even if malloc(sizeof(allocation_record_t)) fails, so
later fabric_secure_free cannot find the record to zero memory; change this to
fail-closed by checking the result of malloc for the allocation_record_t and on
failure free the allocated secure buffer and return NULL (do not expose an
untracked allocation). Apply the same pattern to every record-creation site (the
blocks that manipulate allocation_record_t around fabric_secure_malloc, the
other two similar blocks referenced) so that on any tracking-memory allocation
failure you clean up any previously allocated resources and return NULL.
- Around line 568-585: The loop computing the longest repeated-byte run can miss
a trailing run because max_consecutive is only updated when a change is seen;
after the for loop ends you must compare consecutive_count with max_consecutive
and update max_consecutive if needed so the final run (at buffer end) is
considered before the threshold check; update symbols: test_buffer,
consecutive_count, max_consecutive, test_size.
- Around line 382-395: The getrandom() call in the __linux__ block (using buffer
and length) should treat EINTR by retrying instead of immediately returning
FABRIC_ERROR_SYSTEM_CALL_FAILED; modify the logic around the getrandom result so
that if result < 0 and errno == EINTR you loop and call getrandom again, if
errno == ENOSYS fall through to the /dev/urandom fallback, and for any other
errno return FABRIC_ERROR_SYSTEM_CALL_FAILED; ensure the loop stops on success
(result == (ssize_t)length) or any non-retryable error.
- Around line 186-210: In fabric_secure_realloc, recover the real tracked old
size before copying when old_size may be 0: call the tracked-size lookup variant
of fabric_secure_free (e.g. fabric_secure_free(ptr, 0) or an equivalent helper)
to obtain actual_old_size, use that actual_old_size to compute copy_size and
memcpy, then call fabric_secure_free(ptr, actual_old_size) to securely free the
original buffer; keep the rest of the logic (allocating new_ptr via
fabric_secure_malloc and handling failures) unchanged.
In `@src/sha2.c`:
- Around line 288-320: Sensitive stack buffers (key_block, key_hash, o_key_pad,
i_key_pad, inner and similar per-function temporaries like u, t, sbuf) are left
on the stack on both success and early-return failure paths; wipe them
explicitly before every return. Update the HMAC/PBKDF2 routines (the block using
key_block/key_hash/... and the other block referenced at 328-358) to call a
secure zeroing routine (e.g., secure_memzero/OPENSSL_cleanse or an internal
constant-time memset) on those local buffers immediately after they are no
longer needed and also just before any early return, ensuring no sensitive
material remains on the stack. Ensure the zeroing covers the full sizeof(...) of
each buffer and is not optimized away by the compiler.
- Around line 264-276: Update the input validation in fabric_sha256 and
fabric_sha512 (and any other functions that currently do if (!ptr || !out)
return 0) to allow NULL data when len == 0 by changing the check to fail only
when (ptr == NULL && len != 0) or when output pointer is NULL; ensure
PBKDF2/HMAC entry points use the same logic. After any use of stack buffers that
hold sensitive material (key_block, o_key_pad, i_key_pad, inner in HMAC; u, t,
sbuf in PBKDF2), securely wipe them with a cryptographic secure zeroing function
(e.g., explicit_bzero/OPENSSL_cleanse/secure_memzero wrapper used by the
project) rather than plain memset, and call it in all return paths (including
error paths) so sensitive data is always cleared; reference functions:
fabric_sha256, fabric_sha512, HMAC-related functions that allocate
key_block/o_key_pad/i_key_pad/inner, and PBKDF2 implementation that uses
u/t/sbuf.
In `@tests/lightning/lightning.service.unit.js`:
- Around line 248-258: The tests currently expect and assert that methods like
createOffer, fetchInvoice, pay, listOffers, createInvoiceRequest,
listInvoiceRequests, and sendInvoice wrap their parameters in a positional array
when calling _makeRPCRequest; change each test to assert that _makeRPCRequest
receives the params as a dictionary/object (e.g., assert.deepStrictEqual(params,
p) rather than [p]) and update the stubbed _makeRPCRequest implementations used
in these specs so they check the named-parameter style (and return the same
mocked response), ensuring each test calls the corresponding method
(createOffer, fetchInvoice, pay, listOffers, createInvoiceRequest,
listInvoiceRequests, sendInvoice) and verifies the returned value as before.
In `@types/bitcoin/transaction.js`:
- Around line 47-71: The stored state field _state.content.raw is set once from
settings.raw in the constructor but the getters hash, txid and id call _rawBuf()
which reads from this.settings.raw, allowing divergence if settings.raw is
mutated; change the implementation so the canonical raw buffer is owned by state
(or normalized once in the constructor) and have _rawBuf() read from
this._state.content.raw (or store a frozen Buffer on construction and use that
for both _state.content.raw and _rawBuf()), and optionally memoize the computed
doubleSha256Hex value used by hash to avoid repeated work.
- Around line 73-77: The signAsHolder function is using doubleSha256Hex and
passing a hex string into this.holder.sign (which converts it as UTF-8),
producing a signature over ASCII bytes instead of the 32-byte hash; replace that
by computing the binary double-SHA256 of this._rawBuf() (a 32-byte Buffer) and
call this.holder.signSchnorrHash(digestBuf) to produce a correct Schnorr
signature (update references in signAsHolder from doubleSha256Hex /
this.holder.sign to the binary hash computation and
this.holder.signSchnorrHash).
---
Nitpick comments:
In `@src/sha2.h`:
- Around line 1-17: The header lacks C++ linkage guards which can cause
name-mangling when included from C++ directly; wrap the declarations in an
extern "C" block guarded by `#ifdef` __cplusplus to preserve C linkage. Modify
src/sha2.h to add the standard pattern (e.g., `#ifdef __cplusplus extern "C" {`
before the declarations and `} `#endif`` after) so all functions like
fabric_sha256, fabric_sha512, fabric_hmac_sha512, and fabric_pbkdf2_hmac_sha512
retain C linkage for C++ consumers.
In `@tests/fabric.chain.js`:
- Around line 90-104: Add an extra assertion to the test to ensure
proposeTransaction returns the same Transaction instance for duplicate payloads:
after creating tx1 and tx2 via Chain.prototype.proposeTransaction (and existing
assertions), assert that tx1 === tx2 so the test verifies both identical ids and
identical object identity (same instance) rather than two separate wrappers.
In `@types/bitcoin/transaction.js`:
- Around line 14-19: The try/catch around Buffer.from(s, 'hex') is unreachable;
remove the catch and instead validate the input string first—if s is falsy or
fails the hex regex /^[0-9a-fA-F]+$/ then return Buffer.alloc(0), otherwise call
Buffer.from(s, 'hex') to decode; update the code that currently uses
Buffer.from(s, 'hex') and Buffer.alloc(0) so the non-hex guard is applied before
decoding.
In `@types/chain.js`:
- Around line 138-141: The branch creating new Transaction instances is dead
because the constructor and proposeTransaction already ensure every entry in
_state.transactions is a Transaction; replace the ternary return (prior
instanceof Transaction ? prior : new Transaction(prior)) with a simple return
prior in the accessor so it always returns the stored Transaction instance
(update the code around references to _state.transactions and the Transaction
type accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84cf117a-9054-418f-8c7a-6f6135057f85
📥 Commits
Reviewing files that changed from the base of the PR and between 8d4426b and ca26005.
⛔ Files ignored due to path filters (2)
package-lock.json is excluded by !**/package-lock.json
#!/bin/bash# Find and read the Lightning service implementation to understand how params are forwarded
fd -n "lightning.js" --path "*/services/*" --type f
Repository: FabricLabs/fabric
Length of output: 291
🏁 Script executed:
#!/bin/bash# Read the specific test file at the mentioned line ranges to see current assertions
sed -n '248,260p; 265,280p; 287,302p; 302,318p; 318,330p; 330,346p; 356,368p' tests/lightning/lightning.service.unit.js
Repository: FabricLabs/fabric
Length of output: 4526
🏁 Script executed:
#!/bin/bash# Inspect the Lightning service implementation for createOffer, fetchInvoice, pay methods
rg -A15 "^\s*(createOffer|fetchInvoice|pay|listOffers|createInvoiceRequest|listInvoiceRequests|sendInvoice)\s*\(" services/lightning.js
Repository: FabricLabs/fabric
Length of output: 43
🏁 Script executed:
#!/bin/bash# Find the Lightning service file
find . -name "*.js" -path "*/services/*"| grep -i lightning | head -5
Repository: FabricLabs/fabric
Length of output: 84
🏁 Script executed:
#!/bin/bash# List services directory to understand structure
ls -la services/ 2>/dev/null | head -20
Repository: FabricLabs/fabric
Length of output: 736
🏁 Script executed:
#!/bin/bash# Find all lightning-related JS files
fd "lightning" --type f -e js | grep -E "(service|index)"| head -10
Repository: FabricLabs/fabric
Length of output: 161
🏁 Script executed:
#!/bin/bash# Read the Lightning service file to understand how parameters are forwarded
wc -l services/lightning.js
Repository: FabricLabs/fabric
Length of output: 87
🏁 Script executed:
#!/bin/bash# Extract the createOffer method implementation
sed -n '/^\s*createOffer\s*(/,/^\s*}/p' services/lightning.js | head -30
Repository: FabricLabs/fabric
Length of output: 43
🏁 Script executed:
#!/bin/bash# Search for how createOffer, fetchInvoice, pay methods call _makeRPCRequest
rg -B2 -A8 "createOffer|fetchInvoice|pay\s*\(" services/lightning.js | head -100
Repository: FabricLabs/fabric
Length of output: 1464
🏁 Script executed:
#!/bin/bash# Continue reading the rest of createInvoiceRequest, listInvoiceRequests, sendInvoice methods
rg -B2 -A8 "createInvoiceRequest|listInvoiceRequests|sendInvoice" services/lightning.js | head -80
Repository: FabricLabs/fabric
Length of output: 1449
🏁 Script executed:
#!/bin/bash# Check if there's any documentation about Lightning RPC params in the repo
find . -name "*.md" -type f | xargs grep -l -i "offer\|fetchinvoice\|lightning.*rpc"| head -5
Repository: FabricLabs/fabric
Length of output: 128
🏁 Script executed:
#!/bin/bash# Look for any Core Lightning documentation or examples in the codebase
find . -name "*.md" -o -name "*.txt"| xargs grep -l "amount_msat\|lightningd"2>/dev/null | head -5
Repository: FabricLabs/fabric
Length of output: 99
🏁 Script executed:
#!/bin/bash# Check if there's a Lightning compatibility documentation
cat docs/LIGHTNING_COMPAT.md 2>/dev/null | head -100
Repository: FabricLabs/fabric
Length of output: 43
🏁 Script executed:
#!/bin/bash# Look for any comments or documentation about RPC params style in the Lightning service
rg -B5 -A5 "_makeRPCRequest\s*\(" services/lightning.js | grep -E "^|--$|//|/\*"| head -50
Repository: FabricLabs/fabric
Length of output: 2116
🏁 Script executed:
#!/bin/bash# Check if there's any reference to Core Lightning JSON-RPC conventions in comments
rg "JSON-RPC|named.*param|dict.*param|position.*param" services/lightning.js
Repository: FabricLabs/fabric
Length of output: 213
🏁 Script executed:
#!/bin/bash# Look at the _makeRPCRequest implementation to see how it handles params
rg -B5 -A15 "^\s*async _makeRPCRequest\s*\(" services/lightning.js
Repository: FabricLabs/fabric
Length of output: 1056
🏁 Script executed:
#!/bin/bash# Continue reading _makeRPCRequest to see how it constructs the JSON-RPC request
rg -A50 "async _makeRPCRequest" services/lightning.js | head -80
Repository: FabricLabs/fabric
Length of output: 1945
🏁 Script executed:
#!/bin/bash# Look for how the request is actually sent - look for JSON.stringify or similar
rg -B5 -A10 "JSON-RPC|jsonrpc|stringify.*method" services/lightning.js | head -100
Repository: FabricLabs/fabric
Length of output: 867
🏁 Script executed:
#!/bin/bash# Verify the current behavior - the implementation does wrap params in arrays# Let's check one more time the exact issue by looking at a specific call pattern
sed -n '400,420p' services/lightning.js
Repository: FabricLabs/fabric
Length of output: 940
🏁 Script executed:
#!/bin/bash# Search for any reference to how Core Lightning expects params in documentation
cat docs/LIGHTNING_COMPAT.md 2>/dev/null | head -50
Repository: FabricLabs/fabric
Length of output: 43
🏁 Script executed:
#!/bin/bash# Check if LIGHTNING_COMPAT.md exists anywhere
find . -name "*COMPAT*" -o -name "*LIGHT*" -type f | head -10
Repository: FabricLabs/fabric
Length of output: 43
🏁 Script executed:
#!/bin/bash# Search for any comments in the code referencing Core Lightning RPC conventions
rg -i "core lightning|cln|offer.*amount|amount_msat". --type md | head -30
Repository: FabricLabs/fabric
Length of output: 1093
🏁 Script executed:
#!/bin/bash# Look at the actual test implementation more carefully# Check if there are integration tests that show how _makeRPCRequest is actually used
sed -n '1,50p' tests/lightning/lightning.service.unit.js
Repository: FabricLabs/fabric
Length of output: 3013
🏁 Script executed:
#!/bin/bash# Look for LIGHTNING_COMPAT.md file
find . -name "LIGHTNING_COMPAT.md"
Repository: FabricLabs/fabric
Length of output: 43
🏁 Script executed:
#!/bin/bash# Check what the test file references about Lightning RPC surface
sed -n '1,30p' tests/lightning/lightning.cln.rpc.surface.test.js
Repository: FabricLabs/fabric
Length of output: 1270
🏁 Script executed:
#!/bin/bash# Look for any existing issues or comments about param formatting
rg -i "array.*param|named.*param|positional" services/lightning.js tests/lightning/
Repository: FabricLabs/fabric
Length of output: 713
🏁 Script executed:
#!/bin/bash# Let me search for any test output or CI logs that might show actual Core Lightning RPC behavior
find . -name "*.log" -o -name "*.txt" -type f | xargs grep -l "offer\|fetchinvoice"2>/dev/null | head -5
Repository: FabricLabs/fabric
Length of output: 126
🏁 Script executed:
#!/bin/bash# Check if there are any real integration tests that show how Core Lightning responds
sed -n '1,100p' tests/lightning/lightning.service.js | grep -A10 -B10 "createOffer\|fetchInvoice\|offer"
Repository: FabricLabs/fabric
Length of output: 43
🏁 Script executed:
#!/bin/bash# Look for any actual RPC calls made during tests
rg -i "amount.*msat|offer.*lno" tests/lightning/ | head -20
Repository: FabricLabs/fabric
Length of output: 1912
🏁 Script executed:
#!/bin/bash# Check if the tests actually run and if there are any clues about what Core Lightning expects
sed -n '240,280p' tests/lightning/lightning.service.unit.js
Repository: FabricLabs/fabric
Length of output: 1733
🏁 Script executed:
#!/bin/bash# Also check if there's any discussion of this issue in DEVELOPERS.md or other docs
cat DEVELOPERS.md 2>/dev/null | grep -A10 -B10 -i "lightning\|rpc\|offer"
Repository: FabricLabs/fabric
Length of output: 9049
🏁 Script executed:
#!/bin/bash# Check the actual parameter names for offer command - see if amount_msat is even correct
rg "amount_msat|amount.*msat" services/lightning.js | head -20
Repository: FabricLabs/fabric
Length of output: 443
Pass dictionary-style params to RPC methods instead of wrapping in positional arrays.
The createOffer, fetchInvoice, pay, listOffers, createInvoiceRequest, listInvoiceRequests, and sendInvoice methods all wrap their parameters in arrays before forwarding to _makeRPCRequest. Core Lightning's JSON-RPC interface accepts both positional (array) and named (object/dictionary) parameter styles, but wrapping objects in arrays sends them as positional arguments. This breaks commands expecting named parameters—for example, offer should receive { amount_msat: '...', description: '...' } directly, not [{ amount_msat: '...', description: '...' }].
The tests reinforce this incorrect pattern by asserting deepStrictEqual(params, [p]) instead of deepStrictEqual(params, p). These assertions must be corrected to expect the proper format.
Also applies to: 265–279, 287–300, 302–316, 318–328, 330–344, 356–366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/lightning/lightning.service.unit.js` around lines 248 - 258, The tests
currently expect and assert that methods like createOffer, fetchInvoice, pay,
listOffers, createInvoiceRequest, listInvoiceRequests, and sendInvoice wrap
their parameters in a positional array when calling _makeRPCRequest; change each
test to assert that _makeRPCRequest receives the params as a dictionary/object
(e.g., assert.deepStrictEqual(params, p) rather than [p]) and update the stubbed
_makeRPCRequest implementations used in these specs so they check the
named-parameter style (and return the same mocked response), ensuring each test
calls the corresponding method (createOffer, fetchInvoice, pay, listOffers,
createInvoiceRequest, listInvoiceRequests, sendInvoice) and verifies the
returned value as before.
The reason will be displayed to describe this comment to others. Learn more.
♻️ Duplicate comments (1)
functions/fabricPaymentBech32.js (1)
42-45: ⚠️ Potential issue | 🟠 Major
Validate number[] bytes before Buffer.from.
Buffer.from(o.hash32) still silently coerces invalid array elements (negatives, >255, non-integers, NaN), producing a corrupt 32-byte hash rather than rejecting bad input. Same concern as prior review — add an element range/integer check in the Array.isArray branch before conversion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/fabricPaymentBech32.js` around lines 42 - 45, The
Array.isArray(o.hash32) branch must validate elements before calling
Buffer.from: ensure o.hash32.length === 32 and every element is a finite integer
between 0 and 255 (Number.isInteger(elem) && elem >= 0 && elem <= 255); if
validation fails throw a TypeError('hash32 must be Buffer, Uint8Array, or byte
array of 32 bytes with values 0-255'); only then call Buffer.from(o.hash32) to
create buf. This targets the Array.isArray branch handling o.hash32 and replaces
the silent coercion by Buffer.from with explicit checks and a clear error.
🧹 Nitpick comments (2)
functions/fabricPaymentBech32.js (2)
100-100: Pass the normalized string to the delegate.
classifyPaymentEncodingString normalizes input into t but then forwards the raw, un-trimmed s to lb.classifyLightningEncodedString. Not incorrect (the delegate re-normalizes), but forwarding t would be consistent and avoids a second trim.
Verify each finding against the current code and only fix it if needed.
In `@functions/fabricPaymentBech32.js` at line 100, classifyPaymentEncodingString
normalizes the input into variable t but currently calls
lb.classifyLightningEncodedString(s); change the call to pass the normalized t
(i.e. lb.classifyLightningEncodedString(t)) so the delegate receives the
already-trimmed/normalized string and avoids redundant re-normalization; update
references in classifyPaymentEncodingString to use t where the normalized value
is needed.
76-81: Simplify redundant length checks.
Line 76 guards raw.length < 34 and line 81 rechecks raw.length !== 34; the first check is subsumed by the second, and interleaving the slice between them is confusing. Consider collapsing to a single if (raw.length !== 34) return null; up front before reading version/routeType/hash32.
♻️ Proposed simplification
- if (raw.length < 2 + 32) return null;+ if (raw.length !== 2 + 32) return null;
const version = raw[0];
const routeType = raw[1];
const hash32 = Buffer.from(raw.slice(2, 34));
if (version !== FABRIC_PAYMENT_VERSION) return null;
- if (raw.length !== 34) return null;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/fabricPaymentBech32.js` around lines 76 - 81, Collapse the
redundant length guards by checking raw.length === 34 once before slicing:
replace the two checks with a single if (raw.length !== 34) return null; placed
before reading version, routeType and hash32, then proceed to set version =
raw[0], routeType = raw[1], and hash32 = Buffer.from(raw.slice(2,34)) and keep
the version comparison against FABRIC_PAYMENT_VERSION as-is; remove the earlier
raw.length < 34 check and the later raw.length !== 34 check to avoid confusion
and redundant work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@functions/fabricPaymentBech32.js`:
- Around line 42-45: The Array.isArray(o.hash32) branch must validate elements
before calling Buffer.from: ensure o.hash32.length === 32 and every element is a
finite integer between 0 and 255 (Number.isInteger(elem) && elem >= 0 && elem <=
255); if validation fails throw a TypeError('hash32 must be Buffer, Uint8Array,
or byte array of 32 bytes with values 0-255'); only then call
Buffer.from(o.hash32) to create buf. This targets the Array.isArray branch
handling o.hash32 and replaces the silent coercion by Buffer.from with explicit
checks and a clear error.
---
Nitpick comments:
In `@functions/fabricPaymentBech32.js`:
- Line 100: classifyPaymentEncodingString normalizes the input into variable t
but currently calls lb.classifyLightningEncodedString(s); change the call to
pass the normalized t (i.e. lb.classifyLightningEncodedString(t)) so the
delegate receives the already-trimmed/normalized string and avoids redundant
re-normalization; update references in classifyPaymentEncodingString to use t
where the normalized value is needed.
- Around line 76-81: Collapse the redundant length guards by checking raw.length
=== 34 once before slicing: replace the two checks with a single if (raw.length
!== 34) return null; placed before reading version, routeType and hash32, then
proceed to set version = raw[0], routeType = raw[1], and hash32 =
Buffer.from(raw.slice(2,34)) and keep the version comparison against
FABRIC_PAYMENT_VERSION as-is; remove the earlier raw.length < 34 check and the
later raw.length !== 34 check to avoid confusion and redundant work.
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18986645-f1ad-4713-82ec-6d91b5ddb143
📥 Commits
Reviewing files that changed from the base of the PR and between ca26005 and 7b564b1.
📒 Files selected for processing (3)
functions/fabricPaymentBech32.js
services/lightning.js
tests/lightning/lightning.service.unit.js
✅ Files skipped from review due to trivial changes (2)
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/security.c (1)
205-218: ⚠️ Potential issue | 🟠 Major
fabric_secure_realloc still silently drops data when the pointer is not tracked.
The previous review suggested returning NULL when fabric_secure_malloc_size(ptr) returns 0. The current fix records the result but does not guard on it — if ptr was never tracked (or tracking was lost), old_size stays 0, a new buffer is allocated, memcpy copies 0 bytes, and fabric_secure_free(ptr, 0) is then called. The caller receives a zero-initialized buffer and the old contents are irretrievably gone with no error signaled.
🛡️ Proposed fix
if (old_size == 0)
{
old_size = fabric_secure_malloc_size(ptr);
+ if (old_size == 0)+ {+ /* Pointer is not tracked; refuse to silently lose data. */+ return NULL;+ }
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/security.c` around lines 205 - 218, fabric_secure_realloc must not
silently drop the caller's data when fabric_secure_malloc_size(ptr) returns 0;
in fabric_secure_realloc, after calling fabric_secure_malloc_size(ptr) into
old_size, if old_size == 0 treat this as an error/unknown pointer and
immediately return NULL without allocating a new buffer, without calling memcpy
or fabric_secure_free(ptr, 0), and preserve the original pointer; update the
code paths around fabric_secure_malloc(new_size), memcpy(new_ptr, ptr,
copy_size) and fabric_secure_free(ptr, old_size) accordingly so that the
function returns NULL on untracked pointers instead of returning a
zero-length/new buffer.
The decoded fixture sets invoice_recurrence_basetime: 1700000000, but the test never asserts it ends up on the summary. Since summarizeBolt12RecurrenceFromDecode has a dedicated branch for this key (see functions/bolt12Semantics.js lines 135-137), adding an explicit check would close a small coverage gap.
Verify each finding against the current code and only fix it if needed.
In `@tests/lightning/bolt12.semantics.unit.js` around lines 30 - 40, The test for
summarizeBolt12RecurrenceFromDecode is missing an assertion that
invoice_recurrence_basetime is preserved; update the test in
tests/lightning/bolt12.semantics.unit.js (the it block calling
bs.summarizeBolt12RecurrenceFromDecode and assigning to sum) to include an
assertion that sum.invoice_recurrence_basetime === 1700000000 so the function's
branch handling invoice_recurrence_basetime is covered.
functions/fabricPaymentBech32.js (2)
80-107: Minor: decoder accepts mixed-case fa1… input that bech32.decode may reject.
isFabricRoutedPaymentString is case-insensitive (line 48), but bech32/bech32m forbids mixed-case strings, so a caller who passes e.g. Fa1… or an otherwise mixed-case value that isFabricRoutedPaymentString happily recognizes will fall into the catch and yield null here rather than being normalized. If you want symmetry between recognition and decode, consider lowercasing str.trim() before calling bech32.decode, or documenting that decode requires a fully-lowercase (or fully-uppercase) input. Not a correctness bug — just a usability/consistency nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/fabricPaymentBech32.js` around lines 80 - 107, The decoder
decodeFabricRoutedPayment currently calls bech32.decode on str.trim(), which
rejects mixed-case bech32m input while isFabricRoutedPaymentString is
case-insensitive; normalize the input by lowercasing the trimmed string before
calling bech32.decode (e.g., use a local const normalized =
str.trim().toLowerCase()), use normalized for the decode call but keep using the
original trimmed value where you need to preserve casing if any, and ensure
subsequent checks (d.hrp, d.spec, and version) operate on the decoded result as
before so the function remains symmetric with isFabricRoutedPaymentString.
114-119: Consider handling non-string inputs consistently with isFabricRoutedPaymentString.
classifyPaymentEncodingString coerces via String(s || ''), so isFabricRoutedPaymentString(t) is called on an already-trimmed, stringified value — fine. However, if s is a non-string truthy object (e.g. {}), String(s) becomes "[object Object]" which then falls through to lb.classifyLightningEncodedString and returns 'unknown'. That matches current behavior but may be worth an explicit typeof s !== 'string' short-circuit returning 'empty' or 'unknown' to avoid surprising delegations. Optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/fabricPaymentBech32.js` around lines 114 - 119,
classifyPaymentEncodingString currently stringifies non-string inputs which can
produce misleading results (e.g. "[object Object]"); update
classifyPaymentEncodingString to first check typeof s !== 'string' and
short-circuit (return 'unknown') before coercing/trim so non-string objects
don't get passed to isFabricRoutedPaymentString or
lb.classifyLightningEncodedString; reference the function
classifyPaymentEncodingString and the helper isFabricRoutedPaymentString and
lb.classifyLightningEncodedString when applying the change.
src/sha2.c (1)
298-304: Function-local #define leaks to file scope.
FABRIC_HMAC_SHA512_WIPE (and FABRIC_PBKDF2_WIPE at line 350) are written inside function bodies but #define is always file-scope in C and is not #undef'd. If any later translation unit included into src/crypto.c defines a same-named helper, you'll get redefinition/shadowing surprises. Either #undef them at the end of each function, or hoist them to a single static inline helper per buffer set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sha2.c` around lines 298 - 304, The file-scope macros
FABRIC_HMAC_SHA512_WIPE and FABRIC_PBKDF2_WIPE are being defined inside function
bodies but remain visible for the whole translation unit; fix by removing these
function-local `#define` usages and either (A) replace each macro with a static
inline helper function (e.g., static inline void fabric_hmac_sha512_wipe(void
*key_block, size_t kb_len, void *key_hash, size_t kh_len, ...) and similarly for
pbkdf2) and call that helper from the function, or (B) if you prefer to keep the
macro form, add an immediate `#undef` FABRIC_HMAC_SHA512_WIPE (and `#undef`
FABRIC_PBKDF2_WIPE) at the end of the function that used it so the macro does
not leak into file scope; update all call sites to use the chosen approach and
keep the helper/macro names matching FABRIC_HMAC_SHA512_WIPE and
FABRIC_PBKDF2_WIPE for easy location.
src/security.c (1)
226-244: fabric_secure_memcmp return value doesn't match memcmp semantics.
The function name and signature (int) strongly evoke memcmp, but the returned value is a folded uint8_t that is only ever 0 (equal) or positive (not equal). Callers porting code like if (memcmp(...) < 0) or sorting with this would silently misbehave. Either rename to something like fabric_secure_memeq returning int boolean, or document the contract explicitly at the declaration.
Also note the if (!a || !b) short-circuit at lines 228-231 is not constant-time — fine for the NULL-guard case, but worth a comment so future readers don't reintroduce data-dependent branches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/security.c` around lines 226 - 244, The function fabric_secure_memcmp
currently returns 0 for equal and a non-zero uint8_t for not-equal, which
violates memcmp semantics; rename it to fabric_secure_memeq (or change its
declaration/documentation) and make it a boolean-style int that returns 0 for
equal and 1 for not-equal (or true/false semantics consistent with its new
name), update its prototype/comment accordingly, and add a note above the
NULL-guard in fabric_secure_memeq that the early NULL check is intentionally not
constant-time to prevent future accidental data-dependent branching; adjust all
callers to use the new name/contract.
🤖 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/security.c`:
- Around line 36-37: The global allocation registry and init flags (allocations,
memory_initialized, random_initialized, urandom_fd, hCryptProv) are not
thread-safe and must be protected to avoid races in track_allocation,
untrack_allocation, fabric_secure_malloc, fabric_secure_free, and
fabric_secure_random_init; add a platform-appropriate mutex (e.g.,
pthread_mutex_t on POSIX / CRITICAL_SECTION on Windows) to serialize access to
the linked list and one-shot init flags (lock around reads/writes in the listed
functions), initialize/destroy the mutex in the module init/shutdown, and use
the mutex to protect both the registry updates and any lazy initialization
checks (or alternatively add debug-only single-thread assertions documenting
single-thread usage).
---
Duplicate comments:
In `@src/security.c`:
- Around line 205-218: fabric_secure_realloc must not silently drop the caller's
data when fabric_secure_malloc_size(ptr) returns 0; in fabric_secure_realloc,
after calling fabric_secure_malloc_size(ptr) into old_size, if old_size == 0
treat this as an error/unknown pointer and immediately return NULL without
allocating a new buffer, without calling memcpy or fabric_secure_free(ptr, 0),
and preserve the original pointer; update the code paths around
fabric_secure_malloc(new_size), memcpy(new_ptr, ptr, copy_size) and
fabric_secure_free(ptr, old_size) accordingly so that the function returns NULL
on untracked pointers instead of returning a zero-length/new buffer.
---
Nitpick comments:
In `@functions/fabricPaymentBech32.js`:
- Around line 80-107: The decoder decodeFabricRoutedPayment currently calls
bech32.decode on str.trim(), which rejects mixed-case bech32m input while
isFabricRoutedPaymentString is case-insensitive; normalize the input by
lowercasing the trimmed string before calling bech32.decode (e.g., use a local
const normalized = str.trim().toLowerCase()), use normalized for the decode call
but keep using the original trimmed value where you need to preserve casing if
any, and ensure subsequent checks (d.hrp, d.spec, and version) operate on the
decoded result as before so the function remains symmetric with
isFabricRoutedPaymentString.
- Around line 114-119: classifyPaymentEncodingString currently stringifies
non-string inputs which can produce misleading results (e.g. "[object Object]");
update classifyPaymentEncodingString to first check typeof s !== 'string' and
short-circuit (return 'unknown') before coercing/trim so non-string objects
don't get passed to isFabricRoutedPaymentString or
lb.classifyLightningEncodedString; reference the function
classifyPaymentEncodingString and the helper isFabricRoutedPaymentString and
lb.classifyLightningEncodedString when applying the change.
In `@src/security.c`:
- Around line 226-244: The function fabric_secure_memcmp currently returns 0 for
equal and a non-zero uint8_t for not-equal, which violates memcmp semantics;
rename it to fabric_secure_memeq (or change its declaration/documentation) and
make it a boolean-style int that returns 0 for equal and 1 for not-equal (or
true/false semantics consistent with its new name), update its prototype/comment
accordingly, and add a note above the NULL-guard in fabric_secure_memeq that the
early NULL check is intentionally not constant-time to prevent future accidental
data-dependent branching; adjust all callers to use the new name/contract.
In `@src/sha2.c`:
- Around line 298-304: The file-scope macros FABRIC_HMAC_SHA512_WIPE and
FABRIC_PBKDF2_WIPE are being defined inside function bodies but remain visible
for the whole translation unit; fix by removing these function-local `#define`
usages and either (A) replace each macro with a static inline helper function
(e.g., static inline void fabric_hmac_sha512_wipe(void *key_block, size_t
kb_len, void *key_hash, size_t kh_len, ...) and similarly for pbkdf2) and call
that helper from the function, or (B) if you prefer to keep the macro form, add
an immediate `#undef` FABRIC_HMAC_SHA512_WIPE (and `#undef` FABRIC_PBKDF2_WIPE) at
the end of the function that used it so the macro does not leak into file scope;
update all call sites to use the chosen approach and keep the helper/macro names
matching FABRIC_HMAC_SHA512_WIPE and FABRIC_PBKDF2_WIPE for easy location.
In `@tests/lightning/bolt12.semantics.unit.js`:
- Around line 30-40: The test for summarizeBolt12RecurrenceFromDecode is missing
an assertion that invoice_recurrence_basetime is preserved; update the test in
tests/lightning/bolt12.semantics.unit.js (the it block calling
bs.summarizeBolt12RecurrenceFromDecode and assigning to sum) to include an
assertion that sum.invoice_recurrence_basetime === 1700000000 so the function's
branch handling invoice_recurrence_basetime is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11f0e167-62e5-475b-a574-cd9f4b063b79
📥 Commits
Reviewing files that changed from the base of the PR and between e00df04 and 090ca70.
⛔ Files ignored due to path filters (1)
package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (9)
functions/fabricPaymentBech32.js
src/security.c
src/sha2.c
src/sha2.h
tests/bitcoin/transaction.js
tests/fabricPaymentBech32.unit.js
tests/lightning/bolt12.semantics.unit.js
tests/lightning/lightning.service.unit.js
types/bitcoin/transaction.js
✅ Files skipped from review due to trivial changes (1)
tests/fabricPaymentBech32.unit.js
🚧 Files skipped from review as they are similar to previous changes (4)
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fabric.environment.js`:
- Around line 34-45: The test must isolate and restore environment variables
used by Environment so external FABRIC_* values don't affect the assertion:
before creating the temp home (near variables prev, home, store, walletPath)
capture the current values of FABRIC_SEED, FABRIC_XPRV, and FABRIC_XPUB and
delete them from process.env (or set to undefined) for the duration of the test,
then create and start the Environment and assert on environment.wallet as
before; in the finally block, restore process.env.NODE_ENV to prev but if prev
is undefined delete process.env.NODE_ENV (do not set the string "undefined"),
and restore the original FABRIC_SEED, FABRIC_XPRV, and FABRIC_XPUB values
(deleting them if they were originally undefined) so subsequent tests are not
polluted.
In `@types/environment.js`:
- Around line 86-94: The explicit-seed selection currently prepends FIXTURE_SEED
in test mode causing FIXTURE_SEED to override configured seeds and also omits
this.local; fix by building the explicit array to include this.settings.seed,
this['FABRIC_SEED'], this.readVariable('FABRIC_SEED') and this.local (in that
order) and only append FIXTURE_SEED when process.env.NODE_ENV === 'test' (do not
unshift), then return explicit.find(any) so configured seeds take precedence
over the fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5141206-78b7-4901-817a-0fe395064269
📥 Commits
Reviewing files that changed from the base of the PR and between 090ca70 and 21ff49d.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/lightning.js`:
- Around line 294-305: In fetchInvoice, the object-form branch (fetchInvoice
when offerOrParams is an object) returns early and discards the separate
invoiceParams argument; update fetchInvoice to either merge invoiceParams into
the offer object or explicitly reject the combination—preferably merge
invoiceParams into offerOrParams (shallow copy) and pass the merged object to
_makeRPCRequest('fetchinvoice', [mergedParams]) so callers like
fetchInvoice({offer: '...'}, {amount_msat, payer_note}) are preserved; modify
the branch that checks typeof offerOrParams === 'object' in the fetchInvoice
method to perform the merge (or throw a clear Error mentioning both offer and
invoiceParams) before calling _makeRPCRequest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b49ecfd-23bd-446c-9cdd-5fab2e4c7c74
📥 Commits
Reviewing files that changed from the base of the PR and between 090ca70 and 2faa6ce.
📒 Files selected for processing (8)
functions/bolt12Semantics.js
services/lightning.js
tests/fabric.chain.js
tests/fabric.environment.js
tests/lightning/bolt12.semantics.unit.js
types/bitcoin/transaction.js
types/chain.js
types/environment.js
✅ Files skipped from review due to trivial changes (2)
tests/fabric.environment.js
tests/lightning/bolt12.semantics.unit.js
🚧 Files skipped from review as they are similar to previous changes (4)
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick comments (2)
tests/lightning/lightning.service.unit.js (2)
23-36: Avoid source-text assertions for module wiring.
Line 35 already proves Lightning.Bolt12Semantics is the exact required module. The source scan on Lines 26-31 can fail after harmless service refactors.
Verify each finding against the current code and only fix it if needed.
In `@tests/lightning/lightning.service.unit.js` around lines 23 - 36, Remove the
brittle source-text check that reads the service file and tests for a literal
"Bolt12Semantics" export; specifically delete the require.resolve/lightningPath,
fs.readFileSync(src), the /\bBolt12Semantics\b/ regex test and its corresponding
assert.ok with the path-based error message, leaving the existing runtime
assertions (assert.ok(Lightning.Bolt12), assert.ok(Lightning.FabricPayment),
assert.ok(Lightning.Bolt12Semantics), and
assert.strictEqual(Lightning.Bolt12Semantics, bolt12Semantics)) intact so the
test relies on actual module wiring rather than source-text scanning.
13-20: Assert the exported doc paths resolve.
These assertions validate the string literals, but not whether the files are actually present in the package root. Adding an existence check would catch stale Lightning.DOCS entries before release.
Test hardening
assert.strictEqual(Lightning.DOCS.fabricPaymentBech32, 'docs/FABRIC_PAYMENT_BECH32.md');
assert.strictEqual(Lightning.DOCS.lightningCompat, 'docs/LIGHTNING_COMPAT.md');
+ for (const docPath of new Set(Object.values(Lightning.DOCS))) {+ assert.ok(+ fs.existsSync(path.resolve(__dirname, '..', '..', docPath)),+ `missing Lightning.DOCS file: ${docPath}`+ );+ }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/lightning/lightning.service.unit.js` around lines 13 - 20, The test
currently only asserts string values on Lightning.DOCS; enhance it to verify the
referenced files actually exist by resolving each Lightning.DOCS entry against
the package root and asserting file existence (e.g., use path.resolve/package
root and fs.existsSync or fs.promises.access) for keys like
Lightning.DOCS.boltCompatibility, Lightning.DOCS.fabricLightningOffers,
Lightning.DOCS.fabricPaymentBech32 and Lightning.DOCS.lightningCompat in the
same describe block so stale doc paths fail the unit test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/lightning/lightning.service.unit.js`:
- Around line 23-36: Remove the brittle source-text check that reads the service
file and tests for a literal "Bolt12Semantics" export; specifically delete the
require.resolve/lightningPath, fs.readFileSync(src), the /\bBolt12Semantics\b/
regex test and its corresponding assert.ok with the path-based error message,
leaving the existing runtime assertions (assert.ok(Lightning.Bolt12),
assert.ok(Lightning.FabricPayment), assert.ok(Lightning.Bolt12Semantics), and
assert.strictEqual(Lightning.Bolt12Semantics, bolt12Semantics)) intact so the
test relies on actual module wiring rather than source-text scanning.
- Around line 13-20: The test currently only asserts string values on
Lightning.DOCS; enhance it to verify the referenced files actually exist by
resolving each Lightning.DOCS entry against the package root and asserting file
existence (e.g., use path.resolve/package root and fs.existsSync or
fs.promises.access) for keys like Lightning.DOCS.boltCompatibility,
Lightning.DOCS.fabricLightningOffers, Lightning.DOCS.fabricPaymentBech32 and
Lightning.DOCS.lightningCompat in the same describe block so stale doc paths
fail the unit test.
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3514eb16-7447-4c56-bcac-b288f01ef783
📥 Commits
Reviewing files that changed from the base of the PR and between 090ca70 and e510e99.
⛔ Files ignored due to path filters (1)
reports/install.log is excluded by !**/*.log
📒 Files selected for processing (9)
functions/bolt12Semantics.js
services/lightning.js
tests/fabric.chain.js
tests/fabric.environment.js
tests/lightning/bolt12.semantics.unit.js
tests/lightning/lightning.service.unit.js
types/bitcoin/transaction.js
types/chain.js
types/environment.js
✅ Files skipped from review due to trivial changes (1)
tests/lightning/bolt12.semantics.unit.js
🚧 Files skipped from review as they are similar to previous changes (4)
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 170-172: The dependency override pins jayson -> uuid@14.0.0 which
is ESM-only and incompatible with jayson (which requires a CJS uuid); update the
override in package.json for the "jayson" entry to use a CJS-compatible uuid
(e.g., set the uuid version to "=13.0.0" or any 8–13.x that satisfies
jayson@4.3.0's ^8.3.2), or remove the uuid override entirely if not needed, then
reinstall to ensure jayson can require('uuid') at runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11314c67-d156-4a39-a8a6-111d5a44cc49
📥 Commits
Reviewing files that changed from the base of the PR and between e510e99 and 16cdecc.
⛔ Files ignored due to path filters (2)
package-lock.json is excluded by !**/package-lock.json
reports/install.log is excluded by !**/*.log
📒 Files selected for processing (2)
examples/http.js
package.json
✅ Files skipped from review due to trivial changes (1)
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain
🌐 Web query:
Does jayson 4.3.0 use uuid via CommonJS require(), and is it known to be compatible with uuid v14's ESM-only / exports layout?
💡 Result:
Yes, jayson 4.3.0 uses uuid via CommonJS require, specifically in its browser client code with const uuid = require('uuid/v4');. This is shown in the source code from older versions (e.g., 3.0.1), and given the Dec 2025 release date with uuid ^8.3.2 dependency (which supports CommonJS), it likely remains unchanged. It is known to be compatible with uuid v14's ESM-only layout? No evidence of incompatibility found; no reported issues. Jayson's Node.js server/client likely use uuid ^8 (CommonJS-compatible), while browser uses a bundled version. Uuid v14 drops CommonJS support per docs, but since jayson pins ^8.3.2, standard installs use compatible version unless manually overridden to v14.
#!/bin/bash# Confirm jayson's declared uuid dependency range and how it imports uuid,# so we can reason about whether forcing uuid@14.0.0 is safe.set -euo pipefail
# 1) What does jayson itself declare as its uuid dep range?
curl -s https://registry.npmjs.org/jayson/4.3.0 | jq '.dependencies.uuid, .devDependencies.uuid'# 2) Check uuid@14.0.0 package.json exports/main to confirm CJS entrypoint still exists.
curl -s https://registry.npmjs.org/uuid/14.0.0 | jq '{main: .main, exports: .exports, type: .type, engines: .engines}'
Repository: FabricLabs/fabric
Length of output: 283
Do not pin jayson to uuid@14.0.0 — it is incompatible and will break at runtime.
The override as written will fail. jayson@4.3.0 declares uuid@^8.3.2 as a dependency and uses CommonJS require('uuid/v4') to import it. However, uuid@14.0.0 is ESM-only (with "type": "module", "main": null, and no CommonJS entrypoint). When jayson tries to require('uuid'), Node.js will not find a CommonJS export and the request will fail at runtime.
To fix jayson, pin to a uuid version that still supports CommonJS — versions 8 through 13 are all compatible with jayson's ^8.3.2 requirement. For example, "uuid": "=13.0.0" (the last CJS-compatible major).
If you have tested this override with jayson in your environment and it works, please share that evidence; otherwise, change the pin immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 170 - 172, The dependency override pins jayson ->
uuid@14.0.0 which is ESM-only and incompatible with jayson (which requires a CJS
uuid); update the override in package.json for the "jayson" entry to use a
CJS-compatible uuid (e.g., set the uuid version to "=13.0.0" or any 8–13.x that
satisfies jayson@4.3.0's ^8.3.2), or remove the uuid override entirely if not
needed, then reinstall to ensure jayson can require('uuid') at runtime.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@types/peer.js`:
- Around line 2014-2026: The function _sendLocalInventoryDocumentsWireResponse
currently returns false when items is empty, causing no reply for
kind:'documents'; change the logic to allow sending an empty inventory: remove
the items.length check from the early-return so the function proceeds when items
is an empty array, construct the obj with items (possibly empty) and send it via
conn._writeFabric as currently done (retain Message.fromVector,
m.signWithKey(this.key), conn._writeFabric(m.toBuffer())); apply the same change
to the similar documents-response path around the other occurrence referenced
(lines ~2058-2062) so an explicit empty catalog is returned instead of no
response.
- Around line 1697-1702: The declaration const relay under the case
'P2P_CHAT_MESSAGE' shares the switch lexical scope and triggers
noSwitchDeclarations; fix it by wrapping the case body in its own block (add {
... } after case 'P2P_CHAT_MESSAGE':) so the
Message.fromVector/relay.signWithKey/this.relayFrom calls (and any commented
debug) are inside that block, isolating the binding for relay.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c4f61bd-ddd1-4338-8691-8d62a9e59faf
📥 Commits
Reviewing files that changed from the base of the PR and between 16cdecc and 39bd835.
⛔ Files ignored due to path filters (2)
package-lock.json is excluded by !**/package-lock.json
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@types/environment.js`:
- Around line 603-617: The code calls readWallet() before entering the try block
so a failing read will throw and bypass the recoverable path; move the call to
readWallet() (and the subsequent text normalization) inside the existing try
that handles tryParsePersistedJson so any read errors are caught, then on catch
set this.wallet = false and emit the same warning via this.emit('warning', ...)
(preserve the message text and use this.settings.path) so the code falls back
safely; refer to walletExists(), readWallet(), tryParsePersistedJson(),
this.emit and this.wallet when making the change.
- Around line 86-95: The seed selection builds the explicit array (explicit)
from candidates like this.settings.seed, this['FABRIC_SEED'],
this.readVariable('FABRIC_SEED'), this.local and FIXTURE_SEED but does not
normalize whitespace, so this.local (read from .FABRIC_SEED) can include
trailing newlines and be chosen raw; update the selection to normalize each
candidate before checking with any by mapping/trimming strings (e.g., for each
candidate from explicit call .trim() and treat empty/undefined results as falsy)
so that the .find(any) operates on trimmed seed values (apply trimming to the
explicit array generation or immediately before .find).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1349e7a5-4575-427a-902c-9aa5cbb6295e
📥 Commits
Reviewing files that changed from the base of the PR and between 39bd835 and cb161b0.
⛔ Files ignored due to path filters (1)
package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
tests/fabric.environment.js
types/environment.js
🚧 Files skipped from review as they are similar to previous changes (1)
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review: found one high-confidence availability issue introduced/exposed by this PR. The new native P2P translation unit compiles the peer implementation into the addon, and reachable peer error paths can return while holding locks, allowing a connected peer to stall P2P connection management.
I did not find a medium+ security issue in the added Lightning/BOLT12 helpers or dependency delta after tracing the changed code paths.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review found 1 high-confidence issue: the newly compiled native P2P translation unit exposes existing peer receive paths that can return while holding locks, allowing a connected peer to permanently stall P2P connection handling. I did not find other medium+ PR-introduced vulnerabilities after validating the current diff and prior automation finding.
Logging the entire seed object leaks sensitive private key material.
console.warn('[FABRIC:KEYGEN]', 'GENERATED_SEED', '=', seed) serializes the full seed object — including phrase (the BIP39 mnemonic) — to the terminal. The accompanying // TODO: remove from log output... comment acknowledges this but leaves it in place for the RC1 cut. Terminal output may be captured in CI logs, shell history, or supervisord logs, making this a PII/key-material leak.
At minimum, remove or gate this line before the RC1 release.
🤖 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 `@scripts/chat.js` at line 138, The console.warn call in scripts/chat.js that
logs the entire seed object (console.warn('[FABRIC:KEYGEN]', 'GENERATED_SEED',
'=', seed)) leaks sensitive key material; remove or gate this log before RC1 by
either deleting the console.warn or wrapping it behind a strict opt-in debug
flag (e.g., only log when process.env.DEBUG_FABRIC_KEYGEN === 'true'), and if
any seed-related info must be emitted, log only non-sensitive metadata or a
redacted/masked representation (do not serialize seed.phrase or full seed
object).
🧹 Nitpick comments (2)
scripts/chat.js (1)
65-70: ⚡ Quick win
_createFromFreshSeed() is called twice for new wallets, generating and discarding the first seed.
When no wallet exists and the user is not running --keygen, both the pre-parse block (line 67) and the post-parse block (line 129) call wallet._createFromFreshSeed(). The first seed is silently discarded. While the end result is functionally correct (the second seed is used), it wastes entropy and a BIP39 derivation. The pre-parse initialization (lines 66–70) should be removed, leaving only the post-parse block.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/chat.js` around lines 65 - 70, The pre-parse initialization in main()
calls wallet._createFromFreshSeed() when !environment.walletExists(), causing a
seed to be generated and discarded; remove that pre-parse branch (the call to
wallet._createFromFreshSeed() and its assignment to seed) and leave only the
post-parse logic that invokes wallet._createFromFreshSeed() or _loadSeedData()
as appropriate so the seed is generated exactly once; ensure the seed variable
is declared in scope for the post-parse block and that any references to the
removed pre-parse code (e.g., seed assignment) are cleaned up.
types/store.js (1)
184-188: 💤 Low value
_GET result is discarded — the read has no effect.
The value returned by this._GET('/entities/${actor.id}') is never captured. If the intent was to check for an existing entity before overwriting, the result needs to be used; if it was just to verify the key is readable (existence check), a comment would clarify intent. As written, this is a silent no-op read that adds a round-trip to the store on every _REGISTER call.
♻️ Proposed fix (if intent is existence-checking)
+ let existing = null;
try {
- await this._GET(`/entities/${actor.id}`);+ existing = await this._GET(`/entities/${actor.id}`);
} catch (E) {
this.warn('[STORE]', '_REGISTER', `Could not read from store:`, E);
}
🤖 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/store.js` around lines 184 - 188, The try/catch around
this._GET(`/entities/${actor.id}`) in _REGISTER discards the result—so either
capture the returned value and act on it or remove the unnecessary read; update
_REGISTER to await and assign the result (e.g., const existing = await
this._GET(`/entities/${actor.id}`)), then use existing to decide whether to
skip/merge/overwrite the actor or proceed to write, and in the catch keep the
this.warn('[STORE]','_REGISTER',...) but include a clear comment explaining
intent if the read was only meant as an existence check; ensure you reference
_GET, _REGISTER, actor.id and preserve error handling.
🤖 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 `@scripts/chat.js`:
- Around line 54-63: _loadSeedData currently returns null on missing/malformed
wallet which causes uncaught TypeErrors when callers access seed.phrase and
seed.xpub; change _loadSeedData to return an empty object ({}) instead of null
on any error or missing wallet and, additionally, add defensive guards at the
call sites that read seed.phrase and seed.xpub (the places that reference
seed.phrase and seed.xpub) to check for falsy seed or missing properties before
accessing them and handle the missing-seed case (early return or fallback) so
the code never dereferences null.
In `@scripts/keygen.js`:
- Line 9: The console.warn is encoding the public key incorrectly by calling
keypair.public.toString('hex'); change the logging to call the adapter's
encodeCompressed method instead so the public key is emitted as a hex string
(use keypair.public.encodeCompressed('hex') in place of
keypair.public.toString('hex') when logging keypair.public).
In `@types/codec.js`:
- Around line 51-58: The encode method can return null when
this.key.encrypt(data) fails, causing Store._setEncrypted to silently store the
string "null"; update encode (in types/codec.js) to check the result of
this.key.encrypt(data) and throw a descriptive Error (e.g. "encryption failed"
or include key id) if blob is null/undefined so callers (like
Store._setEncrypted) can detect the failure; ensure you perform this check after
calling this.key.encrypt and before registering actors/labels or returning the
blob so no corrupt state is persisted.
- Around line 61-68: The decode method (decode) currently lets
this.key.decrypt(blob) return null and quietly propagates that to callers (e.g.,
Store._getEncrypted), making decryption failures indistinguishable from a
missing key; update decode to check the result of this.key.decrypt(blob) and
throw a descriptive Error (including context like "decryption failed for
label/key" and relevant identifiers such as this.key.pubkey and actor.id) when
data === null or undefined, while preserving the existing Actor and Label
registration logic (_registerActor/_registerLabel) only after successful
decryption.
In `@types/store.js`:
- Line 191: Before calling this._SET to cache the actor type, check that the
Actor's .type getter actually returns a defined value; if actor.type is
undefined skip the storage. Locate the call that writes to `/types/${actor.id}`
(the await this._SET(...) line) and wrap it in a guard that only invokes _SET
when typeof actor.type !== 'undefined' (or actor.type != null if you want to
exclude null too) so you don't store undefined values.
---
Outside diff comments:
In `@scripts/chat.js`:
- Line 138: The console.warn call in scripts/chat.js that logs the entire seed
object (console.warn('[FABRIC:KEYGEN]', 'GENERATED_SEED', '=', seed)) leaks
sensitive key material; remove or gate this log before RC1 by either deleting
the console.warn or wrapping it behind a strict opt-in debug flag (e.g., only
log when process.env.DEBUG_FABRIC_KEYGEN === 'true'), and if any seed-related
info must be emitted, log only non-sensitive metadata or a redacted/masked
representation (do not serialize seed.phrase or full seed object).
---
Nitpick comments:
In `@scripts/chat.js`:
- Around line 65-70: The pre-parse initialization in main() calls
wallet._createFromFreshSeed() when !environment.walletExists(), causing a seed
to be generated and discarded; remove that pre-parse branch (the call to
wallet._createFromFreshSeed() and its assignment to seed) and leave only the
post-parse logic that invokes wallet._createFromFreshSeed() or _loadSeedData()
as appropriate so the seed is generated exactly once; ensure the seed variable
is declared in scope for the post-parse block and that any references to the
removed pre-parse code (e.g., seed assignment) are cleaned up.
In `@types/store.js`:
- Around line 184-188: The try/catch around this._GET(`/entities/${actor.id}`)
in _REGISTER discards the result—so either capture the returned value and act on
it or remove the unnecessary read; update _REGISTER to await and assign the
result (e.g., const existing = await this._GET(`/entities/${actor.id}`)), then
use existing to decide whether to skip/merge/overwrite the actor or proceed to
write, and in the catch keep the this.warn('[STORE]','_REGISTER',...) but
include a clear comment explaining intent if the read was only meant as an
existence check; ensure you reference _GET, _REGISTER, actor.id and preserve
error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8929718-c77f-4342-92c3-cdd636255df0
📥 Commits
Reviewing files that changed from the base of the PR and between f93d3f1 and 0444633.
this.key.decrypt(blob) returns null in multiple cases (missing private key, bad IV format, cipher error). With the try/catch removed, a null return silently propagates to Store._getEncrypted (line 167–168), which returns null — the same value returned for a legitimately absent key. A caller has no way to detect that decryption failed vs. the key never existing.
At minimum, throw when data is null after decryption so the failure is observable:
🛡️ Proposed fix
decode (blob) {
const actor = new Actor(blob);
const label = new Label(`${this.key.pubkey}/${actor.id}`);
const data = this.key.decrypt(blob);
+ if (data == null) throw new Error('[FABRIC:CODEC] Decryption failed — private key unavailable or corrupted ciphertext');
this._registerActor(actor);
this._registerLabel(label);
return data;
}
🤖 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/codec.js` around lines 61 - 68, The decode method (decode) currently
lets this.key.decrypt(blob) return null and quietly propagates that to callers
(e.g., Store._getEncrypted), making decryption failures indistinguishable from a
missing key; update decode to check the result of this.key.decrypt(blob) and
throw a descriptive Error (including context like "decryption failed for
label/key" and relevant identifiers such as this.key.pubkey and actor.id) when
data === null or undefined, while preserving the existing Actor and Label
registration logic (_registerActor/_registerLabel) only after successful
decryption.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Check whether Actor has a .type getter/property
rg -n "get type" types/actor.js -A 3
rg -n "\.type" types/actor.js -A 2 | head -40
Repository: FabricLabs/fabric
Length of output: 840
Verify actor.type is defined before storing it.
The Actor class has a .type getter that returns this._state['@type'], which can be undefined if the state lacks an @type property. Storing undefined at /types/${actor.id} is wasteful and defeats the purpose of caching the type.
🤖 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/store.js` at line 191, Before calling this._SET to cache the actor
type, check that the Actor's .type getter actually returns a defined value; if
actor.type is undefined skip the storage. Locate the call that writes to
`/types/${actor.id}` (the await this._SET(...) line) and wrap it in a guard that
only invokes _SET when typeof actor.type !== 'undefined' (or actor.type != null
if you want to exclude null too) so you don't store undefined values.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review outcome: one high-confidence High severity issue remains. The PR newly adds src/p2p.c, which compiles the native P2P implementation into the addon; the exposed receive path can be remotely driven into a lock leak and P2P denial of service. I did not find other medium+ vulnerabilities with a concrete attacker-controlled path in the modified code.
The reason will be displayed to describe this comment to others. Learn more.
Security review found high-confidence native P2P DoS issues introduced by the new blocking Noise I/O paths. Prior Cursor security threads about decrypt-failure unlocks were revalidated and resolved as addressed.
The reason will be displayed to describe this comment to others. Learn more.
High: this forces the socket into blocking mode before the Noise handshake loop, but the reads/writes below have no deadline. listener_thread_function() reaches this while holding the connection-table write lock, so an unauthenticated client can send the initial key bytes and then stall the Noise frame to block all connection management for a P2P DoS. Keep the handshake bounded/nonblocking or release shared locks before network I/O.
The reason will be displayed to describe this comment to others. Learn more.
High: this frame read is now blocking while connections_rwlock and conn_mutex are already held. A connected peer can send a partial varint/frame, or stop sending entirely, and keep those locks held indefinitely, blocking later send/receive/disconnect paths for a P2P DoS. Use bounded nonblocking reads/timeouts or avoid holding shared locks across the read.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Further cleanup for 0.1.0-RC1.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores
Tests