feat: Complete Lightning Wallet — Auto-verification, Budget Control, Styled Receipts#38
Open
welliv wants to merge 3 commits intogetAlby:masterfrom
Open
feat: Complete Lightning Wallet — Auto-verification, Budget Control, Styled Receipts#38welliv wants to merge 3 commits intogetAlby:masterfrom
welliv wants to merge 3 commits intogetAlby:masterfrom
Conversation
…l, styled receipts
📝 WalkthroughWalkthroughReplaces docs to reposition the repo as an Alby Bitcoin Lightning Wallet Skill (Community Edition), adds a runtime Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AutoLedger as auto_ledger.js
participant NWC as NWC Client
participant Nostr as Nostr Network
participant Storage as Ledger Storage
User->>AutoLedger: start
AutoLedger->>NWC: connect(NWC_URL)
NWC->>AutoLedger: listTransactions(limit=50)
loop per transaction
AutoLedger->>AutoLedger: dedupe(payment_hash)
alt tx.state == "settled"
AutoLedger->>AutoLedger: if preimage present → sha256(preimage)
alt hash matches
AutoLedger->>Storage: write verified ledger entry + proof file
else
AutoLedger->>Storage: write ledger entry (unverified)
end
end
end
AutoLedger->>Nostr: subscribe(notifications)
loop on notification
Nostr->>AutoLedger: payment_received/payment_sent/hold_invoice_accepted
AutoLedger->>AutoLedger: process & persist as above
end
User->>AutoLedger: SIGINT
AutoLedger->>Nostr: unsubscribe
AutoLedger->>NWC: close()
AutoLedger->>Storage: final ledger flush
sequenceDiagram
participant User
participant CLI as pay.js
participant Storage as Pending Store
participant NWC as NWC Client
participant Node as Wallet Node
User->>CLI: pay(invoice or lnaddr, sats)
CLI->>CLI: decode input
CLI->>Storage: add pending(payment_hash, sats)
CLI->>NWC: payInvoice(invoice)
alt success returns preimage
NWC-->>CLI: preimage
CLI->>CLI: verify sha256(preimage) == payment_hash
CLI->>Storage: remove pending
CLI-->>User: success + proof
else error
CLI->>Node: listTransactions(recent)
alt payment found settled
Node-->>CLI: preimage
CLI->>CLI: verify & remove pending
CLI-->>User: reconciled success
else still pending
CLI-->>User: leave pending for reconciliation
end
end
CLI->>NWC: close()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (26)
scripts/qr_invoice.js (1)
8-8: Remove unusedfsimport.The
fsmodule is imported but never used in this script.🧹 Remove unused import
const { NWCClient } = require("@getalby/sdk/nwc"); const { getFiatValue } = require("@getalby/lightning-tools/fiat"); const qrcode = require("qrcode"); -const fs = require("fs");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/qr_invoice.js` at line 8, Remove the unused fs import by deleting the const fs = require("fs"); statement from the top of the file; locate the require statement for fs in scripts' qr generation script (the symbol "fs") and remove it so the module is no longer imported when it's not used.scripts/balance.js (3)
47-49: Redundant nullish coalescing onsym.
symis already guaranteed to be a string (line 47 uses|| ''), sosym ?? ''on line 49 is redundant.🧹 Simplify
if (rate === null) { - console.log(`${sym ?? ''} (rate unavailable) ${code}`); + console.log(`${sym} (rate unavailable) ${code}`); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/balance.js` around lines 47 - 49, The template uses a redundant nullish coalescing on sym (sym is already computed as const sym = symbols[code] || ''), so update the log call in the branch that checks rate === null to use sym directly (e.g., `${sym} (rate unavailable) ${code}`) instead of `${sym ?? ''} ...`; this removes the unnecessary `?? ''` and keeps behavior identical for variables sym, code, and rate.
7-7: Remove unusedgetFiatBtcRateimport.
getFiatBtcRateis imported but never used.🧹 Remove unused import
-const { getFiatValue, getFiatBtcRate } = require("@getalby/lightning-tools/fiat"); +const { getFiatValue } = require("@getalby/lightning-tools/fiat");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/balance.js` at line 7, The import list includes an unused symbol getFiatBtcRate; remove getFiatBtcRate from the require call that currently destructures getFiatValue and getFiatBtcRate so it only imports getFiatValue (i.e., update the require statement where getFiatBtcRate is referenced) to eliminate the unused import.
21-29: ThePromise.allon line 21 is misleading — it doesn't await the rate promises.The code works because the promises are created during
.map()and later awaited on line 29, but the structure is confusing. The firstPromise.allwraps an array of plain objects, not promises.♻️ Clearer pattern
- const rateData = await Promise.all( - currencies.map(c => ({ - code: c, - rate: getFiatValue({ satoshi: 1, currency: c }).catch(() => null), - })) - ); - - // Resolve all rate promises - const rates = await Promise.all(rateData.map(r => r.rate)); + // Fetch rates for all currencies in parallel + const rates = await Promise.all( + currencies.map(c => getFiatValue({ satoshi: 1, currency: c }).catch(() => null)) + );Then use
currencies[i]instead ofrateData[i].codein the loops below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/balance.js` around lines 21 - 29, The first Promise.all is misleading because it wraps an array of plain objects instead of awaiting the actual rate promises; replace that pattern by mapping currencies to promises that resolve to objects containing the currency code and its awaited rate (use getFiatValue({satoshi:1, currency: c}).catch(() => null) inside the mapping) and then await a single Promise.all to get the resolved array (refer to rateData and rates and the getFiatValue call), and update subsequent code to use currencies[i] (or the resolved object's code) rather than relying on the original rateData[i].code.scripts/gen_card.py (3)
15-21: Bareexceptclause; consider catching specific exceptions.The bare
except:catches all exceptions includingKeyboardInterruptandSystemExit. For font loading, catchingOSErrororIOErrorwould be more appropriate. That said, falling back to default font is a reasonable strategy here.♻️ More specific exception handling
def load_font(path, size): try: if os.path.exists(path): return ImageFont.truetype(path, size) - except: - pass + except (OSError, IOError): + pass # Font file exists but couldn't be loaded return ImageFont.load_default()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gen_card.py` around lines 15 - 21, The load_font function uses a bare except which swallows all exceptions; change it to catch more specific exceptions (e.g., OSError and IOError) around the ImageFont.truetype call in load_font so only expected file/IO errors are handled and other exceptions (like KeyboardInterrupt/SystemExit) propagate; keep the existing fallback to ImageFont.load_default() when those specific exceptions occur and optionally log the caught exception for debugging.
1-1: Use portable shebang.Hardcoding
/usr/bin/python3may fail on systems where Python is installed elsewhere. Useenvfor portability.🔧 Portable shebang
-#!/usr/bin/env /usr/bin/python3 +#!/usr/bin/env python3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gen_card.py` at line 1, Replace the hardcoded interpreter shebang at the top of the script (the current "/usr/bin/python3" shebang line) with a portable env-based shebang so the system locates Python via PATH (use the env-based shebang that invokes python3); update the first line of scripts/gen_card.py accordingly to ensure portability across environments.
195-195: Remove extraneousfprefix from string without placeholders.Per static analysis (Ruff F541):
f"Bitcoin Lightning Network"has no interpolation.🧹 Fix
- footer_text = f"Bitcoin Lightning Network" + footer_text = "Bitcoin Lightning Network"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gen_card.py` at line 195, The assigned string literal for footer_text uses an unnecessary f-string; update the footer_text assignment in scripts/gen_card.py (the footer_text variable) to use a regular string literal without the f prefix (i.e., replace f"Bitcoin Lightning Network" with "Bitcoin Lightning Network") to satisfy static analysis (Ruff F541).scripts/hold_invoice.js (1)
90-90: Inconsistent error handling — missingprocess.exit(1).Other scripts in this PR (e.g.,
qr_invoice.js,decode.js) exit with code 1 on error. This script only logs the error, which may cause issues in automation pipelines.🔧 Consistent error handling
-main().catch(e => { console.error(e.message); }); +main().catch(e => { console.error(e.message); process.exit(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hold_invoice.js` at line 90, The script's top-level promise rejection handler only logs the error (main().catch(e => { console.error(e.message); });) which is inconsistent with other scripts; modify the handler for the main function to exit with a non-zero status on failure (for example call process.exit(1) or set process.exitCode = 1) after logging the error so automation pipelines detect the failure.scripts/decode.js (1)
8-12: Stdin listeners are attached even when command-line input is provided.When
process.argv[2]orprocess.env.INPUTexists, the IIFE still executes and attachesdata/endlisteners to stdin, which are never cleaned up. Consider lazy stdin reading.♻️ Lazy stdin approach
-const input = process.argv[2] || process.env.INPUT || (() => { - let data = ''; - process.stdin.on('data', chunk => data += chunk); - return new Promise(resolve => process.stdin.on('end', () => resolve(data.trim()))); -})(); +function readStdin() { + let data = ''; + process.stdin.on('data', chunk => data += chunk); + return new Promise(resolve => process.stdin.on('end', () => resolve(data.trim()))); +} + +const input = process.argv[2] || process.env.INPUT || readStdin();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/decode.js` around lines 8 - 12, The current assignment to the input constant runs the IIFE unconditionally, attaching process.stdin 'data' and 'end' listeners even when process.argv[2] or process.env.INPUT is present; change to a lazy/conditional read: replace the IIFE with a function (e.g., getInput or makeInputPromise) that first checks process.argv[2] and process.env.INPUT and only sets up stdin listeners and returns a Promise when neither is provided, ensuring listeners are attached only when needed (use once/removeListener semantics or resolve on 'end' to avoid leaking listeners); reference the existing input constant, the IIFE, and the process.stdin 'data'/'end' handlers when making the change.scripts/streaks.js (1)
48-51: Consider single-pass aggregation for cleaner code.Four iterations over the same array could be consolidated. This is minor given the 100-transaction limit but improves readability.
♻️ Single-pass alternative
- const totalIncoming = sorted.filter(t => t.type === "incoming").length; - const totalOutgoing = sorted.filter(t => t.type === "outgoing").length; - const totalSatsIncoming = sorted.filter(t => t.type === "incoming").reduce((s, t) => s + t.amount/1000, 0); - const totalSatsOutgoing = sorted.filter(t => t.type === "outgoing").reduce((s, t) => s + t.amount/1000, 0); + const stats = sorted.reduce((acc, t) => { + const sats = t.amount / 1000; + if (t.type === "incoming") { + acc.inCount++; + acc.inSats += sats; + } else { + acc.outCount++; + acc.outSats += sats; + } + return acc; + }, { inCount: 0, outCount: 0, inSats: 0, outSats: 0 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/streaks.js` around lines 48 - 51, The four separate passes over sorted (computing totalIncoming, totalOutgoing, totalSatsIncoming, totalSatsOutgoing) should be replaced with a single reduce that iterates sorted once and accumulates {totalIncoming, totalOutgoing, totalSatsIncoming, totalSatsOutgoing}; inside the reducer increment counts for t.type === "incoming"/"outgoing" and add t.amount/1000 to the corresponding sats sum, then destructure those totals where the current variables totalIncoming, totalOutgoing, totalSatsIncoming, totalSatsOutgoing are used.package.json (1)
1-9: All package versions are valid and up-to-date; consider adding package metadata for better project identification.The specified dependency versions exist and are already at their latest releases. The minimal
package.jsonstructure is acceptable for an internal skill. Optionally add"name","version", and"private": truefor clearer project identification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 1 - 9, The package.json is minimal and missing basic metadata; add a "name" (unique project identifier), a "version" (semver string like "0.1.0"), and "private": true to the root object to improve project identification and prevent accidental publishing; update the package.json top-level object accordingly so the dependency block (the "dependencies" key) remains unchanged.scripts/export_ledger.js (2)
40-45: CSV output goes to both stdout and file.The CSV is printed to console (line 40) and then also saved to a file (line 44). When piping output (e.g.,
node export_ledger.js csv > out.csv), the "Saved: ..." message on line 45 will pollute the CSV.Use stderr for the "Saved" message
fs.writeFileSync(csvPath, [header, ...rows].join("\n")); - console.log(`\nSaved: ${csvPath}`); + console.error(`Saved: ${csvPath}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/export_ledger.js` around lines 40 - 45, The script currently writes the CSV to stdout using console.log([header, ...rows].join("\n")) and then prints a status message with console.log(`\nSaved: ${csvPath}`), which will corrupt piped CSV output; change the status message to write to stderr instead (e.g., use process.stderr.write or console.error) so the CSV stays on stdout and the "Saved: ..." message (built from csvPath) goes to stderr; keep the CSV write using fs.writeFileSync(csvPath, [header, ...rows].join("\n")) and ensure no leading newline is printed to stderr that could affect piped output.
3-3: Misleading usage comment.The comment mentions
NWC_URL="..."but this script doesn't use NWC at all — it only reads from the local ledger file.Fix usage comment
-// Usage: NWC_URL="..." node export_ledger.js [csv|summary|json] [days] +// Usage: node export_ledger.js [csv|summary|json] [days]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/export_ledger.js` at line 3, The top-of-file usage comment in export_ledger.js is misleading because it references NWC_URL even though the script reads the local ledger file; update the comment string (the existing "Usage: NWC_URL=\"...\" node export_ledger.js [csv|summary|json] [days]") to accurately reflect how to run the script (for example: "Usage: node export_ledger.js [csv|summary|json] [days]" or include the actual local ledger env/arg name if the script supports one) so the usage message in export_ledger.js correctly describes how to invoke the script.scripts/wallets.js (1)
10-10: Unused import.
cryptois imported but never used in this file.Remove unused import
-const crypto = require("crypto");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/wallets.js` at line 10, The file imports crypto via "const crypto = require('crypto');" but never uses it; remove this unused import to clean up the module (or if cryptographic functions were intended, use the crypto variable in the relevant function such as any wallet generation/seed functions like createWallet or deriveKey); most likely simply delete the const crypto = require("crypto"); line to resolve the lint warning.scripts/validate.js (2)
14-14: Unused import.
Invoicefrom@getalby/lightning-toolsis imported but never used.Remove unused import
-const { Invoice } = require('@getalby/lightning-tools');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate.js` at line 14, The import of Invoice from `@getalby/lightning-tools` in scripts/validate.js is unused; remove the const { Invoice } = require('@getalby/lightning-tools'); statement (or, if Invoice is actually needed, replace the unused import by using the Invoice symbol in the appropriate function such as validation logic), ensuring no other code references Invoice remains broken.
59-60: Use explicit radix withparseInt.
parseInt(amount)without a radix can lead to unexpected results if the string has leading zeros (octal interpretation in some engines).Add explicit radix
- if (amount) amount = parseInt(amount) / 1000; // millisats -> sats + if (amount) amount = parseInt(amount, 10) / 1000; // millisats -> sats🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate.js` around lines 59 - 60, The code parses an amount from decoded.sections into a numeric value using parseInt(amount) which can be ambiguous; update the parsing in the amount handling (the variable named amount where decoded.sections.find(...) is used) to use an explicit radix (e.g., parseInt(amount, 10)) or a safer alternative such as Number(amount) before dividing by 1000 so the conversion from millisats to sats is deterministic and not sensitive to leading zeros.references/payment-validation.md (1)
30-36: Code snippet missing null safety forpayment_hashlookup.The example code will throw if the invoice lacks a
payment_hashsection. While this is documentation, users may copy it directly.Suggested improvement
function validatePayment(invoice: string, preimage: string): boolean { const decoded = decodeBolt11(invoice); - const paymentHash = decoded.sections.find(s => s.name === 'payment_hash').value; + const hashSection = decoded.sections.find(s => s.name === 'payment_hash'); + if (!hashSection) throw new Error('payment_hash not found in invoice'); + const paymentHash = hashSection.value; const preimageBytes = Uint8Array.from(preimage.match(/.{1,2}/g).map(h => parseInt(h, 16))); const computed = crypto.createHash('sha256').update(preimageBytes).digest('hex'); return computed === paymentHash; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@references/payment-validation.md` around lines 30 - 36, The validatePayment function assumes decoded.sections contains a payment_hash and will throw if missing; update validatePayment to null-check the payment_hash lookup (e.g., use find result into a variable like paymentHashSection), handle the case where it is undefined by returning false or throwing a clear error, and also guard preimage parsing (ensure match(...) is not null) before converting to bytes; keep references to decodeBolt11, validatePayment, and the 'payment_hash' section when making these checks.scripts/health_check.js (1)
45-46: Ledger file is read and parsed twice.The ledger JSON is read at line 46 and again at line 60. This is wasteful for potentially large files.
Read ledger once and reuse
+ // 4. Ledger + let ledger = null; let ledgerStatus = "⚠️ Not started"; if (fs.existsSync(LEDGER_FILE)) { - const ledger = JSON.parse(fs.readFileSync(LEDGER_FILE, "utf8")); + ledger = JSON.parse(fs.readFileSync(LEDGER_FILE, "utf8")); if (ledger.length > 0) { // ... existing code ... } } checks.push(`📖 Ledger: ${ledgerStatus}`); // 5. Last Activity - if (fs.existsSync(LEDGER_FILE)) { - const ledger = JSON.parse(fs.readFileSync(LEDGER_FILE, "utf8")); - if (ledger.length > 0) { + if (ledger && ledger.length > 0) { const last = ledger[ledger.length - 1]; const d = new Date(last.settled_at * 1000); checks.push(`⏱️ Last activity: ${d.toLocaleString()} (${last.type}, ${last.sats} sats)`); - } - } else { + } else if (!ledger) { checks.push("⏱️ Last activity: N/A"); }Also applies to: 59-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/health_check.js` around lines 45 - 46, The code calls fs.readFileSync + JSON.parse for LEDGER_FILE more than once; change the logic so you read and parse the ledger only once (e.g., when fs.existsSync(LEDGER_FILE) is true) into a single variable (e.g., ledger or ledgerData) and reuse that parsed object wherever JSON.parse(fs.readFileSync(LEDGER_FILE, "utf8")) is currently called; update all subsequent uses (the second read at the later check) to reference the single parsed variable and remove the duplicate read/parse to avoid re-reading large files.scripts/budget_guardian.js (1)
106-108: String compared with number.
pctis a string (fromtoFixed(1)), andpct > 90relies on implicit coercion. This works but is error-prone and unclear.Use numeric comparison
- const pct = (spent / budget.weekly_cap * 100).toFixed(1); + const pctNum = spent / budget.weekly_cap * 100; + const pct = pctNum.toFixed(1); const status = remaining > 0 ? "✅ Under budget" : "❌ OVER BUDGET"; // ... later ... - if (pct > 90) { + if (pctNum > 90) { console.log(`\n⚠️ WARNING: You're at ${pct}% of your weekly budget!`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/budget_guardian.js` around lines 106 - 108, The comparison uses pct (a string produced by toFixed(1)) against 90, causing implicit coercion; convert to a numeric value before comparing and use the formatted string only for display. For example, keep a numeric variable (e.g., numericPct or pctNum) derived with Number(...) or parseFloat(...) from the raw value (or from pct before toFixed), use numericPct > 90 in the if, and then use pct (the toFixed string) only when logging; update the references around pct in the budget_guardian.js block accordingly.scripts/summary.js (1)
72-76: Hardcoded EUR and KES exchange rates will drift from reality.The EUR (0.87) and KES (130) multipliers are static approximations. These rates change daily and will become increasingly inaccurate over time.
Use getFiatValue for each currency
- const usd = (sats * rate).toFixed(2); - const eur = (sats * rate * 0.87).toFixed(2); - const kes = Math.round(sats * rate * 130); + const [usd, eur, kes] = await Promise.all([ + (sats * rate).toFixed(2), + getFiatValue({ satoshi: sats, currency: "EUR" }).then(v => v.toFixed(2)), + getFiatValue({ satoshi: sats, currency: "KES" }).then(v => Math.round(v)) + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/summary.js` around lines 72 - 76, Replace the hardcoded EUR (0.87) and KES (130) multipliers with dynamic rates by calling the existing getFiatValue helper: compute eur via getFiatValue('EUR', sats) and kes via getFiatValue('KES', sats) (and consider using getFiatValue('USD', sats) instead of the manual usd calculation) then preserve formatting (eur.toFixed(2) for cents, Math.round or toLocaleString for KES). Update the lines that set usd, eur, kes and the output line so they use these getFiatValue results and keep the same visual formatting (currency symbols and spacing).scripts/auto_ledger.js (2)
158-161: Unhandled rejection in main catch may exit without cleanup.If
main()rejects after subscriptions are established, the process exits without callingunsub()orclient.close(). Consider moving client cleanup to afinallyblock or ensuring graceful shutdown on errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/auto_ledger.js` around lines 158 - 161, The catch on main() exits immediately and can skip cleanup if subscriptions or a client are active; update main() (or its caller) to ensure unsub() and client.close() are always invoked by adding a finally block or a top-level try/finally around the startup/teardown logic so that unsub() and client.close() are called even when main() rejects, and propagate or log the original error before exiting; locate the code paths that create the subscription and the client (references: main(), unsub(), client.close()) and move their cleanup into that guaranteed-finally section.
129-140: Add error handling for subscription callback.If
processTransactionthrows an error inside the subscription callback, it will be an unhandled rejection. Consider wrapping the callback body in try-catch to prevent the monitoring process from silently breaking.🛡️ Proposed fix
const unsub = await client.subscribeNotifications( async (notification) => { + try { const tx = notification.notification; console.log(`\n🔔 ${notification.notification_type === "payment_received" ? "→ INCOMING" : notification.notification_type === "payment_sent" ? "← OUTGOING" : "🔒 HOLD"} | ${tx.amount / 1000} sats`); const result = await processTransaction(tx); if (result) { console.log(`✅ Verified & saved: ${result.sats} sats (${result.usd} USD) — ${result.crypto_proof.verified ? "CRYPTO PROOF ✅" : "NO PREIMAGE"}`); } + } catch (err) { + console.error(`⚠️ Error processing notification: ${err.message}`); + } }, ["payment_received", "payment_sent", "hold_invoice_accepted"] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/auto_ledger.js` around lines 129 - 140, The subscription callback passed to client.subscribeNotifications can throw unhandled rejections when processTransaction fails; wrap the entire async callback (the function receiving notification and using tx and calling processTransaction) in a try-catch, catch errors, log a clear error message including the notification/tx id and the error, and avoid letting the exception bubble (optionally mark the notification as failed or continue); update the callback used in client.subscribeNotifications and keep the unsub handling unchanged.scripts/hold_invoice_manual.js (1)
89-89: Process may hang on main rejection.If
main()rejects (e.g., NWC connection fails), the error is logged butprocess.exitisn't called, potentially leaving the process hanging.🔧 Proposed fix
-main().catch(e => { console.error(e.message); }); +main().catch(e => { console.error(e.message); process.exit(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hold_invoice_manual.js` at line 89, The catch on main() only logs e.message and doesn't terminate the process, which can leave the script hanging on failures; update the main() rejection handler to log the full error (or stack) for diagnostics and then call process.exit(1) to ensure the process exits on failure (i.e., replace the current main().catch(e => { console.error(e.message); }); handler with one that logs the error and calls process.exit(1)).SKILL.md (1)
15-21: Add language specifiers to fenced code blocks.Several code blocks are missing language specifiers, which affects syntax highlighting and accessibility. Consider adding appropriate language identifiers.
📝 Proposed fix to add language specifiers
-``` +```text User: "send 500 to alice@getalby.com"For balance/transaction/proof examples (lines 56, 63, 69), use
textor leave as-is if intentionally plain. For the Budget Guardian commands (line 105):-``` +```bash node budget_guardian.js setup 5000 # 5000 sats/weekAlso applies to: 56-60, 63-66, 69-73, 105-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SKILL.md` around lines 15 - 21, Add explicit language specifiers to the fenced code blocks in SKILL.md so syntax highlighting and accessibility are correct; update the examples such as the user utterance blocks (e.g., the block containing User: "send 500 to alice@getalby.com" and the other User examples like "balance", "create invoice for 1000 sats", "my wallet", "verify this payment") to use ```text and change the Budget Guardian example (the block showing node budget_guardian.js setup 5000 # 5000 sats/week) to use ```bash; apply the same treatment to the other plain examples around the referenced areas (the blocks near lines with balance/transaction/proof examples and the Budget Guardian command) so all fenced code blocks include a language specifier.scripts/analytics.js (2)
30-34: Handle case where transactions exist but none are settled.The early return checks
transactions.length === 0, but if all fetched transactions are non-settled, the report will show "0 settled" with zeros for all amounts, which may be confusing. Consider also returning early whensettledCount === 0.💡 Proposed enhancement
+ // Count settled transactions first + const settledTxs = transactions.filter(t => t.state === "settled"); + - if (transactions.length === 0) { + if (transactions.length === 0 || settledTxs.length === 0) { console.log(`No transactions in the last ${daysBack} days.`); client.close(); return; }Then use
settledTxsin the loop instead of filtering inline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/analytics.js` around lines 30 - 34, The code currently only early-returns when transactions.length === 0, but if transactions exist and none are settled you should also early-return: after computing settledCount (and settledTxs = transactions.filter(t => t.settled)), check if settledCount === 0, log a clear message like "No settled transactions in the last ${daysBack} days.", call client.close(), and return; then replace inline filtering in the subsequent loop with the precomputed settledTxs so the report logic only iterates settledTxs.
83-94: Consider reusing filtered results from the aggregation loop.The code filters
settledtransactions twice: once in the aggregation loop (lines 40-61) and again for "Top transactions" (line 84). Consider collecting settled transactions during the first pass to avoid redundant filtering.♻️ Proposed optimization
let totalIn = 0, totalOut = 0, totalFees = 0; - let settledCount = 0; + const settledTxs = []; const descriptions = {}; for (const t of transactions) { if (t.state !== "settled") continue; - settledCount++; + settledTxs.push(t); // ... rest of loop } + const settledCount = settledTxs.length; // Later, for top transactions: - const sorted = [...transactions] - .filter(t => t.state === "settled") - .sort((a, b) => b.amount - a.amount) + const sorted = [...settledTxs] + .sort((a, b) => b.amount - a.amount) .slice(0, 5);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/analytics.js` around lines 83 - 94, During the initial aggregation loop that currently iterates over transactions to compute totals, also collect settled items into a new array (e.g., settledTransactions) instead of re-filtering transactions later; then replace the second filter used to build const sorted = [...transactions].filter(...) with sorting/slicing settledTransactions (or a copy) so the "Top transactions" block (the sorted variable and subsequent for (const t of sorted) loop) reuses the already-collected settled entries; update variable names and any references accordingly (transactions, settledTransactions, sorted) and ensure the same settled criteria (t.state === "settled") is applied exactly once during the first pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 35-37: Update the README clone and install commands so they
reference the correct repository and install all required packages: change the
git clone URL from getAlby/alby-agent-skill to getAlby/builder-skill (or the
actual repo name for this PR), keep the destination directory
~/.hermes/skills/alby-bitcoin-payments if intended, and expand the npm install
invocation to include the missing dependencies listed in package.json
(bolt12-decoder, qrcode-terminal, bolt11) alongside `@getalby/sdk`,
`@getalby/lightning-tools`, light-bolt11-decoder, qrcode so the install step
matches the repository's runtime requirements.
In `@scripts/auto_ledger.js`:
- Line 79: The fees assignment can produce NaN when tx.fees_paid is undefined;
update the fees calculation where the object sets "fees: tx.fees_paid / 1000" to
guard against undefined by coalescing to 0 and converting to a Number (e.g., use
tx.fees_paid ?? 0 or Number(tx.fees_paid ?? 0) before dividing) so "fees" is
always a valid numeric value; locate the assignment that uses tx.fees_paid in
scripts/auto_ledger.js and replace it with the guarded expression.
- Around line 14-16: The console.log override (origLog and reassignment of
console.log) appends an extra newline because origLog already writes one; remove
the override or change uses to write directly to stdout. Specifically, delete or
revert the console.log override block that defines origLog and reassigns
console.log, or replace logging calls to use process.stdout.write where
immediate unbuffered output is required; ensure any remaining logging does not
call origLog and then process.stdout.write("\n") to avoid double newlines.
In `@scripts/budget_guardian.js`:
- Around line 110-115: The avg-trend block is dead because budget.history is
initialized in loadBudget() but never populated; either remove the block or
implement history tracking by appending a weekly summary object to
budget.history whenever the week resets (e.g., in resetBudget() or wherever
spend totals are rolled), then persist it via saveBudget()/writeBudget(); ensure
the summary shape matches the reader code (objects with a spent numeric field)
so the existing slice(-4).reduce((s,w)=>s+w.spent,0) calculation works as
intended.
- Line 123: The usage string in budget_guardian.js currently advertises a
non-existent "check" command; update the console.log call that prints the usage
(the line with console.log(`Usage: node budget_guardian.js [setup
<weekly_sats>|check|reset|status]`)) to remove "check" so it only lists
implemented commands (setup, reset, status), or alternatively implement a real
"check" handler if you intended that command—preferably fix the usage string to
match existing handlers.
In `@scripts/decode.js`:
- Around line 45-46: The decodeBolt11 function currently strips a leading
"LNURL" or "lnurl" from its input before calling decodeInvoice, which is
incorrect because LNURL is a separate bech32-encoded URL protocol and should not
be mangled into a BOLT-11 invoice; remove the pr.replace(/^LNURL|lnurl/, '')
transformation so decodeInvoice receives the original string, and if LNURL
support is required add a separate code path that detects LNURL (e.g., by bech32
prefix or "lnurl" indicator) and handles fetching/processing it instead of
passing it into decodeInvoice.
In `@scripts/gen_card.py`:
- Around line 96-108: The else branch in the title selection can reference an
undefined title if card_type is unknown and kwargs lacks 'title'; update the
logic in scripts/gen_card.py around the card_type handling (the card_type
variable and title assignments) to ensure title always has a default before
use—either initialize title = kwargs.get('title', 'UNKNOWN CARD') (or a suitable
default like 'CUSTOM') before the if/elif chain, or change the final else to set
title = kwargs.get('title', 'UNKNOWN CARD') so title is never referenced before
assignment.
In `@scripts/hold_invoice_manual.js`:
- Around line 79-86: The timeout handler for cancelling the HOLD invoice can
race with stdin command handlers because both call unsub() and client.close();
introduce a shared guard and cancelable timeout: store the setTimeout id (e.g.,
timeoutId) and a boolean flag (e.g., finished) at module scope, then in every
path that settles or cancels (the stdin handlers that call
client.cancelHoldInvoice or client.settleHoldInvoice) first check and set
finished (if (finished) return; finished = true), clearTimeout(timeoutId) and
then call unsub() and client.close(); also wrap the existing timeout callback to
set finished before performing cancelHoldInvoice/unsub()/client.close() so only
one path runs.
- Around line 62-76: The settle/cancel branches call await
client.settleHoldInvoice and await client.cancelHoldInvoice without error
handling so exceptions skip cleanup; wrap each await in try/catch (or use a
single try around both branches) and ensure unsub() and client.close() run in a
finally block, logging the caught error (include function name
settleHoldInvoice/cancelHoldInvoice in the message) and calling process.exit(1)
on failure so cleanup always executes regardless of thrown errors.
In `@scripts/monitor_payments.js`:
- Around line 35-36: The code uses the internal API process.stdout._handle
without guarding for environments where it may be undefined; update the block
around process.stdout._handle.setBlocking(true) to first check that
process.stdout and process.stdout._handle are defined (and optionally that
process.stdout.isTTY is truthy) before calling setBlocking on it, so the call to
setBlocking(true) is only executed when process.stdout._handle exists and is
safe to use.
In `@scripts/qr_invoice.js`:
- Around line 21-24: Promise.all is causing invoice creation to abort when
getFiatValue rejects; change the logic so invoice creation (client.makeInvoice)
always runs even if fiat lookup fails—e.g., call client.makeInvoice separately
or use Promise.allSettled for [getFiatValue, client.makeInvoice], then extract
invoice from the fulfilled result and set fiatUSD to null/undefined on failure,
and log the fiat error; update references to fiatUSD and invoice accordingly
(symbols: getFiatValue, client.makeInvoice, Promise.all/Promise.allSettled,
fiatUSD, invoice).
In `@scripts/streaks.js`:
- Around line 8-10: The main function creates an NWCClient without validating
the required environment variable NWC_URL; add a guard at the start of main
(before new NWCClient(...)) that checks process.env.NWC_URL and, if missing,
logs a clear error (e.g., console.error or processLogger.error) and exits/throws
(process.exit(1) or throw new Error) so the script fails fast with a helpful
message; update the code around main and the NWCClient instantiation to perform
this check.
- Around line 33-45: The gap-handling is inconsistent: treat any gap of 2 or
more days as a streak break rather than incrementing on daysApart == 2. Replace
the existing branch with: if daysApart <= 1 then increment tempStreak and update
longestStreak; else (daysApart >= 2) treat as a break — reset tempStreak = 1 and
reset/adjust currentStreak appropriately (remove the unclear condition using i <
sorted.length / 2 and currentStreak === tempStreak). Update longestStreak when
appropriate and ensure the variables daysApart, tempStreak, longestStreak,
currentStreak, sorted, and i are used consistently so a 2-day gap breaks the
streak.
In `@scripts/summary.js`:
- Around line 51-54: The summary script currently computes spent using a rolling
7-day window (filter on settled_at >= Date.now() - 7*86400), which differs from
budget_guardian.js that uses the week-start (Monday) window; update the logic in
the block referencing b.weekly_cap (the outgoing.filter and spent calculation)
to compute the timestamp for the start of the current week (Monday) the same way
budget_guardian.js does (derive startOfWeekUtc in seconds from Date.now() or new
Date(), adjust by day index so Monday is start, and use that value in the
settled_at comparison), then recalc spent/pct based on that startOfWeek
timestamp so both scripts yield consistent percentages. Ensure you use the same
timezone/UTC convention and rounding as budget_guardian.js when converting to
seconds.
- Around line 62-64: The output box row is incomplete: the line that currently
does lines.push('║') must be replaced with a padded row that matches the box
width and includes the closing box character so alignment isn't broken; update
the lines.push call in scripts/summary.js (the second lines.push in the shown
block) to compute the interior width from the top border (or a fixed width used
for the header) and push a string like '║' + spaces(padding) + '║' so the
alias/header row and subsequent empty row have consistent padding and closing
characters.
In `@scripts/validate.js`:
- Around line 33-38: Replace the incorrect BOLT12Decoder.decode usage: require
the package's default function (e.g., const decodeBolt12 =
require('bolt12-decoder')) and call decodeBolt12(invoice) instead of
BOLT12Decoder.decode(invoice); after decoding (variable decoded) inspect and
extract the correct field used for payment validation (replace paymentHash with
the appropriate property found on decoded, e.g., decoded.offer_chains or
decoded.payment_hash as applicable), throw an error if that property is missing,
and return { paymentHash: <extracted>, type: 'BOLT12', decoded } so the rest of
the validation logic (using trimmed and invoice) works with the correct API.
In `@scripts/wallets.js`:
- Around line 112-124: The switch updates config_local.json but other scripts
read process.env.NWC_URL, so update those scripts to fall back to
config_local.json when the env var is not set: add a small helper (e.g.,
readConfigLocal or getNwcConfig) used by balance.js, analytics.js, streaks.js,
and budget_guardian.js to return {nwc_url, nwc_wallet_pubkey} by reading and
parsing ~/.hermes/config_local.json and returning config.wallet.nwc_url and
config.wallet.nwc_wallet_pubkey if process.env.NWC_URL or
process.env.NWC_WALLET_PUBKEY are falsy; keep precedence as env vars first, then
config_local.json, and surface errors clearly if neither source yields values so
callers know to set the env or update config_local.json.
- Around line 116-119: The code assumes config.wallet exists before assigning
properties, which will throw if missing; modify the block that reads and updates
the config (the config variable and config.wallet assignments) to ensure
config.wallet is initialized (e.g., if (!config.wallet) config.wallet = {})
before setting config.wallet.nwc_url and config.wallet.nwc_wallet_pubkey, then
write the file as before (fs.writeFileSync with configPath and JSON.stringify).
Also improve the catch/error handling to surface a clear message mentioning
configPath and that the wallet object was missing if an error occurs.
- Around line 47-49: The code uses new URL(nwcUrl) which can throw on malformed
input; wrap the URL parsing (new URL(nwcUrl) and subsequent pubkey/short
derivation) in a try/catch around the block that constructs url, pubkey, and
short so parsing failures are caught, log or handle the invalid nwcUrl (e.g.,
skip that entry or emit a clear warning) instead of letting the script crash,
and ensure downstream code that expects pubkey/short is only executed when
parsing succeeds.
---
Nitpick comments:
In `@package.json`:
- Around line 1-9: The package.json is minimal and missing basic metadata; add a
"name" (unique project identifier), a "version" (semver string like "0.1.0"),
and "private": true to the root object to improve project identification and
prevent accidental publishing; update the package.json top-level object
accordingly so the dependency block (the "dependencies" key) remains unchanged.
In `@references/payment-validation.md`:
- Around line 30-36: The validatePayment function assumes decoded.sections
contains a payment_hash and will throw if missing; update validatePayment to
null-check the payment_hash lookup (e.g., use find result into a variable like
paymentHashSection), handle the case where it is undefined by returning false or
throwing a clear error, and also guard preimage parsing (ensure match(...) is
not null) before converting to bytes; keep references to decodeBolt11,
validatePayment, and the 'payment_hash' section when making these checks.
In `@scripts/analytics.js`:
- Around line 30-34: The code currently only early-returns when
transactions.length === 0, but if transactions exist and none are settled you
should also early-return: after computing settledCount (and settledTxs =
transactions.filter(t => t.settled)), check if settledCount === 0, log a clear
message like "No settled transactions in the last ${daysBack} days.", call
client.close(), and return; then replace inline filtering in the subsequent loop
with the precomputed settledTxs so the report logic only iterates settledTxs.
- Around line 83-94: During the initial aggregation loop that currently iterates
over transactions to compute totals, also collect settled items into a new array
(e.g., settledTransactions) instead of re-filtering transactions later; then
replace the second filter used to build const sorted =
[...transactions].filter(...) with sorting/slicing settledTransactions (or a
copy) so the "Top transactions" block (the sorted variable and subsequent for
(const t of sorted) loop) reuses the already-collected settled entries; update
variable names and any references accordingly (transactions,
settledTransactions, sorted) and ensure the same settled criteria (t.state ===
"settled") is applied exactly once during the first pass.
In `@scripts/auto_ledger.js`:
- Around line 158-161: The catch on main() exits immediately and can skip
cleanup if subscriptions or a client are active; update main() (or its caller)
to ensure unsub() and client.close() are always invoked by adding a finally
block or a top-level try/finally around the startup/teardown logic so that
unsub() and client.close() are called even when main() rejects, and propagate or
log the original error before exiting; locate the code paths that create the
subscription and the client (references: main(), unsub(), client.close()) and
move their cleanup into that guaranteed-finally section.
- Around line 129-140: The subscription callback passed to
client.subscribeNotifications can throw unhandled rejections when
processTransaction fails; wrap the entire async callback (the function receiving
notification and using tx and calling processTransaction) in a try-catch, catch
errors, log a clear error message including the notification/tx id and the
error, and avoid letting the exception bubble (optionally mark the notification
as failed or continue); update the callback used in
client.subscribeNotifications and keep the unsub handling unchanged.
In `@scripts/balance.js`:
- Around line 47-49: The template uses a redundant nullish coalescing on sym
(sym is already computed as const sym = symbols[code] || ''), so update the log
call in the branch that checks rate === null to use sym directly (e.g., `${sym}
(rate unavailable) ${code}`) instead of `${sym ?? ''} ...`; this removes the
unnecessary `?? ''` and keeps behavior identical for variables sym, code, and
rate.
- Line 7: The import list includes an unused symbol getFiatBtcRate; remove
getFiatBtcRate from the require call that currently destructures getFiatValue
and getFiatBtcRate so it only imports getFiatValue (i.e., update the require
statement where getFiatBtcRate is referenced) to eliminate the unused import.
- Around line 21-29: The first Promise.all is misleading because it wraps an
array of plain objects instead of awaiting the actual rate promises; replace
that pattern by mapping currencies to promises that resolve to objects
containing the currency code and its awaited rate (use getFiatValue({satoshi:1,
currency: c}).catch(() => null) inside the mapping) and then await a single
Promise.all to get the resolved array (refer to rateData and rates and the
getFiatValue call), and update subsequent code to use currencies[i] (or the
resolved object's code) rather than relying on the original rateData[i].code.
In `@scripts/budget_guardian.js`:
- Around line 106-108: The comparison uses pct (a string produced by toFixed(1))
against 90, causing implicit coercion; convert to a numeric value before
comparing and use the formatted string only for display. For example, keep a
numeric variable (e.g., numericPct or pctNum) derived with Number(...) or
parseFloat(...) from the raw value (or from pct before toFixed), use numericPct
> 90 in the if, and then use pct (the toFixed string) only when logging; update
the references around pct in the budget_guardian.js block accordingly.
In `@scripts/decode.js`:
- Around line 8-12: The current assignment to the input constant runs the IIFE
unconditionally, attaching process.stdin 'data' and 'end' listeners even when
process.argv[2] or process.env.INPUT is present; change to a lazy/conditional
read: replace the IIFE with a function (e.g., getInput or makeInputPromise) that
first checks process.argv[2] and process.env.INPUT and only sets up stdin
listeners and returns a Promise when neither is provided, ensuring listeners are
attached only when needed (use once/removeListener semantics or resolve on 'end'
to avoid leaking listeners); reference the existing input constant, the IIFE,
and the process.stdin 'data'/'end' handlers when making the change.
In `@scripts/export_ledger.js`:
- Around line 40-45: The script currently writes the CSV to stdout using
console.log([header, ...rows].join("\n")) and then prints a status message with
console.log(`\nSaved: ${csvPath}`), which will corrupt piped CSV output; change
the status message to write to stderr instead (e.g., use process.stderr.write or
console.error) so the CSV stays on stdout and the "Saved: ..." message (built
from csvPath) goes to stderr; keep the CSV write using fs.writeFileSync(csvPath,
[header, ...rows].join("\n")) and ensure no leading newline is printed to stderr
that could affect piped output.
- Line 3: The top-of-file usage comment in export_ledger.js is misleading
because it references NWC_URL even though the script reads the local ledger
file; update the comment string (the existing "Usage: NWC_URL=\"...\" node
export_ledger.js [csv|summary|json] [days]") to accurately reflect how to run
the script (for example: "Usage: node export_ledger.js [csv|summary|json]
[days]" or include the actual local ledger env/arg name if the script supports
one) so the usage message in export_ledger.js correctly describes how to invoke
the script.
In `@scripts/gen_card.py`:
- Around line 15-21: The load_font function uses a bare except which swallows
all exceptions; change it to catch more specific exceptions (e.g., OSError and
IOError) around the ImageFont.truetype call in load_font so only expected
file/IO errors are handled and other exceptions (like
KeyboardInterrupt/SystemExit) propagate; keep the existing fallback to
ImageFont.load_default() when those specific exceptions occur and optionally log
the caught exception for debugging.
- Line 1: Replace the hardcoded interpreter shebang at the top of the script
(the current "/usr/bin/python3" shebang line) with a portable env-based shebang
so the system locates Python via PATH (use the env-based shebang that invokes
python3); update the first line of scripts/gen_card.py accordingly to ensure
portability across environments.
- Line 195: The assigned string literal for footer_text uses an unnecessary
f-string; update the footer_text assignment in scripts/gen_card.py (the
footer_text variable) to use a regular string literal without the f prefix
(i.e., replace f"Bitcoin Lightning Network" with "Bitcoin Lightning Network") to
satisfy static analysis (Ruff F541).
In `@scripts/health_check.js`:
- Around line 45-46: The code calls fs.readFileSync + JSON.parse for LEDGER_FILE
more than once; change the logic so you read and parse the ledger only once
(e.g., when fs.existsSync(LEDGER_FILE) is true) into a single variable (e.g.,
ledger or ledgerData) and reuse that parsed object wherever
JSON.parse(fs.readFileSync(LEDGER_FILE, "utf8")) is currently called; update all
subsequent uses (the second read at the later check) to reference the single
parsed variable and remove the duplicate read/parse to avoid re-reading large
files.
In `@scripts/hold_invoice_manual.js`:
- Line 89: The catch on main() only logs e.message and doesn't terminate the
process, which can leave the script hanging on failures; update the main()
rejection handler to log the full error (or stack) for diagnostics and then call
process.exit(1) to ensure the process exits on failure (i.e., replace the
current main().catch(e => { console.error(e.message); }); handler with one that
logs the error and calls process.exit(1)).
In `@scripts/hold_invoice.js`:
- Line 90: The script's top-level promise rejection handler only logs the error
(main().catch(e => { console.error(e.message); });) which is inconsistent with
other scripts; modify the handler for the main function to exit with a non-zero
status on failure (for example call process.exit(1) or set process.exitCode = 1)
after logging the error so automation pipelines detect the failure.
In `@scripts/qr_invoice.js`:
- Line 8: Remove the unused fs import by deleting the const fs = require("fs");
statement from the top of the file; locate the require statement for fs in
scripts' qr generation script (the symbol "fs") and remove it so the module is
no longer imported when it's not used.
In `@scripts/streaks.js`:
- Around line 48-51: The four separate passes over sorted (computing
totalIncoming, totalOutgoing, totalSatsIncoming, totalSatsOutgoing) should be
replaced with a single reduce that iterates sorted once and accumulates
{totalIncoming, totalOutgoing, totalSatsIncoming, totalSatsOutgoing}; inside the
reducer increment counts for t.type === "incoming"/"outgoing" and add
t.amount/1000 to the corresponding sats sum, then destructure those totals where
the current variables totalIncoming, totalOutgoing, totalSatsIncoming,
totalSatsOutgoing are used.
In `@scripts/summary.js`:
- Around line 72-76: Replace the hardcoded EUR (0.87) and KES (130) multipliers
with dynamic rates by calling the existing getFiatValue helper: compute eur via
getFiatValue('EUR', sats) and kes via getFiatValue('KES', sats) (and consider
using getFiatValue('USD', sats) instead of the manual usd calculation) then
preserve formatting (eur.toFixed(2) for cents, Math.round or toLocaleString for
KES). Update the lines that set usd, eur, kes and the output line so they use
these getFiatValue results and keep the same visual formatting (currency symbols
and spacing).
In `@scripts/validate.js`:
- Line 14: The import of Invoice from `@getalby/lightning-tools` in
scripts/validate.js is unused; remove the const { Invoice } =
require('@getalby/lightning-tools'); statement (or, if Invoice is actually
needed, replace the unused import by using the Invoice symbol in the appropriate
function such as validation logic), ensuring no other code references Invoice
remains broken.
- Around line 59-60: The code parses an amount from decoded.sections into a
numeric value using parseInt(amount) which can be ambiguous; update the parsing
in the amount handling (the variable named amount where
decoded.sections.find(...) is used) to use an explicit radix (e.g.,
parseInt(amount, 10)) or a safer alternative such as Number(amount) before
dividing by 1000 so the conversion from millisats to sats is deterministic and
not sensitive to leading zeros.
In `@scripts/wallets.js`:
- Line 10: The file imports crypto via "const crypto = require('crypto');" but
never uses it; remove this unused import to clean up the module (or if
cryptographic functions were intended, use the crypto variable in the relevant
function such as any wallet generation/seed functions like createWallet or
deriveKey); most likely simply delete the const crypto = require("crypto"); line
to resolve the lint warning.
In `@SKILL.md`:
- Around line 15-21: Add explicit language specifiers to the fenced code blocks
in SKILL.md so syntax highlighting and accessibility are correct; update the
examples such as the user utterance blocks (e.g., the block containing User:
"send 500 to alice@getalby.com" and the other User examples like "balance",
"create invoice for 1000 sats", "my wallet", "verify this payment") to use
```text and change the Budget Guardian example (the block showing node
budget_guardian.js setup 5000 # 5000 sats/week) to use ```bash; apply the same
treatment to the other plain examples around the referenced areas (the blocks
near lines with balance/transaction/proof examples and the Budget Guardian
command) so all fenced code blocks include a language specifier.
🪄 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: fe85bb8b-fad3-4e07-bef3-276bfc67b001
📒 Files selected for processing (21)
README.mdSKILL.mdpackage.jsonreferences/payment-validation.mdregenerate-types.shscripts/analytics.jsscripts/auto_ledger.jsscripts/balance.jsscripts/budget_guardian.jsscripts/decode.jsscripts/export_ledger.jsscripts/gen_card.pyscripts/health_check.jsscripts/hold_invoice.jsscripts/hold_invoice_manual.jsscripts/monitor_payments.jsscripts/qr_invoice.jsscripts/streaks.jsscripts/summary.jsscripts/validate.jsscripts/wallets.js
💤 Files with no reviewable changes (1)
- regenerate-types.sh
Comment on lines
+35
to
+37
| git clone https://github.com/getAlby/alby-agent-skill.git ~/.hermes/skills/alby-bitcoin-payments | ||
| cd ~/.hermes/skills/alby-bitcoin-payments | ||
| npm install @getalby/sdk @getalby/lightning-tools light-bolt11-decoder qrcode |
There was a problem hiding this comment.
Clone URL may not match repository name.
The PR is on getAlby/builder-skill but the clone command references alby-agent-skill. Also, the npm install command is missing some dependencies listed in package.json (e.g., bolt12-decoder, qrcode-terminal, bolt11).
Verify and fix installation commands
-git clone https://github.com/getAlby/alby-agent-skill.git ~/.hermes/skills/alby-bitcoin-payments
+git clone https://github.com/getAlby/builder-skill.git ~/.hermes/skills/alby-bitcoin-payments
cd ~/.hermes/skills/alby-bitcoin-payments
-npm install `@getalby/sdk` `@getalby/lightning-tools` light-bolt11-decoder qrcode
+npm install📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| git clone https://github.com/getAlby/alby-agent-skill.git ~/.hermes/skills/alby-bitcoin-payments | |
| cd ~/.hermes/skills/alby-bitcoin-payments | |
| npm install @getalby/sdk @getalby/lightning-tools light-bolt11-decoder qrcode | |
| git clone https://github.com/getAlby/builder-skill.git ~/.hermes/skills/alby-bitcoin-payments | |
| cd ~/.hermes/skills/alby-bitcoin-payments | |
| npm install |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 35 - 37, Update the README clone and install commands
so they reference the correct repository and install all required packages:
change the git clone URL from getAlby/alby-agent-skill to getAlby/builder-skill
(or the actual repo name for this PR), keep the destination directory
~/.hermes/skills/alby-bitcoin-payments if intended, and expand the npm install
invocation to include the missing dependencies listed in package.json
(bolt12-decoder, qrcode-terminal, bolt11) alongside `@getalby/sdk`,
`@getalby/lightning-tools`, light-bolt11-decoder, qrcode so the install step
matches the repository's runtime requirements.
Comment on lines
+14
to
+16
| // Force stdout unbuffered for background process visibility | ||
| const origLog = console.log; | ||
| console.log = (...args) => { origLog(...args); process.stdout.write("\n"); }; |
There was a problem hiding this comment.
Console.log override causes double newlines.
The override appends an extra newline after every log call, since origLog already outputs a newline. This will result in double-spaced output.
🔧 Proposed fix
If the intent is to flush stdout immediately for background visibility, consider using process.stdout.write directly or simply remove the override since Node.js streams are already line-buffered for TTYs:
-// Force stdout unbuffered for background process visibility
-const origLog = console.log;
-console.log = (...args) => { origLog(...args); process.stdout.write("\n"); };
+// Ensure stdout is unbuffered for background process visibility
+if (process.stdout.isTTY === false) {
+ process.stdout.cork && process.stdout.uncork();
+}Or simply remove lines 14-16 if not strictly needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Force stdout unbuffered for background process visibility | |
| const origLog = console.log; | |
| console.log = (...args) => { origLog(...args); process.stdout.write("\n"); }; | |
| // Ensure stdout is unbuffered for background process visibility | |
| if (process.stdout.isTTY === false) { | |
| process.stdout.cork && process.stdout.uncork(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/auto_ledger.js` around lines 14 - 16, The console.log override
(origLog and reassignment of console.log) appends an extra newline because
origLog already writes one; remove the override or change uses to write directly
to stdout. Specifically, delete or revert the console.log override block that
defines origLog and reassigns console.log, or replace logging calls to use
process.stdout.write where immediate unbuffered output is required; ensure any
remaining logging does not call origLog and then process.stdout.write("\n") to
avoid double newlines.
| state: tx.state, | ||
| sats, | ||
| usd: usd || "N/A", | ||
| fees: tx.fees_paid / 1000, |
There was a problem hiding this comment.
Guard against undefined fees_paid to prevent NaN.
tx.fees_paid may be undefined for incoming transactions or certain transaction types, causing undefined / 1000 to produce NaN. This would propagate to the ledger and break downstream consumers like export_ledger.js.
🛡️ Proposed fix
- fees: tx.fees_paid / 1000,
+ fees: (tx.fees_paid || 0) / 1000,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/auto_ledger.js` at line 79, The fees assignment can produce NaN when
tx.fees_paid is undefined; update the fees calculation where the object sets
"fees: tx.fees_paid / 1000" to guard against undefined by coalescing to 0 and
converting to a Number (e.g., use tx.fees_paid ?? 0 or Number(tx.fees_paid ?? 0)
before dividing) so "fees" is always a valid numeric value; locate the
assignment that uses tx.fees_paid in scripts/auto_ledger.js and replace it with
the guarded expression.
Comment on lines
+110
to
+115
| // Show trend | ||
| if (budget.history.length > 1) { | ||
| const avg = budget.history.slice(-4).reduce((s, w) => s + w.spent, 0) / | ||
| Math.min(budget.history.length, 4); | ||
| console.log(`\nAvg recent: ${Math.round(avg).toLocaleString()} sats/week`); | ||
| } |
There was a problem hiding this comment.
Dead code — history is never populated.
The history array is initialized as empty in loadBudget() and never written to. This average calculation will never execute.
Either remove this dead code or implement history tracking (e.g., append weekly summaries on reset).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/budget_guardian.js` around lines 110 - 115, The avg-trend block is
dead because budget.history is initialized in loadBudget() but never populated;
either remove the block or implement history tracking by appending a weekly
summary object to budget.history whenever the week resets (e.g., in
resetBudget() or wherever spend totals are rolled), then persist it via
saveBudget()/writeBudget(); ensure the summary shape matches the reader code
(objects with a spent numeric field) so the existing
slice(-4).reduce((s,w)=>s+w.spent,0) calculation works as intended.
Comment on lines
+62
to
+64
| lines.push(`╓─ ${(info.alias || 'WALLET').toUpperCase()} ───────────────────────────╖`); | ||
| lines.push(`║ ║`); | ||
| lines.push(`║`); |
There was a problem hiding this comment.
Incomplete output line formatting.
Lines 63-64 appear to have incomplete box drawing — line 64 is just ║ without padding or closing characters, breaking the visual alignment.
Fix formatting
lines.push(`╓─ ${(info.alias || 'WALLET').toUpperCase()} ───────────────────────────╖`);
lines.push(`║ ║`);
- lines.push(`║`);
+ lines.push(`║ ║`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lines.push(`╓─ ${(info.alias || 'WALLET').toUpperCase()} ───────────────────────────╖`); | |
| lines.push(`║ ║`); | |
| lines.push(`║`); | |
| lines.push(`╓─ ${(info.alias || 'WALLET').toUpperCase()} ───────────────────────────╖`); | |
| lines.push(`║ ║`); | |
| lines.push(`║ ║`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/summary.js` around lines 62 - 64, The output box row is incomplete:
the line that currently does lines.push('║') must be replaced with a padded row
that matches the box width and includes the closing box character so alignment
isn't broken; update the lines.push call in scripts/summary.js (the second
lines.push in the shown block) to compute the interior width from the top border
(or a fixed width used for the header) and push a string like '║' +
spaces(padding) + '║' so the alias/header row and subsequent empty row have
consistent padding and closing characters.
Comment on lines
+33
to
+38
| // BOLT12 | ||
| if (trimmed.startsWith('lni')) { | ||
| const decoded = BOLT12Decoder.decode(invoice); | ||
| const paymentHash = decoded.paymentHash; | ||
| if (!paymentHash) throw new Error('BOLT12: paymentHash not found in invoice'); | ||
| return { paymentHash, type: 'BOLT12', decoded }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check bolt12-decoder package API and verify decode method exists
npm show bolt12-decoder@1.0.0 --json 2>/dev/null | jq '.description, .homepage'
# Search for any existing usage patterns in the codebase
rg -n "bolt12-decoder" --type=jsRepository: getAlby/builder-skill
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Search for all usages of BOLT12Decoder to understand the API better
rg -n "BOLT12Decoder\." --type=js -A 2 -B 1
# Check if there are any test files or documentation in the repo
fd -t f "test|spec|example" | head -20
# Look for package.json to see the exact version and dependencies
cat package.json | grep -A 5 -B 5 "bolt12-decoder"Repository: getAlby/builder-skill
Length of output: 625
🌐 Web query:
bolt12-decoder npm package API decode method documentation
💡 Result:
The "bolt12-decoder" package appears to be a GitHub repository (https://github.com/lnbc1QWFyb24/bolt12-decoder) rather than a published npm package, as no direct npm page was found. It provides a default export decodeBolt12 for decoding BOLT12 offers, invoices, and invoice requests. Installation: npm i bolt12-decoder or yarn add bolt12-decoder API - decode method: import decodeBolt12 from 'bolt12-decoder' const bolt12 = 'lno1qgsqvgnwgcg35z6ee2h3yczraddm72xrfua9uve2rlrm9deu7xyfzrc2q42xjurnyyfqys2zzcssx06thlxk00g0epvynxff5vj46p3en8hz8ax9uy4ckyyfuyet8eqg' const result = decodeBolt12(bolt12) // Returns object matching CoreLN decode RPC, e.g.: // { // offer_chains: ['06226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f'], // type: 'bolt12 offer', // valid: true // // ... other fields // } Caveats (from repo): Does not decode recurrence or invoice_paths/invoice_blindedpay. Alternative published packages: - light-bolt12-decoder (npm, v1.0.3, author 22388o): Likely similar lightweight decoder, but no detailed API docs found. - bolt12-ts (npm, v0.2.1, author nova-carnivore): Modern TypeScript BOLT12 encoder/decoder, no specific decode docs in results.
Citations:
- 1: https://github.com/lnbc1QWFyb24/bolt12-decoder
- 2: https://github.com/clams-tech/bolt12-decoder-app
- 3: https://www.npmjs.com/search?q=keywords%3Abolt12
Fix BOLT12 decoder API usage - incorrect method and missing property access.
The code incorrectly uses the bolt12-decoder package API:
- The package exports a default function
decodeBolt12, not a class with a.decode()method - The decoded object does not contain a
paymentHashproperty (it contains fields likeoffer_chains,type,valid)
Correct usage should be:
const decodeBolt12 = require('bolt12-decoder');
const decoded = decodeBolt12(invoice);
// Access appropriate fields from decoded objectVerify what property should be extracted from the decoded BOLT12 object for payment validation purposes, as paymentHash is not part of the API return value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/validate.js` around lines 33 - 38, Replace the incorrect
BOLT12Decoder.decode usage: require the package's default function (e.g., const
decodeBolt12 = require('bolt12-decoder')) and call decodeBolt12(invoice) instead
of BOLT12Decoder.decode(invoice); after decoding (variable decoded) inspect and
extract the correct field used for payment validation (replace paymentHash with
the appropriate property found on decoded, e.g., decoded.offer_chains or
decoded.payment_hash as applicable), throw an error if that property is missing,
and return { paymentHash: <extracted>, type: 'BOLT12', decoded } so the rest of
the validation logic (using trimmed and invoice) works with the correct API.
Comment on lines
+47
to
+49
| const url = new URL(nwcUrl); | ||
| const pubkey = url.hostname; | ||
| const short = pubkey.substring(0, 8) + "..." + pubkey.substring(pubkey.length - 4); |
There was a problem hiding this comment.
URL parsing can throw on malformed input.
new URL(nwcUrl) will throw a TypeError if the URL is malformed, even if it passes the prefix check. This would crash the script with an unhelpful stack trace.
Wrap in try/catch
- const url = new URL(nwcUrl);
- const pubkey = url.hostname;
+ let pubkey;
+ try {
+ const url = new URL(nwcUrl);
+ pubkey = url.hostname;
+ } catch {
+ console.error("Invalid NWC URL format");
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const url = new URL(nwcUrl); | |
| const pubkey = url.hostname; | |
| const short = pubkey.substring(0, 8) + "..." + pubkey.substring(pubkey.length - 4); | |
| let pubkey; | |
| try { | |
| const url = new URL(nwcUrl); | |
| pubkey = url.hostname; | |
| } catch { | |
| console.error("Invalid NWC URL format"); | |
| return; | |
| } | |
| const short = pubkey.substring(0, 8) + "..." + pubkey.substring(pubkey.length - 4); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/wallets.js` around lines 47 - 49, The code uses new URL(nwcUrl) which
can throw on malformed input; wrap the URL parsing (new URL(nwcUrl) and
subsequent pubkey/short derivation) in a try/catch around the block that
constructs url, pubkey, and short so parsing failures are caught, log or handle
the invalid nwcUrl (e.g., skip that entry or emit a clear warning) instead of
letting the script crash, and ensure downstream code that expects pubkey/short
is only executed when parsing succeeds.
Comment on lines
+112
to
+124
| // Update config_local.json for other scripts | ||
| try { | ||
| const configPath = path.join(process.env.HOME, ".hermes", "config_local.json"); | ||
| if (fs.existsSync(configPath)) { | ||
| const config = JSON.parse(fs.readFileSync(configPath, "utf8")); | ||
| config.wallet.nwc_url = wallets.wallets[name].url; | ||
| config.wallet.nwc_wallet_pubkey = wallets.wallets[name].pubkey; | ||
| fs.writeFileSync(configPath, JSON.stringify(config, null, 2)); | ||
| console.log("Updated config_local.json"); | ||
| } | ||
| } catch (e) { | ||
| console.log("Note: Could not update config_local.json - " + e.message); | ||
| } |
There was a problem hiding this comment.
switch updates config_local.json, but other scripts read from process.env.NWC_URL.
Per the codebase context, scripts like balance.js, analytics.js, streaks.js, and budget_guardian.js all read NWC_URL directly from environment variables, not from config_local.json. The switch command won't affect them unless users also update their environment.
Consider either:
- Documenting that users must also update their shell environment after switching
- Having scripts fall back to reading from
config_local.jsonwhenNWC_URLenv is not set
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/wallets.js` around lines 112 - 124, The switch updates
config_local.json but other scripts read process.env.NWC_URL, so update those
scripts to fall back to config_local.json when the env var is not set: add a
small helper (e.g., readConfigLocal or getNwcConfig) used by balance.js,
analytics.js, streaks.js, and budget_guardian.js to return {nwc_url,
nwc_wallet_pubkey} by reading and parsing ~/.hermes/config_local.json and
returning config.wallet.nwc_url and config.wallet.nwc_wallet_pubkey if
process.env.NWC_URL or process.env.NWC_WALLET_PUBKEY are falsy; keep precedence
as env vars first, then config_local.json, and surface errors clearly if neither
source yields values so callers know to set the env or update config_local.json.
Comment on lines
+116
to
+119
| const config = JSON.parse(fs.readFileSync(configPath, "utf8")); | ||
| config.wallet.nwc_url = wallets.wallets[name].url; | ||
| config.wallet.nwc_wallet_pubkey = wallets.wallets[name].pubkey; | ||
| fs.writeFileSync(configPath, JSON.stringify(config, null, 2)); |
There was a problem hiding this comment.
Potential crash if config.wallet is undefined.
If config_local.json exists but lacks a wallet key, config.wallet.nwc_url = ... will throw. The outer try/catch will catch it, but the error message won't clearly indicate the structural issue.
Initialize wallet object if missing
if (fs.existsSync(configPath)) {
const config = JSON.parse(fs.readFileSync(configPath, "utf8"));
+ config.wallet = config.wallet || {};
config.wallet.nwc_url = wallets.wallets[name].url;
config.wallet.nwc_wallet_pubkey = wallets.wallets[name].pubkey;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const config = JSON.parse(fs.readFileSync(configPath, "utf8")); | |
| config.wallet.nwc_url = wallets.wallets[name].url; | |
| config.wallet.nwc_wallet_pubkey = wallets.wallets[name].pubkey; | |
| fs.writeFileSync(configPath, JSON.stringify(config, null, 2)); | |
| const config = JSON.parse(fs.readFileSync(configPath, "utf8")); | |
| config.wallet = config.wallet || {}; | |
| config.wallet.nwc_url = wallets.wallets[name].url; | |
| config.wallet.nwc_wallet_pubkey = wallets.wallets[name].pubkey; | |
| fs.writeFileSync(configPath, JSON.stringify(config, null, 2)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/wallets.js` around lines 116 - 119, The code assumes config.wallet
exists before assigning properties, which will throw if missing; modify the
block that reads and updates the config (the config variable and config.wallet
assignments) to ensure config.wallet is initialized (e.g., if (!config.wallet)
config.wallet = {}) before setting config.wallet.nwc_url and
config.wallet.nwc_wallet_pubkey, then write the file as before (fs.writeFileSync
with configPath and JSON.stringify). Also improve the catch/error handling to
surface a clear message mentioning configPath and that the wallet object was
missing if an error occurs.
|
This is a solid set of wallet primitives. A few architectural considerations for production-grade agent spending controls: On the budget enforcement layer:
The receipt/card generation with PIL is a nice touch for operator UX — agents need human-readable audit trails too. How are you handling the offline/reattachment edge case where a payment succeeds on Lightning but the confirmation response times out? |
… counter, and pay.js with double-spend protection Reconciliation engine (reconcile.js): - Post-fallback reconciliation against the node's source of truth - Detects settled payments missing from ledger (backfills them) - Resolves pending payments: settled, in-flight, or stale_expired - Double-spend risk detection (same invoice paid multiple times) - Dry-run mode for auditing Budget Guardian enhancements: - Rolling window budget (default 24h, configurable via --window) - Pending counter — in-flight payments count as spent immediately - Node-verified spending (reads from node, not local state) - New commands: pending, clear-pending Payment flow (pay.js): - Pre-send: budget check with rolling window + pending counter - Pre-send: register pending (prevents double-spend if crash mid-flight) - Post-send: timeout triggers reconciliation after 5s delay - If node confirms → payment was delivered late - If node shows in-flight → keep pending, protect budget - If node has no record → safe to retry - Post-send: preimage verification + proof
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/pay.js`:
- Around line 65-72: The code only fetches the first page (limit: 500) from
client.listTransactions and so may miss outgoing transactions in the rolling
window; change the logic around client.listTransactions/windowTx to paginate
through all pages (use the response cursor/next token and keep calling
client.listTransactions with that cursor) accumulating transactions that match
from: windowStart, then compute settledInWindow from the full accumulated list
(filter by t.state === "settled" and sum t.amount / 1000) instead of using just
windowTx.transactions; reference: client.listTransactions, windowTx,
settledInWindow, windowStart.
- Around line 165-185: The budget check is vulnerable to a race because
checkBudget() only reads state and registerPending() persists the reservation
later; make this atomic by adding a single operation (either extend checkBudget
into checkAndReserve or create registerPendingAndCheck) that both verifies
budget and writes the pending reservation before returning success. Modify
callers in pay.js to call that new atomic function (passing paymentHash,
amountArg, bolt11, description) and rely on its returned
ok/reason/effective_remaining fields; on failure abort as before. Ensure the new
function uses the same underlying persistence/lock mechanism used by
registerPending (or a DB transaction/compare-and-swap) so two concurrent pay.js
runs cannot both pass the check.
- Around line 145-166: The code uses amountArg for budget, fiat conversion and
reservations even though the actual pay amount may differ in the decoded BOLT11;
after decoding the invoice produced by LightningAddress.requestInvoice or when
parsing a raw BOLT-11 (symbols: LightningAddress, ln.requestInvoice,
decodeBolt11, bolt11, amountArg), extract the invoice's satoshi amount from the
decoded invoice and treat it as the source of truth (e.g., assign a single
canonical amountSats = decoded invoice amount || amountArg fallback) and use
that amountSats for getFiatUsd, checkBudget, pending reservation, and any
output/receipt and before calling client.payInvoice so budget/reservation
reflect the real spend. Ensure you handle invoices without explicit amounts by
falling back to amountArg.
- Around line 200-218: The current logic only checks client.listTransactions({
limit: 20 }) for paymentHash and then calls clearPending(paymentHash) and exits,
which can falsely mark retries safe; update the code to paginate through
transactions (repeatedly call client.listTransactions with the returned
pagination cursor) or use a server-side lookup by payment_hash if available,
scanning until you either find a matching transaction with state/fields
(nodeTx.preimage, nodeTx.fees_paid) or exhaust the transaction history up to a
safe cutoff (e.g., paymentAttemptTimestamp minus a safety window or a larger
configurable max pages); only call clearPending(paymentHash) and process.exit(3)
when you have exhausted the paginated results or hit the cutoff and are
confident the payment truly doesn’t exist, otherwise leave the pending
reservation intact and return the “in-flight” or retry decision accordingly so
you avoid double-spend retries.
In `@scripts/reconcile.js`:
- Around line 77-90: The reconciliation currently treats a pending entry missing
from the recent client.listTransactions({ from: windowStart, limit: 500 }) slice
as sufficient to expire it, which can prematurely drop real settlements if the
agent was offline or results are paginated; update the reconcile logic that
iterates over pending (pending_payments.json) so that a missing tx in nodeTxs
does NOT immediately expire the pending entry — instead either (A) lookup the
specific transaction by id using the node client (e.g.,
client.getTransaction(txId) or equivalent) before marking it settled/expired, or
(B) page listTransactions backwards until the pending's created_at/timestamp is
older than the earliest fetched tx or until no more pages, and only then mark
expired; modify the code paths that reference windowStart,
client.listTransactions, nodeTxs and the pending-handling block to perform one
of these checks before removing entries from pending_payments.json.
- Around line 215-233: The double-spend detector currently groups all outgoing
attempts regardless of final status; update the collection step so only
settled/successful outgoing payments are considered (e.g., change the filter
used when building outgoingByInvoice from nodeTxs.filter(t => t.type ===
"outgoing") to include the success state check like t.state === "settled" or the
correct settled enum used in your codebase), so outgoingByInvoice, the
subsequent loop over [invoice, txs], and the results.double_spend_risk payload
only reflect actually settled payments.
🪄 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: d507345f-dbf5-4cab-8e3a-4331662ce373
📒 Files selected for processing (3)
scripts/budget_guardian.jsscripts/pay.jsscripts/reconcile.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/budget_guardian.js
Comment on lines
+65
to
+72
| const windowTx = await client.listTransactions({ | ||
| type: "outgoing", | ||
| from: windowStart, | ||
| limit: 500, | ||
| }); | ||
| const settledInWindow = (windowTx.transactions || []) | ||
| .filter(t => t.state === "settled") | ||
| .reduce((sum, t) => sum + t.amount / 1000, 0); |
There was a problem hiding this comment.
Page through the full rolling window before enforcing the cap.
Line 65 only asks the node for 500 outgoing transactions. Once the rolling window contains more than that, settledInWindow undercounts actual spend and the budget guard can approve sends that should be blocked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pay.js` around lines 65 - 72, The code only fetches the first page
(limit: 500) from client.listTransactions and so may miss outgoing transactions
in the rolling window; change the logic around client.listTransactions/windowTx
to paginate through all pages (use the response cursor/next token and keep
calling client.listTransactions with that cursor) accumulating transactions that
match from: windowStart, then compute settledInWindow from the full accumulated
list (filter by t.state === "settled" and sum t.amount / 1000) instead of using
just windowTx.transactions; reference: client.listTransactions, windowTx,
settledInWindow, windowStart.
Comment on lines
+145
to
+166
| if (recipient.toLowerCase().includes("@")) { | ||
| // Lightning address | ||
| const ln = new LightningAddress(recipient); | ||
| await ln.fetch(); | ||
| const inv = await ln.requestInvoice({ satoshi: amountArg }); | ||
| bolt11 = inv.paymentRequest; | ||
| decoded = decodeBolt11(bolt11); | ||
| paymentHash = decoded.sections.find(s => s.name === "payment_hash")?.value; | ||
| description = decoded.sections.find(s => s.name === "description")?.value || ""; | ||
| } else { | ||
| // Raw BOLT-11 invoice | ||
| bolt11 = recipient; | ||
| const decoded = decodeBolt11(bolt11); | ||
| paymentHash = decoded.sections.find(s => s.name === "payment_hash")?.value; | ||
| description = decoded.sections.find(s => s.name === "description")?.value || ""; | ||
| // Override amount from invoice if needed | ||
| } | ||
|
|
||
| const { usd } = await getFiatUsd(amountArg); | ||
|
|
||
| // 2. Budget check | ||
| const budgetCheck = await checkBudget(client, amountArg); |
There was a problem hiding this comment.
Use the decoded invoice amount as the source of truth for spend.
After either LightningAddress.requestInvoice() or raw BOLT-11 input, the amount that will actually be paid comes from bolt11, but Lines 163, 166, 185, and 245 still trust amountArg. A payee can return or supply an invoice for more sats than requested, and the budget check / pending reservation / output will all understate the real spend while client.payInvoice() pays the larger invoice amount.
Also applies to: 185-185, 245-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pay.js` around lines 145 - 166, The code uses amountArg for budget,
fiat conversion and reservations even though the actual pay amount may differ in
the decoded BOLT11; after decoding the invoice produced by
LightningAddress.requestInvoice or when parsing a raw BOLT-11 (symbols:
LightningAddress, ln.requestInvoice, decodeBolt11, bolt11, amountArg), extract
the invoice's satoshi amount from the decoded invoice and treat it as the source
of truth (e.g., assign a single canonical amountSats = decoded invoice amount ||
amountArg fallback) and use that amountSats for getFiatUsd, checkBudget, pending
reservation, and any output/receipt and before calling client.payInvoice so
budget/reservation reflect the real spend. Ensure you handle invoices without
explicit amounts by falling back to amountArg.
Comment on lines
+165
to
+185
| // 2. Budget check | ||
| const budgetCheck = await checkBudget(client, amountArg); | ||
| console.log(`\n💰 Payment Preview`); | ||
| console.log(`─────────────────────────`); | ||
| console.log(` Amount: ${amountArg.toLocaleString()} sats (~$${usd})`); | ||
| console.log(` To: ${recipient}`); | ||
| console.log(` Memo: ${description || "—"}`); | ||
| console.log(` Payment: ${paymentHash?.substring(0, 16)}...`); | ||
| console.log(`\n🛡️ Budget Guardian`); | ||
| console.log(`─────────────────────────`); | ||
| if (budgetCheck.ok) { | ||
| console.log(` ✅ Budget OK: ${budgetCheck.effective_remaining} sats remaining`); | ||
| console.log(` (Rolling ${budgetCheck.rolling_window_min}min window, ${budgetCheck.pending_count} pending)`); | ||
| } else { | ||
| console.log(` ❌ ${budgetCheck.reason}`); | ||
| client.close(); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // 3. Register pending (prevents double-spend if we crash mid-flight) | ||
| registerPending(paymentHash, amountArg, bolt11, description); |
There was a problem hiding this comment.
The budget guard is still subject to a check-then-send race.
Line 166 approves the spend, but the reservation is not persisted until Line 185. Two concurrent pay.js invocations can both pass the cap check against the same state and then both send, overshooting the configured limit. This needs one atomic “check + reserve” operation, not two separate steps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pay.js` around lines 165 - 185, The budget check is vulnerable to a
race because checkBudget() only reads state and registerPending() persists the
reservation later; make this atomic by adding a single operation (either extend
checkBudget into checkAndReserve or create registerPendingAndCheck) that both
verifies budget and writes the pending reservation before returning success.
Modify callers in pay.js to call that new atomic function (passing paymentHash,
amountArg, bolt11, description) and rely on its returned
ok/reason/effective_remaining fields; on failure abort as before. Ensure the new
function uses the same underlying persistence/lock mechanism used by
registerPending (or a DB transaction/compare-and-swap) so two concurrent pay.js
runs cannot both pass the check.
Comment on lines
+200
to
+218
| try { | ||
| const txs = await client.listTransactions({ limit: 20 }); | ||
| const nodeTx = txs.transactions.find(t => t.payment_hash === paymentHash); | ||
|
|
||
| if (nodeTx && nodeTx.state === "settled") { | ||
| result = { preimage: nodeTx.preimage, fees_paid: nodeTx.fees_paid }; | ||
| console.log(`✅ Payment was DELIVERED (confirmed via node reconciliation)`); | ||
| } else if (nodeTx && (nodeTx.state === "pending" || nodeTx.state === "in-flight")) { | ||
| console.log(`⏳ Payment is IN-FLIGHT on node. Will resolve on next reconciliation.`); | ||
| console.log(` Pending counter is already active — budget is protected.`); | ||
| client.close(); | ||
| process.exit(2); // special exit: in-flight | ||
| } else { | ||
| // Not on node → payment likely never went through, safe to retry | ||
| // Clear the pending entry | ||
| clearPending(paymentHash); | ||
| console.log(`❌ Payment NOT found on node. Safe to retry.`); | ||
| client.close(); | ||
| process.exit(3); // special exit: failed, safe to retry |
There was a problem hiding this comment.
Don't declare a retry safe from a 20-transaction sample.
Line 201 only inspects the latest 20 node transactions, then Lines 215-218 clear the reservation and tell callers the payment definitely did not happen. On a busy wallet or with delayed settlement visibility, the matching payment can fall outside that sample and a retry becomes a real double-spend.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pay.js` around lines 200 - 218, The current logic only checks
client.listTransactions({ limit: 20 }) for paymentHash and then calls
clearPending(paymentHash) and exits, which can falsely mark retries safe; update
the code to paginate through transactions (repeatedly call
client.listTransactions with the returned pagination cursor) or use a
server-side lookup by payment_hash if available, scanning until you either find
a matching transaction with state/fields (nodeTx.preimage, nodeTx.fees_paid) or
exhaust the transaction history up to a safe cutoff (e.g.,
paymentAttemptTimestamp minus a safety window or a larger configurable max
pages); only call clearPending(paymentHash) and process.exit(3) when you have
exhausted the paginated results or hit the cutoff and are confident the payment
truly doesn’t exist, otherwise leave the pending reservation intact and return
the “in-flight” or retry decision accordingly so you avoid double-spend retries.
Comment on lines
+77
to
+90
| const windowStart = now - RECON_WINDOW_SEC; | ||
|
|
||
| console.log(`\n🔍 Reconciliation window: last ${RECON_WINDOW_SEC}s`); | ||
| console.log(` Ledger entries: ${ledger.length}`); | ||
| console.log(` Pending payments: ${pending.length}`); | ||
| console.log(` Window start: ${new Date(windowStart * 1000).toISOString()}`); | ||
| console.log(` Window end: ${new Date(now * 1000).toISOString()}`); | ||
| console.log(""); | ||
|
|
||
| // Step 1: Fetch node's transaction history for the window | ||
| console.log("📡 Fetching transaction history from node..."); | ||
| let nodeTxs = []; | ||
| try { | ||
| const txs = await client.listTransactions({ from: windowStart, limit: 500 }); |
There was a problem hiding this comment.
Absence from the current fetch window is not enough to expire a pending payment.
Line 90 only loads transactions from the last RECON_WINDOW_SEC seconds, but Lines 200-205 and 254 eventually treat “not found in that slice” as enough to drop the pending entry. If the agent was offline longer than the window or the node paginates older results away, a real settlement can be removed from pending_payments.json and the budget guard gets released too early.
Also applies to: 184-206, 252-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/reconcile.js` around lines 77 - 90, The reconciliation currently
treats a pending entry missing from the recent client.listTransactions({ from:
windowStart, limit: 500 }) slice as sufficient to expire it, which can
prematurely drop real settlements if the agent was offline or results are
paginated; update the reconcile logic that iterates over pending
(pending_payments.json) so that a missing tx in nodeTxs does NOT immediately
expire the pending entry — instead either (A) lookup the specific transaction by
id using the node client (e.g., client.getTransaction(txId) or equivalent)
before marking it settled/expired, or (B) page listTransactions backwards until
the pending's created_at/timestamp is older than the earliest fetched tx or
until no more pages, and only then mark expired; modify the code paths that
reference windowStart, client.listTransactions, nodeTxs and the pending-handling
block to perform one of these checks before removing entries from
pending_payments.json.
Comment on lines
+215
to
+233
| const outgoingByInvoice = new Map(); | ||
| for (const tx of nodeTxs.filter(t => t.type === "outgoing")) { | ||
| if (!tx.invoice) continue; | ||
| if (!outgoingByInvoice.has(tx.invoice)) outgoingByInvoice.set(tx.invoice, []); | ||
| outgoingByInvoice.get(tx.invoice).push(tx); | ||
| } | ||
|
|
||
| for (const [invoice, txs] of outgoingByInvoice) { | ||
| if (txs.length > 1) { | ||
| const totalPaid = txs.reduce((sum, t) => sum + t.amount / 1000, 0); | ||
| results.double_spend_risk.push({ | ||
| invoice: invoice.substring(0, 50) + "...", | ||
| count: txs.length, | ||
| total_sats: totalPaid, | ||
| payment_hashes: txs.map(t => t.payment_hash), | ||
| }); | ||
| console.log(`\n🚨 DOUBLE-SPEND DETECTED: invoice paid ${txs.length}x (${totalPaid}sats total)`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Filter double-spend detection to settled payments.
This currently groups every outgoing attempt by invoice, regardless of state. A failed retry plus one successful payment will be reported as DOUBLE-SPEND DETECTED, even though only one payment actually settled.
Possible fix
- for (const tx of nodeTxs.filter(t => t.type === "outgoing")) {
+ for (const tx of nodeTxs.filter(t => t.type === "outgoing" && t.state === "settled")) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/reconcile.js` around lines 215 - 233, The double-spend detector
currently groups all outgoing attempts regardless of final status; update the
collection step so only settled/successful outgoing payments are considered
(e.g., change the filter used when building outgoingByInvoice from
nodeTxs.filter(t => t.type === "outgoing") to include the success state check
like t.state === "settled" or the correct settled enum used in your codebase),
so outgoingByInvoice, the subsequent loop over [invoice, txs], and the
results.double_spend_risk payload only reflect actually settled payments.
Member
|
Hi, have you seen https://github.com/getAlby/payments-skill ? this is more for giving an agent wallet access. The builder skill is specifically for teaching agents how to build apps. This also needs some planning and discussion about the intent before making a huge PR with many files. Otherwise our tools will become unmaintainable. |
Author
|
Noted on payments-skill and the need for upfront planning. I've built and tested this end-to-end against a live wallet—feel free to review and let me know how you'd like to handle it. Happy to split it, move it, or adapt it wherever it fits best for the project. |
…ity, add setup/run notes
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SKILL.md`:
- Line 15: The Markdown file SKILL.md contains fenced code blocks with missing
language identifiers causing MD040 lint warnings; update each triple-backtick
fence in SKILL.md (the generic code blocks shown in the diff) to include an
appropriate language tag (e.g., ```text for plain output blocks or ```bash for
shell snippets) so every ``` is replaced with a language-specified fence,
keeping the content unchanged.
- Line 112: The setup docs conflict about where package.json lives: the line
that reads "Run `npm install` (uses `package.json` in skill root)" and the later
lines referencing "clone root" must be unified; choose the authoritative path
(e.g., "skill root" or "repo/clone root"), then update both occurrences so they
use identical wording and include a clear command (e.g., "cd <chosen-root> &&
npm install") and a note to run the command from that directory; change the
lines containing the exact phrases "Run `npm install` (uses `package.json` in
skill root)" and the later "clone root" wording to match the chosen path.
- Line 37: The SKILL.md statement "Read URL only from
`~/.hermes/config_local.json` (chmod 600) — never ask user for it again"
conflicts with handling expired URLs; update SKILL.md to define one explicit
renewal policy by either (A) stating a secure re-provisioning flow (e.g., agents
must detect expired URL, remove/rotate the entry in
`~/.hermes/config_local.json`, and invoke an authenticated re-provisioning
CLI/UX flow) or (B) allowing a controlled re-prompting rule (e.g., only via an
interactive admin command and logged audit) — replace the ambiguous line and the
duplicate at the other occurrence with the chosen policy and include clear steps
for detection, authorization, and storage (mention the
`~/.hermes/config_local.json` file and the exact phrase to be removed/updated).
- Around line 118-126: The inline Python one-liners in SKILL.md that call
open('~/.hermes/config_local.json') will fail because tilde isn't expanded by
Python; update the export NWC_URL commands (used for scripts/auto_ledger.js and
scripts/balance.js invocations) to use os.path.expanduser (i.e., import os and
call open(os.path.expanduser('~/.hermes/config_local.json'))) or otherwise
resolve the full path, and also normalize any other occurrences that hardcode
/root/ to consistently use expanduser so the commands work for non-root users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| If they do not have a wallet yet [here are some options](./references/production-wallets.md) No newline at end of file | ||
| User → says anything natural → agent does everything → confirmation. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (MD040).
These fences are missing language tags and trigger markdownlint warnings.
Suggested cleanup
- ```
+ ```text
...
- ```
+ ```text
...
- ```
+ ```text
...
- ```
+ ```text
...
- ```
+ ```bashAlso applies to: 54-54, 61-61, 67-67, 103-103
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SKILL.md` at line 15, The Markdown file SKILL.md contains fenced code blocks
with missing language identifiers causing MD040 lint warnings; update each
triple-backtick fence in SKILL.md (the generic code blocks shown in the diff) to
include an appropriate language tag (e.g., ```text for plain output blocks or
```bash for shell snippets) so every ``` is replaced with a language-specified
fence, keeping the content unchanged.
| - NWC returns millisats → always divide by 1000 | ||
| - Always include fiat equivalent (USD default) | ||
| - NWC URL → never echo, never log, never display in any form | ||
| - Read URL only from `~/.hermes/config_local.json` (chmod 600) — never ask user for it again |
There was a problem hiding this comment.
Clarify expired-URL flow vs “never ask again” rule.
These lines conflict operationally. Define one explicit policy for renewal (e.g., secure re-provisioning flow) so agents don’t behave inconsistently.
Also applies to: 163-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SKILL.md` at line 37, The SKILL.md statement "Read URL only from
`~/.hermes/config_local.json` (chmod 600) — never ask user for it again"
conflicts with handling expired URLs; update SKILL.md to define one explicit
renewal policy by either (A) stating a secure re-provisioning flow (e.g., agents
must detect expired URL, remove/rotate the entry in
`~/.hermes/config_local.json`, and invoke an authenticated re-provisioning
CLI/UX flow) or (B) allowing a controlled re-prompting rule (e.g., only via an
interactive admin command and logged audit) — replace the ambiguous line and the
duplicate at the other occurrence with the chosen policy and include clear steps
for detection, authorization, and storage (mention the
`~/.hermes/config_local.json` file and the exact phrase to be removed/updated).
| ## Initial Setup | ||
|
|
||
| 1. `cd ~/.hermes/skills/alby-bitcoin-payments` | ||
| 2. Run `npm install` (uses `package.json` in skill root) |
There was a problem hiding this comment.
Setup instructions conflict on package.json location.
Line 112 says package.json is in skill root, while Line 148 says it may only exist in clone root. Please unify this into one authoritative setup path to avoid failed installs.
Also applies to: 148-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SKILL.md` at line 112, The setup docs conflict about where package.json
lives: the line that reads "Run `npm install` (uses `package.json` in skill
root)" and the later lines referencing "clone root" must be unified; choose the
authoritative path (e.g., "skill root" or "repo/clone root"), then update both
occurrences so they use identical wording and include a clear command (e.g., "cd
<chosen-root> && npm install") and a note to run the command from that
directory; change the lines containing the exact phrases "Run `npm install`
(uses `package.json` in skill root)" and the later "clone root" wording to match
the chosen path.
Comment on lines
+118
to
+126
| 5. Start auto-ledger: `export NWC_URL=$(python3 -c "import json; print(json.load(open('~/.hermes/config_local.json'))['wallet']['nwc_url'])") && node scripts/auto_ledger.js &` | ||
|
|
||
| ### Running Scripts | ||
|
|
||
| Always export NWC URL first (scripts read from env, not config file): | ||
| ```bash | ||
| cd ~/.hermes/skills/alby-bitcoin-payments | ||
| export NWC_URL=$(python3 -c "import json; print(json.load(open('~/.hermes/config_local.json'))['wallet']['nwc_url'])") | ||
| node scripts/balance.js |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python, does open("~/.hermes/config_local.json") expand "~" automatically, or is os.path.expanduser required?
💡 Result:
No. open("~/.hermes/config_local.json") does not expand ~—tilde expansion is a shell feature, not something open() applies. Use os.path.expanduser() (or pathlib.Path("~/.hermes/config_local.json").expanduser()) before opening. [1], [2]
🏁 Script executed:
# Check if SKILL.md exists and view the relevant lines
if [ -f "SKILL.md" ]; then
echo "=== Lines 115-130 of SKILL.md ==="
sed -n '115,130p' SKILL.md
echo ""
echo "=== Full context around line 118 and 125 ==="
sed -n '110,135p' SKILL.md
else
echo "SKILL.md not found, searching for it..."
find . -name "SKILL.md" -type f
fiRepository: getAlby/builder-skill
Length of output: 1790
🏁 Script executed:
# Search for any wrapper scripts or references to these commands
echo "=== Search for references to 'open' with config_local.json ==="
rg "config_local\.json" -A 2 -B 2
echo ""
echo "=== Search for shell scripts or Python files that might wrap these commands ==="
fd -e "sh" -e "py" -e "js" | head -20 | xargs -I {} rg -l "NWC_URL|nwc_url" {}
echo ""
echo "=== Check if there are any shell startup scripts or bashrc ==="
find . -name ".bashrc" -o -name ".bash_profile" -o -name "*.sh" -type f | head -10Repository: getAlby/builder-skill
Length of output: 3506
Fix open('~/.hermes/...') to expand tilde for portable path handling.
The Python open() function does not expand ~ automatically—tilde expansion is a shell feature. These commands will fail at runtime with FileNotFoundError. Additionally, the file contains inconsistent path handling (some use ~, others use hardcoded /root/). Use os.path.expanduser() for portable, user-agnostic paths.
Suggested fix
- export NWC_URL=$(python3 -c "import json; print(json.load(open('~/.hermes/config_local.json'))['wallet']['nwc_url'])") && node scripts/auto_ledger.js &
+ export NWC_URL=$(python3 -c "import json, os; p=os.path.expanduser('~/.hermes/config_local.json'); print(json.load(open(p))['wallet']['nwc_url'])") && node scripts/auto_ledger.js &
- export NWC_URL=$(python3 -c "import json; print(json.load(open('~/.hermes/config_local.json'))['wallet']['nwc_url'])")
+ export NWC_URL=$(python3 -c "import json, os; p=os.path.expanduser('~/.hermes/config_local.json'); print(json.load(open(p))['wallet']['nwc_url'])")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SKILL.md` around lines 118 - 126, The inline Python one-liners in SKILL.md
that call open('~/.hermes/config_local.json') will fail because tilde isn't
expanded by Python; update the export NWC_URL commands (used for
scripts/auto_ledger.js and scripts/balance.js invocations) to use
os.path.expanduser (i.e., import os and call
open(os.path.expanduser('~/.hermes/config_local.json'))) or otherwise resolve
the full path, and also normalize any other occurrences that hardcode /root/ to
consistently use expanduser so the commands work for non-root users.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR transforms the basic payment tutorial into a production-grade Lightning wallet skill for AI agents.
What is Added
New Scripts (16 scripts total)
Safety Architecture
The timeout / double-spend edge case:
If payInvoice() throws after the payment was routed on Lightning, the agent previously had no way to know whether it succeeded, creating a double-spend risk on retry. This is now resolved:
Pending counter — Before sending, the amount is registered in a pending file. Budget checks deduct
settled + pending. If the agent crashes mid-flight, the budget is still protected.Post-timeout reconciliation — After a send failure, the script:
Rolling window budgets — Instead of calendar-day caps, the budget checks the last N minutes (default 24h, configurable). This catches retry-loop exhaustion that a daily cap would miss.
Node-verified spending — Budget reads from the node's transaction history, not local state. A stale or deleted client-side budget file cannot bypass enforcement.
Enhanced SKILL.md
Package
package.jsonwith all dependenciesDocumentation
references/payment-validation.md— SHA-256 proof verification guideSafety
All outgoing payments require explicit user confirmation before execution. The budget guardian enforces configurable spending limits. Every transaction is verified cryptographically. The reconciliation engine prevents double-spend on timeout.
Testing
All 16 scripts have been tested individually and as an integrated system against a live NWC wallet.
Summary by CodeRabbit
New Features
Documentation