Check the whole booking a payment sends back, not only its lines - #1990
Conversation
When a buyer pays, the provider hands back what was being bought. Only the lines being bought were checked; every other field was taken at its word. A field that had drifted or been tampered with therefore reached the booking code unchecked — modifiers sent as anything other than a list fell over the moment booking tried to walk them, after the buyer had already paid. The booking now has one description of what it is, in its own file, and the whole thing is checked against that description on the way back in. Text that cannot be read at all becomes a value nothing accepts, so a garbled field fails the whole reading rather than going quietly missing. Three consequences, each on purpose: - A checkout that is ours but will not read is raised for the owner. It cannot be booked, and asking the provider again would only bring back the same unreadable answer — but the buyer has been charged, so it goes to the owner's log rather than passing in silence. That is the whole difference from a checkout that might not be ours at all, which is still left alone without comment. - A day count that is not a whole number now fails the booking instead of being quietly dropped. Our checkout only ever writes a whole number there, so anything else means what came back is not what we sent. - A written answer whose stored text cannot be found is still forgiven. That can genuinely be lost between the form and the callback, and the money is already taken, so that one answer is dropped and noted while the rest of the order goes through. Also adds direct tests for five parts of the payment path that had none of their own — the page a cancelled checkout lands on, judging whether a checkout is ours, rebuilding a booking from its written rows, finishing off a paid booking, and picking up a booking whose write ended in doubt. Each was reached only through a whole request, if at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (43)
Comment |
![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: 1768bc2b3a
ℹ️ 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".
| * Classify a paid session and, when it is provably ours, pull out its booking | ||
| * intent in one step. Returns `null` for an "ignore" verdict — a session with | ||
| * no valid price proof (a foreign, replayed, or corrupt session) that we must | ||
| * not process or refund. Otherwise returns the signed verdict and the booking | ||
| * intent: a valid proof means the metadata is byte-for-byte what we signed, so | ||
| * `extractIntent` always parses. | ||
| * not process or refund — and for a session whose proof is valid but whose | ||
| * booking will not read back. |
There was a problem hiding this comment.
Condense this 12-line block to the one non-obvious constraint: a verified but unreadable booking is logged. The rest repeats the ignore and null branches immediately below, contrary to the repository requirement that comments normally stay within one or two lines and never re-narrate the code; keeping this much duplicated prose makes future behavior changes likely to leave the explanation stale.
AGENTS.md reference: AGENTS.md:L71-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, and taken in 40628fa. Twelve lines down to four.
The cut keeps only what the code below cannot say for itself — that both answers stop the session, but only one is quiet, because only one of them means the buyer was charged. Everything else was me restating the two branches immediately underneath, which is exactly the smell the guidance names: prose that has to be kept in step with code it merely repeats.
Small note on the badge: this came through as P1 alongside a real money-path finding on the same file. A comment that is too long is worth fixing, but it is not the same order of problem as a charged buyer with no booking, and grouping them at the same severity makes the important one easier to miss.
Generated by Claude Code
| if (intent === null) { | ||
| logUnreadableBooking( | ||
| `Signed session's booking could not be read (session=${session.id})`, | ||
| ); | ||
| return null; |
There was a problem hiding this comment.
When the price proof verifies but extractIntent rejects one metadata field—such as the signed modifiers: "{}" case added in the webhook test—returning null sends the session down the unrecognized/ignored path before reservation or payment processing. The provider refund is therefore never called, every redelivery repeats the same outcome, and the buyer remains charged without a booking; a valid proof already establishes that the session is ours, so this case needs a terminal refund/recovery path rather than being conflated with a foreign session.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right about the mechanism, and I am not taking the fix. Let me separate the two.
The facts are as you describe. classifySessionIntent returning null reaches handlePaymentWebhook's ignore branch, which acknowledges without processing or refunding. Redelivery repeats it. The buyer stays charged with no booking.
But auto-refunding here would be the wrong call, and this PR is the reason why. The most likely cause of "our own signed metadata will not read" is not tampering — it is us tightening the schema. This very change does exactly that: a day_count that is not a whole number now fails the whole booking where it used to be dropped. Deploy that, and any checkout signed minutes earlier under the looser rules reads as unreadable. Auto-refund would then give back money for a queue of perfectly good, paid bookings because of a change we made, irreversibly, before anyone noticed.
That is the case AGENTS.md reserves for the operator: "Silently moving money … hides a real decision behind a guess; an explicit operator choice keeps the irreversible call — especially anything that touches the money ledger — with the human who can see the context." A refund is the irreversible arm here; leaving the money and raising it is the recoverable one. If the reading is unreadable because of our own deploy, the fix is to relax the rule and let the booking through, and the money still being there is what makes that possible.
Where I agree with you: a log line is a weak terminal state. The buyer is charged, and "the owner might read their log" is not a process. The right home is a payment case the owner acts on — a durable row that says what happened, holds the evidence, and offers refund as an explicit choice. That machinery is #1973's payment_cases and payment_case_decisions, deliberately left out of this slice.
So this PR moves it from silence to a classified error that reaches the activity log, ntfy and Sentry, which is strictly better than before but is not the destination. I will record the gap against the case work on #1973 rather than let it disappear with this thread.
One correction to the framing: it is not "conflated with a foreign session" any more. Both return null, but only the provable one is logged, and it carries its own step so the two read differently to whoever is looking. That distinction is what the change added.
Generated by Claude Code
| src/shared/sort-listings.ts:50:30 1 → 0 # non-negative result; see block comment above | ||
| src/shared/sort-listings.ts:50:30 1 → 2 # sign-preserving magnitude change; see block comment above |
There was a problem hiding this comment.
These two entries describe the positive return 1 branch, which is still at src/shared/sort-listings.ts:49:30; line 50 contains return -1, whose literal mutant is separately recorded at column 31. Because ignore.ts matches equivalents by exact path, line, column, and operators, the new 50:30 1 → ... keys match no generated mutant: the global equivalent-mutant audit reports them as missing, and a mutation run covering sort-listings.ts leaves the real line-49 equivalents unsuppressed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this was my bug — fixed in 40628fa.
The cause is worth naming because it is subtle. When I shifted these entries I assumed the recorded column counts from zero, and calibrated that against the ?? → || entries, where it holds. It does not hold for the plain-number ones: on line 49, return 1 puts the 1 at zero-based column 29, so the recorded 30 is one-based. My check therefore failed to see a match on the correct line, searched outward, and landed on line 50 — moving a working entry onto a line where nothing matches, which is the exact fault the shift was meant to fix.
Redone from the list the mutation runner itself printed rather than from my own detection: sixteen entries, all in files this change touches, and the entries I had wrongly moved — these two plus accounting/queries.ts and db/groups.ts — are back untouched. git diff on that file is now exactly 16 lines changed.
Using the tool's own output as the source of truth was the right move from the start; my checker was only ever needed to find candidates, not to decide them.
Generated by Claude Code
Two things a review caught. Shifting the known-equivalent mutant entries, I assumed the recorded column counts from zero. It does for the operator entries but not for the plain-number ones, so two entries about `return 1` were moved onto the next line, where nothing matches — which suppresses nothing and hides the real mutant, the exact fault the shift was meant to fix. Redone from the list the mutation runner itself printed rather than from my own guess at which entries had moved: sixteen entries, all in files this change touches, and the entries I had wrongly moved are left alone. The comment above the session check had grown to twelve lines that mostly said again what the four lines below it do. Cut to the one thing that is not obvious from reading them: both answers stop the session, but only one is quiet, because only one of them means the buyer was charged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
The test that proves stripe-mock is given time to shut down leaned on the two second default budget. Under a full coverage run the mock can be starved long enough to miss it, and it looks like the stopper killed it too soon. Pin the budget in the test, the way the sibling kill test already does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a71208cae
ℹ️ 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".
| ), | ||
| ); | ||
|
|
||
| export const BookingItemsSchema = v.pipe( |
There was a problem hiding this comment.
BookingItemsSchema no longer has an external production consumer: production only references it within this module through BookingIntentSchema, while its sole external import is the dedicated test. Keeping this public surface after migrating the production parser to the whole-intent schema leaves a test-only export; make it private and exercise item rules through the production schema instead.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and taken in 3f5ff7a. BookingItemsSchema is now private.
Confirmed before changing it: production's only reference is the items field of BookingIntentSchema in the same file, and the sole outside import was the test. That is the dead-export signal in AGENTS.md, and the fix there is to remove the export rather than keep it alive for the test.
So the item cases now go through BookingIntentSchema, each line checked as the single line of a whole booking — which is how production checks one anyway, so the tests got closer to the real thing rather than further from it. The array rules moved with them: no lines at all, and one bad line among good ones, are now stated as things a booking refuses.
BookingItem (the singular type) stays exported — it has real production callers in create.ts, package-pricing.ts, store-refund.ts and signed-metadata.ts.
Generated by Claude Code
|
|
||
| /** A piece of text that may be left out, but has to pass the test when it is | ||
| * there. */ | ||
| export type OptionalCheckedString = v.OptionalSchema< |
There was a problem hiding this comment.
OptionalCheckedString has no consumer outside this file; it only annotates optionalStringThat locally. Exporting it creates an unused public API with no production caller, so remove the export modifier while retaining the local return annotation.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, done in 3f5ff7a — the export is gone and the return annotation stays.
Both references were inside validation/string.ts itself. Worth noting the annotation is not optional here: AGENTS.md asks for an explicit return type on every exported function, and optionalStringThat returns a nested valibot schema type that is unpleasant to re-infer at each call site. Naming it locally keeps that benefit without publishing a type nobody imports.
Generated by Claude Code
| * paid. Whether the key names a listing that was actually booked is a question | ||
| * only the booking code can answer, and is noted in TODO.md. |
There was a problem hiding this comment.
This comment says that checking whether an answer key names a booked listing is recorded in TODO.md, but a repo-wide search of that file finds no such entry. That leaves the deferred validation question undocumented while directing maintainers to a nonexistent plan; add an actionable TODO entry or remove the claim.
AGENTS.md reference: AGENTS.md:L71-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caught me — the comment promised something I had not written. Fixed in 3f5ff7a by writing it, since the question is real and worth keeping.
The new TODO.md entry says what the shape rule cannot: a key of "12" on an order for listings 3 and 7 is written exactly like a listing id, so it passes, and then saveSessionAnswers walks the booked listings, finds nothing filed under 3 or 7, and saves no answers. The buyer answered a question and it goes nowhere quietly — which is the same failure the shape rule exists to prevent, just arriving by a different route.
It also says why the schema is the wrong place for it: the schema validates one booking's metadata on its own, and which listings were actually bought is settled later, after the items are priced and loaded. saveSessionAnswers is the first point that holds both halves, so that is where the entry points, along with the test file where the shape rule is covered and this one is not.
Generated by Claude Code
| /** Per-listing answer IDs: maps listingId → answerIds for that listing's questions */ | ||
| listingAnswerIds?: Record<string, number[]> | undefined; | ||
| /** Per-listing free-text string refs: maps listingId → question/string ids. */ | ||
| listingTextAnswerIds?: Record<string, TextAnswerRef[]> | undefined; |
There was a problem hiding this comment.
ListingAnswerRefs is also the type used by CheckoutIntent, but its listingTextAnswerIds now uses the tolerant callback-side TextAnswerRef, whose s value may be undefined. This weakens the checkout writer contract from the previous required numeric string ID, so checkout construction can compile with an unresolved ID; JSON.stringify then omits s, and after payment the callback deliberately drops that answer. Use StoredTextAnswerRef for checkout/writer data and keep the tolerant shape confined to parsed callback data.
AGENTS.md reference: AGENTS.md:L90-L90
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the real one of the four, and you are right. Fixed in 3f5ff7a.
ListingAnswerRefs has exactly one consumer — CheckoutIntentBase in payments.ts:98, so it is purely the writer shape, built in ticket-submit/parse.ts. Pointing its listingTextAnswerIds at the tolerant TextAnswerRef let checkout compile with s missing, JSON.stringify would then drop the key, and the reader would do exactly what it is designed to do: treat the answer as unresolvable and drop it after the buyer had paid. A forgiving reader is right; a forgiving writer just moves the failure somewhere nobody is looking.
So the type now uses StoredTextAnswerRef and TextAnswerRef stays confined to parsed callback data, where its only consumer is resolveTextAnswerRefs in create.ts.
The doc comment was wrong as well and I have corrected it: it claimed the type was "shared by the booking and checkout intents", which stopped being true when BookingIntent became inferred from BookingIntentSchema. Nothing on the booking side references it any more.
Generated by Claude Code
Stop exporting the item-array schema and the checked-string helper type. Both had no caller outside their own file, and the schema's only outside use was its own test, so the item rules are now checked the way production checks them: as the lines of a whole booking. Put the strict reference back on the checkout side. Only the reader needs to forgive a string id it cannot use; letting the writer share that shape meant a checkout could be built with an id it never resolved, and the answer would be dropped after the buyer had paid. Write down the answer-key question the comment already promised was written down: a key can be shaped like a listing id and still name a listing nobody bought, and only the booking code can tell.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f5ff7a8fd
ℹ️ 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".
| /** One signed booking line, schema-first: `e` listing id, `q` quantity, `p` | ||
| * signed line total in minor units, and the optional edge tag (`k` code + `r` | ||
| * group id) the webhook's nodeKey revalidation reconstructs. The writer | ||
| * (signedEdgeFor) and every reader parse against THIS schema, so a drifted or | ||
| * tampered blob is a loud parse failure — never a silently-wrong nodeKey. |
There was a problem hiding this comment.
Replace these consecutive docblocks with a short note containing only the non-obvious edge-pair constraint. The first block already explains the compact fields and provenance, while this second 16-line block repeats those details and narrates the schema immediately below, making the explanation likely to drift when the schema changes.
AGENTS.md reference: AGENTS.md:L71-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken in ef43c64, and it was worse than duplication.
Neither block was attached to BookingItemSchema. Both had ended up above const positiveInt = integerAtLeast(1) — so a one-line constant carried twenty-six lines about a schema further down, and the schema itself had no comment at all. That is fallout from moving this code out of payments.ts, and I did not spot it.
It is one block now, sitting on the schema, keeping only the two things the code cannot say for itself: that a quantity of zero is deliberate rather than a bug, and that the edge tag is a pair because half of one would quietly rebuild the wrong nodeKey instead of failing loudly. The field-by-field narration is gone — e, q and p are named in the schema three lines below it.
26 lines down to 8.
Generated by Claude Code
Moving this out of payments.ts left two doc blocks saying the same thing, and both had drifted onto the small constant above the schema rather than the schema itself. One block now, attached to the schema, keeping only what the code cannot say: why a quantity of zero is allowed, and why the edge tag is a pair.
2 participants
A slice off #1973, small enough to read and merge on its own. It carries the bug fix from that branch and none of the new tables.
What was wrong
When a buyer pays, the provider hands back what was being bought. We checked the lines being bought properly, and took every other field at its word.
So a field that had drifted, or been tampered with, reached the booking code unchecked. Modifiers sent as anything other than a list fell over the moment booking tried to walk them — after the buyer had already paid.
What this does
The facts a booking is made of now live in one file,
src/shared/booking-intent.ts, as a single description of what a booking is. The whole booking is checked against that description on the way back in, not just its lines. Text that cannot be read at all becomes a value nothing accepts, so a garbled field fails the whole reading rather than one field going quietly missing.Three consequences, each on purpose:
A checkout that is ours but will not read is raised for the owner. It cannot be booked, and asking the provider again would only bring back the same unreadable answer — but the buyer has been charged, so it goes to the owner's log. That is the whole difference from a checkout that might not be ours at all, which is still left alone without comment: we do not act on money we cannot prove is ours.
A day count that is not a whole number now fails the booking instead of being quietly dropped. Our own checkout only ever writes a whole number there, so anything else means what came back is not what we sent — and booking the wrong number of days is worse than not booking.
A written answer whose stored text cannot be found is still forgiven. That one can genuinely be lost between the form and the callback, and the money is already taken, so that single answer is dropped and noted while the rest of the order goes through. It is the only thing the check forgives, and it says so at the definition.
Tests for five things that had none
Five parts of the payment path had no test of their own, and were reached only through a whole request, if at all:
Writing them turned up four places where existing tests looked like they were checking something and were not: half a rebuilt booking went unexamined behind a partial match, four log calls could all have been deleted with everything still green, a discount was only ever tested at zero, and tickets were never checked for being handed back separately.
Also here
Nineteen entries in the known-equivalent mutants list named lines that had since moved. An entry pointing at the wrong line suppresses nothing and hides the real mutant, so each is shifted to where its operator actually is.
Size
The src half is close to a wash because most of it is the booking description moving out of
payments.tsinto its own file.🤖 Generated with Claude Code
https://claude.ai/code/session_01JxEwtb9N1DGK1ks2VM3345
Generated by Claude Code