A cancelled enrollment stops occupying its seat (#315) - #324
Merged
Conversation
Closes the root cause rather than working around it. enrollments_session_student_type_unique was UNIQUE (session_id, student_id, student_type) over every row. Drops do not delete -- they set status = 'cancelled' -- so a child who enrolled, dropped and re-registered still had a row holding that slot. create_for_payment's conflict arbiter names a different index (enrollments_payment_dedup, scoped to payment_id), and a named arbiter absorbs only its own, so the collision RAISED: inside the settlement transaction, after Stripe had captured. The transaction rolled back, the die released the webhook dedup claim by design, and every redelivery reproduced it. Money taken, no enrollment, no waitlist row, no refund owed, nothing to find. The constraint's own comment explains it as allowing one logical student in different contexts -- different student_type. Blocking re-enrolment after a drop was never its purpose, and children drop and re-join. The rule now says so: a partial unique index over live rows only. Two live seats for one child in one session are still refused, which is what was actually wanted. The migration nearly shipped as a no-op where it matters. clone_schema's LIKE ... INCLUDING ALL copies the constraint under a name Postgres generates -- enrollments_session_id_student_id_student_type_key -- so dropping the registry name in the tenant loop silently did nothing on every cloned tenant, leaving the bug intact in the schemas that hold the money while verify and the round-trip both passed. It drops by discovered name now, matching on the column set. Confirmed reaching both registry and a cloned tenant. The revert adds the tenant constraint UNNAMED so Postgres regenerates the same name clone_schema used; naming it after the registry constraint made the round-trip correctly call it a failed revert. The revert also documents that it can legitimately fail -- if anyone re-enrolled after dropping, which is the point, restoring the total rule refuses that pair, and resolving those rows by hand beats silently cancelling one. Two earlier verify scripts asserted the constraint exists. They accept either form now; only the verify scripts are touched, since sqitch's script_hash covers the deploy script alone and editing a verify does not mark a change modified. t/dao/payment-finalization-idempotency.t asserted the raise was correct. It existed because of this bug: its comment lays out the dilemma -- raise and roll back a captured settlement, or silently skip a seat the parent paid for -- and picked raising as the lesser evil. The third option now exists, so it asserts that, plus that exactly one LIVE seat results. Files=281 Tests=2416 PASS. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
perigrin
added
bug
…ainst it Closes the first and largest gap in #319, which turned out to be a money defect rather than only a coverage one. cart_seat_state scoped its lookup by payment_id, so a live seat held by a DIFFERENT payment -- a free enrolment with payment_id IS NULL, an admin add, an earlier purchase -- read as 'none'. The caller then adjudicated as though the child were unseated and tried to insert, colliding with the uniqueness rule inside a settlement Stripe had already captured. Same failure as #315, reached through a different door: the transaction rolls back, the die releases the webhook dedup claim, and every redelivery reproduces it. There is now a fourth state, 'foreign', and it is deliberately not 'seated'. payment_fits_session already counts foreign rows in $taken, so crediting one to this cart's %granted would count the same seat twice and under-count the capacity left for the next sibling in the cart. The test pins exactly that: two children, one already seated by another payment, and the sibling who genuinely needs a seat still gets one. A cart that paid for a seat the child already holds is owed its share back. There is nothing to seat and nothing of ours to demote -- the row that exists belongs to whoever paid for it first -- but the parent paid. It goes through the same refund_share_for path as a lost seat, with the same manual-review fallback when the share cannot be computed. Why nothing caught this: it is the conjunction trap #319 describes. Three mechanisms shape "one seat per child per session" -- this filter, payment_fits_session's payment exclusion, and the uniqueness rule -- and every fixture in the suite creates its enrollment rows FROM the cart under test, so all three agree and the tests grade only their aggregate. Removing the filter left four files green. Mutation-verified three ways: dropping the payment_id filter, reporting a foreign row as 'seated', and taking the foreign branch without owing the refund all fail now. Files=281 Tests=2415 PASS. Note on sequencing: this branch is off origin/main, so #324's partial-index migration is not in it. The collision the test reproduces is against the total constraint; after #324 it is against the live-only index. Same collision for an active foreign row, so the fix holds either way, but the two want a rebase check before both land. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
…ry, tenant coverage)
…t-recancel-constraint
Fixes the gap the review found in #324: the partial index closed the cancelled case, and I wrote a test that certified a property the code did not have. A waitlisted row still collided, and waitlisted is not exotic -- it is what the platform's own capacity gate writes, and nothing in lib/ ever moves a row back out of it. An admin-created row carries no payment_id and collided too. cart_seat_state now mirrors enrollments_session_student_type_live exactly rather than keying on seat_holding_statuses. If the index would refuse the insert, the lookup reports it. A cancelled foreign row still reads 'none', because the index lets that insert through and refusing it would deny a seat the parent paid for. This is #315 option 2: decide the collision in Perl, and keep the index because it is worth having, not because it is the fix. The mutation test that was supposed to grade this migration could not run. sql/test-schema.sql was stale -- still carrying WHERE (status <> 'cancelled') from before the deploy script moved to IS DISTINCT FROM, and missing a table. Every direct prove run in this branch had been executing against the NULL-unsafe predicate. make test regenerates the dump through its sql/deploy/*.sql dependency; a bare prove does not, and that is the trap. Against a correct dump, two mutants that had survived now die: narrowing the predicate to status = 'active', and reverting IS DISTINCT FROM to <>. The first is graded by a subtest that walks every live status; the second by a raw-SQL NULL row, which is the only way to reach that state now that both DAO write paths default the column. Depth nothing grades is depth that rots back. Rejected from the review, with evidence: the deploy's contype='u' discovery does not miss a bare unique index, because LIKE ... INCLUDING ALL copies a unique constraint as a constraint under a generated name (probed: src_ab_key becomes dst_a_b_key, contype='u'). clone_schema cannot produce the shape that finding needs, and the by-shape verify catches anything hand-made. Full suite: Files=278, Tests=2396, PASS. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
Round 7 review, critical finding. The duplicate-seat branch owed a refund and wrote nothing, so cart_seat_state kept answering 'foreign' and every redelivery owed the same child again -- each under a fresh refund_seq, so a fresh Stripe idempotency key, so Stripe did not deduplicate them. Reproduced: three deliveries on a two-child cart owed 10000 of a 10000 cart while holding one delivered seat, in two increments naming the same child. A page refresh was enough. _apply_intent reports already_completed for any settled status and refund_pending is one; finalize_enrollment's gate is _money_returned, which deliberately is not. Every neighbouring branch is idempotent because it wrote something a later pass reads back -- demotion leaves a waitlisted row, a drop leaves a cancelled one. This branch now writes a cancelled row for the payment before owing, which is the honest record (this cart paid for a seat it did not receive) and is exactly the state cart_seat_state already reads as 'closed' and skips. cancelled sits outside enrollments_session_student_type_live, so it cannot collide with the seat the other payment holds. My first reproduction was wrong and passed: on a single-child cart the debt equals the whole cart, so record_capacity_obligation's clamp absorbs every later one. A sibling is what leaves the headroom that makes a second increment representable, and the helper says so. Also repairs two assertions that could not discriminate: - t/database/enrollment-reenrol-tenant.t's fixture guard matched any total unique index, so enrollments_pkey always satisfied it -- the guard whose only job is to catch a fixture that proves nothing could not fire. It now filters on the seat column set, the same way the subtest below it already did. - t/dao/enrollment-seat-vocabulary.t graded demote_to_waitlisted by the status it produced. For an already-waitlisted row that status cannot change, so the waitlisted iteration read is(0,0) and held however the method behaved. It now grades the RETURN, which is what the settlement loop relies on to owe a share once, and keeps the status as a second assertion. Verified by mutation: widening demote_to_waitlisted's own predicate to re-demote a waitlisted row now FAILS the file and did not before. Money-path files: Files=11, Tests=57, PASS. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
Three review findings, all one mistake.
tenant_reenrol_revert_names existed to remember each tenant's constraint name so
the revert could restore it, justified by the claim that restoring everything
unnamed would strand a total constraint when flexible-enrollment-architecture is
later reverted. Measured, that claim is false: that revert runs ALTER TABLE ...
DROP COLUMN student_type, and a constraint on that column goes with it whatever
it is called.
before DROP COLUMN: [enrollments_session_id_student_id_student_type_key]
after DROP COLUMN: []
So the table bought nothing, and cost. clone_schema copies every registry table,
so each tenant received its own copy of migration bookkeeping, and this change's
revert dropped only the registry one -- stranding one per tenant, permanently,
with another added every deploy/revert cycle. It had also become part of the
shipped schema. The deploy now drops it in registry and in each tenant it was
cloned into, and the revert's IF/ELSE and its dead SELECT ... INTO orig go with
it. Net -9 lines and one table out of sql/test-schema.sql.
The verify script could not see the defect it was rewritten to catch. Its
predicate test was LIKE '%cancelled%', which the NULL-unsafe <> form satisfies
just as well as IS DISTINCT FROM:
i_ne (s <> 'cancelled'::text) LIKE %cancelled% -> MATCHES
i_idf (s IS DISTINCT FROM 'cancelled'::text) LIKE %cancelled% -> MATCHES
That matters because the deploy script was edited in place after being deployed,
with no sqitch rework, so any database that took it at e56bd45 still carries <>
and sqitch will never re-run the change. Requiring '%IS DISTINCT FROM%' turns
that from permanently silent into a failed deploy verify.
It also kills a mutant the review found surviving: reverting only the TENANT
copy of the predicate to <> -- the copy in the schemas that hold customer money,
graded by nothing, because the registry copy is the one the dump captures.
baseline: PASS
tenant-only <> mutant: FAIL (was PASS)
Migration suite: t/database/{enrollment-reenrol-tenant,revert-round-trip,
migration-verification}.t + the two DAO files, Files=5 Tests=25 PASS.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
This was referenced Aug 27, 2026
Round 7 review, second half. Four mutants survived the last round: each is a
guard whose deletion changed no test outcome, so any of them could have been
removed or drifted in a later refactor without a sound.
IS DISTINCT FROM -> <> in cart_seat_state survived -> FAIL
arbiter -> (id) DO NOTHING survived -> FAIL
tenant-only predicate -> <> survived -> FAIL
manual-review flag off the duplicate catch survived -> FAIL
The arbiter one is the instructive one. Its comment says the named arbiter makes
calling create_for_payment twice for one payment safe, and replacing it so a
replay RAISES left every money-path file green -- because finalize_enrollment's
`next if $held eq 'seated'` short-circuits before the second insert on every
replay path a test drives. A conjunction trap: the arbiter is only asked when
two deliveries race and both read 'none'. It is now asked directly, with no
settlement loop in the way.
Two code fixes the review found alongside them.
cart_seat_state omitted student_type while claiming in a comment to mirror
enrollments_session_student_type_live exactly. The index keys on three columns.
A foreign row of another type is not a collision, so reporting one refunds a
seat the insert would have granted. Both queries now carry the term and the
comment is true.
A negative share silently netted off a sibling's real debt. The obligation
writer guards the cart TOTAL, so +5000 with -2000 recorded 3000, tripped no
guard, wrote no flag and warned nobody. The guard now lives in refund_share_for,
where both accumulation sites already route through it -- one guard rather than
two that can drift. My first test for this graded nothing: with one child the
total goes negative and the OLD aggregate guard fires, so it passed either way.
The masking needs a sibling, and the test says so.
payment_fits_session dereferenced ->hash unguarded, so a session not visible to
the settling connection raised "Can't use an undefined value as a HASH
reference" inside a captured settlement. There is no recovery -- treating it as
unlimited seats the child and the FK refuses one line later -- so it now dies
with a sentence an operator can act on.
sql/test-schema.sql is a generated artefact that goes stale in silence, which is
how this branch spent several commits grading a schema its own migration had
moved past. CI runs a bare `prove -lr t/`, never `make test`, so nothing
regenerates. t/database/schema-dump-current.t deploys to an ephemeral database,
dumps, and compares DDL only -- COPY payloads carry fresh UUIDs every run, which
is why `make test-schema && git diff --exit-code` cannot be the gate. It catches
the exact historical failure and names the line:
first divergence at DDL line 1366
deploy produces: ... WHERE (status IS DISTINCT FROM 'cancelled'::text);
committed has: ... WHERE (status <> 'cancelled'::text);
Not built, filed instead: #327 (an un-cancelling UPDATE races the session lock;
no live caller), #328 (enrollments.status should be NOT NULL -- the root cause
under three separate findings, but a tenant-loop schema change earns its own
round trip), #329 (Waitlist::accept_offer), #330 (a half-provisioned tenant
fails seven verify scripts). _lock_cart_sessions now records why it works at
all: the mutual exclusion comes from enrollments_session_id_fkey, not from the
lock statement, and it covers INSERTs only.
Full suite: Files=279, Tests=2405, PASS.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
Last round's fix changed what an operator will find, and the runbook still
described the old world. It opened by stating that a refund_pending row means "a
capacity re-check at capture found a child's seat gone, waitlisted them, and
recorded the debt" -- one cause, one shape. Since the duplicate-seat branch
started leaving evidence, there are two, and they leave DIFFERENT enrollment
state:
a demoted child -> one 'waitlisted' row for that payment
a duplicate seat -> one 'cancelled' row for that payment
Measured, not assumed; both reach refund_pending. An operator following the old
text would go looking for a waitlisted row on a duplicate-seat payment, fail to
find one, and reasonably conclude the row was corrupt. The section now names
both cases and gives the query that says which one you are holding.
The manual-review paragraph had the same problem in miniature. It described one
cause -- a child whose share could not be computed -- and refund_share_for now
also flags a NEGATIVE share, which is a different thing calling for a different
action: the line item exists, a pricing plan is misconfigured, and refunding the
number as found would send a negative refund. Fixing the plan matters as much as
fixing the payment, or it recurs.
The distinction is documented, so it is a promise the code has to keep, and
nothing was holding it -- every other test in that file inspects payments, not
enrollments. It now has a test, which fails if the marker is written with any
other status.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
This was referenced Aug 27, 2026
Round 8 review. Last round's fix introduced this, which is the shape almost
every critical in this milestone has had.
The duplicate-seat branch wrote its idempotency marker BEFORE resolving the
refund share, on the reasoning that the evidence should land even if the share
throws. That is backwards. An unresolvable share -- a child with no line item,
which the code's own comment says is legitimate, or a negative one now that
refund_share_for refuses those -- left the row behind, flipped cart_seat_state
from 'foreign' to 'closed' permanently, and no later delivery would look at that
child again. Not even after a human supplied the missing line item.
got: 'closed' expected: 'foreign'
got: '0' expected: '5000' (after remediation)
So round 7 traded a debt owed repeatedly for a debt owed never, and only the
second cannot be recovered. The share now resolves first; the marker is written
only on success. Leaving the state at 'foreign' is exactly what lets the retry
work, and the manual-review flag is what stops the cart settling clean until it
does.
Both properties are now pinned by DIFFERENT subtests, which is the point:
marker written before the share (round-7 ordering) -> retryability FAILS
no marker at all (pre-round-7) -> idempotency FAILS
A re-enrolment sent the parent no confirmation email. The dedup key was
(user, type, session, child), with no time bound and nothing deleting the
notification when an enrolment is dropped -- sound only while a second enrolment
for that pair was impossible, which is precisely what this branch set out to
allow. A family could pay twice and be told once, and enrollment_confirmation is
the only email the paid path sends. create_for_payment now returns the
enrollment id, inserted or pre-existing, so a redelivery resolves to the same
row; the confirmation is keyed on the enrolment it is about. Verified through
finalize_enrollment: 1, still 1 on redelivery, 2 after re-enrolling.
The marker also now carries drop_reason = 'duplicate_seat_refunded'. It is not a
drop, and it set none of drop_reason/dropped_at/dropped_by, so the unfiltered
admin export read it as a family dropping a session their child still attends.
The tenant bookkeeping cleanup moved above the enrollments-table guard: that
table was cloned into tenants independently of whether the tenant ever got an
enrollments table, so a half-provisioned one kept its copy forever.
Correcting my own last commit message, which claimed the deploy "now drops it in
registry and in each tenant it was cloned into". That holds for fresh deploys
only -- sqitch never re-runs a deployed change, so a database that took the
earlier draft keeps the residue and nothing here removes it. The script says so
now instead of implying otherwise. The table is inert and this is pre-alpha, so
the residue is cosmetic and cleared by hand.
Filed rather than fixed: #331 (three unfiltered admin queries, incl. a
SUM(capacity) fan-out that makes utilization FALL as enrolment rises, and whose
two tests re-implement the query instead of calling production), #332 (Monthly
Revenue drops a whole cart for a partial refund), #333 (find() is
non-deterministic now that (session, student) repeats). #328 gained the
student_type twin: the same nullable-column-plus-CHECK hole, which this branch
made load-bearing by mirroring it in cart_seat_state.
Full suite: Files=279, Tests=2410, PASS.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
Round 8, mutation lens. 32 mutants, 25 killed; the survivors that mattered were
all in guards this branch added last round.
The one that would have fired next: the staleness detector I added is blind to
seed data, and the revenue-share rate is seed data. Registry's rule is that rate
assertions read the DB and never a literal, so t/priceops/revenue-share.t reads
sql/test-schema.sql -- and that file's rate lives in a COPY block the DDL
normaliser discards by design. Measured, in the direction that matters:
migration says 0.025, committed dump says 0.02
staleness detector: PASS rate test: PASS
A migration moving the launch rate to 2.5% without `make test-schema` would
leave the whole suite asserting 2% while production charged 2.5%. The dump
comparison now also compares a money-seed projection -- scope, name, type,
model, amount_cents, percentage, monthly base, refund-fee flag -- with no ids
and no timestamps, which is exactly the stable subset the COPY skip exists to
avoid. It kills a changed rate and a changed amount.
The verify predicate was still defeatable. Last round I answered '%cancelled%'
matching the NULL-unsafe <> form by ALSO requiring '%IS DISTINCT FROM%'. That is
satisfied by
status IS DISTINCT FROM 'cancelled' AND status IS DISTINCT FROM 'waitlisted'
which is unique, partial, carries the right column set, passes verify -- and
stops covering waitlisted rows, so two of them can hold one seat and a
waitlisted row stops blocking an active insert. The same hole one status over.
Substring tests on a predicate are a game the predicate wins; it now requires
the whole rendered expression, which is what pg_dump already writes. Two mutants
that previously either survived or died incidentally to a behavioural subtest
are now killed by verify itself.
payment_fits_session's new die was graded by nothing: replacing it with return 1
or return 0 left 192 files and 1547 tests green, while return 1 seats a child
for a session that does not exist and return 0 refunds one for a session that
does. Now graded.
cart_seat_state's own-row query keyed on student_type, which I added last round
for symmetry with the index. That symmetry was wrong. The own-row query asks
"does this cart already hold a row here", and what decides that is
enrollments_payment_dedup -- (session_id, student_id, payment_id), no
student_type -- because that arbiter is what silently absorbs the insert. A row
of another type would have read 'none', we would insert, and the arbiter would
swallow it: money taken, no enrollment. Only the foreign query mirrors the
index, and only it should.
Also: one shared pg_dump version list. The new test searched {18,17,16,15,14}
while Test::Registry::DB searches {17,16,15,14}, so on a host without the
pg_wrapper and with 18 installed the test would dump with a different major than
`make test-schema` writes with -- a permanent false failure.
Recorded on #330: t/database/migration-verification.t cannot exercise ANY
migration's tenant loop, because its database has no cloned tenant and both
loops CONTINUE immediately. All tenant-side verify coverage rests on one file,
and only incidentally, via [deploy] verify = true.
Accepted, not fixed: comment and blank-line drift inside stored function bodies
is invisible to the normaliser. That is the harmless half of its miss set --
anything kept on one side and stripped on the other still shifts the comparison.
Full suite: Files=279, Tests=2412, PASS.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
The staleness detector I added in round 7 has been FAILING in CI since it
landed, and I did not notice because I was reading `make test` locally instead
of the CI result. `make test` runs a curated directory list; CI runs
`prove -lr t/` over 284 files. Three commits went out described as "full suite
PASS" while the PR's test job was red. The suite result I should have been
quoting is the one the merge gate reads.
The failure was my test, not the schema. It compared a freshly generated dump
against the committed FILE, and pg_dump's output is version-specific. CI runs
postgres:14; this workstation runs 18.4, which emits `SET transaction_timeout =
0` -- a parameter that does not exist on 14. So it diverged at line 4, in the
preamble, on a schema that was perfectly current:
first divergence at DDL line 4
deploy produces: SET client_encoding = 'UTF8';
committed has: SET transaction_timeout = 0;
Both sides are now re-dumped by the same binary against the same server inside
the run, which cancels every version-specific rendering difference.
That exposed a second trap. Postgres RE-RENDERS some expressions when it
re-reads them, so a round-tripped dump differs from a first-generation dump of
identical DDL:
ANY ((ARRAY['pending'::character varying, ...])::text[])
ANY (ARRAY[('pending'::character varying)::text, ...])
Same constraint, two spellings. So both sides go through the SAME number of
parse cycles -- the migrations are dumped, reloaded and dumped again; the
committed dump, already a first-generation dump, is loaded and dumped once.
Verified against PostgreSQL 14 locally, which is what CI runs:
ok 2 - the committed dump has as many DDL lines as a fresh deploy produces
ok 3 - sql/test-schema.sql is current with sql/deploy/
ok 4 - the seeded money configuration matches what the migrations produce
and still fatal to both staleness mutants -- a changed index predicate in the
dump, and a migration that moves the revenue-share rate without regenerating it.
The duplicated pg_dump version list is gone with it: one helper, same search
order as Test::Registry::DB, so this file cannot dump with a different major
than `make test-schema` writes with.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
Round 9. Round 8's fix introduced this, which is now the fourth consecutive
round where the critical came from the previous round's remedy.
To make the duplicate-seat marker conditional on the refund share resolving, I
moved the INSERT inside the try. That put a database write in a block whose
catch begins with another database write. Any error on the insert -- deadlock,
serialization failure, a statement_timeout cancel, a trigger -- aborts the
surrounding transaction, and flag_refund_manual_review's UPDATE then cannot run
on it. It dies there, so:
escaping error: DBD::Pg::st execute failed: ERROR: current transaction is
aborted, commands ignored until end of transaction block at Payment.pm:857
Three failures from one fault. The genuine error is destroyed and replaced by a
message about the transaction state; no manual-review flag is written, so
nothing lands in the runbook's query; and the webhook 500s into a retry loop
reproducing it forever on an error nobody can read.
Only the read is in the try now. The marker is written after it, guarded on the
share being defined, which keeps both properties the last two rounds
established. If the insert itself fails there is no marker and no debt, the
error propagates intact, and the redelivery retries the whole item -- correct,
because a marker we could not write is a promise we cannot keep.
Three properties, three subtests, none of which overlap:
marker inside the try -> the real error is destroyed FAILS
marker before the share -> an unresolvable share is stranded FAILS
no marker at all -> a duplicate is owed every delivery FAILS
The runbook was falsified by that same round-8 fix, 66 minutes after I wrote it.
It states there are exactly two shapes behind refund_pending and gives the
operator a query distinguishing them. Making the marker conditional created a
third -- refund_pending with NO enrollment rows, which is what an unresolvable
share leaves -- and an operator following the old text would find nothing and
have no reading for it. The branch's own retryability test proves the doc wrong,
since it asserts the state stays 'foreign', which requires there be no row.
t/database/schema-dump-current.t's `or die` on loading could not fire: psql
exits 0 even when every statement in the file failed. A dump that would not load
therefore surfaced as the DDL comparison failing with "run `make test-schema`" --
the wrong instruction for the wrong cause, on the one test whose job is
diagnosing that artefact. ON_ERROR_STOP=1, and stderr is no longer discarded.
Also recorded: notification rows queued before the enrollment_id re-key carry no
such key, so they no longer silence anything. Reaching that needs a redelivery
for an already-seated child, which finalize_enrollment skips earlier, and after
a drop and re-enrol a second confirmation is correct regardless.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
This was referenced Aug 28, 2026
CI went red again on my own last commit, for the reason the commit was meant to
fix. Arming t/database/schema-dump-current.t's load with ON_ERROR_STOP made a
pre-existing, silently-ignored failure fatal:
ERROR: unrecognized configuration parameter "transaction_timeout"
loading sql/test-schema.sql failed
sql/test-schema.sql is generated on 18.4, whose pg_dump preamble sets
transaction_timeout -- a GUC added in 17. PostgreSQL 14, 15 and 16 reject it.
Filtering that in the one test would have fixed the symptom in one of the two
places that load this file.
So it is filtered at the source instead. generate_dump now drops the preamble
SETs on the way out: they are session settings that cannot affect the resulting
schema, and they are the only part of a dump that is not portable across
majors. The committed artefact now loads cleanly on every version we support:
before: 1 error on PG14 (exit status still 0)
after: 0 errors
Which makes the real fix possible. Test::Registry::DB::_load_from_dump -- run
for EVERY test file -- had the same unarmed guard: psql exits 0 through errors
unless told otherwise, and stderr was discarded, so `or die "psql load failed"`
could never fire and the whole suite would have run against whatever fraction of
the schema happened to load. It is armed now, which was not safe until the dump
became portable.
I checked whether that had already cost us anything: loading the committed dump
into 14 produced exactly one error, the GUC line, and a complete schema -- 41
tables, 6 routines, 177 indexes on both 14 and 18. So the guard was unarmed
rather than the schema partial. Worth knowing rather than assuming; a partial
schema in CI would have invalidated every result this branch has reported.
Verified on both majors: t/database t/priceops t/seed t/unit on PostgreSQL 14,
Files=11 Tests=114 PASS, and schema-dump-current.t green on 14 and 18
separately.
Filed, out of scope here: #334 (ci.yml declares postgres:14 but Test::PostgreSQL
builds its own server and picks the runner's highest major -- so nobody tests
14, and the version matrix never fires on a PR because head_commit is null
there; also the documented 12+ floor is really 13+, and five different versions
are declared across the repo with production pinned nowhere), #335
(_find_pg_tool's descending list stops at 17 while the server can be 18, and an
older pg_dump against a newer server is fatal).
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
Round 10, the coherence and test-infrastructure lenses. This round's own fix
needed two further fixes, and the full suite -- not review -- caught the first.
generate_dump could not report a failed pg_dump. `pg_dump | grep > out` returns
GREP's status, so a dump that died partway wrote a truncated
sql/test-schema.sql and announced success. Measured: kill the postmaster
mid-stream and the pipeline still exits 0 with a 15MB partial file. In a commit
titled "so both loaders can mean their `or die`" I armed one guard and disarmed
its sibling three lines above. It dumps to a file with a checked status now and
filters in Perl.
Then the filter itself was wrong, twice.
`/\A\s*SET\s+\w+\s*=/` matches INDENTED SET, and registry.copy_workflow -- which
clones tenant workflows -- has two inside a dynamic EXECUTE string. Stripping
them left CREATE FUNCTION succeeding, because they are inside a string literal,
so the artefact loaded clean and tenant creation died later on:
UPDATE test_tenant.workflow_steps new_steps
WHERE new_steps.id IN (...) -- the SET clause is gone
That is what took the suite from 2413 tests to 2284. Anchored to column 0 now.
Anchoring is still not enough. Dropping the whole preamble to solve one
incompatible line discards two that do work: check_function_bodies = false is
why a dump whose functions precede its tables loads at all, and
client_min_messages = warning is what keeps NOTICE out of TAP. Both were fine
today and both were latent -- the first takes out all 279 files the moment a
migration adds a %ROWTYPE declaration. So exactly one line is stripped, by name:
transaction_timeout: 0 check_function_bodies: 1 client_min_messages: 1
zero errors loading on PostgreSQL 14
transaction_timeout arrived in 17, so 14, 15 and 16 reject it. Another such GUC
will fail this load loudly, naming itself, which is the right way to find out.
The same over-broad regex was stripping those lines from BOTH sides of
schema-dump-current.t's comparison, so drift on them was invisible to the one
test whose job is to see it. Now caught:
first divergence at DDL line 411
deploy produces: SET template_id = ' || quote_literal(new_template_id) || '
committed has: SET template_id = ' || quote_literal(old_template_id) || '
Coherence findings folded, all verified against the code first:
- The vocabulary doc promised three answers, listed four, and the code returns
five -- omitting `foreign`, the one this branch exists for. It also sat above
the wrong sub, under an orphan fragment from a sub deleted last year.
- The duplicate-seat branch argued its whole case twice, fifty lines apart:
two rounds each wrote it beside the code they were touching and neither
deleted the other. The surviving copy sits with the code, and states the
requirement rather than narrating which ordering was tried first.
- 'foreign, not seated' claimed payment_fits_session counts foreign rows in
$taken. It counts seat-holding rows only, so a foreign waitlisted row is not
among them. Outcome unchanged, reasoning corrected.
- payment-capacity-obligation.t still described the pre-PR notification dedup
key, in a file this branch never opened. That comment is the argument the
fixture grades the regression, so a stale one is worse than none.
- cart_seat_state's student_type parameter had no caller; inlined, with the
ceiling named. The own-row lookup deliberately does NOT key on it, because
what decides whether our insert is swallowed is enrollments_payment_dedup.
- The three subtests that deliberately provoke an unresolvable share now capture
their warnings and assert them, so output is pristine and the diagnostic an
operator reads is graded.
- Dead migration code deleted: nothing creates tenant_reenrol_revert_names any
more, so its cleanup and 22 lines explaining it would have re-run on every
fresh deploy forever against an artefact that only existed on this machine.
Rejected, with evidence: the claim that the parent-return settlement path has no
transaction, which would have made the aborted-transaction rationale and the
session-lock comment both false. process_payment_async opens $db->begin and
invokes the settle callback inside it. Accepting that would have rewritten two
true comments into false ones.
Full suite: Files=279, Tests=2414, PASS.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
This was referenced Aug 29, 2026
Open
Round 11. A security lens found a working exploit that recreates, through the front door, exactly the failure this milestone exists to eliminate. MultiChildSessionSelection validated session selections against selected_child_ids -- which is CLIENT data, harvested by SelectChildren from child_<id>=1 checkboxes with no check that the ids name anything. So the guard compared the input to itself. @children holds only the ids that RESOLVED to a row, and that is what the step's own comment already meant by "the children this run actually chose". An unresolvable id rides into the cart priced by nothing, because calculate_enrollment_total also iterates the resolved children. The total and the Stripe charge look entirely normal. It detonates at settlement on enrollments_family_member_id_fkey, inside the transaction, after capture -- rolling back the paying child's enrollment and the webhook dedup claim with it, so every redelivery reproduces it identically. Money taken, no enrollment, forever. It is not self-limiting either: sustained 500s are how Stripe disables a webhook endpoint, as Webhooks.pm's own comment notes, so a few poisoned registrations degrade settlement for every tenant on the platform. One line, and a test that fails when it is reverted. Two bugs in _share_or_flag, which I added earlier today: - it was the only method in Payment.pm touching $db without the class-wide `$db = $db->db if $db isa Registry::DAO` coercion, while dereferencing ->dbh, which Registry::DAO does not have. Both production callers pass a Mojo::Pg handle, so it worked by luck. - ROLLBACK TO SAVEPOINT leaves the savepoint defined. Without the RELEASE, a cart re-issuing the same name once per child stacks a subtransaction per failure. And two in the dump generator I rewrote yesterday: - print, close and unlink were unchecked, so a write that failed partway -- a full disk being the ordinary case -- shipped a truncated sql/test-schema.sql, announced success, and then deleted the only complete copy. The pipeline it replaced caught that case; losing it while fixing the other half was a poor trade. Measured: a 64KB rlimit yields a 2074-line artefact and exit 0. - arming ON_ERROR_STOP introduced an undocumented psql CLIENT-MINOR floor. \restrict is a meta-command from the Aug-2025 minors; an older client calls it invalid, which ON_ERROR_STOP turns from a warning into an abort before any DDL runs -- a database with zero tables and a load that stopped at line 5. Both it and transaction_timeout are now stripped by one shared predicate, so the generator and the comparison cannot drift apart. schema-dump-current.t's comment filter had the same too-permissive shape as the SET filter fixed last round, three lines away from it: /\A\s*--/ strips the 36 indented comment lines inside plpgsql bodies from BOTH sides, hiding drift on them from the one test whose job is to see it. Anchored, and graded -- drift in a clone_schema comment now fails. The captured warnings are counted as well as matched, so a second, ungraded diagnostic cannot hide behind the first. Filed, all pre-existing and out of scope here: #336 (X-As-Tenant is honoured for any authenticated user with no membership check, and it selects both the schema and the Stripe destination account -- every tenant-isolation conclusion in this review sits on it), #337 (no ownership check on child_id: a payer can enrol any child in the tenant), #338 (paying twice for a seat already held burns Stripe fees on an automatic refund), #339 (cart items reach WHERE clauses with no scalar guard; unreachable today only because of step ordering), #340 (the exception template renders source paths to end users). Rejected: an "uninitialized value" warning reported alongside the real one was an artefact of the reviewer's own mutation, not live code. Confirmed by asserting the warning counts, which pass at one apiece. Full suite: Files=279, Tests=2416, PASS. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
Validating the cart against @children established that an id resolves to a row. It did not establish that the row is ours: FamilyMember->find carries no family_id predicate, and SelectChildren harvests child_<id>=1 without an ownership check either, so any family_members row in the tenant was admitted as legitimate by construction. The cost is not a stranger's enrollment. The snapshot this step writes carries child_name, birth_date and grade into the run, thence into payment metadata and the confirmation email. And the live row it creates makes the victim family's own paid cart read 'foreign' -- so their settlement refunds their share and never seats them, against a seat they paid for. family_members.family_id is the run's user_id, which Family::add_child sets, so the scope is exact rather than a heuristic. prepare_template_data is scoped with it too: rendering a child the processor would refuse still discloses their name and age, and hands the client a control aimed at them. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
The duplicate-seat branch deliberately writes no marker row when the share cannot be resolved, so the state stays 'foreign' and a later delivery can still settle it once the line item is repaired. That retryability is the point, and it is graded. Its cost was that the branch re-enters on every delivery while the flag writer appended unconditionally. finalize_enrollment does not early-return on refund_pending, and a parent refreshing the Stripe return URL re-enters the finalizer through already_completed -- so the array grew by one identical entry per refresh, without bound, on a money row. The runbook prints that array, so one fault read as N faults. Guarded on the pair, not on the status: a debt on a row that already reached a terminal refund status still has to be recorded, because it cannot be represented as an obligation at all. The sibling demotion branch never had this shape -- demote_to_waitlisted writes its row before the share is resolved, so it flags exactly once. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
_lock_cart_sessions takes FOR UPDATE on the sessions rows, and its comment explains that this serialises concurrent INSERTs into enrollments through the FK's FOR KEY SHARE. That mechanism is real, and it is the only thing it serialises. It does not cover the way a foreign seat actually disappears. Both drop paths -- ProcessEnrollmentDrop and DropRequest -- UPDATE status alone, touching no session_id, so RI_FKey_check_upd short-circuits and neither takes any lock on sessions. Measured: an INSERT blocks 1.5s behind the session lock, a status-only UPDATE proceeds in 0.00s. So the foreign row could be cancelled between the read and the marker write two statements later. The cart then refunds its share for a seat that just became free and writes a cancelled marker -- which makes cart_seat_state answer 'closed' from then on, so no later delivery re-adjudicates. The family is refunded and the child holds no seat in a session that has room, with nothing recorded to find. The comment above the session lock said this gap existed only in the cancelled->live direction, where there is no caller. The live->cancelled direction has two. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
The foreign half mirrors enrollments_session_student_type_live exactly -- status IS DISTINCT FROM 'cancelled' -- so a NULL-status row occupies the seat there, and a subtest pins that. The own-row half sent NULL down the terminal branch through `// ''`. One row, two contradictory answers. Another cart saw a collision it must not duplicate, while the cart that owns the row skipped the child entirely: not seated, not demoted, not owed a refund, nothing flagged, and the payment settled 'completed' looking clean. Only NULL moves. 'cancelled' keeps its terminal reading, which is load-bearing -- the duplicate-seat marker is a cancelled row of our own and the loop is meant to skip it. Reachable by hand-written SQL or an import rather than by the application: nothing in lib/ writes a NULL status, but the column is nullable and its CHECK passes on NULL (#328). enrollment-seat-vocabulary.t derives its status list from that CHECK, so it was structurally blind to this case. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
The plans projection answers "what rates exist". It does not answer "which of them is on offer", and that lives in a different table: pricing_relationships carries the status, and two migrations exist only to move rows in it -- suspend-rateless-tenant-plans and retire-registry-plus-plan. So a dump that still shows a retired plan as active left this file green, because the plan itself had not changed. Measured in that direction: flipping the suspend-rateless-tenant-plans row back to 'active' in the committed dump left both the DDL comparison and the plans projection passing, with the suite running against a rateless plan the migrations had retired. Mutation-graded -- with that flip in place only the new subtest fails, which is what makes it worth having. Same stability property as the projection beside it: no ids, no timestamps. Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
Scoping the child lookup by family_id assumed the scope value could be
trusted. It could not, and the scoping alone was worth nothing.
_apply_server_owned_data vets the FLAT param hash and touches only the exact
keys user_id and __tenant_slug. expand_form_params runs afterwards, inside
WorkflowStep, and its bracket branch does
$ref->{$p} = {} unless ref $ref->{$p} eq 'HASH';
which destroys the scalar the controller just wrote and replaces it with
whatever structure the client sent. So `user_id[!=]=<uuid>` POSTed to any
base-class step -- landing and camper-info both qualify, and camper-info sits
immediately before session-selection -- put an operator hashref into run data.
Measured: the authenticated user_id came back as a HASH.
Both keys are then used as SQL::Abstract WHERE values, where a hashref is an
OPERATOR and not a value. family_id => { '!=' => $x } renders `family_id != ?`
and matches every other family in the tenant. The existing security test
covered the flat form of this and passed; the bracketed form went straight
through.
Three doors, not one:
- The controller now strips bracketed variants of the server-owned keys at
the same boundary that already owns them.
- Family::list_children refuses a non-scalar family id outright. It is the
widest reader -- name, age, grade and allergies -- and its argument comes
from run data.
- select-children.html.ep resolved identity itself, calling list_children
with the run's user_id and rendering the result beside ready-made
child_<id> checkboxes. A template is the one layer with no way to refuse,
so the lookup moves into SelectChildren::prepare_template_data.
The scoped lookups keep a fail-closed ref guard as belt to that brace.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
…writes
Two corrections from the confirmation round, neither changing behaviour.
The comment above the NULL branch claimed the child had been "not seated, not
demoted, not owed a refund, nothing flagged". All of that is still true after
the change: finalize_enrollment takes the same `next` for 'seated' and
'closed'. What actually moves is the capacity credit -- payment_fits_session
excludes this payment's own rows from $taken, so a seat we hold has to be
counted in %granted or the cart is invisible to itself and oversells on the
next delivery. That is worth having; it is not what the comment described.
And the cross-family subtest asserted on $_->{child_name} where the snapshot
writes first_name, so that one line was vacuous -- it passed whether or not
the hole was open. The rest of the subtest graded the fix correctly, which is
why the failure was not visible.
Claude-Session: https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2
This was referenced Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #315 at the root rather than working around it.
The bug
enrollmentscarried two uniqueness rules:enrollments_session_student_type_unique(session_id, student_id, student_type)enrollments_payment_dedup(session_id, student_id, payment_id)WHERE payment_id IS NOT NULLcreate_for_paymentnames the second as its conflict arbiter. A named arbiterabsorbs only its own index, so a violation of the first raised rather than
being skipped.
Drops don't delete — they set
status = 'cancelled'. So a child who enrolled,dropped, and re-registered still had a row holding that slot, and the new
payment's insert raised inside the settlement transaction, after Stripe had
captured. The transaction rolled back, the
diereleased the webhook dedupclaim by design, and every redelivery reproduced it identically.
Money taken. No enrollment, no waitlist row, no
refund_pending, noobligation. Nothing to find and nothing that heals.
The decision
The constraint's own migration comment explains it as allowing one logical
student in different contexts — different
student_type. Blocking re-enrolmentafter a drop was never its purpose, and children drop and re-join.
So the rule now says that: a partial unique index over live rows only. Two
live seats for one child in one session are still refused, which is what was
actually wanted. Every code-side alternative was a workaround for a rule that
shouldn't have existed.
The migration nearly shipped as a no-op where it matters
clone_schema'sLIKE ... INCLUDING ALLcopies the constraint under a namePostgres generates —
enrollments_session_id_student_id_student_type_key. Sodropping the registry name in the tenant loop silently did nothing on every
cloned tenant, leaving the bug intact in the schemas that hold the money —
while
verifypassed and the round-trip passed.It drops by discovered name now, matching on the column set. Confirmed
reaching both
registryand a cloned tenant:Two details worth knowing
The revert can legitimately fail, and should. If anyone re-enrolled after
dropping — the point of this change — restoring the total rule refuses that
pair. Resolving those rows by hand beats silently cancelling one, and the revert
says so.
Two earlier verify scripts asserted the constraint exists. They accept
either form now. Only the verify scripts are touched: sqitch's
script_hashcovers the deploy script alone, so editing a verify does not mark a deployed
change modified.
A test changed sides, deliberately
t/dao/payment-finalization-idempotency.tasserted the raise was correct. Itexisted because of this bug — its comment lays out the dilemma (raise and roll
back a captured settlement, or silently skip a seat the parent paid for) and
picks raising as the lesser evil. The third option now exists, so it asserts
that, plus that exactly one live seat results.
What nine review rounds added
The partial index above closes one door. Reviewing it found three more money
defects on the same path, each fixed here, each with a test that fails without
the fix.
A seat held by a different payment was invisible.
cart_seat_statescopedits lookup by
payment_id, so a live seat from another payment -- a freeenrolment, an admin add, an earlier purchase -- read as no row at all. The cart
adjudicated as though the child were unseated and collided on insert, inside a
captured settlement. This was #319's first gap and the other half of #315's
surface. The lookup now asks exactly what the index asks.
A duplicate seat was refunded once per delivery. When a child already holds
a seat, the cart owes their share back. That branch wrote nothing, so
cart_seat_statekept answeringforeignand every redelivery owed the samechild again -- each under a fresh
refund_seq, so a fresh Stripe idempotencykey, which Stripe does not deduplicate. A page refresh was enough: three
deliveries on a two-child cart owed the entire cart while holding one delivered
seat. It now records a
cancelledrow for the payment, which is both the honestrecord and the state the loop already skips.
Then the fix for that introduced its own defect, caught the following round:
writing the marker before resolving the share made an unresolvable share
permanently unresolvable, because the state flipped to
closedand no laterdelivery would look at the child again. The share resolves first now. Both
properties are pinned by different subtests -- the old ordering fails
retryability, no marker at all fails idempotency.
A re-enrolment sent the parent no confirmation email. The dedup key was
(user, type, session, child)with no time bound, and nothing deletes thenotification when an enrolment is dropped. That was safe only while a second
enrolment for the pair was impossible -- which is exactly what this PR allows. A
family could pay twice and be told once.
create_for_paymentnow returns theenrollment id and the confirmation is keyed on the enrolment it is about.
Two guards that were grading nothing
sql/verify/enrollment-reenrol-after-drop.sqltested the index predicate bysubstring.
LIKE '%cancelled%'accepts the NULL-unsafe<>form; addingLIKE '%IS DISTINCT FROM%'still acceptsstatus IS DISTINCT FROM 'cancelled' AND status IS DISTINCT FROM 'waitlisted',which stops covering waitlisted rows. It now requires the whole rendered
expression.
sql/test-schema.sqlis a generated artefact that goes stale silently -- whichhappened here, for several commits, while the suite ran green against the wrong
predicate.
t/database/schema-dump-current.tnow gates it, comparing DDL andthe money seed. The seed half matters because Registry's rule is that rate
assertions read the DB rather than a literal, so they read that dump: a
migration moving the launch rate without
make test-schemawould leave thewhole suite asserting the old rate.
Test evidence
Local
make test:Files=279, Tests=2412, Result: PASS.CI (
prove -lr t/, postgres:14): passing --test,e2e,lint,securityandstripe-e2eall green.Migration deploy, verify and revert round-trip pass, including against a
clone_schema-provisioned tenant.t/stripe-live/andt/playwright/were notrun locally; CI runs its own gated jobs.
Every fix above is mutation-graded: the guard was broken and the test confirmed
to fail. Where a mutant survived, the assertion was rewritten until it did not.
Deferred, with issues
Found during review, out of scope here: #327 (an un-cancelling UPDATE races the
settlement's session lock; no live caller), #328 (
enrollments.statusandstudent_typeare nullable and their CHECK passes on NULL -- the root causeunder several findings, but a tenant-loop schema change earns its own round
trip), #329 (
Waitlist::accept_offer), #330 (half-provisioned tenants, andmigration-verification.tcannot exercise any tenant loop), #331 (threeunfiltered admin reporting queries, including a
SUM(capacity)fan-out thatmakes utilization fall as enrolment rises), #332 (Monthly Revenue drops a whole
cart for a partial refund), #333 (
find()non-determinism now that(session, student)can repeat).Closes #315. Closes the first gap of #319.
Round 12: six lenses, and one fix that was worth nothing on its own
A fixed-point review round found no defect in the change itself -- the partial
index, the constraint discovery by column set, the
cart_seat_stateasymmetry and the branch ordering were each independently checked and held.
Everything below is either adjacent code this PR touches or a regression the
new branches introduced.
Three regressions in this PR's own new code, each measured before fixing:
foreignbranch re-entered on every delivery while the flag writerappended unconditionally, so
refund_manual_reviewgrew by one identicalentry per parent page-refresh, without bound, on a money row. Measured
1,2,3,4 across four deliveries.
FOR UPDATEdoesnot cover it: both drop paths UPDATE status alone, touching no
session_id,so
RI_FKey_check_updshort-circuits. Measured -- an INSERT blocks 1.5sbehind the session lock, a status-only UPDATE proceeds in 0.00s. The window
loses a child's seat silently, against a session that has room.
cart_seat_statedisagreed about a NULLstatus: one row, two contradictory answers about the same index.
A guard that was grading nothing.
schema-dump-current.tprojected onlypricing_plans, so it could not seepricing_relationships-- where "whichplan is on offer" lives, and the sole subject of two migrations. Flipping a
retired plan back to
activein the committed dump left both the DDLcomparison and the plans projection green. The new projection is
mutation-graded: with that flip in place, only it fails.
The cross-family hole, and why scoping the lookup did not close it
Four lenses independently found that
FamilyMember->findcarried nofamily_idpredicate, so any parent could enrol another family's child andread their name, birth date and grade. One proved it by execution.
Scoping the lookup by
family_id => $run->data->{user_id}was the obviousfix and it was worth nothing on its own, because the scope value is not
trustworthy.
_apply_server_owned_datavets the flat param hash and touchesonly the exact keys;
expand_form_paramsruns afterwards and its bracketbranch does
destroying the scalar the controller just wrote. So
user_id[!=]=<uuid>POSTed to any base-class step --
landingandcamper-infoboth qualify, andcamper-infosits immediately beforesession-selection-- put an operatorhashref into run data. Measured: the authenticated
user_idcame back as aHASH. Both server-owned keys reach SQL::Abstract as WHERE values, where ahashref is an operator:
family_id => { '!=' => $x }rendersfamily_id != ?and matches every other family in the tenant.This is the bracketed bypass of the invariant #314 established. That PR's
security test covers the flat form and passes; the bracketed form went
straight through it. That test now covers both.
Three doors, not one:
boundary that already owns them;
Family::list_childrenrefuses a non-scalar family id -- it is the widestreader (name, age, grade, allergies) and its argument comes from run data;
select-children.html.epresolved identity itself, callinglist_childrenwith the run's
user_idand rendering the result beside ready-madechild_<id>checkboxes. A template is the one layer with no way to refuse,so the lookup moved into
SelectChildren::prepare_template_data.Two corrections
The review's
information_schema.schematafinding rested on a false premise-- PG14's
schemataview is byte-identical to PG18's, both carryingOR has_schema_privilege(...). It was never owner-only. The gate is leftalone: removing it is a fail-loud behaviour change, not a redundancy cleanup,
and it deserves its own decision.
The NULL-status commit message overclaimed.
finalize_enrollmenttakes thesame
nextforseatedandclosed, so the child is skipped either way;what actually changes is the capacity credit, which stops the cart being
invisible to itself and overselling on the next delivery. The comment says
that now. A cross-family assertion was also vacuous -- it read
child_namewhere the snapshot writes
first_name-- and is fixed.Test evidence, round 12
Local
carton exec prove -lr t/:Files=284, Tests=2456, Result: PASS(from 279/2412). CI on
e19de93:test,e2e,lint,securityandstripe-e2eall green.Every fix above was written test-first with the failure observed, and the
schema-dump guard was mutation-graded rather than merely asserted.
Filed rather than fixed here: the unvalidated
session_for_*value, which ispre-existing and reaches settlement after capture;
DropRequest::approvewriting an illegal
refund_statusthat silently discards the approval andreports success; and
database-compatibilitynever running on pull requests.https://claude.ai/code/session_01UMLwCP8cMNQ2kc4LnfeVc2