-
-
Notifications
You must be signed in to change notification settings - Fork 3
Close the plan review round that landed after the merge #2058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0f4e20c
019c412
7beab51
7a3962e
90a54c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,12 +198,13 @@ Src target: 400–700. | |
| overlapping refunds until the provider's cumulative total catches up while | ||
| counting a completed refund immediately; reject duplicate resources, wrong | ||
| currencies, wrong parents, over-refunds, and money on a free checkout. | ||
| - Conflicts that need an owner decision (multiple captures and kin): detect, | ||
| record, and alert through the existing error classes, but keep today's | ||
| behavior — these outcomes are not cut over, today's classifier keeps governing | ||
| them, and no automatic work is stopped or stranded before an owner can act. | ||
| The case workflow arrives one merge later (M5) and the page actions with | ||
| M7/M8. Build no owner tooling on the legacy engines. | ||
| - Conflicts that need an owner decision (multiple captures and kin): `outcomeOf` | ||
| is the only classifier here too — the displaced classifier is deleted in this | ||
| same merge, so two judges can never disagree about the same money. One handler | ||
| maps these outcomes onto today's behavior: detect, record, and alert through | ||
| the existing error classes, and no automatic work is stopped or stranded | ||
| before an owner can act. The case workflow arrives one merge later (M5) and | ||
| the page actions with M7/M8. Build no owner tooling on the legacy engines. | ||
|
|
||
| Standalone value: the live system stops repeat refunds and detects captured | ||
| money combinations it currently misses, with one classifier where there were | ||
|
|
@@ -276,13 +277,27 @@ happen in production. | |
| providers at once. SumUp keeps its local payment, checkout, and transaction | ||
| IDs distinct and uses stored currency. A checkout spanning several listings | ||
| stays one payment with one shared order: the stored intent allocates the | ||
| expected total across its listing lines exactly — the existing | ||
| largest-remainder rules make the parts sum to the whole — and reconciliation | ||
| validates money against that allocation, so no listing is ever credited the | ||
| full payment and the shared order is never collapsed. Store the provider on | ||
| each charge: M6's own reconciliation reads it to validate and deduplicate | ||
| charge identity, and the M7 engine routes refunds by it, closing the | ||
| multi-provider gap. | ||
| ticket-line money across its listing lines exactly — the existing | ||
| largest-remainder rules make the parts sum to the whole — and keeps every | ||
| booking fee or price-modifier extra as its own fact, never folded into a | ||
| listing's share, because Money credits those to their own accounts. Listing | ||
| lines plus extras sum to the payment total, reconciliation validates money | ||
| against that allocation, and no listing is ever credited the full payment or | ||
| another line's fee; the shared order is never collapsed. A deposit | ||
| (reservation) checkout stores two levels for every line and extra: the amount | ||
| charged now, whose parts sum to this payment's total, and the full obligation | ||
| it represents, so the later balance payment knows what remains and the full | ||
| modifier fact is never lost to the deposit fraction — today's pricing folds | ||
| the deposit share into ticket lines while Money records the whole modifier, | ||
|
Comment on lines
+287
to
+291
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For a refunded reservation, these two levels drive different irreversible effects: the provider must return only the cash charged now, while Money must cancel the full ticket and modifier obligation (plus the associated fee/payment legs) or the attendee retains debt for a cancelled booking. M7 later refers only to the stored per-listing allocation and queued amounts, without selecting a level or including the separately stored extras, so the new dual-level record still permits either an excessive provider refund or an incomplete ledger reversal. Define the provider refund from the charged facts and the Money cancellation from the full obligations and every extra. AGENTS.md reference: AGENTS.md:L73-L79 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 7beab51. M7's queued pages now draw two amounts from the stored facts: the provider refund returns only money actually charged, while the Money cancellation consumes the full obligation each line and extra represents — the buyer gets back exactly what they paid and a cancelled booking leaves no debt behind. Recorded as fault F62. Generated by Claude Code
Comment on lines
+287
to
+291
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For a reservation with a negative price modifier, this two-level record still covers only lines and extras: AGENTS.md reference: AGENTS.md:L73-L79 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR merged before the round landed, so the fix is in #2060 (commit 9fb5ec7): the canonical allocator now stores every price-modifier application as its own signed fact — discount or surcharge, even where today's pricing folds it into line prices — so completion and refund accounting never net a discount into listing income. Recorded as fault F76. Generated by Claude Code |
||
| and both facts must survive as themselves. One canonical allocator produces | ||
| this record for the whole payment — every listing line and every extra in one | ||
| pass, each part with a stable identity — and the record is stored with the | ||
| payment: reconciliation, refund routing, and balance completion read the | ||
| stored allocation and never re-derive it, so no two consumers can disagree | ||
| about a part. Today's ticket-only `allocateReservationDeposit` does not | ||
| survive the move. Store the provider on each charge: M6's own reconciliation | ||
| reads it to validate and deduplicate charge identity, and the M7 engine routes | ||
| refunds by it, closing the multi-provider gap. | ||
| - Reads: every provider read goes behind one strict observation contract | ||
| covering missing, invalid, unavailable, pending, paid, free, and failed. | ||
| Square payment IDs are named by the order, not scanned from a short list. | ||
|
|
@@ -357,8 +372,41 @@ over). | |
| Individual, bulk, balance, automatic, and case-decision refunds run through | ||
| one one-or-many engine. On a multi-listing payment, refund Money is recorded | ||
| against the payment's stored per-listing allocation — never the whole payment | ||
| to one listing. The migrated-payment caller arrives in M11, when migrated | ||
| payments first exist. | ||
| to one listing. The bulk arm runs to explicit provider, database, and total | ||
| subrequest budgets. The whole job — every payment identity it will refund, | ||
| plus a cursor — commits as durable due work before the first provider call; | ||
| each request then refunds a bounded page, records each payment's result in the | ||
| same transaction, and advances the cursor only past payments with a terminal | ||
| result. Terminal means the provider confirmed the refund or permanently | ||
| refused it — a permanent refusal becomes owner-review work, never silently | ||
| done. A transient failure (provider unreachable, rate limited) is not | ||
| terminal: it stays due for bounded retries and escalates to owner review when | ||
| they run out, so one stuck payment can neither spin forever nor be finished | ||
| around. Every refund item persists its provider idempotency key with the job | ||
| before the first call and reuses it on every retry — the shipped | ||
| `pending_refund_idempotency_key` column — so a provider call that succeeded | ||
| just before a lost local commit can never refund the same money twice. A crash | ||
| mid-run therefore leaves a job that still names every untouched payment — a | ||
| large refund-all can never end with an initial subset refunded and nothing | ||
| recorded. Today's `processRefundBatch` loops every group unbounded, and that | ||
| shape does not survive the move. Each queued page is self-contained: it | ||
| carries the provider-qualified payment identities, amounts, and allocation | ||
| facts it acts on — never a live attendee lookup. Refunding a reservation | ||
|
Comment on lines
+392
to
+394
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A queued page can wait while reconciliation records a second captured charge, an external refund, or another contradiction that changes AGENTS.md reference: AGENTS.md:L304-L313 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR merged before the round landed, so the fix is in #2060 (commit 9fb5ec7): each queued refund item stores the payment-evidence revision it was built from, and the claiming transaction re-runs the outcome and blocking-case checks before every provider call — a payment whose evidence moved on (a second captured charge, an external refund, a newly opened case) parks as owner-review work instead of being refunded from a stale snapshot. Recorded as fault F74. Generated by Claude Code |
||
| separates two kinds of reversal: each payment page returns and reverses only | ||
| the cash that payment actually moved (the deposit's charge, the balance's | ||
| charge), while the booking-level obligation — sale, modifier, and fee facts | ||
| shared by all of that reservation's payments — is cancelled exactly once, | ||
| idempotently, however many payments the refund touches. That cancellation is | ||
| recorded as a completion effect under one stable booking-level identity, | ||
|
Comment on lines
+397
to
+400
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When M8 refunds a reservation because its all-or-none completion failed, such as when one listing sold out, the booking transaction committed no sale, modifier, or fee obligations. This rule nevertheless directs the shared M7 refund path to cancel the full booking-level obligation, which would post inverse accounting legs for a booking that never existed; the current AGENTS.md reference: AGENTS.md:L73-L79 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR merged before the round landed, so the fix is in #2060 (commit 9fb5ec7): a refund for a failed all-or-none completion is an unhonoured-payment refund — it returns cash only and posts no booking-level obligation cancellation, because the commit never posted the obligation; a cancellation runs only with proof the booking obligation effect completed. Recorded as fault F75. Generated by Claude Code |
||
| claimed atomically with the refund result, so recovery retries it when the | ||
| provider reversal succeeded but the local write failed — and can never run it | ||
| twice. The buyer gets back what they paid, nothing is reversed twice, and a | ||
| cancelled booking leaves no debt behind. While an attendee has unfinished | ||
| refund pages, merging or deleting that attendee fails closed naming the | ||
| pending work — a merge posts its own Money adjustments, and replaying a | ||
| pre-merge allocation after them could reverse income twice; M8's general | ||
| repointing for queued work then replaces this fence. The migrated-payment | ||
|
Comment on lines
+407
to
+408
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The fresh wording now explicitly replaces the M7 fence with M8 repointing, which reintroduces the earlier accounting race. When a merge resolves conflicting bookings, AGENTS.md reference: AGENTS.md:L304-L313 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR merged before the round landed, so the fix is in #2060 (commit 9fb5ec7), agreeing with your analysis: the fence now outlives M8 for refund pages — repointing cannot make a frozen pre-merge allocation safe to replay after the merge's own adjustments, so merges stay refused until the refund work settles or the owner cancels it, and M8's general repointing covers only queued work whose facts a merge leaves unchanged. Recorded as fault F73. Generated by Claude Code |
||
| caller arrives in M11, when migrated payments first exist. | ||
| - Persist provider refund identity before local completion; queue and schedule | ||
| repair when provider success is followed by a local failure; keep callback and | ||
| admin replay idempotent; keep refunds available while new sales are disabled. | ||
|
|
@@ -400,9 +448,32 @@ one machine and merge together. | |
| Money effects before running them; snapshot paid facts so listing edits cannot | ||
| change delivery. Complete each effect idempotently, schedule unfinished work, | ||
| and stop one permanent failure from starving later payments. A multi-listing | ||
| payment completes per listing line from M6's stored allocation — each line's | ||
| capacity, tickets, and Money draw only that line's amount, and the shared | ||
| order survives completion. | ||
| payment draws each line's capacity, tickets, and Money from M6's stored | ||
| allocation, but the commit is all lines or none in one transaction — as | ||
| `createBookingAtomic` commits the shared order today — so one line selling out | ||
| after payment sends the whole completion down the failure path (refund or | ||
| owner case) rather than half-booking the order. Before the effect runner | ||
| claims its first payment, an idempotent cutover pass adopts the M6-window | ||
| history: an aggregate payment the legacy path already completed has its folded | ||
| result marked done, never re-booked or re-posted to Money; a paid aggregate | ||
| payment with no completion result becomes due work; and a paid payment whose | ||
| folded result records a completion failure becomes the matching durable | ||
| failure effect — its chosen refund path or an owner case — never marked done | ||
| and never re-run as a fresh booking. So pre-M8 unfinished completions gain | ||
| durable recovery instead of being stranded. The pass runs only after the write | ||
| fence has risen and in-flight legacy commits have drained or failed, and the | ||
| runner revision-rechecks the folded state when claiming each payment, so a | ||
| legacy commit that landed between scan and claim is honoured, never redone. | ||
| Adoption also gates on a completion-safe `outcomeOf` state with no open | ||
| blocking case: a paid payment stopped for owner review — captured money on a | ||
| failed checkout, multiple captured charges — stays in its case workflow, | ||
| because due work must never bypass a required owner choice. Refund and | ||
| completion claims are mutually exclusive through one payment-wide claim: the | ||
| payment session row's shipped lease (`lease_token`, `lease_expires_at`). | ||
| Refund jobs, the adoption pass, and the effect runner each acquire that lease | ||
| atomically before acting and verify no unfinished refund job or effect owns | ||
| the payment, so two runners can never both read "nothing done yet" and act, | ||
|
Comment on lines
+471
to
+475
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A payment-session lease excludes work only on that one session row, but a reservation can have separate deposit and balance payments sharing one booking-level obligation. If a refund runner holds the deposit session while the balance completion runner claims the balance session, both checks can pass and the booking can complete while its deposit is being irreversibly refunded. Use a booking-level claim, or atomically acquire and verify every related payment-session lease before either path acts. AGENTS.md reference: AGENTS.md:L73-L79 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR merged before the round landed, so the fix is in #2060 (commit 9fb5ec7): the claim spans every payment sharing the booking-level obligation — a runner acquires the lease of each related payment session, always in one fixed order, before acting, so a deposit refund and a balance completion can never split one booking between two sessions. Recorded as fault F72. Generated by Claude Code |
||
| and a booking can never complete while its irreversible refund is in flight. | ||
| - From this cutover on, completion stops storing payment references in attendee | ||
| PII — the aggregate owns the attendee-to-payment link — so the attendee-PII | ||
| reference source M11 reads is closed at M8 and cannot grow while the copy | ||
|
|
@@ -685,6 +756,27 @@ mechanism and a regression test, or — if implementation proves the finding wro | |
| | F48 | A multi-listing payment credited in full to each listing, or its shared order lost | M6, M7, M8 | | ||
| | F49 | Legacy-only booking facts dropped when a dual-store row is marked settled | M11 | | ||
| | F50 | Unmigratable evidence keeping buyer PII or ticket tokens forever | M11, M12 | | ||
| | F51 | Two classifiers disagreeing about the same settled money between M4 and M5 | M4 | | ||
| | F52 | Checkout fees or price modifiers misallocated into a listing's income | M6, M8 | | ||
| | F53 | A bulk refund run exceeding the request budget, refunding only an initial subset | M7 | | ||
| | F54 | One sold-out line half-booking a multi-listing order after payment | M8 | | ||
| | F55 | The M8 runner re-completing sales the legacy path already finished | M8 | | ||
| | F56 | A deposit checkout losing the full modifier fact to the charged fraction | M6, M8 | | ||
| | F57 | The adoption pass racing an in-flight legacy commit and re-running its completion | M8 | | ||
| | F58 | Adoption turning an owner-review payment into due work, bypassing the required choice | M8 | | ||
| | F59 | A queued refund page stranded by an attendee merge or delete in the M7 window | M7 | | ||
| | F60 | A refund-all crash after its first page losing the unrecorded remainder | M7 | | ||
| | F61 | An attendee merge rewriting Money while refund pages are still queued | M7 | | ||
| | F62 | A reservation refund confusing money charged now with the full obligation | M7 | | ||
| | F63 | A deposit-plus-balance refund reversing the booking obligation twice or not at all | M7 | | ||
| | F64 | Adoption stranding payments whose folded result records a completion failure | M8 | | ||
| | F65 | A cursor advancing past a transiently failed refund, finishing the job around it | M7 | | ||
| | F66 | A booking completing while its payment's irreversible refund is in flight | M8 | | ||
| | F67 | A retried refund minting a fresh provider idempotency key and refunding twice | M7 | | ||
| | F68 | Transient and permanent refund failures collapsing into one boolean | M7 | | ||
| | F69 | An obligation cancellation without a stable identity re-running or never retrying | M7 | | ||
| | F70 | Two runners both reading "nothing done yet" and acting on one payment | M8 | | ||
| | F71 | A consumer re-deriving the allocation and disagreeing with the stored record | M6 | | ||
|
|
||
| ## Done means | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reservation checkouts with a positive modifier, the current pricing model deliberately folds the modifier-adjusted deposit into ticket lines and omits modifier extras (
src/shared/checkout-pricing.tslines 333–345), while Money records the full modifier application separately (src/shared/checkout-ledger.tslines 44–50). The existing £10 ticket + £5 modifier + 10% deposit test therefore produces a £1.50 ticket line and £1.50 booking fee, not a £5 modifier extra. Requiring every modifier extra to remain outside the listing share while also requiring lines plus extras to equal the payment total leaves no correct representation for this valid checkout: implementing it literally either breaks payment reconciliation or loses the full modifier accounting fact. Specify separate stored facts for the amount charged now and the full modifier obligation.AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 019c412. The allocation now stores two levels for a deposit checkout, per line and extra: the amount charged now (whose parts sum to this payment's total — reconciliation stays exact) and the full obligation it represents (so the balance payment knows what remains and the full modifier fact survives the deposit fraction). The plan names the current shape — pricing folds the deposit share into ticket lines while Money records the whole modifier — and requires both facts to survive as themselves. Recorded as fault F56.
Generated by Claude Code