feat: connect report-result stats review flow (rebased onto main) - #96
Merged
Conversation
Resolves the six conflicts that accumulated after the trade (#94) and scoped roster roles / drops (#95) work landed on main. Resolutions, all union-style — no behavior from either side dropped: - apps/bot/src/index.ts: keep main's help/tr_/dr_/trade: button routes and add the mr_stats: route ahead of sc_up:. No prefix collisions. - lib/command-access.ts: OperationalCapability becomes the union of both sides ('trade' from main, 'enter-match-stats' from this branch). The SAL_MATCH_STATS_ROLE_IDS override and its fail-closed validation are kept. - lib/command-access.test.ts: keep both sides' assertions in the admin override test, plus the three match-stats capability tests. - lib/outbox-projections.ts: union the discord.js and channels imports; route match_report review rows to projectSubmittedMatchReport ahead of the legacy pending_stat_record branch. Dropped this branch's duplicate messageHasMarker helper — main introduced an identical one for the trade and drop projections, so the callers now share that single definition. - packages/db/src/queries/index.ts: export match-reports alongside main's roster-trades, roster-drops, and roster-role-mappings. - docs/commands.md: take main's fuller command table and apply this branch's updated /report-result description. Note for review: hasCommandAccess now runs validateCommandAccessEnv on every call, so a malformed (optional) SAL_MATCH_STATS_ROLE_IDS fails closed for /report-result and /log-scouter, not just the stats button. That is this branch's intended fail-closed posture; leaving the variable unset keeps the operator-allowlist fallback. Verification on the merge result: - pnpm typecheck: 5/5 tasks - pnpm test: 182 passing (144 bot, 31 db, 7 shared) - pnpm lint: 3/3, pnpm build: 3/3 - pnpm check:db-contract: verified against released db-v1.20.0 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MxSTraGzCWDmpCS4h6tCYC
![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: bc5ce901e5
ℹ️ 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".
This was referenced Aug 31, 2026
Addresses the Codex P1 on this PR. AGENTS.md "No Silent Mutations" requires every state change to `matches` to write an `audit_logs` entry with the actor and old/new values, but setProofThread did a bare `.update()`. I originally raised this rather than patching it, on the grounds that the gap predated this branch and that the audit actor would have to be synthetic. Both concerns turned out to be wrong on closer inspection: - Not synthetic. Within this branch every proof-thread write flows through match-result-discord.ts, which holds action.requestedByDiscordId — the operator whose /report-result submission caused the thread to exist. That is a real Discord identity, so the audit row has a genuine actor. - Not a schema change. audit_logs.action_type is plain text with no CHECK constraint and no enum, so the new "proof_thread_recorded" action type is a code-level contract only and needs no database release. setProofThread now reads the current pointer, writes the update, and records the audit entry. Both the first attachment and the crash-recovery re-attachment go through it, so the history reads the same either way rather than recording a pointer on retry but not on first creation. Verification: - new coverage in matches.test.ts for both call shapes: first attachment (null previous pointer) and recovery over a stale pointer, asserting the actor and the old/new values that land in audit_logs - proof-thread.test.ts and report-result-stats.test.ts assert the actor is threaded through rather than dropped - pnpm test — 186 passing (144 bot, 33 db, 9 shared), up from 182 - pnpm typecheck 5/5, pnpm lint 3/3, pnpm build 3/3 - pnpm check:db-contract — db-v1.20.0 (20260823130000) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MxSTraGzCWDmpCS4h6tCYC
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
2 participants
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.
What
Carries the full contents of #90 and merges current
maininto it, resolving the six conflicts that accumulated after the trade workflow (#94) and scoped roster roles / drops (#95) landed.Functionally this is #90 —
/report-resultatomically creates/recovers the pending result and match report, authorizes the host review link through a dedicatedenter-match-statscapability, mirrors host-submitted screenshots, and posts the admin stats-review link idempotently. This PR adds no new behavior of its own beyond the conflict resolution.This supersedes #90, which is stuck at
mergeable_state: dirty. Close #90 in favor of this branch, or cherry-pick the merge commit onto it — whichever you prefer.Conflict resolutions
All six were union-style; no behavior from either side was dropped.
apps/bot/src/index.tshelp:/tr_/dr_/trade:button routes, addedmr_stats:ahead ofsc_up:. No prefix collisions.lib/command-access.tsOperationalCapabilityis now the union of both sides —tradefrom main,enter-match-statsfrom #90. Kept theSAL_MATCH_STATS_ROLE_IDSoverride and its fail-closed validation.lib/command-access.test.tslib/outbox-projections.tsdiscord.jsandchannelsimports;match_reportreview rows route toprojectSubmittedMatchReportahead of the legacypending_stat_recordbranch. Dropped #90's duplicatemessageHasMarkerhelper — main independently added an identical one for the trade/drop projections, so callers now share one definition.packages/db/src/queries/index.tsmatch-reportsalongside main'sroster-trades,roster-drops,roster-role-mappings.docs/commands.md/report-resultdescription.Notes for review
hasCommandAccessnow runsvalidateCommandAccessEnvon every call, so a malformed (optional)SAL_MATCH_STATS_ROLE_IDSfails closed for/report-resultand/log-scouter, not only the stats button. That is feat: connect report-result stats review flow #90's intended fail-closed posture; leaving the variable unset keeps the operator-allowlist fallback.tradeis in the capability union but has no call site —/tradeand/dropauthorize throughroster-authorization.ts— so this does not affect them.db-v1.18.0; that merged on 2026-08-19 and production is now atdb-v1.20.0. Merging main brought the current contract lock with it, so the "small pin commit" feat: connect report-result stats review flow #90 anticipated is not needed here —check:db-contractverifies clean against releaseddb-v1.20.0.Dependencies and rollout
host-tokenroute this branch calls). Merge and deploy #262 first — this branch'sissueMatchReportHostReviewLinkposts to/api/internal/match-reports/[id]/host-token, which only exists there. Note #262's contract lock is still atdb-v1.17.0and needs refreshing todb-v1.20.0.SAL_SITE_URLandSAL_SITE_INTERNAL_TOKEN(matching sal-site'sINTERNAL_SERVICE_TOKEN).SAL_MATCH_STATS_ROLE_IDSfor a narrower stats-entry role; unset falls back to the operator allowlist, and admins always retain access.No bot deployment or production write was performed from this branch.
Verification
Run against the merge result, not either parent:
pnpm typecheck— 5/5 taskspnpm test— 182 passing (144 bot including the 7report-result-statstests, 31 db, 7 shared)pnpm lint— 3/3pnpm build— 3/3pnpm check:db-contract— verifieddiese-tech/sal-database db-v1.20.0(20260823130000)Full Discord → deployed site → database verification still requires #262 deployed and the secrets above set.
Generated by Claude Code