feat(match-report): add audited corrections for published results - #111
Merged
Conversation
A completed match report could not be repaired. `resolve_match_report_review` returns early for a report already `done` -- `already_processed`, `applied = false`, writing nothing -- so an admin who spotted a misread scoreboard after approval had no path to fix it, and a consumer that resubmitted saw the original scores echoed back as though they had been saved. That terminal behavior is a safety property, not the bug: the bot and outbox flows retry, and making approval mutate on retry would let a duplicate call silently overwrite a published league result. It is left exactly as it was. Correcting a published result becomes its own explicit entry point instead, mirroring the post-persist scouter correction path in 20260805031500: - `correct_match_report_result` requires an administrator, the revision it expects, and a reason; replaces the complete stat set; updates the still-completed match; republishes official stats through `publish_match_report_stats`; and enqueues its own standings recalculation under a revision-scoped deduplication key. - `match_report_corrections` keeps immutable receipts carrying the request, the full before-and-after snapshot, and the returned result, so an exact retry returns the recorded outcome rather than correcting twice. - Reviewed-payload rules move into `private.validate_match_report_games` so a correction cannot accept anything approval would reject. 030_match_report_result_corrections.test.sql covers the approval path staying terminal, revision-checked replacement, receipt and dual audit evidence, standings re-enqueue, retry safety, authorization and lifecycle boundaries, and validation parity with the approval path. Verified against PostgreSQL by applying the migration to a fixture schema and exercising all 23 assertions: happy path, exact retry, stale revision, blank reason, non-admin, report still in review, unlinked identity, tied series, duplicate game number, and key replay across reports -- each rejection leaving the published result untouched. The pinned Supabase CLI containers are unreachable from this environment, so generated/database.types.ts was written by hand against the generator's existing output; regenerate and confirm the drift check before cutting the release.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1895a9a757
ℹ️ 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".
…ion credit Addresses the schema-contract failure and three P1 review findings. - 001_schema_contract: the suite asserts an exact inventory, so the new table and RPC move it to 52 tables and 54 public functions. - Concurrent retries (P1): two retries sharing a key could both observe no receipt; the loser then reloaded the report behind the winner's revision bump and raised the stale-revision error instead of returning the recorded outcome. Reproduced with two concurrent transactions, which failed exactly that way. A transaction-scoped advisory lock on the correction key now serializes them, and the same race returns `already_corrected` with one receipt and one revision bump. - Key reuse (P1): the receipt lookup compared only the report, so a client that accidentally retained a key had its next correction silently discarded as a retry. The key is now bound to the actor, reason, expected revision, and games payload; a mismatch raises rather than replaying. - Cross-division credit (P1): an organization can hold a season roster in more than one division, and the roster lookup checked only season and organization, so a player rostered in another division could be stamped with this match's division. The correction path now requires the roster division to match. That last rule makes the correction path stricter than approval, which has the same gap. Tightening approval means replacing a function that publishes canonical league stats and belongs in its own reviewed change; the repair path takes the stricter rule now, and the migration comment says so. All 26 assertions of 030 pass against the fixture schema, including three new ones for key reuse and cross-division rosters.
CI failed the exact-retry assertion with "Correction key already recorded for a different correction." The fixture's per-game player aggregate had no ORDER BY, so two calls could serialize the ten players in different orders and the second request no longer matched the recorded one. It passed locally only because that cluster happened to return a stable heap order. The builder now orders players by id. Also documents, where a caller will read it, that the games comparison is exact including array order: a retry is a resend of the same request, not an equivalent one. 001_schema_contract passes with the updated inventory; this was the only remaining failure.
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.
Problem
A completed match report could not be repaired.
resolve_match_report_reviewreturns early for a report alreadydone—already_processed,applied = false, writing nothing — so an admin who spotted a misread scoreboard after approval had no path to fix it. Worse,sal-sitediscarded that flag and rendered "Result Submitted", so a resubmit looked successful while the corrections were dropped (fixed separately in diese-tech/sal-site#266).That terminal behavior is a safety property, not the bug. The bot and outbox flows retry; making approval mutate on retry would let a duplicate call silently overwrite a published league result.
resolve_match_report_reviewandresolve_match_report_review_unpublishedare therefore left exactly as they were, and030asserts the approval path is still terminal after a correction.Change
Correcting a published result becomes its own explicit entry point, mirroring the post-persist scouter correction path in
20260805031500.public.correct_match_report_result(uuid, text, integer, text, text, jsonb)— service-role-only. Requires an administrator, the revision it expects, and a reason. Replaces the complete stat set, updates the still-completed match, republishes official stats throughprivate.publish_match_report_stats, and enqueues its own standings recalculation under a revision-scoped deduplication key.public.match_report_corrections— immutable receipts carrying the request, the full before-and-after snapshot, and the returned result. A transaction-scoped advisory lock on the correction key serializes concurrent retries, and the key is bound to the whole request (actor, reason, expected revision, games) so a reused key is reported rather than silently replayed. The games comparison is exact including array order: a retry is a resend of the same request, not an equivalent one.private.validate_match_report_games(text, jsonb)— the reviewed-payload rules, factored out so a correction cannot accept anything approval would reject.Verification
CI is green:
database-contractruns a cleansupabase db reset,db lint, all 30 pgTAP suites, and a generated-type regeneration diffed against the committed file. All 26 assertions of030pass there.Locally I also reproduced the concurrency bug before fixing it — two concurrent transactions sharing a correction key gave
B: applied/A: ERROR: Match report changed since it was loaded, and now giveA: applied/B: already_correctedwith one receipt and one revision bump.already_corrected, no second mutation23505already_corrected5500042501review55000(approval owns that transition)22023235142351422023/23505already_processed, stats untouchedContract bumped
db-v1.20.0→db-v1.21.0at head20260901120000.030_match_report_result_corrections.test.sqlis registered inrequiredDatabaseTests.One thing for maintainers
The approval path has the same cross-division gap the correction path now closes:
resolve_match_report_review_unpublishedchecks only season and organization, so it can publish a stat row for a player rostered in a different division of the same organization. I did not change it here — that function publishes canonical league stats, and replacing it belongs in its own reviewed change rather than riding along on a repair path. The correction path takes the stricter rule now, and the migration comment records the divergence and why. Worth a follow-up issue.(An earlier revision of this description asked you to re-run
supabase gen typesbecause the generator's container image is unreachable from my environment. CI has since regenerated and diffed the types successfully, so the committed file matches the generator and no manual step is needed.)