Condense the payment plan into standalone milestones

[stefan-burke]

What changed

PLAN.md shrinks from 759 to ~630 lines, and the work it describes loses two full integration passes. Every binding decision, decided behavior, and known fault survives — most now live in a numbered fault ledger (F1–F50) instead of repeated prose. TODO.md is also reformatted in passing: the entries from #2046 had landed unformatted, which was failing deno fmt --check repo-wide.

Why

The merged plan-PRs ran 4–15x their written budgets (#2020: 714 src lines but 10,519 total; the two PR-3 slices #2048 + #2050: 12,187 lines and PR 3 still isn't finished). The plan also paid for the same logic twice: Phase 0 wired the new pure rules and a full case product into the legacy path that Phase 1 then deletes, and the PR 6/7 split forced throwaway compatibility projections.

The restructuring

• Standalone merges are now rule 1 (owner decision): every merge leaves a complete, smoke-testable system — no release trains.
• Milestones replace PR numbers (M1–M13, each noting the PR it was), since "PR 3" is already three real PRs. A status table records what has merged where.
• M4 (was PR 4) cuts live only fail-closed remedies; owner-decision conflicts are detected and alerted but stay governed by today's classifier until M5.
• M5 (was PR 5) slims to detection, buyer messaging, owner pages, alerts, and an acknowledge decision. Money-moving case actions ship with the engines that perform them (M7 refunds, M8 completion); until then the case page links to still-live admin tools, and a case re-checks its evidence so a fix made through a linked tool closes it.
• M6 (was PRs 6+7) moves checkout creation and reads in one merge, feeding the existing validated-session completion contract — so the sumup_checkouts projection, the Stripe/Square metadata preservation, and their repair machinery are never built. The M6 release carries the restore-deploy guard, which rejects pre-cutover commits outright once the cutover ships. A multi-listing checkout stays one payment with one shared order and an exact per-listing allocation that reconciliation validates. Its legacy read-through folds both stores' rows into one answer per sale — nothing counted or refunded twice, no local booking fact hidden — and legacy deletions in this window first record a sale's folded facts on the aggregate record. Pre-cutover sales keep today's deletion and retention lifecycle until M8's fence, a deliberate boundary the plan now explains.
• M7 (was PR 8) keeps the legacy-refund adapter for uncopied rows; its fail-closed cases ship with a full owner decision — supply every missing verified fact (and assign a shared reference to exactly one attendee) or reject. Refund Money is recorded against the payment's stored per-listing allocation.
• M8 (was PRs 9+10) merges completion with its failure-path refund (one machine) and installs the legacy write fence after the last routine legacy writer moves — one exemption instead of seven categories. Multi-listing payments complete per listing line from the stored allocation. Merge/delete/prune always complete; attendee deletion is fenced against unfinished payment work, attendee-held payment facts are snapshotted before deletion so M11 loses nothing, and completion stops storing payment references in attendee PII so that migration source is closed before the copy.
• M9/M10 carry explicit per-run call budgets and idempotent-retry rules (stable delivery identities; renewal idempotency keys or read-before-retry), so uncertain successes are never blindly replayed. Site assignment claims a built site atomically inside the mutating statement, and site builds carry a stable creation identity so a retry adopts the already-created site. Their merge order is free but neither is deferrable, so rule 7's deadline for payment_completion_deliveries holds whichever merges first.
• M11 (was PRs 13+14) combines migration verify and copy under one fence-and-drain protocol — which also pauses attendee PII edits, the one writer that could change legacy payment references mid-copy — and retires both staged adapters, the refund adapter behind an explicit revision-fenced drain. A source row is "settled" when copied and verified or terminally preserved as unmigratable — the one condition the snapshot release, adapter drain, and M13 gate share — and before a dual-store row settles, its legacy-only booking facts are folded idempotently onto the canonical records. Unmigratable evidence is a bounded, allowlisted accounting record (never buyer PII, ticket tokens, or credentials), encrypted and owner-only, permanently retained because there is nothing in it to redact.
• Rules deduplicated to nine, stated once each: the 800-line src cap gains an explicit atomic-cutover exception (M13 claims it explicitly for its above-cap range); the mutation gate is mandatory with no per-PR waivers — relief requires the owner to change AGENTS.md policy first; PR descriptions must state provider/database call budgets; total-diff budgets are dropped as consistently wrong; source-branch tests are ported rather than rewritten.
• Stale paths fixed: the pure-rule modules live in src/shared/payment/ beside what actually landed, not the never-created payment-state/ directory.
• Twenty-six faults previously buried in prose or surfaced by review became ledger rows (e.g. deleteAllStaleReservations deleting uncopied rows, the unconditional table drop against restored backups, two completions claiming one built site, a multi-listing payment credited in full to each listing, unmigratable evidence keeping buyer secrets forever).

Review rounds

Fifteen bot-review rounds ran (CodeRabbit three times, Codex twelve times — each pass reviewing the previous fixes): 62 findings total, every one resolved on its thread. Genuine gaps were folded in across aec5502, f7d67fb, a371133, fc0ef3f, 2419e83, ae7b8c8, 0b983b5, 133e8fb, 0298673, 4e04da0, 5e0d7e4, 663f4b9, 1381422, ca0cc49, and c2e0e88; the misreadings were declined with reasons. Highlights the reviews contributed: cases self-close after linked-tool fixes, the deletion-snapshot losslessness chain, the restore-guard's pre-cutover blind spot, the no-blind-replay rules for deliveries, renewals, and site builds, the fold-not-suppress read-through with folding preserved through deletion and migration settling, the attendee PII-edit pause during the migration copy, the atomic built-site claim, the revision-fenced adapter drain, the shared "settled" condition, per-listing allocation for multi-listing payments, and the bounded unmigratable-evidence record.

Judgment calls to review

• The "owner can perform every supported required action" decided behavior allows a case page to link to a still-live admin tool that genuinely resolves that case kind, until the case-page action ships in M7/M8. This is what makes M5 and M6 legal without building actions twice — veto it if you want page actions before any aggregate authority.
• Pre-cutover sales deleted or pruned between M6 and M8 stay deleted — the plan keeps today's live-store lifecycle until M8's fence rather than extending buyer-data retention or shipping M8's snapshot machinery early. Veto this if you want preservation to start at M6.
• Unmigratable-row evidence keeps only allowlisted accounting facts, so the buyer's identity for such a payment dies with the old tables at M13 — the same outcome M12's redaction gives every migrated payment. Veto this if you want full source content retained instead.
• F1 (refunds disabled with sales off) is marked closed by Keep existing payments working when new sales are off #2020, with its recorded per-charge-provider gap carried forward as F13, owned by M6 and M7.

Verification

Documentation-only diff (PLAN.md + TODO.md formatting). deno fmt --check passes repo-wide. CI (checks, test, evidence, merge-check) has been green on every reviewed head; the latest push re-runs the same docs-only checks. No src, test, or locale changes, so the wider precommit suite does not apply.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AqLNrpvmNdGSSqUtSafiMd

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

The payment integration plan now uses milestones M3–M13 across four stacks. It defines payment behavior, aggregate operations, delivery, migration, retirement, fault ownership, and completion criteria. TODO.md only receives formatting changes.

Payment integration plan

Layer / File(s)	Summary
Plan constraints and architecture PLAN.md	The plan documents source roles, milestone status, delivery constraints, aggregate-table requirements, deployment constraints, retry rules, and shared payment architecture.
Provider and aggregate payment flows PLAN.md	Stacks A and B define provider ownership, payment cases, aggregate checkout operations, reconciliation, refunds, completion, repairs, and migration fencing.
Delivery, migration, and retirement PLAN.md	Stacks C and D define recoverable messages, webhooks, renewal work, historical migration, secret controls, restore handling, and legacy storage retirement.
Fault ownership and completion criteria PLAN.md	The plan assigns 47 findings to milestones and adds criteria for testing, migration, redaction, and closure.
TODO formatting TODO.md	The document reflows existing follow-up text without changing its tasks or semantics.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• chobbledotcom/tickets#2030: Both PRs modify PLAN.md to organize the payment-aggregate integration roadmap.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: restructuring the payment plan into standalone milestones.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/plan-md-review-jarsho

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch claude/plan-md-review-jarsho

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 8

🤖 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 6-24: Update PLAN.md to remove branch names, merge-base details,
prior-PR references, and historical narration from the introduction and all
referenced headings or prose. Retain only current status, source roles, decided
behavior, milestones, and binding constraints, rewriting explanations to
describe present behavior concisely; move any necessary traceability to a
separate change log.
- Around line 62-66: Update the Gates instructions to specify the full mutation
gate as `nix develop -c deno task precommit:mutation`, replacing the bare `deno
task precommit:mutation` command while preserving the existing waiver and
targeted-run guidance.
- Around line 379-383: Update the M11 copy plan to select and specify one
lossless copy-consistency protocol instead of allowing fencing/draining or
versioning/reconciliation interchangeably. Define lease ownership and renewal,
all covered writers including applyAttendeeMerge, deleteAttendee, the prune
task, and deleteAllStaleReservations, handling of adapter versioned writes, and
the final reconciliation boundary before row verification and adapter
retirement.
- Around line 194-197: Update the milestone plan so owner-decision conflict
detection creates an owner case before the M4 merge, rather than deferring owner
action to M5. Ensure each case kind links to its live resolving admin tool and
remove the statement that no owner tooling is needed on legacy engines, while
preserving existing error detection and alerting behavior.
- Around line 258-262: Define the M6 replacement entry point for the
payment-session contract, then migrate the webhook, cancel, and paid-session
validation callers from resolveWebhookSession and retrieveSession to it. Update
the PaymentProvider interface and all three provider implementations
consistently, removing the obsolete methods so callbacks and redirects use the
M6 reconciliation path.
- Around line 349-352: Update the M12 redaction scope to explicitly cover the
durable delivery records created by M9, including prepared message bodies,
webhook bodies, and buyer facts. Define redaction eligibility and cleanup
behavior for completed and permanently failed deliveries, and add corresponding
tests; alternatively, enforce and verify that these stored payloads contain no
sensitive data.
- Around line 273-275: Update the M6 migration plan to avoid routing uncopied
old-table rows directly through production readers; either move the affected
panels, exports, statistics, and refund-target consumers to the M7 adapter
before M6, or define the bounded read-through as a named staged-migration
adapter with an explicit contract and removal milestone.
- Around line 372-375: Define the migration’s private-key handling before
execution: derive the owner CryptoKey server-side from the owner-authenticated
session, never accept or persist raw JWK or private-key strings, and exclude
unwrapped key material from migration state, progress records, logs, traces,
audit records, backups, and errors. Specify bounded lifetimes and cleanup for
migration-held key references and decrypted-PII caches, and record only key
provenance.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 88e0b5d6-ac99-4cc1-bbb9-fc7e1a32d37a

📥 Commits

Reviewing files that changed from the base of the PR and between 81eb61d and 4a82ddc.

📒 Files selected for processing (1)

• PLAN.md

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a82ddc6a1

ℹ️ 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".

[chatgpt-codex-connector]

Do not make the branch mutation gate optional

When an owner waives this gate for one of the large M6–M8 cutovers, the listed targeted runs cover only selected payment modules and cannot prove every changed src/ file; this permits merging without the required 100% branch mutation result. Keep precommit:mutation mandatory and use targeted runs only while a layer is still above another stack layer.

AGENTS.md reference: AGENTS.md:L1509-L1512

Useful? React with 👍 / 👎.

[stefan-burke]

Tightened in f7d67fb: the gate is stated as mandatory, and a waiver is the owner's call alone, case by case — never the implementer's — recorded in the description with the stand-in targeted runs. The clause is kept deliberately: it documents a prerogative the owner already exercised (#2048 merged with the branch run "skipped … by owner request") so waivers are recorded instead of silent. A plan rule cannot remove the repository owner's ability to waive their own gate; it can require the waiver be explicit, which is what this does.

---

Generated by Claude Code

[chatgpt-codex-connector]

Keep atomic cutovers below the source limit

For M6 and M8, this exception explicitly permits the 1,200–1,800-line targets later in the plan, but PR_WORKFLOW.md:213-215 requires every PR to remain below the repository source-line limit and to split oversized behavior into smaller complete invariants. Remove the exception and redraw these milestones as independently useful vertical slices before implementation begins.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Keeping the exception, with the conflict resolved in f7d67fb: PR_WORKFLOW.md says "the repository's source-line limit" without defining a number — this rule is that limit, and rule 3 now says so explicitly, exception included. The exception exists because the owner chose it when approving this restructure: splitting the M6 cutover back into two merges forces the sumup_checkouts projection and checkout-metadata preservation layers, which cost more source lines than the split saves and are deleted one PR later. A bigger honest PR beats building a bridge to demolish it.

---

Generated by Claude Code

[chatgpt-codex-connector]

Account for the legacy read-through bridge

During M6–M11, the plan later requires a bounded old-table read-through for panels, exports, statistics, and refund targets, so the M7 refund adapter is not actually the only compatibility bridge. Calling it the sole sanctioned bridge leaves the read-through either forbidden by this rule or undocumented as an exception; name that second staged adapter here with its M11 removal path, or remove it by canonicalizing records before production reads.

AGENTS.md reference: AGENTS.md:L164-L169

Useful? React with 👍 / 👎.

[stefan-burke]

Already fixed in aec5502 (this review ran against the earlier commit 4a82ddc): rule 2 now names two sanctioned staged-migration adapters, both removed in M11 — the M6 legacy read-through and the M7 legacy-refund adapter — and the M6 bullet defines the read-through's bounded contract (panels, exports, statistics, refund targets).

---

Generated by Claude Code

[chatgpt-codex-connector]

Exclude M11 from the not-started range

The preceding row says that the M11 verifier is already underway in #2056, but this range immediately marks M11 as not started. Anyone using this table to coordinate milestone ownership gets two incompatible statuses and could duplicate the verifier work; list the remaining unstarted ranges without M11.

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in f7d67fb — the row now reads "M4–M10 and M12–M13", so M11 carries only its started status.

---

Generated by Claude Code

[chatgpt-codex-connector]

Do not defer legacy-row maintenance to M11

After M8 but before M11—or indefinitely if later milestones are cancelled—legacy rows have not been copied, so applyAttendeeMerge, deleteAttendee, and pruning must either block or skip their destructive work under this option. That makes M8 depend on a later milestone despite the new standalone-merge rule and regresses current admin maintenance for attendees with historical payments; canonicalize the affected rows on demand in M8 or retain a named bounded adapter that lets these actions finish safely.

AGENTS.md reference: AGENTS.md:L400-L404

Useful? React with 👍 / 👎.

[stefan-burke]

Clarified in f7d67fb — blocking the admin action was never the intent, and the text now says it outright: merge, delete, and prune always complete for attendees with uncopied historical rows; only the legacy row's own removal waits (the row is left for M11 to copy and prune). No admin maintenance regresses, and M8 does not depend on M11 to function.

---

Generated by Claude Code

[chatgpt-codex-connector]

Restore the subrequest bound for site work

When many site-assignment or renewal effects are due, a bounded database query alone does not bound the remote and database calls made per effect. This edit removes the previous requirement to schedule unfinished site work within the subrequest budget, so the M10 worker can cross Bunny's hard 50-subrequest limit and abort repeatedly instead of making durable progress; restore explicit provider, database, and total call budgets here.

AGENTS.md reference: AGENTS.md:L796-L804

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in f7d67fb — M10 restores the explicit bound: unfinished site work is scheduled within explicit provider, database, and total subrequest budgets, matching M8's wording. Good catch; that clause was lost in the condensation.

---

Generated by Claude Code

[chatgpt-codex-connector]

Restore standalone values for M9 and M10

Unlike the surrounding milestones, M9 and M10 no longer state what improves if later work is cancelled or identify the exact current production caller receiving the change; they only summarize implementation steps. This removes the information needed to verify that these proposed slices are independently useful rather than effect kinds added for later activation, so restore each milestone's current-system value and caller before implementation approval.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in f7d67fb — both milestones have their standalone-value lines back: M9 names the M8 completion runner as its production caller (current owner address, scheduled recovery, one failed destination no longer blocks the queue), and M10 states the recovery guarantee for paid site delivery and renewal.

---

Generated by Claude Code

[chatgpt-codex-connector]

Close M5 cases after linked legacy actions

When an owner follows one of these links and successfully refunds or completes the booking in the existing tool, the plan defines no transition that refreshes the case evidence, closes the case, or changes the buyer's stable “needs review” result; M6's unified case refresh does not exist yet, and M5's only recorded decision is acknowledge/keep-unchanged. Wire each linked action back into M5 case reconciliation or retain the integrated case action so the tool genuinely resolves the case before this milestone merges.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in f7d67fb — this was a real gap in the slimmed M5. A new requirement closes the loop: a case re-checks its evidence on schedule and whenever its payment's records change, so a refund or completion performed through a linked admin tool causes an outcomeOf re-validation that closes the case and updates the buyer's stable result. The links must genuinely resolve the case, not only the money.

---

Generated by Claude Code

[chatgpt-codex-connector]

Keep Stack C within the planned stack size

As drawn, Stack C contains only the M9 and M10 pull requests, while folding either one into Stack B leaves a one-PR stack; both outcomes contradict this plan's claim that stacks follow the repository's normal three-to-seven-PR shape. Fold both milestones into an adjacent stack or identify at least three independently useful PR layers for Stack C.

AGENTS.md reference: AGENTS.md:L397-L404

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in f7d67fb — the section is renamed "Group C" and no longer claims to be a stack: M9 and M10 are independent single PRs on top of merged M8 (M10 does not depend on M9), merging directly to main or riding as top layers of Stack B. The three-to-seven rule applies to real stacks only.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f7d67fbbf9

ℹ️ 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".

[chatgpt-codex-connector]

Preserve attendee facts before completing deletion

When an attendee with uncopied payment history is deleted between M8 and M11, retaining only the legacy payment row is not lossless: M11 explicitly needs attendee PII and attendee-only payment references, but completing the current physical deletion removes the attendee that holds those facts. Fresh evidence in the revised plan is the explicit promise that deletion completes while only the legacy row's removal waits; canonicalize all attendee-dependent migration facts first, or defer/soft-delete the attendee itself.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in a371133 — real gap, well spotted. M8 now requires that before a merge or delete completes for an attendee with uncopied payment history, the attendee-held migration facts (legacy payment references and the payment-identifying facts M11 reads, still encrypted) are preserved in a durable migration snapshot — or that attendee's payments are canonicalized on the spot. The M11 verifier's input list now includes those snapshots, and the fault ledger gained F38 for it. The deletion still completes; nothing the copy needs is lost.

---

Generated by Claude Code

[chatgpt-codex-connector]

Ship the restore guard with M6

Before M6, no database has been forward-migrated—the plan defers data migration until M11—so this prerequisite has no live state to protect and its own PR is useful only to a later cutover. That makes the proposed small PR a dormant foundation despite the plan's standalone-merge rule; include the guard in M6 or another PR that gives it immediate production value.

AGENTS.md reference: AGENTS.md:L400-L404

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in a371133 — the guard now ships inside the M6 release itself (its own commit is fine) rather than as an earlier standalone PR, and it must be live before the first aggregate write can happen in production. The fault ledger row's owner moved from "Before M6" to "M6" accordingly.

---

Generated by Claude Code

[chatgpt-codex-connector]

Defer owner-decision conflicts until M5

When outcomeOf detects multiple captured charges in M4, these lines explicitly retain today's automatic behavior even though the binding contract requires those charges to receive owner review, and the actionable case workflow does not arrive until M5. If later work is cancelled, M4 therefore leaves a money conflict that it recognizes but still resolves without the required choice; defer these conflict outcomes from the live cutover or ship their complete owner workflow in M4.

AGENTS.md reference: AGENTS.md:L400-L404

Useful? React with 👍 / 👎.

[stefan-burke]

This asks for what the plan already specifies: M4's owner-decision conflict outcomes are not cut over — that is exactly what "cut live only the outcomes whose remedy is refuse-and-record" plus "keep today's behavior" means. Today's classifier keeps governing those payments until M5, so M4 does not "resolve a recognized conflict without the required choice" — the same automatic path that runs today keeps running, unchanged, and the decided behavior attaches to M5, the milestone that ships it. Since two reviewers have now tripped on the same sentence, a371133 states it outright: "these outcomes are not cut over, today's classifier keeps governing them." CodeRabbit raised this identical point and withdrew it after the same explanation.

---

Generated by Claude Code

[chatgpt-codex-connector]

Add an action for missing legacy refund facts

When a reference is shared by attendees or lacks its provider/account/money facts, merely opening a case leaves the refund permanently stranded: M7's refund action must re-read a known provider, while the plan supplies no required owner choice for assigning the reference or providing verified evidence until M11. Ship that conflict-resolution action with these new M7 case kinds so the owner can actually unblock or reject the refund rather than waiting for a later migration milestone.

AGENTS.md reference: AGENTS.md:L304-L313

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in a371133 — the M7 fail-closed case kinds now ship with their required decision: the owner supplies verified provider, account, and amount evidence (after which the refund proceeds through the engine) or rejects the refund. No case is stranded waiting for M11; this is the same owner-evidence rule M11 applies to ambiguous account assignment, arriving with the case kind that needs it.

---

Generated by Claude Code

[chatgpt-codex-connector]

Give the delivery table a role by the end of Stack B

This rule cannot be satisfied for payment_completion_deliveries: its schema stores prepared delivery data, but M9 is the first milestone that stores message and webhook bodies, after Stack B has ended. The table is therefore still dormant after M8; dropping it there would cause the forbidden drop-and-recreate churn when M9 arrives, while retaining it violates this stated gate. Move the delivery behavior into Stack B or redraw the table-lifecycle rule so the stack has one implementable outcome.

AGENTS.md reference: AGENTS.md:L170-L178

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in a371133 — rule 7 now names its one straggler: payment_completion_deliveries earns its role in M9, immediately after Stack B, and if M9's design ends up not using it, M9 drops it. Every other aggregate table still activates or drops inside Stack B, so the rule has one implementable outcome and no drop-and-recreate churn.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a371133fbb

ℹ️ 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".

[chatgpt-codex-connector]

Include call budgets in the PR checklist

For provider-heavy milestones such as M6–M10, this checklist omits the database and provider call budget, and its “full field list” pointer names steps 2 and 6 even though PR_WORKFLOW.md:204-211 requires that budget in step 5. An implementer following this plan can therefore submit a slice without demonstrating that it stays below Bunny's hard 50-subrequest limit; add the budget explicitly and point to step 5.

AGENTS.md reference: AGENTS.md:L796-L804

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in fc0ef3f — rule 9 now requires the database and provider call budget whenever a slice touches providers or adds queries (naming Bunny's hard 50-subrequest limit), and the pointer covers steps 2, 5, and 6 of PR_WORKFLOW.md. That field existed in the original plan and was lost in the dedup — good catch.

---

Generated by Claude Code

[chatgpt-codex-connector]

Reconcile direct M9/M10 merges with the done criteria

When M9 or M10 uses the explicitly permitted direct-to-main option at lines 371–373, it is not part of a stack, but this acceptance criterion still requires every milestone to merge through a three-to-seven-PR stack. Fresh evidence after the earlier Stack C thread is this unchanged final criterion, which makes the documented direct-PR route unable to satisfy “Done means”; qualify this requirement so it applies only when work is actually stacked.

AGENTS.md reference: AGENTS.md:L397-L404

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in fc0ef3f — the done criterion now reads: merged through stacks of three to seven PRs where the work is stacked, or as the independent single PRs Group C names, each merge standing alone under the delivery rules. The direct-merge route and the acceptance criteria agree.

---

Generated by Claude Code

[chatgpt-codex-connector]

Retire deletion snapshots after migration

When an attendee is deleted before M11, M8 creates a durable encrypted migration snapshot containing the attendee-held payment references and identifying facts (lines 353–358). M11 consumes those snapshots, but this retention list covers only intent, evidence, tokens, completion payloads, and delivery records, while neither M11 nor M13 requires the snapshots to be deleted or redacted; the duplicate PII can therefore remain indefinitely after its payment has been copied and verified. Add snapshot cleanup at verification, including the interrupted/restore path, to the privacy contract.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in fc0ef3f — M11 now deletes each M8 deletion snapshot the moment its payment is copied and verified (the same gate as any source row, idempotent across interrupted or restored runs), and M13 verifies none remain. Duplicate buyer facts cannot outlive the migration.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fc0ef3f201

ℹ️ 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".

[chatgpt-codex-connector]

Require ownership assignment for shared legacy references

When one provider reference belongs to multiple attendees, the preceding condition opens a case, but this decision only lets the owner supply provider, account, and amount evidence or reject the refund; it never requires them to assign the charge to the correct attendee or payment. Fresh evidence in the revised text is that the new decision now enumerates its accepted evidence while still omitting ownership. Accepting that evidence therefore cannot disambiguate the refund target, leaving M7 either blocked or forced to guess an irreversible money choice; include an explicit required ownership assignment.

AGENTS.md reference: AGENTS.md:L304-L313

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 2419e83 — the decision now requires that when one reference spans several attendees, the owner assigns the charge to exactly one attendee as part of accepting the evidence, so an accepted decision always names one unambiguous refund target before any money moves.

---

Generated by Claude Code

[chatgpt-codex-connector]

Fence attendee deletion against unfinished completion work

When deleteAttendee runs after paid completion creates an attendee but before the remaining M8 effects finish, these requirements fence listing deletion and repoint attendee merges only. M5's guard covers open cases, but ordinary unfinished completion work need not have a case, so deletion can remove facts that a later effect still needs and orphan or misapply paid work despite F6. Define how attendee deletion is transactionally blocked, settled, or repointed before this milestone ships.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 2419e83 — M8 now fences attendee deletion itself: an attendee with unfinished completion work or durable effects cannot be deleted until that work settles or is repointed, and the check runs inside the deleting transaction so it cannot race the effect runner. This covers caseless unfinished work, which the M5 open-case guard alone did not.

---

Generated by Claude Code

[chatgpt-codex-connector]

Remove the legacy read-through in M11

After the final source row is verified, this step retires only the M7 refund adapter; the M6 legacy read-through used by panels, exports, statistics, and refund targets has no corresponding removal step. Leaving those callers attached means M13's prerequisite that no production caller read an old table cannot become true, despite delivery rule 2 promising that both staged adapters are removed in M11. Switch every read-through caller to the current readers and delete that adapter here.

AGENTS.md reference: AGENTS.md:L164-L169

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 2419e83 — M11 now explicitly switches every read-through caller (panels, exports, statistics, refund targets) to the current readers and deletes the M6 adapter in the same milestone, so rule 2's "both adapters removed in M11" is honored in the milestone text and M13's no-production-caller precondition is reachable.

---

Generated by Claude Code

[chatgpt-codex-connector]

Bound each M9 delivery run by the subrequest budget

When many messages or webhooks are due, the bounded due-work query in Group C does not bound the external fetches and database writes performed for each returned delivery. Unlike M10, M9 says only to attempt deliveries independently and gives no provider, database, or total per-run budget, so a sufficiently large page can cross Bunny's hard 50-subrequest limit and abort before durable recovery finishes. Specify the delivery batch and call budgets here.

AGENTS.md reference: AGENTS.md:L796-L804

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 2419e83 — M9 delivery runs now carry the same explicit provider, database, and total subrequest budgets as M10, so a large due-work page cannot cross Bunny's 50-subrequest limit mid-run.

---

Generated by Claude Code

[chatgpt-codex-connector]

Use the stored charge provider before M6 merges

When a site has charges from more than one provider after a settings switch, M6 stores the provider on each aggregate charge but explicitly leaves the admin refund writer unchanged. The current handleAttendeeRefund resolves one global provider through requirePaymentProvider and never reads the aggregate charge's provider, so the newly stored fact has no production caller and a refund can still be sent to the wrong provider or fail until M7. This leaves F13 unresolved in the independently deployable M6; route the live refund action by the stored charge provider in M6 or include the refund cutover there.

AGENTS.md reference: AGENTS.md:L400-L404

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 2419e83 by naming the real callers instead of moving the refund cutover: the stored charge provider is read by M6's own reconciliation for charge-identity validation and deduplication — a same-PR production caller — and refund routing by stored provider arrives with the M7 engine one merge later. F13's ledger owner is now "M6, M7" so the fault is not claimed closed before the refund side ships. Pulling the refund cutover into M6 would recreate exactly the oversized everything-at-once merge this plan was restructured to avoid.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2419e83a1b

ℹ️ 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".

[chatgpt-codex-connector]

Collect every fact needed to release legacy refunds

When an attendee-only legacy reference lacks currency or completion state, the finalized remediation still lets the owner supply only provider, account, and amount evidence (plus ownership for shared references). Those inputs cannot satisfy the immediately preceding fail-closed condition, so the refund remains stranded or the implementation must guess a required money fact. Fresh evidence after the earlier legacy-facts thread is that the final text explicitly lists currency and completion state as potentially missing while still omitting them from the required decision; collect both as verified evidence, or define the provider read that deterministically supplies them.

AGENTS.md reference: AGENTS.md:L304-L313

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in ae7b8c8 — the decision now collects verified evidence for every fact the fail-closed condition names: provider, account, captured amount, currency, and completion state, any of which the engine's provider re-read may confirm or supply deterministically. The evidence set and the condition it releases are now the same list.

---

Generated by Claude Code

[chatgpt-codex-connector]

Give retried deliveries a stable idempotency identity

When an email or webhook is accepted remotely but writing its local completion marker fails, the scheduled runner still sees the delivery as due and sends it again. Storing the body and applying call budgets do not resolve this uncertain-success window, so buyers can receive duplicate messages and webhook consumers can repeat side effects. Define a stable delivery identity that the provider or webhook consumer can deduplicate, or an equivalent reconciliation step, before calling this recovery path durable.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in ae7b8c8 — every M9 delivery now carries a stable identity: webhooks send it in a header consumers can deduplicate, and the attempt record stores the provider's acceptance evidence before the delivery is marked complete, so a retry after an uncertain success re-sends the same identity instead of minting a new message.

---

Generated by Claude Code

[chatgpt-codex-connector]

Reconcile successful renewals before retrying them

When the remote renewal succeeds but the local effect-completion write times out or fails, the scheduled runner retries the same remote mutation and can extend the site twice. Serializing concurrent payments prevents two different renewals from racing, but it does not make this uncertain-success replay safe; require a stable provider idempotency key or read-after-failure reconciliation for each persisted renewal effect.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in ae7b8c8 — each persisted renewal effect carries a stable provider idempotency key, or the runner reads the remote state before re-attempting; an uncertain success is never blindly replayed, so a site cannot be extended twice.

---

Generated by Claude Code

[chatgpt-codex-connector]

Reject pre-cutover code when restoring old backups

When the operator restores a pre-M6 backup, that database has no aggregate-write marker, so this guard still permits deploying the backup's pre-cutover commit. I checked .github/workflows/restore-deploy.yml, which explicitly tells the operator to deploy the commit recorded in the backup; following that flow would therefore restart legacy payment writes despite this plan's no-rollback rule and M11's requirement to restore old schemas only into the current application. Reject pre-cutover commits after the cutover regardless of the restored database's marker, and let the current code migrate the old schema forward.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in ae7b8c8 — the guard no longer keys off the restored database at all: once the cutover release ships, restore-deploy.yml rejects any pre-cutover commit outright, precisely because a pre-M6 backup carries no aggregate marker and its recorded commit would restart the legacy writers. Restoring an old backup means loading it into the current application, which migrates it forward per M11's restore contract. Sharp catch — the marker-based wording had exactly the blind spot you describe.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ae7b8c8e07

ℹ️ 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".

[chatgpt-codex-connector]

Move attendee maintenance into M6

When M6 is deployed and an attendee is merged or deleted before M8, the now-authoritative aggregate keeps the old payment_sessions.attendee_id: the current merge updates only processed_payments (src/shared/merge/attendee-merge.ts:824-836), deletion handles only the legacy dependent tables (src/shared/db/attendees/delete.ts:62-68), and this plan defers repointing aggregate payment work until M8. Current readers and refund targets can therefore remain attached to a deleted source attendee for an indefinitely standalone M6 release; move the aggregate repoint/deletion fence into M6 rather than leaving maintenance unchanged.

AGENTS.md reference: AGENTS.md:L400-L404

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 0b983b5 — M6 now owns the basic attendee maintenance from the moment aggregate rows are authoritative: applyAttendeeMerge repoints aggregate payment sessions, charges, and cases inside its transaction, and deleteAttendee settles or repoints aggregate rows before the attendee row goes (the text names that today both touch only legacy tables). M8 extends the same guarantee to durable effects and queued work, and F6's ledger owner now includes M6.

---

Generated by Claude Code

[chatgpt-codex-connector]

Make email retries idempotent

When an email provider accepts a request but the local acceptance-evidence write fails, the runner still has no evidence and sends the email again; reusing the same local identity does not stop a duplicate unless the provider accepts that identity as an idempotency key or supports reconciliation. Fresh evidence in the revised plan is that it now treats storing acceptance evidence and resending the same identity as the fix, while the existing Mailgun, Postmark, Resend, and SendGrid adapters in src/shared/email.ts:176-229 do not send such an identity, so specify a provider-enforced key or a read-after-failure mechanism for email.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 0b983b5 with the honest version: email uses the provider's idempotency key where one is offered; where it is not (most of the adapters you cite), the attempt record persists acceptance evidence before completion, and if that evidence write itself is lost the retry may duplicate an email — an accepted, documented outcome for messages only, never for webhooks or money. A rare duplicate informational email is the right trade against inventing reconciliation machinery those providers don't expose.

---

Generated by Claude Code

[chatgpt-codex-connector]

Ship a working refund action with M5

When a captured checkout cannot be honoured, the current processor deliberately stores a quantity-0 placeholder (src/features/api/payment-processing/index.ts:232-248), but the existing attendee refund route rejects it because hasActiveBookingLine requires quantity > 0 (src/features/admin/attendee-refunds.ts:103-109 and src/shared/db/attendees/queries.ts:325-332). The proposed M5 refund link therefore either leads to an action that returns “no payment to refund” or must be hidden, leaving exactly this paid-failure case without the required refund choice until M7; ship a case-aware refund action in M5 or move the M7 action forward.

AGENTS.md reference: AGENTS.md:L290-L303

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 0b983b5 — good code-level catch. M5 now scopes its promise honestly: a case kind ships only when a live tool genuinely resolves it. For captured money on a failed booking — where the in-app refund route rejects the stored quantity-0 placeholder — the case links to the provider's own dashboard refund and closes once the refund reaches the payment's records (provider callback or the scheduled re-check). A kind with no genuine live resolution keeps M4's detect-and-alert behavior until its engine action ships, so M5 never persists a case nothing can finish. Building a case-aware refund on the legacy engine was rejected deliberately: it is exactly the built-twice owner tooling this restructure removed.

---

Generated by Claude Code

[chatgpt-codex-connector]

Add a migration-resolution action in M11

When M11 finds a legacy charge whose provider account cannot be assigned unambiguously, this text only creates an M5 case and requires evidence: M5 provides only acknowledge/keep-unchanged, while M7's evidence decision proceeds with or rejects a refund rather than storing a migration account assignment. No named action therefore records the required account choice and resumes verification, so that source row can remain unverified and prevent M13 from retiring the old tables; ship a required account-assignment decision and revision-fenced copy retry with M11.

AGENTS.md reference: AGENTS.md:L304-L313

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 0b983b5 — M11 now ships a required migration decision for ambiguous account assignment: the owner assigns the provider account, or marks the row unmigratable with a reason, recorded in the decision union; a revision-fenced copy retry consumes the decision so verification resumes. No row can block M13 forever.

---

Generated by Claude Code

[chatgpt-codex-connector]

Fence listing deletion before deleting its attachment

When an owner tries to delete a listing while payment work is pending, the current performListingDelete deletes the attachment from storage before calling the database deletion (src/shared/listings-actions.ts:484-488). Merely adding the planned fence inside deleteListing would therefore detect the pending work only after the irreversible file deletion, reproducing F25 even though the listing itself remains; require an atomic deletion claim or another ordering that blocks new payment work and establishes the fence before storage cleanup.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 0b983b5 — the fence claim must be established before any irreversible step, and the text now names the concrete ordering hazard: performListingDelete removes the stored attachment before the database delete, so the fence precedes storage cleanup, not only the row delete.

---

Generated by Claude Code

[coderabbitai]

Actionable comments posted: 6

🤖 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 383-388: The M8 migration snapshot lifecycle must not delete an
attendee-scoped snapshot after only the first payment is copied. Update the M11
payment-copy/deletion flow to retain each snapshot until every linked payment
and shared buyer fact has been copied and verified, or enforce a
one-snapshot-per-payment invariant consistently across the related migration
steps.
- Around line 500-505: Expand the M9 redaction eligibility definition to
explicitly cover failed, cancelled, expired, and free terminal payment outcomes,
specifying whether each is eligible for cleanup or why buyer data must be
retained. Add cleanup tests for every newly covered state and preserve
pagination behavior so one ineligible or malformed record does not prevent later
eligible records from being redacted.
- Around line 474-477: Clarify the migration decision and retirement flow for
rows marked unmigratable, including the corresponding M11 section and the retry
logic around the revision-fenced copy. Either define unmigratable as a terminal,
durably evidenced disposition that satisfies M13 without deleting the only
source copy, or explicitly require such rows to block M13; ensure the documented
decision union, verification requirements, and old-table retirement behavior are
consistent.
- Around line 296-303: Update the M6 plan around the aggregate readers and
legacy writers to define how writes to already-aggregated payments remain
consistent, specifying the required transaction, invalidation, or reconciliation
handoff during the interval before M7 and M8. Preserve the existing completion
contract and clarify which legacy write paths participate in the rule.
- Around line 334-339: Update the legacy adapter refund-write contract in
PLAN.md to define both replay-safe paths: specify the monotonic version or
change record used when atomically writing provider_refunded_at, and specify the
source, identity, and replay rule when consulting the new refund record. Ensure
M11 cursor processing can detect and replay completion writes so late refunds
cannot leave copied state stale.

In `@TODO.md`:
- Around line 29-30: Update the sentence in the TODO entry around
validateChildEdges/validateParentEdges to insert “that” before edgeFieldError,
so it reads “it commits a relationship that edgeFieldError would now reject.”

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1d9c84f4-7641-431e-b0bf-d3d833d11549

📥 Commits

Reviewing files that changed from the base of the PR and between 4a82ddc and 0b983b5.

📒 Files selected for processing (2)

• PLAN.md
• TODO.md

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 133e8fb5aa

ℹ️ 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".

[chatgpt-codex-connector]

Fence attendee edits during the migration copy

When an owner edits an attendee while the bounded copy is running, applyAttendeeAtomicEdit can update the pii_blob (src/shared/db/attendees/atomic-update.ts:320), which contains the attendee PII and legacy payment_id that M11 reads, but this fence pauses only merge, delete, and prune writers. A page can therefore be verified from one PII revision while a later edit changes its source facts, leaving the canonical payment stale or internally inconsistent; fence attendee PII writers too, or revision-check and replay those edits before verification.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 0298673. The fence-and-drain list now pauses attendee PII edits too — applyAttendeeAtomicEdit, named explicitly, since its unconditional pii_blob write can change the attendee-only legacy payment references the copy resolves. (Attendees carry no revision column, so extending the pause is the honest fix rather than inventing a replay mechanism; the M7 adapter remains the only permitted concurrent writer.) Recorded as fault F40.

---

Generated by Claude Code

[chatgpt-codex-connector]

Preserve unmigratable evidence through table retirement

When M11 marks a row unmigratable, lines 487–490 make its case evidence the only durable copy of the source and call that disposition terminal; this broad M12 rule can then redact that evidence before M13 drops the old tables. That permanently deletes the payment facts M11 explicitly preserved, so unmigratable evidence must be excluded from redaction or reduced into a non-redactable accounting record before the source tables are retired.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 0298673. Both ends now state the exclusion: M11's unmigratable disposition says "M12 never redacts this evidence — after M13 it is that payment's only record", and M12's eligibility paragraph permanently excludes the source content M11 preserves on unmigratable-row cases, retained as documented accounting history. Recorded as fault F41.

---

Generated by Claude Code

[chatgpt-codex-connector]

Deduplicate new aggregate payments in the legacy read-through

When a post-M6 checkout completes before M8, it has an aggregate payment and also a legacy processed_payments row because lines 301–303 deliberately keep the legacy completion writers unchanged. This read-through serves old rows beside aggregate data to panels, statistics, exports, and refund targets, but it never requires an anti-join or provider-qualified identity check to suppress legacy rows already represented by an aggregate payment; the same sale can therefore be counted twice or exposed twice for refund until M11. Require the adapter to deduplicate those rows at its read boundary.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 0298673. The M6 read-through bullet now requires deduplication at its read boundary: it suppresses any legacy row whose provider-qualified payment identity an aggregate payment already carries, because every post-cutover sale is written to both stores until M7 and M8 retire the legacy writers — without that rule the same sale is counted twice and offered for refund twice. Recorded as fault F39.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0298673779

ℹ️ 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".

[chatgpt-codex-connector]

Stop paid completions from changing the migration source

When a post-M8 checkout completes while M11 is copying history, this is not the only concurrent source write: the current paid-booking path still passes session.paymentReference as paymentId in src/features/api/payment-processing/create.ts:217-229, which is encrypted into the new attendee's pii_blob, and attendee PII is explicitly one of M11's migration sources. Pausing applyAttendeeAtomicEdit does not stop those inserts, so the cursor can encounter an already-canonical payment as new legacy input and duplicate it or open a false identity conflict. Require M8 to stop storing aggregate payment references in attendee PII, or make M11 atomically recognize and exclude these canonical references.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 4e04da0, from both ends. M8 now closes the source: from the completion cutover on, payment references are no longer stored in attendee PII (the aggregate owns the attendee-to-payment link), so attendee inserts during the M11 copy cannot add new legacy references. And M11's copy now recognizes already-canonical input: a row or reference whose provider-qualified payment identity the aggregate already carries — every post-M6 sale, plus any reference a paid booking wrote into PII before M8 closed that path — is verified against its aggregate payment and recorded as already canonical, never copied as new legacy input. Recorded as fault F43.

---

Generated by Claude Code

[chatgpt-codex-connector]

Remove the mutation-gate waiver

When an owner grants the waiver described here, a source-changing PR can merge without running precommit:mutation, even though the repository requires that gate before every such merge and it demands a 100% kill rate. CI does not provide an equivalent fallback, so recording targeted runs in the description does not satisfy the required branch-wide check; remove this waiver or change the repository policy first.

AGENTS.md reference: AGENTS.md:L1509-L1512

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 4e04da0. The per-PR waiver is gone: rule 4 now states the branch gate runs before merge as AGENTS.md requires, with no per-PR waivers — if an exceptional cutover genuinely cannot run it, the owner first changes the policy in AGENTS.md, because this plan cannot loosen a repository rule. That keeps one source of truth and still leaves the decision with the owner, through the proper channel.

---

Generated by Claude Code

[chatgpt-codex-connector]

Preserve local completion state during M6

When a post-M6 payment is successfully booked, the retained completion path records the attendee and terminal result locally (processPaymentSession/completePaidBooking), but these lines say it never updates the aggregate and that a later provider read supplies the missing effects. A provider can report the captured charge or refund, not which attendee was created, its ticket result, or whether local booking completion failed; moreover, the new read-through rule suppresses this legacy row once the aggregate identity exists. Subsequent panels, refund targets, and buyer reloads can therefore see an aggregate payment with no attendee or local result until M8. M6 needs a durable bridge that records the legacy completion result in the aggregate, or it must keep and merge the legacy local facts rather than suppressing them.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 4e04da0. The read-through's dedup is now a fold, not a suppression: it returns one answer per provider-qualified payment identity, and a legacy row the aggregate already represents folds into the aggregate answer. Folding is explicitly not dropping — until M8 moves completion, that legacy row still owns the sale's local completion facts (the attendee it booked, the ticket result, a recorded local failure), which no provider read can supply, so the combined answer keeps them visible. Nothing is counted or refundable twice, and no local fact disappears. Recorded as fault F42.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e04da03c8

ℹ️ 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".

[chatgpt-codex-connector]

Reconcile site creation before retrying assignment

When no assignable site exists, assignSitesForEntries calls buildAssignableSite, whose builder creates a remote database and hosting site before the effect can be marked complete locally. If that remote creation succeeds but its response or the following local write is lost, the scheduled M10 retry can provision a second site or become stuck on the duplicate name; this idempotency/read-back requirement covers only renewal effects. Require a stable creation identity, adoption, or provider reconciliation for site-assignment builds as well.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 5e0d7e4. Site builds now get the same no-blind-replay rule as renewals: the persisted effect carries a stable creation identity, and a retry after a lost response reads the provider and adopts the site that identity already created, instead of provisioning a second one or failing on the duplicate name. Recorded as fault F46.

---

Generated by Claude Code

[chatgpt-codex-connector]

Preserve folded legacy facts during M6 deletion

When deleteAttendee runs during the standalone M6 release, the current implementation deletes the attendee's processed_payments row and PII, while M6 leaves the legacy maintenance writers unchanged until M8. These lines now establish that this legacy row is the only copy of the booked attendee, ticket result, and local failure, so merely settling or detaching the aggregate row cannot preserve those facts; M8's migration snapshot arrives too late. Fresh evidence after the earlier attendee-maintenance thread is this explicit sole-ownership requirement, so M6 must canonicalize or snapshot these facts before allowing deletion.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 5e0d7e4. M6's merge/delete bullet now makes settling carry the folded facts: before any legacy deletion path in this window — deleteAttendee, merge's row moves, the prune task — removes a legacy row that carries local completion facts for a sale the aggregate represents, it records those facts on the aggregate record, so no deletion strips the combined answer. A pre-cutover sale with no aggregate row keeps today's deletion behavior until M8's snapshot machinery arrives (a deliberate deletion of that history, unchanged from the live system). Recorded as fault F44.

---

Generated by Claude Code

[chatgpt-codex-connector]

Claim built sites atomically during assignment

When two paid completions run concurrently while an assignable built site exists, both current getAssignableBuiltSites calls can select that same site. assignBuiltSite retries revision conflicts, but its update does not require the reloaded site to remain assignable, so the losing writer can overwrite the first attendee's assignment; M10 serializes only renewals, and persisting both effects does not claim the shared inventory. Require an atomic compare-and-set claim that checks assignable and the revision, then make the loser choose another site.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 5e0d7e4. M10 now requires an atomic claim: the mutating statement itself requires the site to still be assignable — the plan names the current gap (assignBuiltSite overwrites without that check, so two concurrent paid completions can take the same site) — and the loser moves to the next site or builds one. This is the house pattern from src/shared/db/capacity.ts (the invariant enforced inside the mutating statement) applied to site inventory. Recorded as fault F45.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e0d7e4e60

ℹ️ 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".

[chatgpt-codex-connector]

Preserve uncopied legacy payments during M6

When deleteAttendee or the maintenance prune runs during the standalone M6–M8 window for a pre-cutover sale with no aggregate row, this explicit exemption leaves today's destructive behavior intact: src/shared/db/attendees/delete.ts:62-68 deletes its processed_payments and checkout_stages rows before removing the attendee/PII, while src/shared/db/prune.ts:50-82 deletes old payment and SumUp rows. The folded-row fix protects only sales that already have an aggregate, so M8's later snapshot and M11's copy cannot recover this history; M6 must defer deletion of every uncopied legacy source or snapshot/canonicalize it first.

AGENTS.md reference: AGENTS.md:L400-L404

Useful? React with 👍 / 👎.

[stefan-burke]

Addressed in 663f4b9 by making the boundary explicit and reasoned rather than changing it — this one is intended behavior, not a gap. Between M6 and M8 the old tables are still the live store, so an operator delete or a retention prune removes data exactly as the live system does today; the migration contract begins when M8's fence freezes the old tables as the source, and M11 — like a backup — copies what exists at that moment, never what was deliberately removed before it. Deferring these deletions from M6 on would hold buyer data past today's privacy retention for a window rule 1 explicitly allows to be indefinite, and shipping M8's snapshot machinery inside the already-oversized M6 cutover is the scope growth this restructure exists to avoid. Two facts keep the loss bounded to what today's system already accepts: prune.ts's payment statement deletes only failed, reference-less, orphaned, or cash-refunded rows past retention (never the successful referenced payments that form the accounting backbone), and deleteAttendee is a deliberate operator PII-removal identical in consequence to one performed the day before M6 merged.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 663f4b914c

ℹ️ 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".

[chatgpt-codex-connector]

Drain legacy refunds before the final reconciliation

When an M7 adapter refund is still in flight as migration finishes, it can complete after this final pass but before the adapter is retired, leaving the canonical payment without the completed refund before the read-through disappears and M13 drops its source row. The plan no longer requires disabling new adapter entries and waiting for existing requests before running the pass; make that drain boundary explicit and revision-fenced so no refund completion can land after reconciliation.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 1381422. Retirement now sits behind an explicit, revision-fenced drain: once every row is verified, new adapter refunds are disabled (every target is canonical by then, so refunds already route through the engine), in-flight adapter requests and their queued repair work must reach a terminal outcome, and only then does the final reconciliation pass run — comparing each row's monotonic version and refund identities against the copy. With the adapter disabled no write can land after the pass, and retirement re-checks the version high-water marks and fails loudly if one moved. Recorded as fault F47.

---

Generated by Claude Code

[chatgpt-codex-connector]

Order M9 before independent M10

When M10 merges first under this explicit independence, payment_completion_deliveries remains dormant even though delivery rule 7 promises that M9 gives it a role immediately after Stack B. Fresh evidence beyond the earlier table-lifecycle thread is the new statement that M10 does not depend on M9, which permits exactly that ordering; require M9 to merge first or relax the lifecycle rule so the documented paths are consistent.

AGENTS.md reference: AGENTS.md:L170-L178

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 1381422 by resolving the tension without imposing a false dependency: Group C now states that M9 and M10's merge order is free but neither is deferrable — both land directly after Stack B, so rule 7's deadline for payment_completion_deliveries holds whichever merges first. Requiring M9 strictly before M10 would reintroduce ordering the milestones don't technically need; what rule 7 actually demands is that M9 not slip, and that is now the stated obligation.

---

Generated by Claude Code

[chatgpt-codex-connector]

Keep M13 within its declared source cap

When M13 reaches the documented 800–1,000 changed src/ lines, it cannot satisfy delivery rule 3, which requires every PR to stay below 800 unless it explicitly invokes the atomic-cutover exception. Unlike M6, M7, M8, and the M11 copy, this milestone does not claim that exception; cap the target below 800 or state why retiring the storage must use the exception.

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in 1381422. M13 now explicitly claims rule 3's atomic-cutover exception for the part of its 600–1,000 range above the cap, with the reason stated: the table drop and every reader, codec, and gate it orphans must leave together, because splitting them would hold dead readers alive across a merge — the same throwaway-bridge shape the exception exists for.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 138142257f

ℹ️ 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".

[chatgpt-codex-connector]

Count unmigratable rows as terminal dispositions

When M11 marks a legacy row unmigratable, the plan explicitly records it as a terminal, verified disposition rather than copying it into a canonical payment, but this M13 gate still requires every source row to be both copied and verified. Any database containing such a row can therefore never satisfy the retirement gate despite M11 promising that it cannot block M13; define the shared completion condition as either successfully canonicalized or terminally preserved as unmigratable, and use it consistently for snapshot cleanup and table retirement.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in ca0cc49, exactly as proposed. M11 now defines the shared completion condition: a source row is settled when it is either copied and verified into a canonical payment or terminally preserved as unmigratable — and the snapshot release, the adapter drain ("once every row is settled", with the note that an unmigratable row's money is owner-case territory so no refund still needs the adapter), and M13's retirement gate ("every source row settled — copied and verified, or terminally preserved as unmigratable") all use that one condition. An unmigratable row now satisfies every gate it was promised not to block.

---

Generated by Claude Code

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

PLAN.md (1)

272-279: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Define allocation for one payment across multiple listings.

The aggregate creation and completion rules require one payment identity and generic booking intent, but they do not state how one payment is allocated across multiple listings or how the shared order is preserved.

The payment scenario in specs/payments/one-payment-many-listings.feature requires each listing to receive only its own amount while both places remain in one order. Define this invariant in M6/M8, including persistence, reconciliation, refund, and Money-effect behavior. Otherwise an implementation can charge once but attribute the full amount to each listing or collapse the shared order.

Also applies to: 391-394

🤖 Prompt for 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.

In `@PLAN.md` around lines 272 - 279, The M6/M8 plan must define allocation for a
single payment covering multiple listings: persist one shared order and payment
identity with per-listing allocated Money amounts, ensure reconciliation and
refunds operate on each listing’s allocation, and preserve Money-effect behavior
so no listing receives the full aggregate amount. Update the relevant aggregate
creation/completion rules and payment scenario references while retaining the
shared order across all listings.

🤖 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 519-525: Update the migration flow described around the M6/M11
rules to fold legacy attendee, ticket-result, and local-failure facts into the
canonical completion or effect records before marking a matching reference
settled. Keep the fold idempotent, verify the provider-qualified payment
identity against the aggregate, and avoid copying duplicate legacy input while
preserving all legacy-only booking outcomes.
- Around line 530-533: Update the unmigratable disposition in the M12/M13
migration plan to define a bounded schema for encrypted payment_cases.evidence
containing only allowlisted accounting facts. Explicitly exclude pii_blob,
attendee PII, ticket tokens, credentials, and unrelated payloads; document the
owner-only read path and a concrete retention or deletion rule instead of
retaining all fields indefinitely.

---

Outside diff comments:
In `@PLAN.md`:
- Around line 272-279: The M6/M8 plan must define allocation for a single
payment covering multiple listings: persist one shared order and payment
identity with per-listing allocated Money amounts, ensure reconciliation and
refunds operate on each listing’s allocation, and preserve Money-effect behavior
so no listing receives the full aggregate amount. Update the relevant aggregate
creation/completion rules and payment scenario references while retaining the
shared order across all listings.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9c024cbd-8e08-4bc3-985b-e72d966e0fd2

📥 Commits

Reviewing files that changed from the base of the PR and between 0b983b5 and ca0cc49.

📒 Files selected for processing (2)

• PLAN.md
• TODO.md

[stefan-burke]

Re CodeRabbit's outside-diff finding ("Define allocation for one payment across multiple listings", PLAN.md lines 272–279/391–394): fixed in c2e0e88. The invariant from specs/payments/one-payment-many-listings.feature is now stated across the three milestones that touch it — M6 stores an exact per-listing allocation of the expected total inside the one shared order (the existing largest-remainder rules make the parts sum to the whole) and reconciliation validates money against it; M8 completes each listing line from that stored allocation, so each line's capacity, tickets, and Money draw only its own amount and the shared order survives completion; M7 records refund Money against the same per-listing allocation, never the whole payment to one listing. Recorded as fault F48 (M6, M7, M8).

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2e0e88a89

ℹ️ 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".

[chatgpt-codex-connector]

Keep checkout extras out of listing allocations

When a multi-listing checkout includes a booking fee or positive modifier, priceCheckout includes those extras in PricedOrder.total (src/shared/checkout-pricing.ts:342-351), while the Money mapper credits them to separate fee or modifier accounts (src/shared/accounting/mappers.ts:132-147). Allocating the entire expected total across listing lines therefore either overstates listing income or makes reconciliation reject a valid payment; define the listing allocation over ticket-line money only and preserve each extra as its own fact for completion and refunds.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in #2058 (this PR merged before the round landed). M6's allocation now covers ticket-line money only: booking fees and price-modifier extras stay their own facts because Money credits them to their own accounts, and listing lines plus extras sum exactly to the payment total. Recorded as fault F52.

---

Generated by Claude Code

[chatgpt-codex-connector]

Keep multi-listing completion all-or-nothing

When one listing sells out after checkout while another remains available, completing capacity, tickets, and Money as per-line effects can commit the available line before the failed line selects the refund path. The current createBookingAtomic path commits the shared order together, but this plan does not require an atomic claim across all lines or define how to undo an already-completed line; a full refund could therefore leave a free live ticket, while a partial refund would introduce an unspecified product outcome. Specify an all-lines atomic commit or an explicit compensating state transition before splitting completion by line.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in #2058 (this PR merged before the round landed). M8 now states the commit is all lines or none in one transaction — as createBookingAtomic commits the shared order today — with per-line accounting from the stored allocation; a line selling out after payment sends the whole completion down the failure path (refund or owner case) rather than half-booking the order. Recorded as fault F54.

---

Generated by Claude Code

[chatgpt-codex-connector]

Route owner conflicts through the single diagnosis

When outcomeOf reports a multiple-capture or similar owner conflict, this milestone simultaneously declares outcomeOf the only judge and leaves today's classifier governing the live behavior. If the two classifiers disagree, alerts can describe the new diagnosis while the buyer or completion path follows the legacy result. Preserve today's action by mapping these outcomes in one outcomeOf-based handler until M5, rather than retaining a second internal classifier.

AGENTS.md reference: AGENTS.md:L164-L169

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in #2058 (this PR merged before the round landed). M4 now keeps exactly one classifier: outcomeOf judges owner-decision conflicts too, one handler maps those outcomes onto today's behavior (detect, record, alert through the existing error classes), and the displaced classifier is deleted in the same merge — two judges can never disagree about the same money. Recorded as fault F51.

---

Generated by Claude Code

[chatgpt-codex-connector]

Bound bulk refunds by the total subrequest budget

When a listing has enough refundable attendees, the bulk arm can exceed Bunny's 50-subrequest limit because the current processRefundBatch loops through every packed group (src/features/admin/refunds/provider.ts:177-209); limiting concurrency to five does not limit the total provider fetches and database writes in the request. This milestone moves that bulk path into the new engine without defining a per-run cap or durable continuation, so a large refund-all can abort after refunding only an initial subset. Specify provider, database, and total call budgets plus a resumable cursor or scheduled remainder.

AGENTS.md reference: AGENTS.md:L796-L804

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in #2058 (this PR merged before the round landed). M7's bulk arm now 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. The plan names today's unbounded processRefundBatch loop as the shape that does not survive the move. Recorded as fault F53.

---

Generated by Claude Code

[chatgpt-codex-connector]

Adopt legacy completion results at the M8 cutover

When a payment completes during the M6–M8 dual-write window, lines 342–345 say its attendee, ticket result, and local failure remain owned by the legacy row even though the aggregate already records the payment. Fresh evidence beyond the resolved M6 read-through issue is that M8 starts aggregate effects without folding those existing results first: scheduling every paid aggregate can book and post Money again, while scheduling only new payments leaves pre-M8 unfinished completions without durable recovery. Add an idempotent cutover that imports or marks every existing legacy completion result before the new effect runner claims those payments.

AGENTS.md reference: AGENTS.md:L73-L79

Useful? React with 👍 / 👎.

[stefan-burke]

Fixed in #2058 (this PR merged before the round landed). M8 now requires an idempotent cutover pass before the effect runner claims its first payment: 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. Recorded as fault F55.

---

Generated by Claude Code