chore(scripts): make the QA-fixture removal safe to run — owner-only trigger disable, scoped post-check, cascade capture by njrini99-code · Pull Request #1759 · njrini99-code/helmv3

njrini99-code · 2026-09-02T17:20:14Z

Follow-up to #1725's migration review (db-migration-reviewer, 2026-09-02). That review found three defects in scripts/remove-qa-fixtures-2026-08-31.sql and its dry run. This PR fixes all three and touches only the two scripts — the migration and HELD.md are unchanged.

Branch note. This branch is cut from the tip of fix/integrity-completed-round-zero-scored-holes (which already has main merged in), plus one commit (a7aed7edc) carrying the two scripts. #1725 was still OPEN when this was opened, so the diff against main currently shows #1725's files too; it shrinks to the two scripts the moment #1725 merges. Review only a7aed7edc.

What was wrong, and what changed

#	Review finding	Fix
1	Deleting 4 completed rounds trips `golf_rounds_guard_lifecycle` (55000); the `atomic` marker does not help — production's guard text (`20260825090000`) honours it for UPDATE only, the HELD rewrite (`20260830120000`) returns bare `NEW` (NULL in BEFORE DELETE → silent no-op).	`ALTER TABLE public.golf_rounds DISABLE TRIGGER golf_rounds_guard_lifecycle` around exactly the round `DELETE`, inside the one transaction, `ENABLE` before the qualifier is touched. Owner-only, enforced: a precondition block refuses unless `current_user = postgres` and it can ALTER the table (owner-role membership or superuser), and unless the trigger is present in state `O`. The post-check refuses to COMMIT unless it reads `O` again. `session_replication_role` deliberately not used (it would silence the FK cascades).
2	Post-check was global — one unrelated broken round anywhere aborts the cleanup.	Post-check gates on the fixture set only (15 in-set counts must be 0, trigger must be `O`, marker must be blank). Integrity check 6's global count is a `NOTICE`, information only.
3	Dry run missed what `ON DELETE CASCADE` / `SET NULL` touch.	Dry run is now one read-only statement returning one result set of full jsonb rows: the CASCADE set (`golf_holes`, `golf_shots`, `golf_round_stats_cache`, `golf_round_reviews`, second-order `golf_review_events`), the SET NULL set (`golf_predictions.related_round_id`, `golf_qualifier_entries.round_id`, `golf_player_focus_areas.from_review_id`), the qualifier's own cascade (entries, round courses, selections), the before-image of the affected players' `golf_player_stats_cache` rows that `trg_update_player_stats_complete` rewrites, a live-catalog listing of every FK/trigger involved (so a new FK cannot hide), 8 `assert` rows and the same count keys the removal prints BEFORE and AFTER.

FK actions were verified against the catalog, not the docs: holes/shots/stats_cache/reviews CASCADE; golf_predictions.related_round_id and golf_qualifier_entries.round_id SET NULL — the review had them right (the predictions column is related_round_id).

Also added: lock_timeout (the ALTER takes SHARE ROW EXCLUSIVE on golf_rounds until COMMIT — fail fast rather than queue writers); refusals if a fixture id sits on a non-demo team or any round outside the set references the QA qualifier; a documented rehearsal path; a read-only post-COMMIT verification SELECT; exact psql command lines in both headers.

Local-stack verification (never run against production)

Five fixture rounds with the production ids (4 completed, 1 in_progress) on a local team carrying the demo team id, with 4 stats-cache rows, a review + review event, a prediction and two entries referencing them.

Dry run: one result set — 12 catalog_fk + 14 catalog_trigger rows, every cascade/set-null row exported, all 8 assert rows at their required values.
Removal: DELETE 5 rounds / 2 entries / 1 qualifier. BEFORE → AFTER: rounds_in_set 5→0, stats_cache_in_set 4→0, reviews_in_set 1→0, review_events_in_set 1→0, predictions_related_round_in_set 1→0 (row survives, column nulled), entries_round_id_in_set 2→0, qualifier_rows 1→0, qualifier_entries 2→0, qualifier_round_courses 1→0. guard_trigger_tgenabled = 'O'. An md5 fingerprint of every row outside the set is byte-identical before and after; the affected players' caches recomputed back to their pre-fixture rounds_played.
Second run: NO-OP notice, DELETE 0 everywhere, commits, trigger 'O'.
Rehearsal path (sed 's/^COMMIT;$/ROLLBACK;/'): DELETE 5/2/1 then ROLLBACK, rows still present.
Proven failure mode — DISABLE line removed, production guard text (20260825090000) swapped in locally: ERROR: Completed rounds are permanent history and cannot be deleted. (55000), psql exit 3, nothing deleted, trigger 'O'. With the HELD guard text: the round DELETE silently no-ops and the entry delete then raises 55000 — also rolled back. Guard text restored afterwards, md5 verified.
Review item 2 exercised directly: the local seed carries 3 unrelated completed rounds with no scored holes; the old global post-check would have aborted, the scoped one reports them as INFO and commits.
Unscored hole on a completed fixture: deleted via the marker (DELETE 1). That one run needed golf_holes_recompute_round_totals disabled in the local session only, because HELD 20260708141000 (local, not production) makes recompute_golf_round_totals raise 42501 without auth.uid(). Not part of the script.
sqlfluff core: 0 violations on both scripts (was 47). sql:ratchet OK, no regressions (scripts/ is outside its scope regardless).

One observation for the owner: the guard fingerprint HELD.md recorded from the live catalog on 2026-08-30 (cbc5671b…, len 3403) matches neither 20260825090000 as applied locally (9fd5acb8…, 3428) nor the HELD rewrite (aeaeb0ce…, 3731). I did not read production to resolve it. It does not change this script — every candidate version raises 55000 on the delete and none lets a marker-only delete through — but the dry run prints the live fingerprint (info/guard_function_fingerprint) so it can be compared before the migration is applied.

Owner run order (all from the repo root, as `postgres`, with psql — the SQL editor drops the NOTICE report)

# 1. Dry run — read-only; the output IS the export, keep it
psql "$HELM_PROD_DB_URL_DIRECT" -X -v ON_ERROR_STOP=1 --csv \
  -f scripts/remove-qa-fixtures-2026-08-31-dryrun.sql \
  > "qa-fixtures-export-$(date -u +%Y%m%dT%H%M%SZ).csv"; echo "exit=$?"
# every 'assert' row must carry the value its id names; STOP otherwise

# 2. (optional) rehearsal — runs everything, commits nothing
sed 's/^COMMIT;$/ROLLBACK;/' scripts/remove-qa-fixtures-2026-08-31.sql \
  | psql "$HELM_PROD_DB_URL_DIRECT" -X -v ON_ERROR_STOP=1 -f -

# 3. Removal — exit 0 + COMMIT in the log = done; exit 3 = stopped before COMMIT, rolled back
psql "$HELM_PROD_DB_URL_DIRECT" -X -v ON_ERROR_STOP=1 \
  -f scripts/remove-qa-fixtures-2026-08-31.sql \
  > "qa-fixtures-removal-$(date -u +%Y%m%dT%H%M%SZ).log" 2>&1; echo "exit=$?"

# 4. Fingerprint check, then apply the migration per its HELD.md row
psql "$HELM_PROD_DB_URL_DIRECT" -X -Atc \
  "select md5(pg_get_functiondef('public.run_integrity_checks()'::regprocedure));"
#    must equal ae683fa1797204f933b261714d3dba84 (HELD.md) — STOP if it differs
#    then apply supabase/migrations/20260901120000_integrity_completed_round_zero_scored_holes.sql
#    check 6 reports pass/0 once the fixtures are gone

Do not merge this on my account — it is the owner's production data-removal script.

🤖 Generated with Claude Code

Production carries four rounds with status='completed', holes_played=18 and a total_score, backed by zero golf_holes rows with a score. Nothing watched for that, so nobody would have known. The forensics say seeded fixture, not lifecycle failure, and the ids are what settle it: 0b000000-0000-4000-b000-00000000000{1,2,3,4} — sequential, patterned literals. Three share created_at to the microsecond (19:01:27.511173+00), every updated_at equals its created_at, none carries a course_id, and current_hole=1 sits against holes_played=18. No application path emits a uuid like that; they were inserted directly with the service role, on demo team 6ecdd1a6. The check ships anyway. save_partial_round_atomic is a REPLACE — it deletes a round's holes and shots and rebuilds them from the client payload — so a malformed snapshot arriving after a durable write reaches the same state by a route that is not seeding. "Completed with nothing scored" is also a state a player can be shown and analytics will count. R3, prepared not applied: checks 1-5 are reproduced verbatim from the live definition (md5 ae683fa1797204f933b261714d3dba84, length 3789, read 2026-09-01) and the file says to re-check that fingerprint before applying. The new check's query was validated read-only against production and returned exactly the four ids above. It will report fail with count=4 on the first run. That is the intended reading — the alert is correct and the data is wrong; it goes green when the fixtures are removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CeMzi9j5PSzd8GprDapP85

…ed-round-zero-scored-holes

Audit item 6. Dry-run + apply, following the convention already in scripts/ (backfill-archive-insufficient-sample-insights{,-dryrun}.sql). Owner-executed; nothing here runs automatically. The set is eight rows on demo team 6ecdd1a6, and it identifies itself: the qualifier is literally named "QA — Round Type Verification 2026-08-31", the five round ids are sequential literals, three share created_at to the microsecond, every updated_at equals its created_at, and none carries a course_id. Blast radius measured against production 2026-09-01: every other team has zero fixture-shaped rows — Guilford 184 rounds, Demo University Golf (Pat) 90, Shenandoah 63, UNC Wilmington 50, Lynchburg 37, all clean. The contamination never left the demo team. Two properties worth stating, because a production DELETE is the wrong place to be clever: - Explicit id lists, never a LIKE pattern. A pattern that widens by accident takes real rows with it. - The script refuses itself. A DO block aborts the transaction if any round in the set carries a scored hole or a shot, so if a genuine round ever holds one of these ids it survives. A second DO block re-checks the invariant after the deletes and rolls back if any completed round still reports zero scored holes — a new one appearing between now and execution should stop the run, not ride along with it. The dry run's OUTPUT IS THE EXPORT: it prints every column of every row plus both assertions. Save it before applying. Verification after apply: integrity check 6 goes fail/4 -> pass/0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CeMzi9j5PSzd8GprDapP85

…ed-round-zero-scored-holes

The registry's keys must equal the set of open PRs; this row makes the PR IN FLIGHT rather than unclassified while it waits on review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

… baseline Only comment lines and indentation moved; no statement changed. The Review Gate's sql-lint ratchet counts every violation in supabase/migrations against .sqlfluff-baseline.json, so a new file must lint clean under --rules core to land. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…assertion db-migration-reviewer (2026-09-02): the predecessors (20260701150000, 20260704130000) each carry REVOKE ALL FROM PUBLIC/anon/authenticated, GRANT EXECUTE TO service_role, and a DO block that fails the migration if anon or authenticated can EXECUTE. This file dropped all three. CREATE OR REPLACE preserves the ACL, so nothing was live-exposed; the assertion is the tripwire for the recreate-re-grants-anon class shipping.md §4 records. Lint-clean under the ratchet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…ed-round-zero-scored-holes # Conflicts: # supabase/migrations/HELD.md

…trigger disable, scoped post-check, cascade capture The db-migration-reviewer pass on #1725 found three defects in scripts/remove-qa-fixtures-2026-08-31.sql and its dry run. All three are fixed here; the migration and HELD.md are untouched. 1. The delete of four completed rounds could not succeed. The lifecycle guard (golf_rounds_guard_lifecycle, BEFORE DELETE) raises 55000 on a completed round, and the helm.golf_lifecycle_write='atomic' marker does not get past it: the guard text applied to production (20260825090000) only honours the marker for UPDATE, so the DELETE still raises; the HELD rewrite (20260830120000) returns bare NEW, NULL in a BEFORE DELETE row trigger, so the delete silently no-ops. The script now disables that one trigger for exactly the round DELETE, inside the single transaction, and re-enables it before the qualifier is touched. Owner-only: a precondition block refuses unless current_user is postgres AND can ALTER the table (owner-role membership or superuser), and unless the trigger is present in state O. The post-check refuses to COMMIT unless it reads O again. session_replication_role is deliberately NOT used — it would silence the FK cascade triggers too. 2. The post-check was global (any completed round with zero scored holes, anywhere), so one unrelated broken round would abort the cleanup. It is now scoped to the fixture set — every in-set count must be 0 — and the global integrity-check-6 count is a NOTICE, information only. 3. The dry run exported only the rounds, qualifier and entries. It now exports, as full jsonb rows in one result set: the CASCADE set (golf_holes, golf_shots, golf_round_stats_cache, golf_round_reviews and the second-order golf_review_events), the SET NULL set (golf_predictions.related_round_id, golf_qualifier_entries.round_id, golf_player_focus_areas.from_review_id), the qualifier's own cascade (entries, round courses, selections), the before-image of the affected players' golf_player_stats_cache rows that trg_update_player_stats_complete rewrites, a live-catalog listing of every FK and trigger involved so a new FK cannot hide, and the same count keys the removal prints BEFORE and AFTER. FK actions were verified against the catalog, not the docs. Also: lock_timeout so the SHARE ROW EXCLUSIVE from ALTER TABLE fails fast instead of queueing writers; refusals if a fixture id sits on a non-demo team or any round outside the set references the QA qualifier; a documented rehearsal path (COMMIT -> ROLLBACK via sed); a read-only post-COMMIT verification SELECT; explicit psql command lines and run order in both headers. Verified on the LOCAL Docker stack only (never against production): - Five fixture rounds with the production ids (4 completed, 1 in_progress) on a local team carrying the demo team id, with 4 stats-cache rows, a review + review event, a prediction and two entries referencing them. - Dry run: one result set, 12 catalog_fk + 14 catalog_trigger rows, all cascade/set-null rows exported, all 8 assert rows at their required values. - Removal: DELETE 5 rounds / 2 entries / 1 qualifier; every in-set count 0 after; SET NULL'd prediction survives with the column nulled; trigger tgenabled='O'; a fingerprint of every row outside the set is byte- identical before and after; affected players' caches recomputed back to their pre-fixture values. - Second run: NO-OP notice, DELETE 0 everywhere, commits, trigger 'O'. - Rehearsal path: DELETE 5/2/1 then ROLLBACK, rows still present. - Failure mode with the DISABLE line removed, production guard text swapped in locally: ERROR "Completed rounds are permanent history and cannot be deleted." (55000), psql exit 3, nothing deleted, trigger 'O'. With the HELD guard text: the round DELETE silently no-ops and the entry delete then raises 55000 (saved qualifier round) — also rolled back. - The local stack's 3 unrelated completed-rounds-with-no-scored-holes are reported as INFO and no longer abort the run (review item 2). - An unscored hole on a completed fixture is deleted via the marker (DELETE 1). That run needed golf_holes_recompute_round_totals disabled in the LOCAL session only, because HELD 20260708141000 (local, not production) makes recompute_golf_round_totals raise 42501 without auth.uid(). Not part of the script. - sqlfluff core: 0 violations on both scripts (was 47). sql:ratchet OK, no regressions (scripts/ is outside its scope regardless). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

qodo-code-review · 2026-09-02T17:20:20Z

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

coderabbitai · 2026-09-02T17:20:22Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 751a2a4f-c631-4717-b88c-ec526c9567ed

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

vercel · 2026-09-02T17:20:22Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated
helmv3	Ignored	Preview	Sep 2, 2026 5:20pm UTC

supabase · 2026-09-02T17:20:26Z

Updates to Preview Branch (agent/fix-qa-fixture-removal-scripts) ↗︎

Deployments	Status	Updated
Database	✅	Wed, 02 Sep 2026 17:21:17 UTC
Services	✅	Wed, 02 Sep 2026 17:21:17 UTC
APIs	✅	Wed, 02 Sep 2026 17:21:17 UTC

Tasks are run on every commit but only new migration files are pushed.
Close and reopen this PR if you want to apply changes from existing seed or migration files.

Tasks	Status	Updated
Configurations	✅	Wed, 02 Sep 2026 17:21:24 UTC
Migrations	✅	Wed, 02 Sep 2026 17:21:35 UTC
Seeding	✅	Wed, 02 Sep 2026 17:21:37 UTC
Edge Functions	⚠️	Wed, 02 Sep 2026 17:21:37 UTC

⚠️ Warning — Only Functions declared in config.toml will be automatically deployed to branches: [functions.my-slug]

View logs for this Workflow Run ↗︎.
Learn more about Supabase for Git ↗︎.

njrini99-code · 2026-09-02T17:30:56Z

Converted to draft, not merged: the owner decided (2026-09-02, relayed from the session handling #1725) to KEEP the five QA fixture rounds and their qualifier in production. Nothing runs either removal script. This PR stays as a reviewed, locally verified removal procedure in case that decision changes; the follow-up for integrity check 6 is to exclude the demo team rather than delete the fixtures.

…he fixtures Owner instruction 2026-09-02: "Don't delete them." The five patterned QA rounds and their qualifier on the demo team stay in production, so the two owner-run removal scripts leave this PR unrun and #1759 (their fixes) is closed. The HELD.md row now records the consequence: check 6 as written reports fail/4 on those fixtures every run, so the migration is held until the check is amended (exclude the demo team or the fixture ids) or the owner accepts the daily fail. The migration file itself is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…e has ended `open-pr-residue` requires the key set to equal the open-PR set, and a row for an in-flight PR lives at that PR's own head rather than on main — so #1836's row rides its own branch. Verified both directions: without it the check names #1836 as unclassified; with it, it does not. Also deletes #1725 and #1738, both MERGED, both past the transitional grace this file's own `$comment` defines — the tool's output says in as many words to delete them "in the PR you are already opening", and that is this one. Leaving them is the exact failure the comment describes: a current-state registry asserting things that stopped being true. **`open-pr-residue` still FAILS, for reasons this PR does not own.** Twelve open PRs carry no row at all — #1834, #1833, #1832, #1831, #1829, #1827, #1759 and five dependabot PRs. That was already true before this commit and is not mine to reconcile; a row is a statement about someone else's work. Recording it here so the red is legible rather than mysterious. Note for merge order: #1835 also edits this file, so whichever lands second resolves that conflict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019euu9jtJM6WvMVj17coqZ1

njrini99-code and others added 9 commits September 1, 2026 09:54

Merge remote-tracking branch 'origin/main' into fix/integrity-complet…

cd3dcdd

…ed-round-zero-scored-holes

Merge remote-tracking branch 'origin/main' into fix/integrity-complet…

1614772

…ed-round-zero-scored-holes

chore(control-plane): record #1725 in the open PR dispositions registry

eb2f859

The registry's keys must equal the set of open PRs; this row makes the PR IN FLIGHT rather than unclassified while it waits on review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Merge remote-tracking branch 'origin/main' into fix/integrity-complet…

1f8cce4

…ed-round-zero-scored-holes # Conflicts: # supabase/migrations/HELD.md

njrini99-code marked this pull request as draft September 2, 2026 17:30

njrini99-code mentioned this pull request Sep 2, 2026

feat(integrity): alert on a completed round that has no scored hole #1725

Merged

njrini99-code mentioned this pull request Sep 5, 2026

feat(golf/messages): the approved design — unread lifts, the tail marks the end, and the token that had no utility #1836

Open

Navigation Menu

chore(scripts): make the QA-fixture removal safe to run — owner-only trigger disable, scoped post-check, cascade capture - #1759

chore(scripts): make the QA-fixture removal safe to run — owner-only trigger disable, scoped post-check, cascade capture#1759
njrini99-code wants to merge 9 commits into
mainfrom
agent/fix-qa-fixture-removal-scripts

njrini99-code commented Sep 2, 2026

Uh oh!

qodo-code-review Bot commented Sep 2, 2026

Uh oh!

coderabbitai Bot commented Sep 2, 2026

Review skipped

Uh oh!

vercel Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

supabase Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

njrini99-code commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

njrini99-code commented Sep 2, 2026

What was wrong, and what changed

Local-stack verification (never run against production)

Owner run order (all from the repo root, as postgres, with psql — the SQL editor drops the NOTICE report)

Uh oh!

qodo-code-review Bot commented Sep 2, 2026

Uh oh!

coderabbitai Bot commented Sep 2, 2026

Review skipped

Uh oh!

vercel Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

supabase Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

njrini99-code commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Owner run order (all from the repo root, as `postgres`, with psql — the SQL editor drops the NOTICE report)

vercel Bot commented Sep 2, 2026 •

edited

Loading

supabase Bot commented Sep 2, 2026 •

edited

Loading