feat: Routstr — Hub-supervised AI gateway with auto top-up - #2490
feat: Routstr — Hub-supervised AI gateway with auto top-up#2490welliv wants to merge 28 commits into
Conversation
…trol RoutstrdService manages the routstrd daemon and cocod Cashu wallet as child processes: start with the Hub, restart on health failure (15s tick), graceful stop. Auto top-up refills the daemon Cashu wallet from the Routstr app's isolated wallet when its balance drops below a threshold (config in app metadata), with a control API: GET /api/routstrd/autorefill/status, POST /start and /stop. Hub-direct money rail: app-scoped invoices (CreateInvoice appId) and fromAppId payments, never the main wallet.
Backups now archive coco.db, routstr.db, their configs, and Bark state alongside the Hub DB. WAL checkpoint (TRUNCATE) before archiving; ../ restore entries resolve against $HOME; restore requires a process restart before unlock.
Routstr internal-app wizard (5 steps), connection page with API key management (Top Up, Refund, Delete, auto top-up Start/Stop), models browse, and a live app page: polling app data (3s) and transactions (10s), with a Total Spent/Received tally that reconciles with the isolated balance. Auto top-up card shows a live status line and warns on fee-dominated refill amounts or a Routstr wallet that cannot cover a refill.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR integrates Routstr into Hub with supervised daemons, app-scoped Cashu funding, auto-refill, backups, HTTP proxies, frontend setup and management flows, model browsing, refunds, deployment tooling, and supporting documentation. ChangesRoutstr integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
The refund dialog only read the daemon Cashu wallet balance, so prepaid provider tokens (the apikey:* entries in /keys/balance) were invisible and unrefundable from the dialog: the card showed 204.07 sats while the dialog showed 165. The dialog now shows wallet + provider tokens (with a split note) and the refund action first reclaims provider tokens via the daemon /refund, then re-reads the wallet and melts the full amount.
Regenerate the setup wizard (5 steps), connection key section, and all four dialogs from the live app. Screenshots are plain captures with no arrows or illustrations. The refund dialog now shows the full refundable balance (wallet + prepaid provider tokens, reclaimed on refund) and the delete dialog shows the balance warning.
readAutoRefillConfig returned nil when the routstr.autoRefill metadata block was missing, so the status endpoint reported threshold 0 / amount 0 while the card inputs showed the defaults (500/1000) and Start used them. Now the server reports the same defaults Start will use, so an unconfigured app shows its true behavior. Also documents the app-deletion hazard: the config and the isolated wallet are owned by the app and die with it (no refund, no on-disk backup); re-enable via Start after any cleanup.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (17)
frontend/src/components/connections/routstr/RefundDialog.tsx-87-117 (1)
87-117: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid creating a Hub invoice on every dialog open.
Each call to
doLoadBalancescreates a real app-scoped invoice through/api/invoicesonly to read the mintfee_reserve. The dialog callsdoLoadBalanceson every open, and it calls it again in the background throughloadBalancesSilent. The unpaid invoices stay in the Routstr app transaction list and grow with each open. Two options: cache the fee quote for the dialog lifetime, or expose the melt-quote fee through the daemon proxy so no Hub invoice is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/RefundDialog.tsx` around lines 87 - 117, Update the fee calculation in doLoadBalances to avoid creating a new app-scoped invoice on every dialog open or silent refresh. Prefer caching the retrieved fee quote for the dialog lifetime and reusing it across loadBalancesSilent calls; otherwise obtain fee_reserve through an existing daemon proxy without creating a Hub invoice. Keep the current fallback of zero when no quote is available.frontend/src/components/connections/routstr/CreateKeyDialog.tsx-75-96 (1)
75-96: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCreate the API key before you fund the wallet.
This handler funds the Cashu wallet first, then creates the key. If
createRoutstrdClientfails, the sats are already spent from the app wallet and no key exists.RoutstrApiKeySection.handleCreateKeyinfrontend/src/components/connections/routstr/ApiKeySection.tsx(lines 471-499) documents the opposite order for exactly this reason. Align the two flows: create the key first, then fund.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/CreateKeyDialog.tsx` around lines 75 - 96, Reorder handleCreateKey so createRoutstrdClient and its API-key validation complete before any funding attempt. Preserve the existing client-name, created-key, and client-ID handling, then set the paying step and call fundFromHub only after key creation succeeds, while retaining the current success completion flow.frontend/src/hooks/useRoutstrd.ts-306-318 (1)
306-318: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not hardcode the mint URL in
refundFromHub.The melt request always targets
https://mint.cubabitcoin.org. If the daemon wallet holds proofs at another mint, the melt fails or targets the wrong mint.getRoutstrdBalance()already returnsactiveMint, andRefundDialogresolves the mint before the refund. Pass the mint through as a parameter.♻️ Proposed change
export async function refundFromHub( amount: number, - appId: number + appId: number, + mintUrl: string ): Promise<number> { if (!appId) { throw new Error("refundFromHub requires the Routstr appId"); } + if (!mintUrl) { + throw new Error("refundFromHub requires the active mint URL"); + } @@ body: JSON.stringify({ invoice, - mintUrl: "https://mint.cubabitcoin.org", + mintUrl, }),Update the caller in
frontend/src/components/connections/routstr/RefundDialog.tsxto pass the resolvedmintUrl.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useRoutstrd.ts` around lines 306 - 318, Update refundFromHub in useRoutstrd.ts to accept a mintUrl parameter and use it for the /wallet/send/bolt11 melt request instead of the hardcoded URL. Update RefundDialog to pass the resolved mintUrl when invoking refundFromHub, preserving the existing refund flow.service/routstrd.go-301-312 (1)
301-312: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick wincocod stale socket and pid files are never reconciled, so a crashed cocod never self-heals. Both sites assume the socket file implies a live daemon and that a leftover pid file needs no cleanup.
docs/troubleshooting.mddocuments the manual recovery for exactly this state:pkill -f cocodthenrm -f cocod.sock cocod.pid. The health check reports the dead daemon as healthy, so supervision never respawns it; and if the health check is corrected, the respawn then collides with the stale socket.
service/routstrd.go#L301-L312: returnr.processExists(pid)when the pid file parses, so a parsed-but-dead pid reports unhealthy. Keep the permissivereturn trueonly for an absent or unreadable pid file.service/routstrd.go#L315-L341: beforecmd.Start(), when the recorded pid is dead, remove~/.cocod/cocod.sockand~/.cocod/cocod.pidso the new daemon can bind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/routstrd.go` around lines 301 - 312, The cocod health check and restart flow must reconcile stale daemon files. In service/routstrd.go lines 301-312, update the health-check logic to return r.processExists(pid) for a successfully parsed PID, while retaining the permissive true result only when the PID file is absent or unreadable. In service/routstrd.go lines 315-341, before cmd.Start(), detect a recorded dead PID and remove ~/.cocod/cocod.sock and ~/.cocod/cocod.pid so the replacement daemon can bind; update the relevant restart logic without changing unrelated startup behavior.docs/security.md-25-27 (1)
25-27: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftThe documented backup KDF is weak for the secrets this PR adds to the archive.
PBKDF2 with 4096 iterations and an 8-byte salt is far below current guidance for password-derived keys. AES-256-OFB also provides confidentiality only, with no integrity check, so a tampered archive is undetectable at decrypt time.
This PR widens what the archive holds. Line 21 of this file states that the cocod config carries the Cashu mnemonic in plaintext, and the PR summary adds the Routstr daemon databases and Bark state to the backup. The value protected by this KDF goes up while the KDF stays the same.
Raising the iteration count or moving to an AEAD is a format change with migration cost, so it may not belong in this PR. At minimum, record the limitation here and state the mitigation: the archive password must be high entropy, because the KDF gives little brute-force resistance on its own.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/security.md` around lines 25 - 27, Update the “Backup crypto” documentation to explicitly record that PBKDF2 with 4096 iterations and an 8-byte salt, combined with unauthenticated AES-256-OFB, provides weak brute-force resistance and no tamper detection. State that the archive password must therefore be high entropy, while preserving the existing format description and linked backup documentation.service/routstrd.go-584-587 (1)
584-587: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not put the raw daemon response body into a published event property.
reconnectNwcembeds up to 2048 bytes of the/nwc/connectresponse body in the returned error.ensureNwcConnectedthen publishes that error text as theerrorproperty of theroutstrd_nwc_reconnect_failedevent at Lines 494-499. Event properties leave the Hub through the event publisher. A daemon that echoes the submitted NWC connection string, or any part of it, in an error response would leak a wallet secret into telemetry.Return the status code and a short, fixed reason for the event property. Keep the body for local debug logging only.
🔒 Proposed fix
if resp.StatusCode < 200 || resp.StatusCode >= 300 { b, _ := io.ReadAll(io.LimitReader(resp.Body, 2048)) - return fmt.Errorf("nwc/connect status %d: %s", resp.StatusCode, string(b)) + logger.Logger.WithFields(logrus.Fields{ + "status": resp.StatusCode, + "body": string(b), + }).Debug("routstrd nwc/connect rejected the request") + return fmt.Errorf("nwc/connect returned status %d", resp.StatusCode) }Based on coding guidelines: "Never log sensitive data such as seeds, macaroons, or tokens".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/routstrd.go` around lines 584 - 587, Update reconnectNwc’s non-2xx response handling so the returned error contains only the HTTP status code and a short fixed reason, never the response body; retain the limited body solely for local debug logging without publishing it. Ensure ensureNwcConnected’s routstrd_nwc_reconnect_failed error property receives only the sanitized error.Source: Coding guidelines
docs/development.md-57-62 (1)
57-62: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThe documented systemd unit runs the Hub as root.
The unit sets
User=rootandWorkingDirectory=/root/hub. Readers copy this block verbatim. The Hub holds a Lightning wallet, spawns two daemons, and serves HTTP, so running it as root gives any compromise of that surface full host control.docs/security.mdin this same PR treats the deployment as security-relevant and states the firewall requirement as a hard requirement, so this recommendation works against that stance.Document a dedicated unprivileged user, and add the standard hardening directives.
🔒 Proposed doc change
[Service] Type=simple -User=root -WorkingDirectory=/root/hub -Environment=PATH=/root/.bun/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin -ExecStart=/root/hub/hub +User=albyhub +Group=albyhub +WorkingDirectory=/opt/albyhub +Environment=PATH=/opt/albyhub/.bun/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin +ExecStart=/opt/albyhub/hub +NoNewPrivileges=true +PrivateTmp=true +ProtectSystem=strict +ProtectHome=true +ReadWritePaths=/opt/albyhub Restart=alwaysThe bun bin directory and the
HOMEof the service user must match the paths thatresolveBinaryinservice/routstrd.goprobes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/development.md` around lines 57 - 62, Update the systemd unit example in the development documentation to use a dedicated unprivileged service user and its home/work directory instead of root, while preserving paths compatible with resolveBinary in service/routstrd.go. Add standard systemd hardening directives to the unit so the Hub service has reduced host access.service/routstrd.go-99-103 (1)
99-103: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
cancelFnwith the mutex.
Startwritesr.cancelFnat Line 103 outsider.mu.Stopreads it at Line 126 outsider.mu.Startruns fromStartAppandStopruns from the shutdown path, so the write and the read can happen on different goroutines. This is a data race on the field, and a concurrentStopcan read a staleniland never cancel the supervision goroutine.Assign
cancelFnwhile the lock is held, and read it under the same lock inStop.🔒 Proposed fix
r.mu.Lock() if r.running { r.mu.Unlock() return fmt.Errorf("routstrd service already running") } r.running = true - r.mu.Unlock() - childCtx, cancel := context.WithCancel(ctx) r.cancelFn = cancel + r.mu.Unlock()Then in
Stop:r.mu.Lock() if !r.running { r.mu.Unlock() return } r.running = false cancel := r.cancelFn r.mu.Unlock() if cancel != nil { cancel() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/routstrd.go` around lines 99 - 103, Protect the cancel function with r.mu in both lifecycle methods: assign r.cancelFn while the lock is held in Start, and in Stop read it into a local variable under the same lock before unlocking and invoking it. Preserve the existing running-state check and cancellation behavior while ensuring concurrent Start and Stop cannot race or observe a stale nil.service/routstrd.go-770-776 (1)
770-776: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not run the immediate refill check on the request thread.
SetAutoRefillEnabledis the handler-facing start/stop entry point. Whenenabledis true it callsr.checkAutoRefill(ctx)inline. That call creates a mint invoice with a 30 second timeout and then callsSendPaymentSync, which waits for a Lightning payment to settle. The HTTP request that pressed Start blocks for the whole duration, and the duration has no Hub-side bound.Run the immediate check in a goroutine and return the current status right away. The UI already polls the status endpoint every 30 seconds, per
docs/user-flow.md, so the result still appears.🔧 Proposed fix
if enabled { logger.Logger.Info("auto-refill: started (immediate check)") - r.checkAutoRefill(ctx) + // The check can wait on a Lightning payment; never block the request. + go r.checkAutoRefill(ctx) } else { logger.Logger.Info("auto-refill: stopped") }Note:
ctxis the request context. If the handler cancels it on response, pass the service context instead so the detached check is not cancelled immediately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/routstrd.go` around lines 770 - 776, Update SetAutoRefillEnabled so the enabled branch launches checkAutoRefill asynchronously instead of running it on the request thread, and return the current status immediately. Use the service-owned context rather than the request ctx for the detached goroutine so it remains active after the handler returns; keep the stopped behavior unchanged.service/routstrd.go-781-801 (1)
781-801: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe metadata read-modify-write can lose concurrent UI edits.
writeAutoRefillConfigreadsapp.Metadatafrom an in-memory struct, merges, and writes the whole column back with oneUpdate. There is no transaction and no version check.docs/reference.mdanddocs/development.mdboth state that the metadata PATCH replaces the whole object, and that the UI performs its own read-modify-write against/api/v2/apps/:id.So if a user saves a new
thresholdoramountin the UI whileSetAutoRefillEnabledwrites theenabledflag, one of the two writes is silently discarded. On this path a lost write means the refill amount or threshold reverts without any user-visible signal.Wrap the read and the write in a single transaction, and re-read the row inside that transaction instead of trusting the cached
app.Metadata.🔒 Proposed fix
func (r *RoutstrdService) writeAutoRefillConfig(app *db.App, cfg *AutoRefillConfig) error { - var meta map[string]interface{} - if err := json.Unmarshal(app.Metadata, &meta); err != nil { - return fmt.Errorf("read app metadata: %w", err) - } - routstrMeta, _ := meta["routstr"].(map[string]interface{}) - if routstrMeta == nil { - routstrMeta = map[string]interface{}{} - } - routstrMeta["autoRefill"] = cfg - meta["routstr"] = routstrMeta - bytes, err := json.Marshal(meta) - if err != nil { - return err - } - if err := r.svc.GetDB().Model(&db.App{}).Where("id = ?", app.ID).Update("metadata", datatypes.JSON(bytes)).Error; err != nil { - return fmt.Errorf("write app metadata: %w", err) - } - app.Metadata = bytes - return nil + var updated []byte + err := r.svc.GetDB().Transaction(func(tx *gorm.DB) error { + // Re-read inside the transaction so a concurrent UI PATCH is not lost. + var current db.App + if err := tx.Clauses(clause.Locking{Strength: "UPDATE"}). + First(¤t, app.ID).Error; err != nil { + return fmt.Errorf("read app: %w", err) + } + var meta map[string]interface{} + if len(current.Metadata) > 0 { + if err := json.Unmarshal(current.Metadata, &meta); err != nil { + return fmt.Errorf("read app metadata: %w", err) + } + } + if meta == nil { + meta = map[string]interface{}{} + } + routstrMeta, _ := meta["routstr"].(map[string]interface{}) + if routstrMeta == nil { + routstrMeta = map[string]interface{}{} + } + routstrMeta["autoRefill"] = cfg + meta["routstr"] = routstrMeta + b, err := json.Marshal(meta) + if err != nil { + return fmt.Errorf("marshal app metadata: %w", err) + } + if err := tx.Model(&db.App{}).Where("id = ?", app.ID). + Update("metadata", datatypes.JSON(b)).Error; err != nil { + return fmt.Errorf("write app metadata: %w", err) + } + updated = b + return nil + }) + if err != nil { + return err + } + app.Metadata = updated + return nil }Note: SQLite does not support
SELECT ... FOR UPDATE. If the deployment is SQLite only, drop the locking clause; the transaction plus in-transaction re-read still closes the window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/routstrd.go` around lines 781 - 801, Update writeAutoRefillConfig to perform the metadata read-modify-write inside a single database transaction, reloading the app row and metadata within that transaction instead of using cached app.Metadata. Apply the autoRefill merge to the freshly read metadata, persist it transactionally, and preserve the in-memory app.Metadata update only after a successful commit; use row locking where supported, but omit SELECT FOR UPDATE for SQLite.README.md-44-49 (1)
44-49: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRequire TLS for external clients.
Line [47] publishes
http://<hub-address>:8080/routstr/v1as the external-device URL. An API key and request data sent over this URL are plaintext outside a trusted host or private network. Publish an HTTPS endpoint through a reverse proxy, or clearly restrict this URL to trusted networks and document the required TLS setup before internet exposure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 44 - 49, Update the external-device endpoint documentation near the “External device” URL to require HTTPS before internet exposure. Replace the published HTTP example with an HTTPS reverse-proxy endpoint, or explicitly restrict the HTTP URL to trusted private networks and document the required TLS setup for external clients; keep the localhost URL unchanged.docs/deploy.md-28-34 (1)
28-34: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUse one reproducible patched daemon artifact.
docs/deploy.mdinstalls an unpinned global package, whiledocs/daemon-patches.mdrequires manual edits to its compiled bundle. Together, these steps allow a fresh deployment to run an incompatible or unpatched daemon. Pin the exact version, apply patches in a reproducible build, verify expected symbols or checksums, and abort before starting Hub when validation fails.
docs/deploy.md#L28-L34: install an exactroutstrdversion from a lockfile or verified artifact.docs/daemon-patches.md#L3-L25: replace manual post-install edits with reproducible patching and fail-closed verification.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/deploy.md` around lines 28 - 34, Update docs/deploy.md lines 28-34 to install an exact routstrd version from a lockfile or verified artifact instead of an unpinned global package. Update docs/daemon-patches.md lines 3-25 to describe reproducible patch application, verify the expected symbols or checksums, and fail closed before starting Hub when validation fails; ensure both documents consistently reference one patched daemon artifact.docs/architecture.md-79-79 (1)
79-79: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake Routstr app deletion refund-or-block.
Both documents record that deleting Routstr app metadata can strand wallet sats. Reclaim provider tokens and the Cashu balance before deletion, or reject deletion while funds remain.
docs/architecture.md#L79-L79: implement the refund-or-block lifecycle instead of documenting stranded funds as an expected hazard.CHANGELOG.md#L39-L39: update the changelog after deletion no longer causes irreversible fund loss.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture.md` at line 79, The Routstr app deletion flow must refund or block when wallet funds remain instead of allowing stranded balances. Update the deletion implementation behind DeleteApp and the Apps Cleanup/app-page Delete paths to reclaim provider tokens and Cashu balance before removal, or reject deletion while funds remain; then revise docs/architecture.md at line 79 to describe the enforced lifecycle and update CHANGELOG.md at line 39 to record the fix.docs/backup-restore.md-39-41 (1)
39-41: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftReplace the unauthenticated backup format.
The backup uses PBKDF2 with an 8-byte salt and 4096 iterations, then AES-256-OFB. OFB does not provide integrity, so backed-up ZIP content can be modified without decryption failure. Use a versioned format with a modern KDF and AEAD, and keep a migration path for existing backups.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/backup-restore.md` around lines 39 - 41, Replace the documented backup cryptography with a versioned format using a modern KDF and AEAD encryption, including sufficiently strong salt and nonce parameters. Update backup restore handling to recognize the new version while retaining decryption support for existing PBKDF2/AES-256-OFB backups so migration remains possible.api/backup.go-92-106 (1)
92-106: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winComment is inaccurate: cocod is not stopped, so it is not idle during the checkpoint.
This comment says "the app is already stopped at this point, so the daemons are idle." But
RoutstrdService.Stop()explicitly does not stop cocod ("do not stop cocod (wallet)"); only routstrd is gracefully stopped.coco.dbcan still be actively written by the live cocod process whilecheckpointSqliteDatabaseruns against it. See the related comment oncheckpointSqliteDatabase(lines 388-425) for the correctness consequence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/backup.go` around lines 92 - 106, Update the comment above the daemon database checkpoint loop to accurately state that routstrd is stopped but cocod remains running and may write to coco.db during checkpointing; remove the claim that both daemons are idle and describe the checkpoint as handling live cocod activity.api/backup.go-257-274 (1)
257-274: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRestore path reconstruction assumes
workDirsits directly under$HOME.This strips every leading
..segment from the zip entry name and joins the remainder to$HOME. That only reconstructs the original absolute path correctly when the common ancestor ofworkDirand the archived file (e.g.~/.cocod/coco.db) is exactly$HOME. IfworkDirlives outside the$HOMEtree (e.g. a container withworkDir=/data/hubandHOME=/root),filepath.Rel(workDir, fileToArchive)can produce a relative path whose remainder (after stripping..) is not directly relative to$HOME, and the restored file lands in the wrong place — silently reproducing the exact failure this code's own comment warns about ("the daemons would never find them, leaving a restored instance with an empty Cashu wallet and no API keys").Consider anchoring daemon files to an explicit, unambiguous prefix at archive time (e.g.
home/.cocod/coco.db) instead of relying onfilepath.Rel(workDir, ...)plus heuristic..-stripping at restore time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/backup.go` around lines 257 - 274, Replace the heuristic path reconstruction in the restore flow around zipName and fsFilePath with an explicit archive prefix for daemon files. Update the archive-path generation logic to store files under an unambiguous home-relative prefix such as home/.cocod or home/.routstrd, then resolve that prefix against $HOME during restore while keeping ordinary workDir-relative entries under workDir/restore. Remove reliance on stripping leading .. segments so restoration works when workDir is outside $HOME.api/backup.go-388-425 (1)
388-425: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheckpoint retry loop does not check the
busyresult, so a blocked checkpoint reports false success.
PRAGMA wal_checkpoint(TRUNCATE)returns a row(busy, log, checkpointed)without a SQL error even when the checkpoint is blocked by an active reader/writer;busyis set to1in that case. This function only retries on a Go/SQL error fromScan, so abusy=1result is logged as informational and the function returnsnil(success) on the first attempt, without actually truncating the WAL.Since cocod is not stopped before this runs (see lines 92-106), this is not a hypothetical: an active cocod writer can leave
coco.db's WAL unmerged while the function reports success, and the backup can miss recent wallet state.Retry (or fail) when
busy != 0, not only on query errors.🐛 Suggested fix to check the busy flag
var lastErr error for attempt := 0; attempt < 3; attempt++ { var busy, logPages, checkpointedPages int err := sqlDb.QueryRow("PRAGMA wal_checkpoint(TRUNCATE)").Scan(&busy, &logPages, &checkpointedPages) if err != nil { lastErr = err time.Sleep(time.Duration(attempt+1) * 500 * time.Millisecond) continue } + if busy != 0 { + lastErr = fmt.Errorf("checkpoint blocked by concurrent reader/writer (busy=%d)", busy) + time.Sleep(time.Duration(attempt+1) * 500 * time.Millisecond) + continue + } logger.Logger.WithFields(map[string]interface{}{ "db": dbPath, "busy": busy, "log_pages": logPages, "checkpointed": checkpointedPages, }).Info("Checkpointed daemon database before backup") return nil } return fmt.Errorf("wal checkpoint failed after retries: %w", lastErr)As per SQLite's own documentation: "The first column is usually 0 but will be 1 if a RESTART or FULL or TRUNCATE checkpoint was blocked from completing, for example because another thread or process was actively using the database."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/backup.go` around lines 388 - 425, Update checkpointSqliteDatabase to treat a successful PRAGMA wal_checkpoint(TRUNCATE) result with busy != 0 as an incomplete checkpoint: record an appropriate retry error, wait, and continue the existing retry loop instead of logging success and returning nil. Only log success and return nil when busy is zero; preserve the existing handling for SQL and Scan errors.
🟡 Minor comments (10)
frontend/src/components/connections/routstr/ModelSelect.tsx-757-759 (1)
757-759: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the
⌘Jhint or implement the shortcut.The trigger shows a
⌘Jkeyboard badge, but no handler listens for that key combination. The badge tells the user about a shortcut that does nothing. Either add a global key listener that callssetOpen(true), or remove the badge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/ModelSelect.tsx` around lines 757 - 759, Remove the misleading ⌘J keyboard badge from the trigger in ModelSelect, unless an existing shortcut flow can be updated to handle the combination by calling setOpen(true). Keep the trigger’s remaining UI and behavior unchanged.frontend/src/components/connections/routstr/TopUpDialog.tsx-36-41 (1)
36-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the amount as a positive integer.
numAmount < 1does not rejectNaN(NaN < 1isfalse) or fractional values. Sats are an integer unit, so a fractional amount is not valid for this flow. The same gap exists in thedisabledcondition on the deposit button.Add an explicit integer check.
🛡️ Proposed fix for amount validation
const handleTopUp = async () => { const numAmount = Number(amount); - if (numAmount < 1) { + if (!Number.isInteger(numAmount) || numAmount < 1) { toast.error("Enter a valid amount"); return; }<LoadingButton loading={isProcessing} onClick={handleTopUp} - disabled={!amount || Number(amount) < 1} + disabled={ + !amount || + !Number.isInteger(Number(amount)) || + Number(amount) < 1 + } >Also applies to: 89-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/TopUpDialog.tsx` around lines 36 - 41, Update handleTopUp to require numAmount to be a finite positive integer, rejecting NaN and fractional values before proceeding. Apply the same explicit integer validation in the deposit button’s disabled condition so its enabled state matches the submission validation.frontend/src/components/connections/routstr/ModelDetailPanel.tsx-44-49 (1)
44-49: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep cheapest provider pricing selection consistent with
ModelPricingStrip.
useRoutstrd.tsreturns providers without sorting, whileModelPricingStrip.tsxusesproviders[0]only after sorting byprompt + completion. Apply the same sort inModelDetailPanel.tsxbefore readingprovs[0].pricing, or change this hook to guarantee cheapest-first ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/ModelDetailPanel.tsx` around lines 44 - 49, Update the provider pricing selection in ModelDetailPanel so the providers are sorted by the combined prompt and completion pricing before reading the first provider’s pricing, matching ModelPricingStrip’s behavior. Apply this ordering at the point where provs[0].pricing is selected, or reuse a hook-level guarantee if one is introduced.docs/reference.md-7-19 (1)
7-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe daemon endpoint table omits five endpoints the Hub calls.
service/routstrd.goin this PR depends on daemon endpoints that this table does not list:
GET /wallet/balance—getCashuWalletBalance, Line 893.GET /nwc/status—checkNwcConnected, Line 513.POST /nwc/connect—reconnectNwc, Line 579.POST /stop—stopRoutstrd, Line 412, andStop, Line 132.POST /refund— documented indocs/user-flow.mdLine 84.
docs/upstream.mddirects readers here when merging upstream, so an incomplete contract table hides real breakage risk. Add the missing rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference.md` around lines 7 - 19, Update the endpoint table in docs/reference.md to include GET /wallet/balance, GET /nwc/status, POST /nwc/connect, POST /stop, and POST /refund, with concise purposes consistent with the existing rows. Preserve the current table entries and formatting.service/routstrd.go-815-850 (1)
815-850: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
findRoutstrAppscans every app row, and theconfiguredbranch is unreachable.Two issues in one function:
Find(&apps)loads every app row and JSON-decodes eachMetadatablob in Go on every supervision tick (15 seconds) and on every status poll (30 seconds). The cost grows with the total app count, and only Routstr apps are relevant. Filter on the metadata in SQL, or cache the resolved app ID.
readAutoRefillConfigreturns a non-nil default config whenever theroutstrmetadata block exists, even when theautoRefillblock is absent. So for any Routstr app thecfg == nilcontinue at Line 835 never fires, andconfiguredis always set to the same app asfallback. The doc comment at Lines 811-814 promises a preference for "the one with any autoRefill config block", but that distinction does not exist at runtime.Make the intent explicit. If the preference matters, have
readAutoRefillConfigreport whether theautoRefillblock was present, separately from the resolved values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/routstrd.go` around lines 815 - 850, Update findRoutstrApp to query only apps whose Metadata identifies them as the Routstr app instead of loading and decoding every app row on each call. Preserve the fallback selection, and change readAutoRefillConfig to distinguish a present autoRefill block from its default resolved configuration so configured is set only when that block exists; retain the preference for an enabled, valid configuration.docs/user-flow.md-74-77 (1)
74-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe unescaped pipe in
start|stopbreaks the table row.The table header declares three columns. Line 76 produces four cells, because GitHub Flavored Markdown splits cells on
|before it parses code spans. The pipe inside`POST /api/routstrd/autorefill/start|stop`therefore ends the cell early, and the rest of the sentence is dropped from the rendered table. markdownlint reports this as MD056.Escape the pipe as
\|.📝 Proposed fix
-| Start / Stop | Turn auto top-up on or off | The Hub supervision loop (15s tick) reads the `autoRefill` config from app metadata: when the balance drops below the threshold, it funds the Cashu wallet from the Routstr wallet, with a 5-minute cooldown. The buttons hit `POST /api/routstrd/autorefill/start|stop`; the status line shows the live pool balance, last refill, and errors (polled every 30s). | +| Start / Stop | Turn auto top-up on or off | The Hub supervision loop (15s tick) reads the `autoRefill` config from app metadata: when the balance drops below the threshold, it funds the Cashu wallet from the Routstr wallet, with a 5-minute cooldown. The buttons hit `POST /api/routstrd/autorefill/start` and `POST /api/routstrd/autorefill/stop`; the status line shows the live pool balance, last refill, and errors (polled every 30s). |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/user-flow.md` around lines 74 - 77, Escape the pipe separator in the `POST /api/routstrd/autorefill/start|stop` endpoint text within the Start / Stop table row as `\|`, preserving the endpoint wording while keeping the row’s three-column Markdown structure intact.Source: Linters/SAST tools
docs/troubleshooting.md-55-61 (1)
55-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLine 60 is prose inside a shell block.
start one cocod daemonis not a command. The block is a recovery procedure, so a reader pastes it whole. That line then fails, or runs an unrelatedstartbinary if one exists on thePATH.Make the last step an actual command or a comment.
📝 Proposed fix
pkill -f cocod # kill ALL cocod daemons rm -f cocod.sock cocod.pid # clear stuck pending mint operations from coco.db: # DELETE FROM coco_cashu_mint_operations WHERE state='pending'; -start one cocod daemon +cocod daemon # start exactly one cocod daemonThe socket and pid paths are
~/.cocod/cocod.sockand~/.cocod/cocod.pidinservice/routstrd.go. Consider using the full paths so the commands work from any directory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/troubleshooting.md` around lines 55 - 61, Replace the prose line “start one cocod daemon” in the shell recovery block with either a valid cocod startup command or a shell comment, and update the cleanup paths to the configured ~/.cocod/cocod.sock and ~/.cocod/cocod.pid locations so the procedure works from any directory.docs/architecture.md-28-35 (1)
28-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language identifier to the request-lifecycle code block.
The Markdown linter reports the fence at Line [28] without a language. Use
textfor this ASCII flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture.md` around lines 28 - 35, Add the text language identifier to the Markdown fence enclosing the request-lifecycle ASCII flow in the architecture documentation, preserving the diagram content unchanged.Source: Linters/SAST tools
CONTRIBUTING.md-14-17 (1)
14-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the frontend build in CONTRIBUTING.md setup.
cmd/http/main.goembeds thefrontend/distfolder, andvite buildcreates that output frombuild:http. Running onlyyarn installhere keeps contributing/quick-build instructions out of sync withREADME.md;runyarn build:httpbeforego build, or document a step that replaces the generatedfrontend/distassets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` around lines 14 - 17, Update the CONTRIBUTING.md setup commands to run the frontend build task yarn build:http after yarn install and before go build, ensuring the embedded frontend/dist assets are generated and the quick-build instructions match the required build flow.deploy.sh-22-27 (1)
22-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEscape
$UNLOCK_PASSWORDbefore embedding it in JSON.The password is interpolated directly into a hand-built JSON string. If it contains
"or\, the request body becomes malformed JSON and the unlock step fails unpredictably. Build the JSON payload with a tool that escapes special characters.🔧 Suggested fix using jq
-TOKEN=$(curl -s -m 15 -X POST http://localhost:8080/api/start \ - -H "Content-Type: application/json" \ - -d "{\"unlockPassword\":\"$UNLOCK_PASSWORD\"}" \ - | python3 -c "import sys,json; print(json.load(sys.stdin).get('token',''))" 2>/dev/null) +TOKEN=$(curl -s -m 15 -X POST http://localhost:8080/api/start \ + -H "Content-Type: application/json" \ + -d "$(jq -n --arg pw "$UNLOCK_PASSWORD" '{unlockPassword:$pw}')" \ + | python3 -c "import sys,json; print(json.load(sys.stdin).get('token',''))" 2>/dev/null)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy.sh` around lines 22 - 27, Update the TOKEN acquisition command in deploy.sh to construct the unlock request payload with a JSON-aware tool such as jq, passing UNLOCK_PASSWORD as a value so quotes, backslashes, and other special characters are escaped correctly. Preserve the existing POST endpoint, token extraction, and failure handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3665d09-fca4-4ebd-8431-4fb542231cca
⛔ Files ignored due to path filters (12)
docs/images/conn-autotopup.pngis excluded by!**/*.pngdocs/images/conn-keysection.pngis excluded by!**/*.pngdocs/images/dialog-delete.pngis excluded by!**/*.pngdocs/images/dialog-models.pngis excluded by!**/*.pngdocs/images/dialog-refund.pngis excluded by!**/*.pngdocs/images/dialog-topup.pngis excluded by!**/*.pngdocs/images/wizard-1-configure.pngis excluded by!**/*.pngdocs/images/wizard-2-topup.pngis excluded by!**/*.pngdocs/images/wizard-3-createkey.pngis excluded by!**/*.pngdocs/images/wizard-4-fundkey.pngis excluded by!**/*.pngdocs/images/wizard-5-done.pngis excluded by!**/*.pngfrontend/src/assets/suggested-apps/routstr.pngis excluded by!**/*.png
📒 Files selected for processing (52)
.gitignoreCHANGELOG.mdCONTRIBUTING.mdREADME.mdSECURITY.mdapi/api.goapi/backup.goapi/models.goapi/transactions.godeploy.shdocs/architecture.mddocs/backup-restore.mddocs/daemon-patches.mddocs/deploy.mddocs/development.mddocs/reference.mddocs/security.mddocs/troubleshooting.mddocs/upstream.mddocs/user-flow.mdfrontend/src/components/TransactionsList.tsxfrontend/src/components/connections/AppTransactionList.tsxfrontend/src/components/connections/AppUsage.tsxfrontend/src/components/connections/DisconnectApp.tsxfrontend/src/components/connections/SuggestedAppData.tsxfrontend/src/components/connections/routstr/ApiKeySection.tsxfrontend/src/components/connections/routstr/CreateKeyDialog.tsxfrontend/src/components/connections/routstr/DeleteKeyDialog.tsxfrontend/src/components/connections/routstr/ModelDetailPanel.tsxfrontend/src/components/connections/routstr/ModelPricingStrip.tsxfrontend/src/components/connections/routstr/ModelSelect.tsxfrontend/src/components/connections/routstr/ModelSelectUtils.tsfrontend/src/components/connections/routstr/RefundDialog.tsxfrontend/src/components/connections/routstr/RoutstrConnectionDetails.tsxfrontend/src/components/connections/routstr/TopUpDialog.tsxfrontend/src/components/connections/routstr/constants.tsfrontend/src/components/layouts/SettingsLayout.tsxfrontend/src/hooks/useRoutstrd.tsfrontend/src/lib/clipboard.tsfrontend/src/routes.tsxfrontend/src/screens/ai/AI.tsxfrontend/src/screens/apps/AppDetails.tsxfrontend/src/screens/internal-apps/Routstr.tsxfrontend/src/screens/settings/RoutstrApiKeys.tsxhttp/http_service.goservice/models.goservice/routstrd.goservice/service.goservice/start.goservice/stop.gotests/mocks/Service.gowails/wails_handlers.go
|
Hi @rolznz @im-adithya @reneaaron @bumi @kiwiidb — first contribution to Alby Hub, and your eyes on it would mean a lot. @sh1ftred @bilthon @fanyiy — from the Routstr side, same ask. What it is: Routstr, an OpenAI-compatible AI gateway your Hub supervises end to end. Point any OpenAI client at your Hub: one key, every model, pay per request in sats, auto top-up so the Cashu pool never runs dry. The daemon routes each request to the cheapest provider in a federation discovered over Nostr. Why it's a gentle fit: no schema changes, standard app creation with isolated wallets, app-scoped invoices, one new service manager (RoutstrdService) that starts, stops and restarts the daemons like any Hub service, and existing UI patterns. The full manual and design rationale live in welliv/alby-hub-routstr. Where I'd genuinely value your input:
And a thank you to every Alby Hub contributor whose patterns this builds on. Happy to walk through anything. Open to whatever changes you think it needs. |
Addresses CodeRabbit findings on the PR: - validate the mint invoice amount (decodepay) before paying it in the auto-refill path; a wrong or hostile daemon response can no longer drain the Routstr wallet - restrict the public /routstr/* proxy to /routstr/v1/*: admin endpoints (/stop, /wallet, /clients, /refund) were publicly reachable through the Hub port against the unauthenticated daemon - guard DeleteApp for Routstr apps with a wallet balance (refund or transfer first); surfaces the reason instead of a generic failure - reconcile stale cocod state: dead pid + stale socket are cleaned on spawn, and a hung daemon (pid alive, socket missing) is killed and respawned instead of being reported healthy - run the Start-triggered immediate refill check off the request thread - guard cancelFn with the mutex; truncate raw daemon bodies in published events; fix findRoutstrApp's configured tier (block presence, not defaults); checkpoint retry now honors the busy result; restore paths derive from workDir instead of assuming it sits under $HOME
… refund mint) - DeleteKeyDialog: a transient balance-check failure no longer reads as zero balance (which allowed deleting a key that still held sats); only a confirmed client-not-found clears the balance - Routstr wizard: Back button inside the configure form gets type=button so it cannot submit the form - CreateKeyDialog: create the API key before funding, so a failed key creation cannot leave sats spent with no key - refundFromHub: take the active mint URL as a parameter instead of hardcoding mint.cubabitcoin.org - RefundDialog: cache the mint fee quote across opens/refreshes; every re-quote created a lingering app-scoped invoice (measured: 7 pending) - TopUpDialog: validate amount as a positive integer (NaN/decimals slipped through); ModelDetailPanel sorts providers like ModelPricingStrip so the cheapest pricing is shown; remove the dead command-J hint
|
Thanks for the thorough review. All findings were fact-checked against the code and addressed: Fixed (code):
Addressed in docs (design tradeoffs, upstream-owned):
Not changed (1): All changes build ( |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/connections/routstr/RefundDialog.tsx (2)
70-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRefund only funds available at the selected mint.
walletBalsums all mint balances.refundFromHubsends the melt request with onlyactiveMint. If another mint holds funds,sendAmountcan exceed the selected mint balance. The melt can then fail after the dialog reports that the amount is refundable.Calculate the refund per mint, or include only the selected mint balance in this dialog.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/RefundDialog.tsx` around lines 70 - 74, Update the balance calculation in RefundDialog around activeMint and walletBal so walletBal reflects only the balance for the selected activeMint, rather than summing all mint balances. Preserve the existing zero fallback when the selected mint or balance is unavailable, ensuring refundFromHub cannot request more than the selected mint’s funds.
94-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not cache a failed fee quote as zero.
If invoice creation or the mint quote fails,
feeremains0. The code then caches that value withlastQuotedBalance. A later open with a balance change below 10 sats reuses the zero fee. The dialog can overstatesendAmount, and the melt can fail.Update
lastQuotedBalanceandlastFeeonly after a valid quote response.Proposed fix
let fee = 0; +let hasFeeQuote = false; ... if (mtResp.ok) { const quote = await mtResp.json(); - fee = typeof quote.fee_reserve === "number" ? quote.fee_reserve : 0; + if (typeof quote.fee_reserve === "number") { + fee = quote.fee_reserve; + hasFeeQuote = true; + } } ... -lastQuotedBalance.current = totalRefundable; +if (hasFeeQuote) { + lastQuotedBalance.current = totalRefundable; + lastFee.current = fee; +} ... -lastFee.current = fee;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/RefundDialog.tsx` around lines 94 - 139, Update the fee-quote flow in the RefundDialog calculation block so lastQuotedBalance.current and lastFee.current are written only when a valid mint quote is received and its fee_reserve is numeric. Do not cache fee as zero when invoice creation, fetching, response parsing, or fee extraction fails; retain the existing fallback behavior without marking the failed balance as quoted.
🧹 Nitpick comments (2)
frontend/src/components/connections/routstr/RefundDialog.tsx (2)
240-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse theme tokens for dialog styles.
sm:max-w-[400px],text-[10px], and fixed amber colors bypass the Tailwind theme. Replace them with existing theme sizes and semantic status colors.As per coding guidelines, “Use the theme system for colors, border-radius, shadows, and design tokens” and “Prefer Tailwind utility classes over custom px definitions or inline styles”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/RefundDialog.tsx` around lines 240 - 299, The RefundDialog confirmation content uses hardcoded sizing and amber color utilities instead of theme tokens. Update the DialogContent, provider-token summary text, and “Balance too low” message within the refund confirmation render to use existing theme-based max-width, text-size, and semantic warning/status color classes, removing the arbitrary pixel values and fixed amber colors while preserving the current layout and conditional behavior.Source: Coding guidelines
113-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a typed mint client for the melt-fee quote.
This post-
request()call directly calls the Cashu mint with rawfetch, bypassing the project’s typed HTTP path. Add a typed mint client with an explicit timeout and response validation before calling it here, becauserequest()targets Hub/apipaths and cannot cover the mint origin.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/connections/routstr/RefundDialog.tsx` around lines 113 - 120, Replace the raw fetch in the RefundDialog melt-quote flow after request() with the project’s typed mint client, configured with an explicit timeout and response validation. Use the normalized activeMint origin for the typed client and preserve the existing invoice and sat-unit request behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/api.go`:
- Around line 337-348: The app deletion flow around the metadata check must not
bypass the isolated wallet-balance guard when metadata is malformed or altered.
Update the Routstr identity verification in the surrounding deletion method to
use an immutable identity, or reject deletion whenever Routstr ownership cannot
be verified; preserve the existing balance and error handling for verified
Routstr apps. Add coverage for malformed metadata and metadata with a removed or
changed app_store_app_id.
In `@README.md`:
- Line 51: Update the “TLS for external use” guidance in README.md to state that
routstrd port 8008 must not be published or forwarded because it binds
externally and exposes unauthenticated administrative endpoints; instruct
operators to expose only /routstr/v1 through the TLS reverse proxy and firewall
port 8008.
In `@service/routstrd_unix.go`:
- Around line 22-24: Update killProcess in service/routstrd_unix.go at lines
22-24 to wrap syscall.Kill errors with fmt.Errorf, including the kill operation
and PID. Also update killProcess in services/routstrd_windows.go at lines 31-37
to wrap errors from windows.OpenProcess and windows.TerminateProcess with
operation and PID context.
In `@service/routstrd_windows.go`:
- Around line 20-37: Validate parsed cocod.pid values in the caller before any
processExists/processAlive checks, rejecting nonpositive values and values
exceeding the platform PID limit instead of passing them through uint32. Keep
processAlive focused on operating only on already-valid PIDs, preserving its
existing process lookup behavior.
---
Outside diff comments:
In `@frontend/src/components/connections/routstr/RefundDialog.tsx`:
- Around line 70-74: Update the balance calculation in RefundDialog around
activeMint and walletBal so walletBal reflects only the balance for the selected
activeMint, rather than summing all mint balances. Preserve the existing zero
fallback when the selected mint or balance is unavailable, ensuring
refundFromHub cannot request more than the selected mint’s funds.
- Around line 94-139: Update the fee-quote flow in the RefundDialog calculation
block so lastQuotedBalance.current and lastFee.current are written only when a
valid mint quote is received and its fee_reserve is numeric. Do not cache fee as
zero when invoice creation, fetching, response parsing, or fee extraction fails;
retain the existing fallback behavior without marking the failed balance as
quoted.
---
Nitpick comments:
In `@frontend/src/components/connections/routstr/RefundDialog.tsx`:
- Around line 240-299: The RefundDialog confirmation content uses hardcoded
sizing and amber color utilities instead of theme tokens. Update the
DialogContent, provider-token summary text, and “Balance too low” message within
the refund confirmation render to use existing theme-based max-width, text-size,
and semantic warning/status color classes, removing the arbitrary pixel values
and fixed amber colors while preserving the current layout and conditional
behavior.
- Around line 113-120: Replace the raw fetch in the RefundDialog melt-quote flow
after request() with the project’s typed mint client, configured with an
explicit timeout and response validation. Use the normalized activeMint origin
for the typed client and preserve the existing invoice and sat-unit request
behavior.
🪄 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 Plus
Run ID: 67549b66-2369-40d7-aa51-9819af2f6164
📒 Files selected for processing (26)
CHANGELOG.mdCONTRIBUTING.mdREADME.mdapi/api.goapi/backup.godeploy.shdocs/architecture.mddocs/backup-restore.mddocs/deploy.mddocs/development.mddocs/reference.mddocs/security.mddocs/troubleshooting.mddocs/user-flow.mdfrontend/src/components/connections/routstr/CreateKeyDialog.tsxfrontend/src/components/connections/routstr/DeleteKeyDialog.tsxfrontend/src/components/connections/routstr/ModelDetailPanel.tsxfrontend/src/components/connections/routstr/ModelSelect.tsxfrontend/src/components/connections/routstr/RefundDialog.tsxfrontend/src/components/connections/routstr/TopUpDialog.tsxfrontend/src/hooks/useRoutstrd.tsfrontend/src/screens/internal-apps/Routstr.tsxhttp/http_service.goservice/routstrd.goservice/routstrd_unix.goservice/routstrd_windows.go
💤 Files with no reviewable changes (1)
- frontend/src/components/connections/routstr/ModelSelect.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- docs/architecture.md
- docs/user-flow.md
- frontend/src/components/connections/routstr/DeleteKeyDialog.tsx
- docs/deploy.md
- docs/backup-restore.md
- docs/security.md
- docs/reference.md
- frontend/src/components/connections/routstr/CreateKeyDialog.tsx
- frontend/src/hooks/useRoutstrd.ts
- docs/development.md
- frontend/src/components/connections/routstr/ModelDetailPanel.tsx
- CHANGELOG.md
- api/backup.go
- deploy.sh
- docs/troubleshooting.md
- frontend/src/components/connections/routstr/TopUpDialog.tsx
- frontend/src/screens/internal-apps/Routstr.tsx
- service/routstrd.go
| var meta map[string]interface{} | ||
| if err := json.Unmarshal(userApp.Metadata, &meta); err == nil { | ||
| if id, _ := meta["app_store_app_id"].(string); id == "routstr" { | ||
| balanceMsat, err := queries.GetIsolatedBalanceMsat(api.svc.GetDB(), userApp.ID) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot delete Routstr app: check wallet balance: %w", err) | ||
| } | ||
| if balanceMsat > 0 { | ||
| return fmt.Errorf("cannot delete Routstr app: the app wallet still holds %d sats. Refund or transfer it out first", balanceMsat/1000) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not let mutable metadata bypass the wallet-balance guard.
If JSON parsing fails, or app_store_app_id is removed or changed, this code deletes the app without checking its isolated balance. A funded Routstr wallet can then be orphaned.
Use an immutable Routstr identity, or fail deletion when the app identity cannot be verified. Add tests for malformed and modified Routstr metadata.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/api.go` around lines 337 - 348, The app deletion flow around the metadata
check must not bypass the isolated wallet-balance guard when metadata is
malformed or altered. Update the Routstr identity verification in the
surrounding deletion method to use an immutable identity, or reject deletion
whenever Routstr ownership cannot be verified; preserve the existing balance and
error handling for verified Routstr apps. Add coverage for malformed metadata
and metadata with a removed or changed app_store_app_id.
| plus your `sk-` key. That is the whole integration surface for any OpenAI-compatible client. | ||
|
|
||
| #### Authentication | ||
| > **TLS for external use.** The public proxy (`/routstr/v1`) is plain HTTP on the Hub port. For anything beyond a trusted LAN, put the Hub behind a TLS reverse proxy (Caddy/nginx) so keys and completions are not sent in cleartext. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document the routstrd port exposure requirement.
TLS for the Hub proxy does not protect direct access to routstrd. http/http_service.go documents that routstrd binds *:8008 and has unauthenticated administrative endpoints.
State that operators must not publish or forward port 8008. Instruct operators to expose only /routstr/v1 through the TLS proxy and firewall the daemon port.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 51, Update the “TLS for external use” guidance in
README.md to state that routstrd port 8008 must not be published or forwarded
because it binds externally and exposes unauthenticated administrative
endpoints; instruct operators to expose only /routstr/v1 through the TLS reverse
proxy and firewall port 8008.
| // killProcess terminates a process (used to recover a hung daemon). | ||
| func killProcess(pid int) error { | ||
| return syscall.Kill(pid, syscall.SIGKILL) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | grep -E '(^(service/routstrd_unix\.go|service/routstrd_windows\.go)$)' || true
printf '\nUnix file:\n'
cat -n service/routstrd_unix.go | sed -n '1,80p'
printf '\nWindows file:\n'
cat -n service/routstrd_windows.go | sed -n '1,90p'
printf '\nUsages of killProcess:\n'
rg -n 'killProcess\(' -S .Repository: getAlby/hub
Length of output: 2618
Wrap process-kill errors with context.
Each killProcess implementation returns an unadorned OS error. If a call fails, wrap it with fmt.Errorf("context: %w", err) and include the operation plus PID so callers and logs carry sufficient debugging context.
service/routstrd_unix.go: wrapsyscall.Kill(pid, syscall.SIGKILL).services/routstrd_windows.go: wrap bothwindows.OpenProcessandwindows.TerminateProcesserrors.
📍 Affects 2 files
service/routstrd_unix.go#L22-L24(this comment)service/routstrd_windows.go#L31-L37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@service/routstrd_unix.go` around lines 22 - 24, Update killProcess in
service/routstrd_unix.go at lines 22-24 to wrap syscall.Kill errors with
fmt.Errorf, including the kill operation and PID. Also update killProcess in
services/routstrd_windows.go at lines 31-37 to wrap errors from
windows.OpenProcess and windows.TerminateProcess with operation and PID context.
Source: Coding guidelines
| func processAlive(pid int) bool { | ||
| h, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid)) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| windows.CloseHandle(h) | ||
| return true | ||
| } | ||
|
|
||
| // killProcess terminates a process via TerminateProcess. No-op-safe: the | ||
| // daemons are Unix binaries, so this path is only reachable on Unix. | ||
| func killProcess(pid int) error { | ||
| h, err := windows.OpenProcess(windows.PROCESS_TERMINATE, false, uint32(pid)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer windows.CloseHandle(h) | ||
| return windows.TerminateProcess(h, 1) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)service/routstrd_windows\.go$' || true
echo "== file content around function and related calls =="
if [ -f service/routstrd_windows.go ]; then
wc -l service/routstrd_windows.go
cat -n service/routstrd_windows.go
fi
echo "== search for processAlive/killProcess callers and PID_file recovery =="
rg -n "processAlive|killProcess|pid[-_ ]file|PID|ReadInt|OpenProcess|TerminateProcess" service -g '*.go' || true
echo "== platform declarations =="
rg -n "//go:build .*windows|// +build .*windows|func processAlive|func killProcess" . -g '*.go' || true
echo "== deterministic conversion examples =="
python3 - <<'PY'
import struct
for pid in [-1, 0, 1, sys.maxsize, 4294967297, 4294967296, 4294967295]:
try:
import sys; print(f"python3 is unavailable?") # should not run
except:
pass
PY
python3 - <<'PY'
import struct
# Go int on common platforms: 64-bit, so 4294967297 as uint32 is PID 1.
pid = 4294967297
print("python3 int same as Go 64-bit int:", pow(2, 63)-1)
print("go uint32(pid) example 4294967297 modulo:", pid & 0xffffffff)
PYRepository: getAlby/hub
Length of output: 4430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== routstrd.go PID file / recovery path =="
sed -n '250,370p' service/routstrd.go | cat -n | sed 's/^/ /'
echo "== pidfile helper and os.ReadFile call site =="
rg -n "ReadFile|pid|PidFile|pidfile|checkCocodHealth|restore|Kill|killProcess|processAlive" service/routstrd.go service -g 'routstrd*.go' || true
echo "== Go conversion semantics examples =="
python3 - <<'PY'
def pid_as_uint32(pid):
return pid & 0xffffffff
values = [-1, 0, 1, 2**63-1, 4294967296, 4294967297, 100]
for v in values:
print(f"{v} -> {pid_as_uint32(v)}")
PYRepository: getAlby/hub
Length of output: 11687
Cap parsed PIDs before processExists calls.
service/routstrd.go accepts pid > 0 from cocod.pid, even values larger than the platform PID space. On Windows, the existing helper then passes those values through uint32, so a stale PID like 4294967297 can become PID 1.
Reject stale PID-file values above the platform PID limit at the caller, and keep the processAlive helper focused on valid PID inputs.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 20-20: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(pid)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 31-31: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(pid)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@service/routstrd_windows.go` around lines 20 - 37, Validate parsed cocod.pid
values in the caller before any processExists/processAlive checks, rejecting
nonpositive values and values exceeding the platform PID limit instead of
passing them through uint32. Keep processAlive focused on operating only on
already-valid PIDs, preserving its existing process lookup behavior.
Source: Linters/SAST tools
The refund previously melted balance - fee - 2 (a fixed buffer) and never
melted the buffer or the change, leaving dust in the cocod wallet. The
refund now drains to zero:
- quote the mint's melt fee fresh every pass (never assumed)
- melt balance - fee, then re-check and drain the returned change in a
loop until the wallet is empty
- handle the coco-cashu-core degenerate melt failure ("amount must be a
non-negative number" when selected proofs exactly equal invoice + fee,
plus input-fee shortfalls) by retrying with a smaller send
- the dialog's net now shows balance - fee (no hidden buffer), and the
minimum is fee + 1 instead of an arbitrary 10 sats
Also: request.ts surfaces daemon error bodies (body.error fallback) so the
drain's retry logic and the user-facing errors show the real message.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/hooks/useRoutstrd.ts (1)
290-321: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCache the melt-fee quote to avoid a throwaway invoice on every pass.
Each drain pass calls
createAppScopedInvoice(walletBal, appId)purely to query the mint'sfee_reserve, then creates a second, separate invoice (meltInvoice) for the actual melt. The quote invoice is never paid, so up to 6 unpaid, app-scoped invoices can accumulate as pending Hub transactions per refund call.RefundDialog.tsx's owndoLoadBalancesdocuments this exact problem for its preview quote and works around it with alastQuotedBalance/lastFeedebounce, but that safeguard is not applied here in the actual drain loop.Apply a similar debounce so consecutive passes with a similar remaining balance reuse the last quote instead of minting a new invoice each time.
🔧 Proposed fix
+ let lastQuotedBalance = 0; + let lastFee = 0; + for (let pass = 0; pass < 6; pass++) { const bal = await getRoutstrdBalance(); const walletBal = bal?.balances ? Object.values(bal.balances).reduce((a, b) => a + b, 0) : 0; if (walletBal <= 0) { break; } - // 1. Quote the mint's melt fee fresh for the FULL remaining balance. - const quoteInvoice = await createAppScopedInvoice(walletBal, appId); - const fee = await getMeltQuoteFee(quoteInvoice, mintUrl); + // 1. Reuse the last quote when the balance barely moved, avoiding a + // throwaway app-scoped invoice on every pass. + let fee = lastFee; + if (lastQuotedBalance === 0 || Math.abs(walletBal - lastQuotedBalance) >= 10) { + const quoteInvoice = await createAppScopedInvoice(walletBal, appId); + fee = await getMeltQuoteFee(quoteInvoice, mintUrl); + lastQuotedBalance = walletBal; + lastFee = fee; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useRoutstrd.ts` around lines 290 - 321, Update refundFromHub’s drain loop to cache the melt-fee quote and reuse it on consecutive passes with a similar wallet balance, tracking the last quoted balance and fee as RefundDialog.tsx’s doLoadBalances does. Only call createAppScopedInvoice and getMeltQuoteFee when the balance is not within the existing debounce threshold; preserve the current fee calculation and reset or invalidate the cache when the balance changes beyond that threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/connections/routstr/RefundDialog.tsx`:
- Around line 275-277: Update the unavailable-refund message logic in
RefundDialog so it distinguishes zero totalRefundable from a positive amount
below minRequired: show the network-fee message when no sats remain refundable,
and show the top-up message when the refundable amount is merely insufficient.
Preserve the existing canRefund and wallet-balance flow.
In `@frontend/src/hooks/useRoutstrd.ts`:
- Around line 400-421: Update getMeltQuoteFee to enforce a 90-second timeout on
its direct fetch request, using AbortController or the existing timeout
mechanism used by routstrdFetch. Ensure the request is aborted when the timeout
expires and that timeout or abort failures follow the existing fallback behavior
by returning 0.
- Around line 323-379: Preserve partial refund progress in the refund flow by
throwing a PartialRefundError containing totalRefunded whenever a later melt
fails or all shrink retries are exhausted, while retaining normal errors when no
sats were refunded. Update RefundDialog.tsx’s handleRefund catch block to detect
PartialRefundError, report error.totalRefunded, and call onRefundComplete() when
it is greater than zero instead of treating the operation as a blanket failure.
---
Outside diff comments:
In `@frontend/src/hooks/useRoutstrd.ts`:
- Around line 290-321: Update refundFromHub’s drain loop to cache the melt-fee
quote and reuse it on consecutive passes with a similar wallet balance, tracking
the last quoted balance and fee as RefundDialog.tsx’s doLoadBalances does. Only
call createAppScopedInvoice and getMeltQuoteFee when the balance is not within
the existing debounce threshold; preserve the current fee calculation and reset
or invalidate the cache when the balance changes beyond that threshold.
🪄 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 Plus
Run ID: e395ea5e-9660-46b1-8e57-ee9cb78c34dc
📒 Files selected for processing (3)
frontend/platform_specific/http/src/utils/request.tsfrontend/src/components/connections/routstr/RefundDialog.tsxfrontend/src/hooks/useRoutstrd.ts
| try { | ||
| // 2. Melt `send` (balance − fee) via an app-scoped invoice. Proofs | ||
| // cover send + the melt's own fee quote, and the mint returns any | ||
| // change to the wallet, drained on the next pass. | ||
| const meltInvoice = await createAppScopedInvoice(send, appId); | ||
| const meltResult = await routstrdFetch<{ message: string }>( | ||
| "/wallet/send/bolt11", | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ invoice: meltInvoice, mintUrl }), | ||
| timeoutMs: 90_000, | ||
| } | ||
| ); | ||
| if (!meltResult?.message) { | ||
| throw new Error("Melt failed: no confirmation from daemon"); | ||
| } | ||
| totalRefunded += send; | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| if ( | ||
| /insufficient|not enough (funds|proofs)|non-negative/i.test(message) | ||
| ) { | ||
| // Known coco-cashu-core degenerate case: when the selected proofs | ||
| // exactly equal invoice + fee, the swap path computes a zero/negative | ||
| // keep amount ("amount must be a non-negative number"). Retry with a | ||
| // smaller send — the wallet's per-proof input fee can also add 1-2 | ||
| // sats at small denominations. The next pass re-quotes anyway. | ||
| let succeeded = false; | ||
| for (let shrink = 1; shrink <= 4 && !succeeded; shrink++) { | ||
| const retrySend = send - shrink; | ||
| if (retrySend <= 0) { | ||
| break; | ||
| } | ||
| const retryInvoice = await createAppScopedInvoice(retrySend, appId); | ||
| const retryMelt = await routstrdFetch<{ message: string }>( | ||
| "/wallet/send/bolt11", | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ invoice: retryInvoice, mintUrl }), | ||
| timeoutMs: 90_000, | ||
| } | ||
| ); | ||
| if (retryMelt?.message) { | ||
| totalRefunded += retrySend; | ||
| succeeded = true; | ||
| } | ||
| } | ||
| if (!succeeded) { | ||
| throw error; | ||
| } | ||
| continue; | ||
| } | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Partial refund progress is lost when a later pass fails.
Melts in earlier passes are real fund movements: the mint has already credited the app wallet for totalRefunded. If a later pass's melt fails and either does not match the insufficient-funds regex (line 344) or exhausts all 4 shrink retries (line 372), the function does throw error (lines 373 and 377), discarding the return value entirely. The caller, RefundDialog.tsx handleRefund (lines 211-213), then reports "Refund failed" via handleRequestError and skips onRefundComplete(), even though sats already moved to the app wallet in prior passes.
Preserve totalRefunded on failure so the caller can report the actual outcome instead of a blanket failure.
🔧 Proposed fix
+export class PartialRefundError extends Error {
+ constructor(message: string, public readonly totalRefunded: number) {
+ super(message);
+ }
+}
+
export async function refundFromHub(- if (!succeeded) {
- throw error;
- }
+ if (!succeeded) {
+ throw new PartialRefundError(message, totalRefunded);
+ }
continue;
}
- throw error;
+ throw new PartialRefundError(message, totalRefunded);
}
}Then in RefundDialog.tsx, check error instanceof PartialRefundError in the catch block and surface error.totalRefunded (and call onRefundComplete()) when it is greater than zero.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/useRoutstrd.ts` around lines 323 - 379, Preserve partial
refund progress in the refund flow by throwing a PartialRefundError containing
totalRefunded whenever a later melt fails or all shrink retries are exhausted,
while retaining normal errors when no sats were refunded. Update
RefundDialog.tsx’s handleRefund catch block to detect PartialRefundError, report
error.totalRefunded, and call onRefundComplete() when it is greater than zero
instead of treating the operation as a blanket failure.
A cocod daemon that dies or hangs while walking pending mint operations (mint rate limiter) never creates its socket, so the supervisor's health check kills and respawns it forever. The recovery now deletes stale pending operations from coco.db (unpaid quotes - no ecash in flight) before respawning, matching the documented manual fix. Hit live today: 25 stuck pending ops from failed auto-refill attempts loop-killed cocod every 15s and blocked routstrd from starting.
…meout) - refundFromHub caches the melt-fee quote and re-quotes only when the remaining balance moved >= 10 sats, avoiding a throwaway app-scoped invoice on every drain pass (up to 6 pending per refund before) - getMeltQuoteFee enforces a 90s AbortController timeout so a slow mint cannot hang the drain loop - PartialRefundError carries totalRefunded when a later pass fails, so the dialog reports the sats that actually moved instead of a blanket failure - RefundDialog: the "balance too low" message now distinguishes an empty wallet (network fee leaves nothing) from a positive amount below the minimum (top up first); dead branch removed
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/hooks/useRoutstrd.ts (1)
399-405: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle errors from each shrink retry.
A failure from
createAppScopedInvoice()orroutstrdFetch()in lines 383-392 escapes this catch block immediately. It bypasses lines 399-405. If an earlier pass refunded sats,RefundDialog.tsxthen reports a full failure instead of the partial refund.Catch each retry error. Continue only for another matching insufficient-funds error. Wrap other retry failures in
PartialRefundErrorwhentotalRefunded > 0.Proposed fix
- const retryInvoice = await createAppScopedInvoice(retrySend, appId); - const retryMelt = await routstrdFetch<{ message: string }>( - "/wallet/send/bolt11", - { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ invoice: retryInvoice, mintUrl }), - timeoutMs: 90_000, - } - ); - if (retryMelt?.message) { - totalRefunded += retrySend; - succeeded = true; + try { + const retryInvoice = await createAppScopedInvoice(retrySend, appId); + const retryMelt = await routstrdFetch<{ message: string }>( + "/wallet/send/bolt11", + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ invoice: retryInvoice, mintUrl }), + timeoutMs: 90_000, + } + ); + if (retryMelt?.message) { + totalRefunded += retrySend; + succeeded = true; + } + } catch (retryError) { + const retryMessage = + retryError instanceof Error ? retryError.message : String(retryError); + if (/insufficient|not enough (funds|proofs)|non-negative/i.test(retryMessage)) { + continue; + } + if (totalRefunded > 0) { + throw new PartialRefundError(totalRefunded, retryMessage); + } + throw retryError; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useRoutstrd.ts` around lines 399 - 405, Update the retry logic surrounding createAppScopedInvoice() and routstrdFetch() so each shrink attempt catches its own error. Retry only when the error is the matching insufficient-funds condition; otherwise, if totalRefunded is greater than zero, throw PartialRefundError with the accumulated refund, and rethrow the original error when no refund has occurred. Preserve the existing final handling for partial progress.
🧹 Nitpick comments (1)
frontend/src/hooks/useRoutstrd.ts (1)
454-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the melt quote as
unknown.
Response.json()suppliesanyhere. Assign the result tounknownand narrow the object before readingfee_reserve.As per coding guidelines, “
frontend/src/**/*.ts: Use strict TypeScript; no any types”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useRoutstrd.ts` around lines 454 - 455, Update the melt-quote handling in the function containing the mtResp response so the result of mtResp.json() is assigned or treated as unknown, then validate it is a non-null object with a numeric fee_reserve before reading that property. Preserve the existing zero fallback for invalid or missing fee_reserve values and avoid any implicit any usage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@frontend/src/hooks/useRoutstrd.ts`:
- Around line 399-405: Update the retry logic surrounding
createAppScopedInvoice() and routstrdFetch() so each shrink attempt catches its
own error. Retry only when the error is the matching insufficient-funds
condition; otherwise, if totalRefunded is greater than zero, throw
PartialRefundError with the accumulated refund, and rethrow the original error
when no refund has occurred. Preserve the existing final handling for partial
progress.
---
Nitpick comments:
In `@frontend/src/hooks/useRoutstrd.ts`:
- Around line 454-455: Update the melt-quote handling in the function containing
the mtResp response so the result of mtResp.json() is assigned or treated as
unknown, then validate it is a non-null object with a numeric fee_reserve before
reading that property. Preserve the existing zero fallback for invalid or
missing fee_reserve values and avoid any implicit any usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de311a52-b4c3-4c2e-919f-1afad5eafd54
📒 Files selected for processing (3)
frontend/src/components/connections/routstr/RefundDialog.tsxfrontend/src/hooks/useRoutstrd.tsservice/routstrd.go
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/connections/routstr/RefundDialog.tsx
- service/routstrd.go
- each shrink retry now catches its own failure: insufficient-funds errors continue to the next smaller send, other errors preserve any progress via PartialRefundError instead of escaping the catch and reporting a blanket failure - getMeltQuoteFee validates the mint response as unknown before reading fee_reserve (no implicit any)
Extract selectRoutstrApp as a pure function (block-presence tiers) and readAutoRefillConfig as a package function (it never used the receiver), so the selection logic is unit-testable without a DB or service.
clearStuckCocodOps now takes the db path so the recovery is testable; in-memory sqlite tests verify pending ops are deleted while finalized and failed ops survive, and that a missing DB is a no-op.
Extract computeRefundSend, shouldRequoteFee, isRetryableMeltError and computeMinRequired into refundLogic.ts so the money math is unit-tested (vitest, 10 cases). The drain, retry classification, and dialog minimum now use the same functions the tests cover.
|
Testing + CI status Since the fork workflows need maintainer approval to run, I've run the CI checks locally and added automated tests:
Could a maintainer approve the workflow run so CI can verify this? Happy to address anything it surfaces. |
deploy.sh pins routstrd 0.3.11 + cocod 0.0.24 (bun add -g is otherwise unversioned, and an update silently breaks the integration), and runs scripts/patch-routstrd-dist.sh before starting the hub: the script re-applies the 30-min catalog TTL + background warm-refresh loop (stock default is 210 min with no warm loop) and fails the deploy loudly if it cannot verify the patch.
The usage line showed the daemon-wide totals from /usage/summary, so a fresh connection displayed requests/sats spent by every key ever created (e.g. a deleted app's history). Filter the clients[] breakdown by this app's clientId from metadata and render only its stats; hide the line entirely when the key has no usage. Also rounds sats to 2 decimals (drops float noise like 87.92999999).
The ~20 sats figure is specific to the Bark backend. Other backends (LDK, LND, phoenixd) have different, often cheaper fees, so quoting a fixed amount is misleading. Keep the general guidance that small refills are fee-inefficient (fees are per-payment on every backend).
The inputs were only visible after enabling, so Start silently used
defaults and a blur-save racing the Start click could apply stale
values. Now:
- Threshold/amount inputs render always (stopped and running): type
your values, press Start, the loop honors exactly those.
- Start persists threshold/amount atomically with enabled=true
(POST /autorefill/start accepts {threshold, amount}).
- The generic app-metadata PATCH can no longer change
routstr.autoRefill.enabled: the server forces the current DB value
(Start/Stop own it via their endpoints), killing the blur-save
race in both directions.
- Blur-save no longer toggles button loading, so clicking Start
while an input is focused isn't swallowed by the disabled state.
Verified live: type 450 -> Start -> enabled stays true after the
race window, loop refilled 50 sats on the spot; Stop returns to
stopped. Also fixed a cosmetic Bark-specific fee figure.
|
Closing — resuming work locally on the fork with thorough verification first. Will reopen with proven results. |
1 participant
Routstr: an AI gateway the Hub supervises
This PR adds Routstr — a self-hosted, OpenAI-compatible AI gateway that pays per request in sats — as a first-class internal app of Alby Hub. Point any OpenAI-compatible client at your Hub, and Routstr routes each request to the cheapest provider in a federation of community-run endpoints discovered over Nostr. One key, every model, no subscriptions.
The full manual lives in the fork: welliv/alby-hub-routstr (lean README + Diátaxis docs + annotated screenshots). This PR description is the summary.
What the fork adds
routstrddaemon and thecocodCashu wallet (15s health tick), like any other backend servicefromAppId, app-scoped invoices) — never the main walletWhy it is a gentle addition
RoutstrdService), app metadata (routstr.*), and standard Hub primitives:createApp, isolated app wallets,SendPaymentSyncwithfromAppId, app-scoped invoices (theappIdparam onCreateInvoice), and the existing app-detail UI patterns.service/*,api/*,http/http_service.go,frontend/src/routes.tsx); everything else is new files.Verification
go test ./...,yarn lint:js(0 warnings),yarn format,yarn tsc:compile— all green.See docs/architecture.md for the design rationale and docs/upstream.md for the merge and patch policy.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation