Review: Checkout API: guest flow (gamertag + email)

review-813-2026-04-03 Doc

review needs-refinement

Project

Re-review (2026-04-03). Previous review flagged PK migration strategy missing. Issue body now specifies: serial id as new PK, nullable twitch_id + UNIQUE, gamertag column, FK update, all db.ts function updates. PK strategy is resolved. Remaining issues: missing file targets and acceptance criteria for downstream consumers.

Template Completeness

[x] Type — Feature
[x] Lineage — "Required by the /play guest checkout flow (#38)"
[x] Repo — forgejo_admin/twitch-2k-wager
[x] User Story — clear challenger-focused story
[x] Context — explains current twitch_id PK design, guest need, and full migration strategy (6-step plan)
[x] File Targets — 5 files + generic "Tests" entry
[x] Acceptance Criteria — 7 testable criteria
[x] Test Expectations — unit tests for guest + auth flows, CI
[x] Constraints — backward compatibility, email dedup for guests, twitch_id dedup for auth, idempotent migration
[x] Checklist — standard PR/test/no-unrelated
[x] Related — links to #38, story notes, arch notes

Traceability

[x] story:challenger-pay label — Secure Buy-In Payment
[x] story note verified — found in project-twitch-2k-wager user-stories table (key: challenger-pay, note id 1158)
[x] arch:dataflow-twitch-2k-wager label — Data Flow: Twitch 2K Wager
[x] arch note verified — arch-dataflow-twitch-2k-wager (id 1155) exists in pal-e-docs
[x] Forgejo issue — forgejo_admin/twitch-2k-wager#39, open

File Targets

Previously flagged (now resolved):

[x] src/lib/schema.sql — verified: twitch_id is TEXT PRIMARY KEY (line 7), game.challenger_id is TEXT REFERENCES challenger(twitch_id) (line 21). Issue now specifies full 6-step migration plan. Accurate.
[x] src/routes/api/checkout/+server.ts — verified: currently requires twitch_id, twitch_username, email (lines 30-38). Guest path needs gamertag + email without twitch_id. Correct target.
[x] src/lib/db.ts — verified: getChallenger queries by twitch_id (line 54), upsertChallenger uses ON CONFLICT(twitch_id) (line 67), createGame takes challengerId as string (line 96), updateConnectStatus uses twitch_id (line 85), listGamesByChallenger queries by challenger_id (line 133). Issue correctly calls for createGuestChallenger + PK updates.
[x] src/lib/types.ts — verified: Challenger.twitch_id is string (line 28), Game.challenger_id is string (line 39). Issue correctly specifies: id (number), twitch_id (optional), gamertag (string).
[x] src/routes/api/webhooks/stripe/+server.ts — verified: queries challenger by twitch_id (line 142-143), joins game.challenger_id as TEXT (line 176). Needs update for new INT PK.

Still missing from issue body:

[ ] src/routes/api/admin/result/+server.ts — MISSING. Calls getChallenger(game.challenger_id) on line 101. Uses challenger.twitch_id on lines 131, 153, 169 for Connect status updates, transfer initiation, and logging. Will break when game.challenger_id changes from TEXT to INT and when getChallenger signature changes.
[ ] src/routes/game/[id]/+page.server.ts — MISSING. Calls getChallenger(game.challenger_id) on line 32. Ownership check on line 26 compares session.twitchId !== game.challenger_id — this comparison breaks entirely when challenger_id becomes INT. Guest games also have no session.twitchId.
[ ] src/lib/stripe.ts — MISSING. Stores challenger_twitch_id in Stripe Checkout metadata (line 84). Guest challengers have no twitch_id — metadata key needs conditional handling (gamertag or challenger serial id).
[ ] src/lib/auth.ts — MISSING (minor). References twitch_id in session type (line 142). Game status page ownership check depends on this.
[ ] Test files — issue says "Tests — update existing tests" generically. Should enumerate the 6 affected files: checkout.test.ts, webhook.test.ts, result.test.ts, page-server.test.ts, db.test.ts, stripe.test.ts. All hardcode twitch_id/challenger_id as string patterns.

Repo Placement

OK. All files in forgejo_admin/twitch-2k-wager. Single repo change. No cross-repo work.

Dependencies

[x] Board item #812 (/play guest checkout route, issue #38) — consumer of this endpoint. In backlog. This ticket is the upstream dependency. No blocker.
[x] Board item #797 (Stripe Checkout + webhook, issue #11) — in validation. The existing auth checkout flow this ticket must remain backward-compatible with. Confirms backward-compat constraint is real.
[x] Board item #796 (Keycloak OIDC auth, issue #10) — in validation. Provides session.twitchId used in game/[id] ownership check. That check must be updated for both auth paths.
[x] Board item #795 (Postgres schema + db.ts, issue #9) — done. The original schema this ticket migrates. Satisfied.
No unresolved blockers.

Acceptance Criteria

7 AC present, all testable by an agent. 3 missing criteria for downstream consumers not listed in file targets:

Missing: "Admin result endpoint works with new challenger PK — getChallenger retrieves by serial id, Connect status updates use correct identifiers"
Missing: "Game status page ownership check works for both Twitch-auth and guest challengers — session.twitchId comparison updated for new PK model"
Missing: "Stripe Checkout metadata includes gamertag for guest flow — challenger_twitch_id is nullable or supplemented with gamertag"

Blast Radius

Significant but contained to one repo. The PK migration from twitch_id (TEXT) to serial id (INT) touches every consumer of the challenger table:

admin/result endpoint — loads challenger by game.challenger_id, uses challenger.twitch_id for Connect status updates and logging. Both the lookup pattern and the twitch_id references need updating.
game/[id] page — ownership check compares session.twitchId to game.challenger_id. After migration challenger_id is INT. Guest games have no session.twitchId at all — need alternative ownership mechanism.
Stripe metadata — stores challenger_twitch_id. Guest flow needs gamertag or serial id as identifier in metadata.
Webhook handler — processPendingPayouts joins game.challenger_id (changes type from TEXT to INT reference).
Architecture notes — arch-domain-twitch-2k-wager ER diagram shows twitch_id as PK. arch-dataflow-twitch-2k-wager shows Twitch-only identity flow. Both need post-implementation updates (separate tickets).
Rollback is straightforward (revert migration + code). No sibling services affected.

Decomposition Assessment

9+ file targets (including 4 missing) in 1 repo. 7 AC present (should be 10). All changes are tightly coupled — schema migration drives type changes drives db function changes drives endpoint changes. Splitting would create merge conflicts and require coordinated migration state. Applying the three-thing limit: 3 conceptual changes (schema migration, guest checkout path, consumer updates). Applying the five-minute rule: borderline at 4-6 minutes with a complete spec, but the tight coupling justifies a single agent pass. No decomposition needed if file targets and AC are completed — the work is cohesive.

Recommendation

[BODY] Add missing file target: src/routes/api/admin/result/+server.ts — update getChallenger call (line 101) and challenger.twitch_id references (lines 131, 153, 169) for new INT PK model
[BODY] Add missing file target: src/routes/game/[id]/+page.server.ts — update ownership check (line 26: session.twitchId !== game.challenger_id) and getChallenger call (line 32) for new PK. Add guest ownership mechanism.
[BODY] Add missing file target: src/lib/stripe.ts — update Stripe Checkout metadata (line 84) to handle guest flow (gamertag or serial id when twitch_id unavailable)
[BODY] Add missing file target: src/lib/auth.ts — session type twitch_id reference (line 142), needed for updated ownership check
[BODY] Enumerate test files explicitly: checkout.test.ts, webhook.test.ts, result.test.ts, page-server.test.ts, db.test.ts, stripe.test.ts
[BODY] Add AC: "Admin result endpoint works with new challenger PK"
[BODY] Add AC: "Game status page ownership check works for both Twitch-auth and guest challengers"
[BODY] Add AC: "Stripe Checkout metadata includes gamertag for guest flow"