Close the plan review round that landed after the merge - #2058
Conversation
- M4 deletes the displaced classifier in the same merge: outcomeOf is the only judge even for owner-decision conflicts, mapped onto today's behavior through one handler, so two judges can never disagree about the same money (F51). - M6's allocation covers ticket-line money only; booking fees and price-modifier extras stay their own facts because Money credits them to their own accounts — lines plus extras sum to the payment total (F52). - M7's bulk refund arm runs to explicit budgets: a bounded page per request, the remainder recorded as durable due work the scheduled runner continues (F53). - M8 keeps multi-listing completion all-or-nothing in one transaction, per-line accounting notwithstanding, and adopts the M6-window history before the effect runner claims payments: already-completed sales are marked done, unfinished ones become due work (F54, F55). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
![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: 0f4e20ce3b
ℹ️ 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".
| 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 |
There was a problem hiding this comment.
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.ts lines 333–345), while Money records the full modifier application separately (src/shared/checkout-ledger.ts lines 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.
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
| 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, and a paid | ||
| aggregate payment with no completion result becomes due work — so pre-M8 |
There was a problem hiding this comment.
During the M8 cutover, M6 explicitly leaves legacy completion writers unfenced, yet this pass can mark a paid payment as due before an in-flight legacy createBookingAtomic commits its folded result. The legacy request can then finish after the scan and the new runner can attempt the same booking and Money work again. The separate fence requirement later in M8 does not define this ordering; require the fence to rise and in-flight legacy commits to drain or fail before the adoption pass, or revision-recheck folded completion while claiming each payment.
AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 019c412, with both belts you proposed: the adoption 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 — a legacy commit that landed between scan and claim is honoured, never redone. Recorded as fault F57.
Generated by Claude Code
| result marked done, never re-booked or re-posted to Money, and a paid | ||
| aggregate payment with no completion result becomes due work — so pre-M8 |
There was a problem hiding this comment.
As written, the cutover queues every paid aggregate payment that lacks a completion result, but the plan also defines paid states that intentionally stop before completion: captured money on a failed checkout and multiple captured charges both require owner review (PLAN.md lines 111–117), and M6 opens an M5 case for the latter. Those payments can have no completion result precisely because automatic work was stopped, so turning them into due work lets the M8 runner bypass the required owner choice. Gate adoption on a completion-safe outcomeOf state with no open blocking case, leaving ambiguous payments in their existing case workflow.
AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 019c412. Adoption now 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. Recorded as fault F58.
Generated by Claude Code
| subrequest budgets: each request refunds a bounded page and records the | ||
| remainder as durable due work the scheduled runner continues, so a large |
There was a problem hiding this comment.
The new durable refund-all remainder becomes live in M7, but protection for queued work during attendee merges and attendee/listing deletion is explicitly deferred to M8 (PLAN.md lines 431–448). In the standalone M7 window, an owner can queue later refund pages and then merge or delete an affected attendee; this is especially unsafe for the M7 adapter's attendee-only legacy references, because the scheduled job can lose the facts or target ID it needs after the provider refund has already happened, stranding local Money repair. Move the relevant repointing and deletion fence into M7, or make these jobs self-contained and keyed only to payment facts that M6 already preserves.
AGENTS.md reference: AGENTS.md:L400-L404
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 019c412, taking the self-contained option: each queued refund page carries the provider-qualified payment identities, amounts, and allocation facts it acts on — never a live attendee lookup — so an attendee merge or delete between pages cannot strand a job whose provider refund already happened. M8 still adds the general repointing for other queued work kinds. Recorded as fault F59.
Generated by Claude Code
- A deposit checkout stores both the amount charged now (parts sum to this payment's total) and the full obligation each line or extra represents, so the balance payment knows what remains and the full modifier fact survives the deposit fraction (F56). - M8's adoption pass runs only after the fence has risen and in-flight legacy commits drained, with a revision re-check at claim time, and gates on a completion-safe outcomeOf state with no open blocking case — owner-review payments stay in their case workflow (F57, F58). - M7's queued refund pages are self-contained, carrying the payment facts they act on rather than live attendee lookups, so a merge or delete between pages cannot strand a job whose provider refund already happened (F59). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 019c412c9d
ℹ️ 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".
| 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 — so an attendee merge or delete between | ||
| pages cannot strand a job whose provider refund already happened; M8 adds the |
There was a problem hiding this comment.
When a later refund page is pending and the owner merges that attendee into one with a conflicting booking, preserving the page's payment identities and allocation facts does not make them safe to apply afterward: applyAttendeeMerge repoints the source's ledger transfers and posts the operator-selected unbill/credit/write-off adjustments (src/shared/merge/attendee-merge.ts lines 791–845), so replaying the pre-merge allocation can reverse listing income twice or post against the discarded money outcome. The fresh self-contained-page wording prevents missing inputs but does not address this semantic change; M7 must fence merges while refund work is unfinished, or atomically update the queued Money work to reflect the required merge decision rather than deferring general protection to M8.
AGENTS.md reference: AGENTS.md:L400-L404
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7beab51 with the fence option: while an attendee has unfinished refund pages, merging or deleting that attendee fails closed naming the pending work — the plan now states the reason you identified (a merge posts its own Money adjustments, and replaying a pre-merge allocation after them could reverse income twice). M8's general repointing then replaces the fence. Recorded as fault F61.
Generated by Claude Code
| to one listing. The bulk arm runs to explicit provider, database, and total | ||
| subrequest budgets: each request refunds a bounded page and records the | ||
| remainder as durable due work the scheduled runner continues, so a large | ||
| refund-all can never abort mid-way with only an initial subset refunded — | ||
| today's `processRefundBatch` loops every group unbounded, and that shape does | ||
| not survive the move. Each queued page is self-contained: it carries the |
There was a problem hiding this comment.
When the request refunds its bounded provider page and then crashes before the remainder is recorded, no scheduled work identifies the untouched payments; the provider-success/local-write repair described below can recover only payments that were attempted, so the exact initial-subset outcome this change is meant to eliminate still occurs. Require the durable refund-all job, cursor, or next-page identities to commit before the first irreversible provider call, and advance that state transactionally as pages are claimed.
AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7beab51. The whole job — every payment identity it will refund, plus a cursor — commits as durable due work before the first provider call, and each page advances the cursor in the same transaction as that page's results. A crash mid-run always leaves a job naming every untouched payment. Recorded as fault F60.
Generated by Claude Code
| (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, |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
- A refund-all job commits its full payment list and cursor as durable due work before the first provider call, and each page advances the cursor in the same transaction as its results, so a crash mid-run always leaves a job naming the untouched remainder (F60). - A reservation refund draws two amounts from the stored facts: the provider returns only money actually charged, Money cancels the full obligation of every line and extra (F62). - Merging or deleting an attendee with unfinished refund pages fails closed naming the pending work, since replaying a pre-merge allocation after the merge's own Money adjustments could reverse income twice; M8's general repointing replaces the fence (F61). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 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. 📝 WalkthroughWalkthrough
ChangesPayment integration plan
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7beab5170c
ℹ️ 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".
| each request then refunds a bounded page and advances the cursor in the same | ||
| transaction as that page's results, and the scheduled runner continues the | ||
| remainder. A crash mid-run therefore leaves a job that still names every |
There was a problem hiding this comment.
When a provider call transiently fails for one payment in a page, this rule still advances the cursor with that page's results, while the later repair rule covers only provider success followed by a local failure. The scheduled runner can therefore continue after the failed payment and eventually finish the job without retrying it. Keep every non-terminal provider result in durable due work, or advance the cursor only past terminal results.
AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a3962e. Each page records per-payment results in its transaction and the cursor advances only past payments with a terminal result — a transient provider failure stays due, so the scheduled runner retries it rather than finishing the job around it. Recorded as fault F65.
Generated by Claude Code
| reservation payment a page draws two amounts from those stored facts: the | ||
| provider refund returns only money actually charged, while the Money | ||
| cancellation consumes the full obligation each line and extra represents, so | ||
| the buyer gets back exactly what they paid and a cancelled booking leaves no |
There was a problem hiding this comment.
When a reservation is later settled by a balance payment, refund-all acts on at least the deposit and balance payment identities. Consuming the full booking obligation for each payment page can reverse the sale, modifier, and fee obligations twice; consuming it only for the deposit leaves the balance payment's Money leg without a defined reversal. Define per-charge cash refunds/reversals separately from one idempotent booking-level obligation cancellation shared by all of the reservation's payments.
AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a3962e. Refunding a reservation now separates the two reversals: 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. Recorded as fault F63.
Generated by Claude Code
| 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, and a paid | ||
| aggregate payment with no completion result becomes due work — so pre-M8 | ||
| unfinished completions gain durable recovery instead of being stranded. The |
There was a problem hiding this comment.
During M6, the combined read explicitly preserves a recorded local completion failure, but this adoption partitions only successful folded results into done and payments with no result into due work. A paid payment with a folded failure has a result but is not completed, so it falls into neither branch and never reaches M8's durable refund or owner-case path. Convert every folded failure into the appropriate durable failure effect before the runner begins.
AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a3962e. The adoption pass now has a third arm: 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. Recorded as fault F64.
Generated by Claude Code
| 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 |
There was a problem hiding this comment.
When an M7 refund page has claimed a payment and its provider request is still in flight, the stored evidence can still satisfy outcomeOf as paid and have no blocking case. M8 can therefore adopt and complete the booking just as the irreversible refund succeeds. Make refund and completion claims mutually exclusive, and require the adoption/runner claim to verify transactionally that no unfinished refund job or effect owns the payment.
AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a3962e. Refund and completion claims are now mutually exclusive: the adoption pass and the effect runner verify in the claiming transaction that no unfinished refund job or effect owns the payment, so a booking can never complete while its irreversible refund is in flight. Recorded as fault F66.
Generated by Claude Code
- Refunding a reservation separates per-payment cash reversal (each page returns only what that payment moved) from the booking-level obligation cancellation, which runs exactly once across the deposit and balance payments (F63). - The refund cursor advances only past terminal results; a transient provider failure stays due instead of being finished around (F65). - M8 adoption converts a folded completion failure into its matching durable failure effect rather than leaving it in neither branch (F64), and refund and completion claims are mutually exclusive, checked in the claiming transaction (F66). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@PLAN.md`:
- Around line 369-381: Update the durable refund-item workflow described around
processRefundBatch to assign and persist a stable idempotency key in the
existing pending_refund_idempotency_key field before any provider refund call.
Ensure retries reuse that same per-refund key rather than generating a new one,
including when a provider call succeeds but the local transaction fails.
- Around line 372-375: Update the M7 refund-processing description around
RefundOutcome to explicitly define the cursor advancement, due-work state, retry
behavior, and owner-review transition for refunded, failed, and errored
outcomes. Clarify how transient versus permanent provider failures are
represented despite both returning false, and require the cursor to advance only
for outcomes classified as terminal.
- Around line 381-386: Update the refund design described in this section to
persist a stable reservation-scoped obligation ID and record the
sale/modifier/fee cancellation effect exactly once across deposit and balance
payments. Claim that effect atomically with the local refund result rather than
relying on payment- or session-scoped ledger groups, and ensure recovery retries
the cancellation when provider reversal succeeds but the local transaction
fails.
- Around line 454-457: Specify a payment-wide durable claim or lease shared by
the M7 refund jobs, M8 adoption pass, and runner, including its atomic
acquisition and ownership rules. Ensure the claiming transaction prevents
concurrent runners from both observing no completed effect and booking the
payment, while preserving the mutual exclusion with unfinished refunds.
- Around line 280-294: Define a canonical complete-payment allocator that covers
every listing line and booking-fee or price-modifier extra, preserving stable
allocation identity, rounding basis, charged-now amount, and full obligation for
each item. Replace the ticket-only input used by allocateReservationDeposit with
the full PricedOrder allocation, persist these fields in the payment schemas,
and ensure reconciliation, refund routing, and balance completion consume the
persisted allocation rather than recomputing or collapsing it.
- M7 defines terminal: the provider confirmed or permanently refused the refund (a permanent refusal becomes owner-review work); transient failures stay due with bounded retries, then escalate (F68). - Every refund item persists its provider idempotency key (the shipped pending_refund_idempotency_key column) before the first call and reuses it on retries, so a lost commit cannot refund twice (F67). - The booking-level cancellation records as a completion effect under one stable booking-level identity, claimed atomically with the refund result, retried when the local write is lost (F69). - M8's refund/completion exclusion runs through the payment session row's shipped lease, acquired atomically by refund jobs, the adoption pass, and the runner (F70). - M6 names one canonical allocator over every line and extra, stored with the payment; consumers read it, never re-derive it (F71). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
|
All five CodeRabbit findings are fixed in 90a54c1 (CodeRabbit has stamped the threads "Addressed" itself):
Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90a54c1aa7
ℹ️ 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".
| (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, |
There was a problem hiding this comment.
For a reservation with a negative price modifier, this two-level record still covers only lines and extras: applyModifiers folds a discount into ticket prices and emits no ExtraLine, while bookingFactsFromOrder posts the modifier application separately. The stored allocation therefore cannot preserve the full discount fact without re-deriving it, so later completion or refund accounting can incorrectly net the discount into listing income. Store signed modifier applications as stable allocation facts alongside lines and positive extras.
AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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
| pre-merge allocation after them could reverse income twice; M8's general | ||
| repointing for queued work then replaces this fence. The migrated-payment |
There was a problem hiding this comment.
The fresh wording now explicitly replaces the M7 fence with M8 repointing, which reintroduces the earlier accounting race. When a merge resolves conflicting bookings, applyAttendeeMerge both repoints ledger rows and posts the operator-selected reversal adjustments; merely repointing a queued refund page afterward leaves its frozen pre-merge allocation unchanged, so running it can reverse the same income again. Keep merges fenced until refund work settles, or atomically rewrite the queued accounting work to reflect the operator's merge decision.
AGENTS.md reference: AGENTS.md:L304-L313
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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
| 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, |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
| 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, |
There was a problem hiding this comment.
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 recordPlaceholderRefund path deliberately records only the captured payment and cash refund in this state. Model an unhonoured-payment refund variant that reverses cash without creating an obligation cancellation, or require proof that the booking obligation effect completed before cancelling it.
AGENTS.md reference: AGENTS.md:L73-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
A queued page can wait while reconciliation records a second captured charge, an external refund, or another contradiction that changes outcomeOf and opens a required owner case. Because the page acts from frozen payment identities, amounts, and allocation facts, a later lease prevents concurrent work but does not invalidate this stale snapshot; the runner can therefore make an irreversible refund using evidence that is no longer completion-safe and bypass the owner decision. Store the source revision and, in the claim transaction before each provider call, re-run the current outcome and blocking-case checks, leaving the item pending when either changed.
AGENTS.md reference: AGENTS.md:L304-L313
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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
Fixes from the review round that landed after #2058 merged: - The M8 refund/completion exclusion spans every payment sharing a booking-level obligation — a runner acquires each related session lease in one fixed order, so a deposit refund and a balance completion can never split one booking (F72). - The merge fence 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 (F73). - Each queued refund item stores its payment-evidence revision, and the claim re-runs the outcome and blocking-case checks before every provider call, parking stale items as owner-review work (F74). - A refund for a failed all-or-none completion returns cash only and never posts an obligation cancellation the booking never posted (F75). - The canonical allocator stores every price-modifier application as its own signed fact, so a discount folded into line prices keeps its modifier identity (F76). Also marks the M3 observation-boundary slice as in review, and widens the asSession test helper to the provider union that now includes "retry" — the test-only type error precommit caught. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
2 participants
What changed
Two commits of
PLAN.mdrefinements. Codex's reviews kept running while #2057 sat in the merge queue and after it merged, so the fixes from those rounds land here (faults F51–F59 in the ledger):From the round that missed the merge (F51–F55):
outcomeOfjudges owner-decision conflicts too, mapped onto today's behavior through one handler, and the displaced classifier is deleted in the same merge (F51).From the review of this PR (F56–F59):
outcomeOfstate with no open blocking case, so owner-review payments stay in their case workflow (F58).Verification
Documentation-only diff.
deno fmt --checkpasses repo-wide. No src, test, or locale changes.🤖 Generated with Claude Code
https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd
Summary by CodeRabbit