Repository navigation
fix(mcp): bound cumulative daily paid-call spend, not just per call - #492
Conversation
|
@AGWAM001 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…ivinity-Labs#405) A per-call cap answers "how much can one mistake cost"; an operator needs "how much can today cost", and an agent in a retry loop could previously pay the per-call maximum without limit. SpendBudget adds a per-UTC-day ceiling with worst-case reservation before each call, refuses with ERR_BUDGET_EXCEEDED (carrying the live snapshot in details.budget) before the payment fetch, exposes both limits in the paid_call description plus a payment_budget tool, and persists the day's total so a restart does not refill the budget. Unreadable or unwritable state fails closed. Also fixes MCP_MAX_PAYMENT_PRICE being passed as maxPrice, which is not an x402 client config option and bounded nothing; it is now spendControls.maxAmountPerPayment.
|
Needs a rebase — #488 (the SSRF guard for Two things worth knowing before you rebase, because three PRs are converging on the same file:
Push the rebase and I will review properly. The spend ceiling is the half of #404 that actually bounds the loss — an allow-list stops the money going somewhere strange, a ceiling stops it going somewhere allowed ten thousand times. |
Coordination note — ten PRs are editing one 179-line fileIf your MCP PR has just gone red with a conflict, this is why, and it is my fault rather than yours. I published 22 MCP issues (#404–#425) as if they were independent. They are not: almost all of them land in Already merged:
Order I intend to take the rest in, so you can rebase once rather than repeatedly:
Please do not all rebase at once. If you are below the one currently in flight, wait until the one above you lands — otherwise you rebase, I merge someone else, and you rebase again. I will comment on your PR when it is your turn. One thing to settle between you: Nothing here reflects on the quality of the work. Several of these are among the best PRs this repo has had. |
|
#487 has landed — the I resolved #487's conflicts myself because nine PRs were stuck behind it. Three things on 1. Two refusal codes, deliberately separate. ERR_URL_BLOCKED // the target resolves somewhere we will never fetch (#488)
ERR_HOST_NOT_ALLOWED // reachable, but not on the paid_call allow-list (#487)If your PR adds a refusal, extend this vocabulary rather than starting a third. "I cannot reach there" and "I will not pay there" are different answers; whatever you add should be a third distinct answer, not a synonym for one of these. 2. The constructor takes 3. Push the rebase and I will review it. If it conflicts in a way that is not obvious, say so and I will resolve it rather than leave you stuck. |
Miracle656
left a comment
There was a problem hiding this comment.
The design is right and I want this. It needs a rebase — the auto-merge against current main produces broken code in four places, and I am not going to reconstruct your test file by guesswork.
What the merge breaks
main gained #487 (paid_call allow-list) and #488 (SSRF guard) after you branched, and both touch the same constructor. Git stitched them together without conflict markers, which is the worst case:
McpServerConfig— the/**opening the allow-list doc comment is swallowed bybudget?: SpendBudget;BazaarMcpServer— two overlapping constructors, one takingconfig, one takingconfig_- the
paid_calldescription — your budget ternary and the allow-list text concatenated with no+ tests/mcp.test.ts— two closing braces short; it no longer parses
I repaired the first three locally to read it, then stopped at the fourth. Reconstructing where your describe blocks were meant to close is me inventing your tests.
Note the constructor parameter is now config_, renamed on main to stop it shadowing the imported config.
The design, which I read carefully and which is excellent
Integer nanodollars via BigInt. Money in floats is how a ceiling quietly becomes $1.0000000000000002. And parseUsdToNanodollars throws at boot on a malformed ceiling rather than running with an unintended budget — the right direction to fail for a spend limit.
Reservations, which I did not ask for and should have.
if (this.spentNanodollars + this.reservedNanodollars + this.perCallMaxNanodollars > this.dailyCeilingNanodollars)Holding the worst case before the call and settling after is what makes the ceiling true under concurrency. Checking only spent lets two in-flight calls both pass and jointly exceed it — the classic way a limit that looks correct is not.
The day-boundary case in commit:
The window closed while the call was in flight, so the payment belongs to the window that has just ended and must not be booked against the new day.
That is a genuinely subtle call and you got it the right way round. Booking it against the new day spends tomorrow's budget on yesterday's call.
The refusal is actionable — spent, ceiling, remaining, the per-call maximum it must cover, and when the window resets. An agent can explain itself or wait, instead of retrying.
And it fails closed when the state file cannot be trusted, telling the operator to fix it and restart rather than silently resuming with a fresh budget.
One thing to consider while rebasing
Holds live in memory. If the process dies between reserve and commit on a payment that did settle, the persisted spentToday loses it, and enough crashes could walk past the ceiling. Not necessarily worth fixing — persisting reservations has its own failure modes — but worth a sentence saying the choice was made deliberately.
Rebase and I will merge it. This is the half of #404 that actually bounds the loss: an allow-list stops money going somewhere strange, a ceiling stops it going somewhere allowed ten thousand times.
|
Merged. This is money code written like money code, and the design decisions are the ones I'd have argued for. It survives a restart. The running total is persisted and reloaded, so restarting the server does not refill the budget. An in-memory counter would have made the ceiling advisory on any host that recycles processes — which is most of them. It fails closed. An unreadable or corrupt state file is treated as fully spent, not as a clean slate, and the error tells the operator to fix the file and restart. That is the exact opposite of the right default for a rate limiter, and exactly right here: a cap that forgets itself when its storage breaks is not a cap. It writes atomically — temp file then rename — so a crash mid-write cannot leave a truncated total that reads as a smaller spend. Money is exact integer nanodollars, parsed from strings, with more than nine decimal places rejected rather than silently rounded. And a rejected payment is not charged, and a call whose worst case no longer fits is refused rather than allowed to overspend the remainder. Those two tests are the ones I'd have written first and they were already there. What merging cost, and whyThe branch was 47 commits behind, and both sides had restructured the same class. Git auto-merged several regions into something that looked clean and did not compile — the worst kind of conflict, because nothing flags it:
One of these was already broken on your branch, not from the merge: description:
'Make a paid API call ... ' +
(this.budget ? this.budget.describeLimits() : 'No cumulative spend ceiling ...'),
'This tool spends real USDC ...' +The The test file would have dropped 19 tests that landed on Your budget harness also needed None of that is a criticism of the work — it is the cost of a 47-commit gap on a file three other PRs touched this week. Rebasing sooner would have surfaced all of it as honest conflicts. Verified on the merge result: Closes #405. I'll review #523 next, since it holds #406 — which I reopened this morning after wrongly closing it with a mistyped issue number in an unrelated merge. |
Twice in one day a branch removed test cases that existed on `main`, and git reported no conflict either time. PR #480 dropped 19 — the Zod argument suite from #555 and the upstream deadline suite from #525. PR #492 would have dropped the same 19. Both branches were opened before those landed, so merging `main` in replaced `tests/mcp.test.ts` wholesale rather than merging it: the result is the older file plus the author's additions, and everything added in between is gone. Both PRs reported "mergeable". Both typechecked. The only way I noticed was counting `it(` on each side, by hand, twice. `scripts/check-dropped-tests.mjs` compares the base against the tree a merge would actually produce — not against the branch tip, which over-reports wildly (a branch 47 commits behind is "missing" every case added since it forked, merged or not). It lists the cases by file and name and exits 1. It does not run the tests and does not judge whether a removal is right. Removing a test is sometimes exactly correct — the feature went away, the case was a duplicate, the assertion was wrong. The point is that the removal is deliberate rather than accidental: say so in the PR and apply `tests-removed-intentionally`. Validated both ways before committing, since a guard nobody has seen fire is an assumption: a branch with a case deleted exits 1 and names it; an unchanged branch exits 0. Known limitation, written down rather than left to be discovered: when the merge conflicts, `git merge-tree` writes a tree containing both sides, so no case looks missing and the check passes. That is the case a human already has to resolve by hand — which is exactly where #492's loss would have happened. The check is strongest for the silent case, which is the one that bit. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
A tool result is written straight into the agent's context window, so an unbounded one is self-inflicted denial of service whose failure gets blamed on the model. All four tools serialise compactly and are capped at MCP_MAX_RESULT_BYTES; a truncated result carries truncated, the total, the page returned, and a notice saying how to ask for more. bazaar_search gains limit/offset arguments, forwards them to the node and pages whatever comes back. lens_get_price keeps structuredContent only while the payload fits, since shipping it regardless would move the blow-up to another field. Upstream bodies are read through a capped stream reader that cancels at the cap, so an oversized body is never buffered; a discovery body too large to parse returns ERR_RESPONSE_TOO_LARGE, which is deliberately not one of the two pre-request refusals: we did fetch it, we will not dump it. Rebased onto main after Divinity-Labs#480 (structured paid-call errors), Divinity-Labs#487 (paid_call host allow-list) and Divinity-Labs#492 (spend ceiling) landed: the bounds now sit on top of those gates rather than beside them, and limit/offset go through the Zod tool schemas.
Summary
A per-call limit answers "how much can one mistake cost". The question an operator actually has is "how much can today cost", and nothing answered it: an agent in a retry loop — or one told to try each of five endpoints — could pay the per-call maximum arbitrarily many times, every payment individually inside the limit and the sum unbounded.
SpendBudget(src/mcp/budget.ts) adds a cumulative ceiling on whatpaid_callmay spend within one UTC day.MCP_DAILY_SPEND_CEILING(default$10.00) beside the per-callMCP_MAX_PAYMENT_PRICE. The constructor refuses nonsense: a non-positive per-call maximum, or one larger than the ceiling, since then no paid call could ever be allowed.ERR_BUDGET_EXCEEDED, returned before the payment fetch, with the live snapshot indetails.budget(spentToday,dailyCeiling,remainingToday,resetsAt). An agent can report being out of budget instead of appearing broken.paid_calltool description, and a newpayment_budgettool reports spent, reserved and remaining without making a paid call.MCP_SPEND_STATE_FILE(default~/.lens-mcp/spend-state.json) and is read back at startup, because a restart does not refund what the previous process already spent. A missing file starts at zero; the window still resets at 00:00 UTC.Decisions
Day window, not sliding 24h. It answers the operator's question directly, needs one stored number rather than one entry per payment, and rolls over from the wall clock — so a restarted server resumes the same day's total instead of reconstructing a history.
Worst-case reservation. Before a call runs, the budget reserves its per-call maximum, so
Nconcurrent calls cannot collectively breach the ceiling, and a call whose worst case no longer fits the remainder is refused rather than allowed to spend the last few cents.Conservative accounting, and why. The wrapped x402 transport hands back a plain
Response; the settled price is not observable at this layer, so the budget cannot debit the real amount. A call that could have settled is booked at the per-call maximum; a seller rejection that proves nothing settled is booked at nothing. Recorded spend is therefore an upper bound — the ceiling is reached early rather than late, which is the safe direction for a surface that signs with a real key.Fail closed on state. An unreadable or unwritable state file means the running total is unknown, and an unknown total against a money ceiling is assumed fully spent: paid calls are refused and the refusal says so until the file is fixed.
Also fixed
MCP_MAX_PAYMENT_PRICEwas passed towrapFetchWithPaymentFromConfigasmaxPrice, which is not an option of the x402 client config and was silently ignored — every payment was capped by the library default of$1whatever the operator asked for. It is nowspendControls.maxAmountPerPayment, so the per-call limit the budget reserves against is the one actually enforced.Tests
tests/mcp.test.tsadds 9 tests: the acceptance case (three$1.00calls fill the ceiling and the fourth is refused withERR_BUDGET_EXCEEDED, with the payment fetch call count proving no money moved), remainder-that-cannot-fit refusal, restart persistence through a shared state file, UTC-midnight rollover with an injected clock, 402 rejection not charging the budget, corrupt state failing closed, both limits visible in the description and viapayment_budget, constructor validation, exact nanodollar parsing.npx vitest run tests/mcp.test.ts→ 13/13 passnpm test(whole repository) → 674 passed, 1 skipped (tests/integration/e2e.test.ts, needs live services), 0 failednpx tsc --noEmit→ cleannpm run build→ cleanNotes
Arithmetic is integer nanodollars throughout, so there is no float drift. No secret key is logged, echoed, or used in a fixture; startup logs limits and file paths only. Config is documented in
.env.exampleand a patch changeset is included.Related issue
Closes #405
Type of change
Checklist
npx tsc --noEmitpassesnpm run buildpasses