Repository navigation
Conversation
The quote currency is caller-supplied and nothing downstream honours it. Approvals reserve funds in intent.currency, the card is issued in the payment provider's currency, and routes/intents.ts derives the intent currency server-side precisely "so ledger entries and card issuance can never diverge". Its one and only consumer was the Telegram approval message: const currency = ((metadata.currency as string) ?? intent.currency).toUpperCase(); which then labelled both lines of that message -- including the Budget line, which renders intent.maxBudget, a value the agent never supplied. An agent quoting an EUR intent with currency "gbp" therefore produced Price: 50.00 GBP Budget: 100.00 GBP while the card was issued and the ledger reserved in EUR. The agent could not change what was spent, but it was the sole author of the label on the one screen where the user consents. Approving a materially mislabelled amount is a consent problem even when the money movement is correct. Two changes: - The notification now labels with intent.currency unconditionally. - The quote route rejects a supplied currency that differs from the intent's with 400 quote_currency_mismatch, so an agent is told rather than having the field silently ignored. currency also becomes optional rather than defaulting to 'eur'. The intent currency comes from the user's payment provider and need not be EUR, so a default would either mislabel the message or, with the new mismatch check, reject a caller that simply omitted the field. Absent now means "use the intent's". Found while reviewing #216, which surfaces this same unvalidated field through a new MCP tool. The bug predates that PR and is fixed here against main instead.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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 happy path created an intent and then quoted it in GBP. routes/intents.ts ignores the caller's currency and derives it from the user's payment provider, which is eur for stripe, so the intent was EUR throughout while the quote said GBP -- and the approval message rendered both the price and the user's budget as GBP. The repository's own end-to-end test was exercising the mislabelling this branch fixes, which is why the new 400 surfaced here first. Quotes in eur now, and the misleading currency field is dropped from the create body since the route never reads it.
|
CI green after Worth recording what the first CI run turned up, because it is the strongest evidence for this change: the integration failure was the repository's own happy path exercising the bug.
The new 400 surfaced it immediately. Fixed the test to quote in the intent's currency and dropped the This also answers the obvious "is a hard 400 too strict?" question: the only caller in the repo that tripped it was one that genuinely was mislabelling. |
|
@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: 3b222e5ecc
ℹ️ 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".
| if (currency !== undefined && currency.toLowerCase() !== intent.currency.toLowerCase()) { | ||
| return reply.status(400).send({ | ||
| error: `Quote currency ${currency.toLowerCase()} does not match intent currency ${intent.currency.toLowerCase()}`, | ||
| code: 'quote_currency_mismatch', |
There was a problem hiding this comment.
With this mismatch check, the documented OpenClaw flow now fails for a Stripe intent: docs/openclaw.md:274-282 and docs/api.md:340 tell callers to send or default to GBP, while intent creation derives EUR from the Stripe provider, so following the published example returns 400 quote_currency_mismatch instead of reaching approval. Update both request descriptions and error tables to explain that omission uses the intent currency and that any supplied value must match it.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
2 participants
Found while reviewing #216. The bug predates that PR — it is live on
maintoday — so it is fixed here againstmainrather than inside a feature branch. Credit to Codex, which flagged the unvalidated field on #216.The bug
agentQuoteSchemaaccepts any 3-charactercurrencyand defaults it to'eur'. The quote route passes it intoreceiveQuoteunvalidated and it lands inintent.metadata.Nothing that moves money reads it.
approvals.ts:121reserves inintent.currency, card issuance uses the provider's currency, androutes/intents.ts:45derives the intent currency server-side with the comment "so ledger entries and card issuance can never diverge."Its single consumer was the Telegram approval message:
which labels both lines it renders — including
Budget, which isintent.maxBudget, a value the agent never supplied. So an agent quoting a EUR intent withcurrency: "gbp"produced:while the card was issued and the pot reserved in EUR.
The agent cannot change what is spent. What it could do is author the label on the one screen where the user gives consent, including the label on the user's own budget. A user approving £100 for what is actually a €100 authorisation has approved something they were not shown, which is a consent problem regardless of the money movement being correct.
The fix
notificationServicelabels withintent.currencyunconditionally.400 quote_currency_mismatchwhen a supplied currency differs from the intent's, so an agent is told rather than having the field quietly ignored.currencybecomes optional instead of defaulting to'eur'. The intent currency comes from the user's payment provider and need not be EUR — a default would either mislabel the message or, combined with the new check, reject a caller that simply omitted the field. Absent now means "use the intent's".Tests
Four added. The notification test fails against the unfixed source with
Expected substring: "50.00 EUR"and passes after, so it pins the actual bug rather than the fix's shape. The route tests cover both the mismatch rejection and the omitted-currency path that the old'eur'default would have broken.416 unit tests, type check, lint and format green on Node 22.
Related
#216 exposes this same field through the
submit_quoteMCP tool. With this merged, that surface inherits the validation rather than needing its own.