Repository navigation
fix(issue-221): reject over-capture and require actualAmount on successful reports - #223
JonasBaeumer wants to merge 4 commits into
Conversation
…ssful reports Harden the POST /v1/agent/result -> settleIntent settlement contract: - Over-capture policy (reject): a success report with actualAmount above the pot's reservedAmount is rejected with 422, an OVER_CAPTURE_REJECTED AuditEvent is recorded, and the intent stays CHECKOUT_RUNNING with no settlement and no card cancellation, so a corrected report can follow. The virtual card's network-level spending limit means the card cannot be charged above the reservation, so such a report is by definition wrong (buggy or malicious worker). - agentResultSchema: actualAmount is now conditionally required when success is true (zod superRefine), closing the settle-0-and-refund hole for direct REST callers. - Defense in depth: settleIntent itself throws OverCaptureError (new, src/contracts/ledger.ts) before any write when actualAmount exceeds reservedAmount, so no future caller can bypass the route check. Regression tests for each behavior (each proven to fail without its fix), plus a ledger-invariant test: reserved - settled - returned = 0 after any accepted settlement. Closes #221 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTR7UwtzY7wvD4c9YjT38e
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (11)
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 |
|
@codex review |
![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: a9f0da0b2f
ℹ️ 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".
| // by definition wrong (buggy or malicious worker). Reject it, audit it, and | ||
| // leave the intent in CHECKOUT_RUNNING so a corrected report can follow. | ||
| const pot = await prisma.pot.findUnique({ where: { intentId } }); | ||
| if (pot && reportedAmount > pot.reservedAmount) { |
There was a problem hiding this comment.
The same actualAmount > reservedAmount business rule is now implemented independently here and in settleIntent (src/ledger/potService.ts:73). If the boundary or policy is later changed in only one place, the route can accept a report, transition the intent to DONE via completeCheckout, and then have settleIntent reject it, leaving intent and ledger state inconsistent; use one shared predicate/assertion so both enforcement points cannot drift.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The duplication is deliberate defense in depth, and the drift scenario cannot produce inconsistent state today or silently in the future:
- No failing input exists now. Both checks are the identical comparison
actualAmount > pot.reservedAmountagainst the samePotrow, and the route returns 422 beforecompleteCheckoutruns — so there is no input today where the route accepts andsettleIntentrejects after a DONE transition. - Drift is pinned by tests at both layers, which is stronger than a shared predicate. This PR adds boundary tests on each enforcement point independently: the wiring test asserts 422 above the reservation and 200 at exactly-equal at the route, and the potService test asserts
OverCaptureErrorat reserved+1 and success at exactly-equal insidesettleIntent. If either implementation's boundary is later changed unilaterally, that layer's own test fails — a shareda > bpredicate would not prevent the more likely drift mode (a call site being removed or reordered), while the tests catch both. - Even in the hypothetical where the route check were bypassed entirely, the
settleIntentguard throws before any pot/user/ledger write (asserted by test), so the ledger can never book an over-capture — the inconsistency is limited to an intent status, not funds.
Extracting a one-line comparison would add indirection without adding a guarantee the tests don't already provide, so leaving both explicit checks in place. Leaving this thread unresolved for the maintainer's judgement.
…re response Codex review finding on PR #223: docs/api.md and docs/openclaw.md still marked actualAmount as optional and omitted the new 422 over-capture rejection from the error tables for POST /v1/agent/result. Both endpoint contracts now state the conditional requirement and the 422 behavior (intent stays CHECKOUT_RUNNING). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTR7UwtzY7wvD4c9YjT38e
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e10928cdd7
ℹ️ 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".
…ment Codex review finding on PR #223: the 409 invalid-state test posted { success: true } without actualAmount, which the refined agentResultSchema now rejects with 400 before the intent lookup. Adding actualAmount restores the test's purpose (reaching the CHECKOUT_RUNNING state guard) and also fixes a cascading failure: the unconsumed mockResolvedValueOnce intent leaked into the '404 for unknown intent' test, turning its 404 into a 403. Verified: errorPaths suite standalone was 14/16 before this change and is 16/16 after (16/16 on the base commit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTR7UwtzY7wvD4c9YjT38e
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e94da9790
ℹ️ 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".
| await completeCheckout(intentId, actualAmount ?? 0, request.agentId); | ||
| await settleIntent(intentId, actualAmount ?? 0); | ||
| // agentResultSchema guarantees actualAmount is present when success is true | ||
| const reportedAmount = actualAmount ?? 0; |
There was a problem hiding this comment.
For every request reaching this branch, agentResultSchema.superRefine has already established that actualAmount is present, so ?? 0 is unreachable defensive scaffolding and preserves the exact zero-settlement behavior this change is intended to eliminate if validation and routing ever drift. Model the schema as a discriminated union so success: true narrows actualAmount to number, then pass it directly rather than retaining a silent financial default.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No constructible input reaches the ?? 0 with actualAmount undefined: agentResultSchema.superRefine rejects success: true without actualAmount with a 400 before the handler body runs, so the branch under discussion only ever sees a present number. The zero-settlement path this PR eliminates is pinned by a dedicated wiring test ("rejects success: true with no actualAmount as 400 and settles nothing", tests/unit/api/wiring.test.ts), which asserts both the 400 and that settleIntent is never called — any validation/routing drift that re-opened the settle-0 path would fail that test before the fallback could matter.
The discriminated-union rewrite would change no observable behavior (same accepted and rejected inputs, same 400s), so there is no regression test that could fail without it — it is a type-narrowing refactor, not a defect fix, and it would also replace the current targeted error message ("actualAmount is required when success is true") with Zod's generic missing-field error. The ?? 0 exists solely because superRefine does not narrow the static type; the comment directly above it documents the schema guarantee. Leaving as is; thread stays open for the maintainer.
Codex review finding on PR #223: the top-level openclaw.md (a divergent copy of docs/openclaw.md) still marked actualAmount optional and omitted the 422 over-capture response. Applied the same contract updates as e10928c. Whether to replace this duplicate with a pointer to docs/openclaw.md is left as a maintainer decision outside this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTR7UwtzY7wvD4c9YjT38e
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Hardens the
POST /v1/agent/result→settleIntentsettlement contract: over-capture reports (actualAmount> reserved) are now rejected with an audit trail and no state change, andsuccess: truewithoutactualAmountis a 400 at the REST boundary instead of silently settling 0 and refunding the whole pot. Closes #221Chosen policy (over-capture): reject. The virtual card's network-level spending limit means the card cannot legitimately be charged above the reservation, so a report claiming more is by definition wrong — buggy or malicious worker. The report is refused (422), an
OVER_CAPTURE_REJECTEDAuditEventrecords both amounts, and the intent staysCHECKOUT_RUNNING(no settlement, no card cancellation) so a corrected report can follow. This is the conservative default the issue frames and the direction leaned toward in the #216 review.Closes #221
Type of change
Module(s) affected
src/contracts/)prisma/,src/db/)src/api/,src/app.ts)src/orchestrator/)src/payments/)src/policy/,src/approval/)src/ledger/)src/queue/,src/worker/)src/telegram/)Checklist
npm testpasses locally (425 tests, 38 suites; was 412 onmain)src/contracts/if shared across modules (OverCaptureErrorinsrc/contracts/ledger.ts).env.exampleupdated if new env vars are introduced (no new env vars)What changed
src/api/routes/agent.ts): onsuccess: true, the route loads the intent's pot and comparesactualAmountagainstpot.reservedAmount(the numberreserveForIntentdeducted frommainBalanceand booked as theRESERVEledger entry). If above, it writes the audit event and returns 422 naming both amounts — beforecompleteCheckout,settleIntent, card cancellation, or the metadata write.src/api/validators/agent.ts):agentResultSchemanowsuperRefinesactualAmountas required whensuccessis true. Failure reports without an amount are unchanged.src/ledger/potService.ts):settleIntentthrows the newOverCaptureErrorbefore any write whenactualAmount > pot.reservedAmount, so no future caller can bypass the route check.Per-behavior verification
Each regression test was proven to fail with the test in place but the src fix stashed (
git stash push -- src), then pass after popping:wiring.test.ts› "rejects actualAmount above the reserved amount with 422 and no side effects"OVER_CAPTURE_REJECTEDAuditEvent with actor/agentId + both amountswiring.test.ts› "records an audit event for the rejected over-capture report"success: truewithoutactualAmount→ 400, nothing settledwiring.test.ts› "rejects success: true with no actualAmount as 400 and settles nothing"validators.test.ts› "rejects success without actualAmount"settleIntentguard throwsOverCaptureError, zero writespotService.test.ts› "throws OverCaptureError when actualAmount exceeds reservedAmount, with no writes"Boundary + invariant pins (pass on
maintoo, added to lock behavior in): settling exactlyreservedAmountstill succeeds (route + potService tests), and a parameterized ledger-invariant test inpotService.test.tsassertsreserved − settled − returned surplus = 0across settlements of 0, 1, partial, reserved−1, and full reservation — implemented at unit level with the existing$transactionmocking pattern (it checks the amountssettleIntentwrites, not real DB rows). A DB-backed version of the same invariant would belong intests/integration/, which needs Docker and runs in CI only.How to test
Manually (dev stack up, intent in CHECKOUT_RUNNING with a pot reserved at e.g. 10000):
Audit trail:
GET /v1/debug/intents/<id>shows theOVER_CAPTURE_REJECTEDevent.Notes for reviewer
tests/unit/api/mcpRestDrift.test.tsdoes not exist onmain— it lives on unmerged PR feat(mcp): replace skill-based agent integration with an MCP server #216, where "REST accepts success-without-actualAmount" is pinned as a deliberate divergence. Once both PRs are merged, that pin must flip to an agreement row (REST and MCP now both require the amount on success). Deliberately not created here to avoid colliding with feat(mcp): replace skill-based agent integration with an MCP server #216.Invalid inputhandling.settleIntentthrowsIntentNotFoundErrorexactly as before — no behavior change on that path.checkoutProcessor) already postsactualAmounton success; no worker changes needed.🤖 Generated with Claude Code
https://claude.ai/code/session_01QTR7UwtzY7wvD4c9YjT38e