demo(payments): enforce a rolling spend budget, not just a per-payment cap - #139
demo(payments): enforce a rolling spend budget, not just a per-payment cap#139mehmetkr-31 wants to merge 3 commits into
Conversation
…t cap The policy guard added in agentcommercekit#97 caps a single payment. Its own comment and the demo README call out that this is not a spend control: splitting one payment into N below-cap payments defeats it entirely. This closes that gap in the demo. - Add `src/spend-ledger.ts`, a small in-memory ledger that records what a payer has put at risk inside a rolling window. - Add an optional `budget` to `PaymentPolicy` and an `authorizePayment` layer that runs the existing per-transaction checks and then the cumulative window check. `evaluatePaymentPolicy` keeps its current signature and behaviour. - Reserve inside the same synchronous step as the check. The handler awaits token verification before policy runs, so a read-then-write guard would let two concurrent payments both observe the pre-payment total and both pass. - Key reservations by payment request id plus payment option id, so the two calls of the Stripe flow authorize one payment once. Commit the reservation once the receipt is issued; release it if the Receipt Service call fails. - Rename the Payment Service's `serverIdentity` local to `payerIdentity`; it holds the payment service key, not the server key that `getTrustedRecipients` reads under the same name. - Document what is still demo-grade: in-memory storage, single-instance atomicity, deny rather than escalate to human approval. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe payments demo adds a rolling-window spend ledger, budget-aware payment authorization, and payer-scoped reservation handling across payment initiation and callbacks. ChangesPayment spend control
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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 |
Both parts of the key come from the Payment Request as unconstrained strings,
so `${paymentRequestId}:${paymentOptionId}` lets `("a:b", "c")` and
`("a", "b:c")` produce the same reference. A collision overwrites the earlier
reservation, dropping its amount from the window and under-enforcing the
budget. A payment request that clears the recipient allowlist can choose both
ids, so this is reachable rather than theoretical.
Encode the pair instead, and move `spendReference` next to the ledger it keys
so it can be tested without importing the Payment Service, which starts a
server on import.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
demos/payments/src/spend-ledger.test.ts (1)
72-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for re-reserving a reference with a larger amount.
reservereplaces the entry for an existing reference. The current tests only re-reserve the same amount, so the limit check on the replacement path is untested. A regression that skipped the limit check for known references would still pass.💚 Proposed additional test
it("counts a re-reserved reference once", () => { reserve("payment-1", 600n) expect(reserve("payment-1", 600n)).toEqual({ status: "reserved", spent: 600n, }) expect(ledger.spentWithin(SUBJECT, "USDC", WINDOW_MS)).toBe(600n) }) + + it("rejects a re-reservation that raises the amount above the limit", () => { + reserve("payment-1", 600n) + reserve("payment-2", 300n) + + expect(reserve("payment-1", 800n)).toEqual({ + status: "exceeded", + spent: 300n, + limit: 1_000n, + }) + expect(ledger.spentWithin(SUBJECT, "USDC", WINDOW_MS)).toBe(900n) + })🤖 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 `@demos/payments/src/spend-ledger.test.ts` around lines 72 - 80, Add a test alongside “counts a re-reserved reference once” that reserves the same reference again with a larger amount and verifies the replacement still enforces the spending limit. Assert the operation’s rejected/limit-exceeded result and confirm the ledger total remains unchanged after the failed re-reservation.
🤖 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 `@demos/payments/src/payment-service.ts`:
- Around line 104-110: Document the post-capture denial scenario in the README
limitations list or add distinct logging around enforcePaymentPolicy in the
re-authorization flow identified by spendReference and paymentRequest.id. Make
the message clearly state that Stripe captured funds but settlement was denied
because the original reservation expired and the budget was consumed by later
payments.
- Around line 56-64: Update the callback handler’s reservation-release flow to
include createJwt in the existing try block, ensuring any JWT creation rejection
triggers reservation release before propagating the error. Keep receipt
generation behavior unchanged for successful JWT creation.
---
Nitpick comments:
In `@demos/payments/src/spend-ledger.test.ts`:
- Around line 72-80: Add a test alongside “counts a re-reserved reference once”
that reserves the same reference again with a larger amount and verifies the
replacement still enforces the spending limit. Assert the operation’s
rejected/limit-exceeded result and confirm the ledger total remains unchanged
after the failed re-reservation.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 335aacc2-e822-4346-b966-b335b9d7262f
📒 Files selected for processing (6)
demos/payments/README.mddemos/payments/src/payment-policy.test.tsdemos/payments/src/payment-policy.tsdemos/payments/src/payment-service.tsdemos/payments/src/spend-ledger.test.tsdemos/payments/src/spend-ledger.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6ce66e5e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function spendReference(paymentRequestId: string, paymentOptionId: string) { | ||
| return `${paymentRequestId}:${paymentOptionId}` |
There was a problem hiding this comment.
When the same PaymentRequest token and option are submitted for another Stripe initiation or callback within the window, this reference is identical, so SpendLedger.reserve treats the new execution as a re-authorization and excludes/replaces the existing entry instead of adding to the rolling total. That lets repeated Stripe payments for a reused request stay charged against the budget only once; include a payment-execution identifier such as a generated session/payment-intent id in the reservation key while still reusing that key for the matching callback.
Useful? React with 👍 / 👎.
Addresses three review findings. Keying the reservation on the Payment Request meant a request presented for execution twice consumed the budget once, so an agent could bypass the budget by re-executing one request. The Payment Service now mints an execution id at initiation and carries it through the callback URL: the two calls of one execution still reserve once, while a second execution is charged separately. The demo client needs no change — it forwards whatever `return_to` it is given — and `return_to` is now URL-encoded, since it carries a query of its own. Move `createJwt` inside the try that releases the reservation. Signing failed outside it, leaving a reservation with no receipt to commit or release. Document the post-capture denial path in the README: the callback re-authorizes after the card is captured, so a denial there stops the receipt without stopping the charge. It needs the initiation reservation to have aged out and later payments to have filled the budget, which the one-hour window makes unlikely, but a real service should settle and flag rather than refuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks both — all three findings were real. Pushed Reserve per payment execution, not per Payment Request (@chatgpt-codex-connector)This was the important one, and it undercut the point of the PR. Keying on the Payment Request meant a request presented for execution twice consumed the budget once, so the agent the budget is meant to bound could bypass it by re-executing one request. The Payment Service now mints an execution id at initiation and carries it through the callback URL, so the two calls of one execution still reserve once while a second execution is charged separately. The demo client needs no change — it forwards whatever Release the reservation when JWT creation fails (@coderabbitai)Correct — Document the post-capture denial path (@coderabbitai)Also correct, and documentation rather than a code change is the right call here: the callback re-authorizes after the card is captured, so a denial there stops the receipt without stopping the charge. Added to the README limitations block, including that a real service should settle and flag an over-budget capture for reconciliation rather than refuse it. Also fixed, self-reportedThe reservation key was VerificationRe-ran the three services and drove the Stripe path over HTTP, extracting
AI Usage DisclosureThese follow-ups were prepared with AI assistance (Claude Code, Opus), used for navigating the review threads, drafting the changes and tests, and running verification. I reviewed the final diff and take responsibility for it. |
|
To use Codex here, create an environment for this repo. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Changed payment demo files ---'
git diff --name-only a6ce66e..HEAD -- demos/payments
printf '%s\n' '--- payment-service structure ---'
ast-grep outline demos/payments/src/payment-service.ts --items all
printf '%s\n' '--- JWT, reservation, and failure handling ---'
rg -n -C 12 -e '\bcreateJwt\s*\(' -e '\.release\s*\(' -e '\.commit\s*\(' -e '\bauthorizePayment\s*\(' demos/payments/src/payment-service.ts
printf '%s\n' '--- Documented capture / denial limitation ---'
rg -n -i -C 5 'capture|post-capture|over-budget|reconciliation|receipt' demos/payments/README.md
printf '%s\n' '--- Execution and reference tests ---'
rg -n -C 4 -e 'execution|spendReference|createJwt|release' demos/payments/src -g '*test.ts'Length of output: 19201
The README now documents the post-capture denial path. It correctly states that production services should settle and flag an over-budget capture for reconciliation. The execution-specific, collision-safe 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
1 participant
Summary
src/spend-ledger.tsevaluatePaymentPolicyunchanged and put the new behaviour in an additiveauthorizePaymentlayerFixes #138.
This closes the gap #97 documented in its own code and README rather than adding anything new to the protocol surface:
Everything stays inside
demos/payments. No package, schema, or protocol change; no new dependency.What changed
src/spend-ledger.ts(new) — records what a subject has put at risk inside a rolling window, keyed by reference.reserve()checks the window and writes in one synchronous step. This is the part that actually matters: the request handler awaitsverifyPaymentRequestTokenbefore policy runs, so a guard that read the running total and then wrote to it would let two concurrent payments both observe the pre-payment total and both pass.commit()marks a reservation settled;release()drops an unsettled one.src/payment-policy.ts—PaymentPolicygains an optionalbudget: { windowMs, maxWindowAmount }, using the same per-currency subunit shape as the existingmaxAutonomousAmount. NewauthorizePayment()runsevaluatePaymentPolicyfirst and then the window check, so a payment rejected by the per-transaction cap or the allowlist never consumes budget.evaluatePaymentPolicykeeps its exact signature, behaviour, and reasons — the amount-parsing and per-currency lookup are extracted into two local helpers now shared by both paths.src/payment-service.ts— both handlers authorize underspendReference(paymentRequest.id, paymentOptionId), so the payment-URL call and the callback reserve once. The callback commits after the receipt is issued and releases if the Receipt Service call fails. The localserverIdentityin the callback is renamed topayerIdentity: it holdsPAYMENT_SERVICE_PRIVATE_KEY_HEX, whilegetTrustedRecipientsuses the same name forSERVER_PRIVATE_KEY_HEXtwo functions below.README.md— replaces the "not a real spend control" warning with a "Rolling spend budget" section, and a new warning that scopes what is still demo-grade: in-memory storage, single-instance atomicity, denying rather than escalating to human approval, and tracking the demo's single autonomous payer because ACK-Pay carries no payer identity on the execution request.Notes on scope
@agentcommercekit/ack-pay. If a shape like this is ever worth publishing, this is the concrete consumer that would justify it.schememodel (upto, sessions, subscriptions). This is a Payment Service enforcing its own budget, which is a different layer; nothing here presumes an outcome for RFC: a scheme model for ACK-Pay (up-to, sessions/batch, subscriptions, streaming, refunds) #111.deniedrather thanapproval_requiredfor a budget breach, mirroring how the per-transaction cap already behaves. The README notes that a real service would escalate instead.Verification
Tests cover the split attack (three below-cap payments approved, the fourth denied), window expiry, per-subject and per-currency isolation, idempotent re-authorization, reservations not being consumed by denied or approval-required payments, commit/release semantics, and the no-budget-configured path.
I also ran the real handlers over HTTP with the Receipt Service live, driving the Stripe path end to end:
initiate=200 callback=200, receipt issued200, 200, 200403 Payment exceeds the autonomous spend budget for the current window200, no additional budget consumed403AI Usage Disclosure
This contribution was AI-assisted using Claude Code (Opus). AI assistance was used for repository and history navigation, reviewing the existing policy guard and its review discussion, drafting the ledger and tests, and running verification. I reviewed the final diff, understand how the reserve/commit/release path interacts with the two-phase Stripe flow and why the check and reserve must not be separated by an await, and take responsibility for the submitted changes.
Summary by CodeRabbit
New Features
Documentation