feat: unblock race

[jamesacres]

No description provided.

[jamesacres]

@claude review PR

[claude]

Claude finished @jamesacres's task in 3m 47s —— View job

---

Review: feat: unblock race

Todo list:

• Gather context (PR diff, changed files)
• Read CLAUDE.md / ARCHITECTURE.md conventions
• Review new unblockrace module (dto/entity/repository/service/controller)
• Review new unblock-race-* utils (reader, selection, s3-fetcher, file-fetcher)
• Review deploy/CDK changes (api-stack.ts, sync-static.sh)
• Review tests (unit/integration/e2e)
• Post final review

---

Overall this is a well-built feature that closely mirrors the existing sudoku module's shape (DTO/entity/repository/service/controller, Model enum entry, @ApiKey() auth) and follows ARCHITECTURE.md's decision tree correctly. The binary reader (unblock-race-reader.ts), S3/file fetchers, and puzzle selection logic are clearly documented, and the test coverage across unit/integration/e2e/CDK is thorough (I checked the math on MOVE_DISTRIBUTION — it does sum to 50 with the claimed 10/10/20/10 split). static/ is correctly excluded from the Lambda bundle via nest-cli.json and .gitignore is untouched, matching the documented S3-sync approach.

A few things worth addressing before merge:

1. ARCHITECTURE.md wasn't updated. The Module Hierarchy list (ARCHITECTURE.md:31-41) still doesn't mention UnblockRaceModule, even though PLAN-unblockrace.md itself calls for this (step 6) and CLAUDE.md requires "md files are updated if they reference something which is no longer true." The feature-module list is now inaccurate.
   Fix this →

2. PLAN-unblockrace.md and TODO.md look like scratch planning artifacts committed to the repo root, and the plan is already stale relative to what was implemented (e.g. it references rush-seeds/rush.bin, seeds:sync, RUSH_BUCKET/RUSH_KEY, whereas the actual code uses static/unblock-race/puzzles.bin, static:sync, STATIC_BUCKET/UNBLOCK_RACE_KEY). Left in place, this will confuse future readers. Consider dropping both files (or folding any still-relevant notes into ARCHITECTURE.md/README) before merge.

3. TODO.md flags "revenuecat api key for unblock race" as outstanding. As shipped, both endpoints are gated only by a shared @ApiKey() (same model as sudoku's ofTheDay/bookOfTheMonth), with no RevenueCat entitlement check. Worth confirming that's intentional for this PR and tracked separately, rather than an accidentally-dropped requirement.

4. Minor, pre-existing pattern, not introduced here: UnblockRaceService.unblockRaceOfTheDay/...CollectionOfTheMonth do find-then-generate-then-upsert, and DynamoDBAdapter.upsert (src/dynamodb/dynamodb-adapter.ts:57-95) is an unconditional SET, not a conditional put. Two concurrent cache-miss requests (e.g. the pre-warm cron firing alongside a real user request at the boundary) would each generate a different random puzzle set and the second write wins, so a caller could hold puzzles that don't match what's persisted for later callers. This mirrors SudokuService.sudokuOfTheDay exactly, so it's an existing accepted trade-off in this codebase, not a regression — flagging only for awareness since the daily pre-warm cron this PR adds increases the chance of overlap with the first live request of the day.

Nice touches: the e2e suite exercises the real committed puzzles.bin end-to-end (not just mocks), the CDK test asserts cron schedules don't overlap, and unblock-race-selection.ts's hardcoded index ranges are well-documented with their derivation method.

I wasn't able to run npm run build:nest / npm test / npm run lint in this environment (command execution required interactive approval that wasn't available here) — worth running those locally before merge per CLAUDE.md's guidance for complex changes touching persistence and app wiring.

[jamesacres]

@claude review pr

[claude]

Claude finished @jamesacres's task in 2m 38s —— View job

---

Review: feat: unblock race

Todo list:

• Gather context (PR diff, changed files)
• Read CLAUDE.md / ARCHITECTURE.md conventions
• Review new unblockrace module (dto/entity/repository/service/controller) — unchanged since the prior review
• Review new unblock-race-* utils — unchanged since the prior review
• Review deploy/CDK changes — unchanged since the prior review
• Review what's changed since the last @claude review (revenuecat-per-app, session gating, splitAppModelId)
• Post final review

---

This PR has evolved since the last automated review (3 commits ago). The core unblockrace module, utils (reader/selection/fetchers), static puzzle DB, and CDK stack are unchanged since that review and I won't repeat that feedback here — see the previous review comment for those. Both action items from that review are now resolved: ARCHITECTURE.md lists UnblockRaceModule (line 39), and PLAN-unblockrace.md/TODO.md have been removed from the repo root.

What's new since then is feat: add revenuecat api key per app and feat: update gate which allows returning friend sessions, which rework RevenuecatService to take a per-App API key and thread App through the session/party gating logic. Findings on that new work:

1. Bug: RevenuecatService.fetchApi will throw a raw TypeError instead of the intended error when the app-specific key is missing. src/revenuecat/revenuecat.service.ts:23:

   const apiKey = config?.[app].apiKey;

   The optional chaining only guards config being undefined, not config[app]. If revenueCat config exists but has no entry for a given App (a very live scenario here — this PR adds App.UNBLOCKRACE and App.MONEYBAGSRACE, and the earlier review already flagged that the RevenueCat key for unblock race hadn't been provisioned yet), this throws Cannot read properties of undefined (reading 'apiKey') instead of the clearer Error('fetchApi missing apiKey') on line 25. In practice both hasEntitlement call sites (parties.service.ts, members.service.ts) wrap this in .catch()/try-catch so it degrades gracefully today, but the error is misleading when debugging, and revenuecat.service.spec.ts:92-97 only tests config being entirely undefined, not config[app] being missing — so this gap isn't covered by tests. Should be config?.[app]?.apiKey.
   Fix this →

2. members.service.ts:53 correctly derives the app from the invite's resourceId (splitAppModelId(splitModelId(invite.resourceId)[1])) to pass into the now-per-app hasEntitlement/grantEntitlement calls — verified this round-trips correctly since partyId is generated as `${appId}-${nanoid()}` in party.repository.ts:21. Good catch wiring this through; this was the correct fix once RevenueCat needed to be app-scoped.

3. sessions.controller.ts/sessions.service.ts gating (APPS_ALLOWING_CROSS_USER_SESSION_LOOKUP, APPS_ALLOWING_PARTY_SESSIONS_IN_RESPONSE) is now driven by named constants in app.enum.ts instead of an inline [App.SUDOKU] array, and both are extended to include App.UNBLOCKRACE and App.MONEYBAGSRACE. New unit tests cover the "app not allowed" branches for findOne/update. This looks correct — worth double-checking with @jamesacres that opening cross-user session lookup and party-session responses to MONEYBAGSRACE (an app not otherwise touched by this PR) is intentional and not incidental scope creep.

4. Minor, pre-existing, not introduced here: src/dynamodb/dynamodb-adapter.ts:265 changed catch (e) to catch (e: any) — harmless but redundant under the current TS config (implicit any for catch bindings unless useUnknownInCatchVariables is on); not worth blocking on.

I wasn't able to run npm run build:nest / npm test / npm run lint in this environment (command execution required interactive approval that wasn't available here) — worth running those locally before merge per CLAUDE.md, especially since none of the existing tests exercise the missing-per-app-key path in finding #1.
· branch feature/unblock-race