new boom mechanic to try out where participant point gain is randomised - #3
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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. Comment |
| // Stamp the random-scoring cutover on first use so every date already in the ledger keeps its | ||
| // legacy 3-2-1 scoring and is never retroactively re-assigned random points. | ||
| if (!this.data.random_scoring_from) { | ||
| this.data.random_scoring_from = DateTime.now().setZone(TZ).toISODate()!; |
There was a problem hiding this comment.
Blocker: cutover stamped as today, so deploying any time after noon on a day that already scored wipes and re-rolls it.
That date is >= from, so scoreFor returns [] and the points vanish from weeklyTotals. The 30s sweep then re-resolves it via duePending and assigns new random values that contradict the podium already in the channel.
Probe against a store holding today's legacy 3-2-1 podium + daily_announced:
totals BEFORE: []
duePending: [{date:'2026-08-28',game:'boom'}, {...,game:'hadeda'}]
totals AFTER: U1:4 U2:4 U3:4
Deploy on a Friday afternoon and the crown disagrees with the posted message. Stamp tomorrow's date instead.
| * Date+game pairs that have entries and a closed tally window but no awards yet — work the | ||
| * in-process timers missed (e.g. the bot restarted mid-window). | ||
| */ | ||
| duePending(nowMs = Date.now()): Array<{ date: string; game: Game; channel_id: string }> { |
There was a problem hiding this comment.
Same trigger as the cutover issue: duePending doesn't exclude dates with daily_announced.
alreadySettled is false (awards empty), so closeGame re-applies medal reactions to a day the old build already medalled. Skipping announced dates here fixes both.
| } | ||
| } | ||
|
|
||
| await announceDay(client, date, logger); |
There was a problem hiding this comment.
A failed chat.postMessage permanently loses the day's results and the Friday crown.
resolveGame flushes awards before the post, so once all games are resolved duePending returns nothing forever and nothing re-enters announceDay. A transient rate-limit at 12:05 means markDailyAnnounced never runs and the day is silently skipped.
On master the announce block sat in the message handler and got retried by the next message. Suggest a pending-announce check driven by the sweep.
| const sweepClient = (app as any).client; | ||
| if (sweepClient) { | ||
| const sweep = setInterval(() => { | ||
| closeDueWindows(sweepClient).catch(() => {}); |
There was a problem hiding this comment.
Sweep passes no logger, so logger?.error?. (l.94) and logger?.warn?. (l.111) are no-ops and .catch(() => {}) eats the rest.
The restart-recovery path, the one most likely to hit a missing channel or a Slack failure, fails completely silently. Pass app.logger.
| // Compare on the message's own ts (full sub-second precision) so a slow delivery of a | ||
| // message that was sent inside the window still counts. | ||
| const tsMs = Math.round(Number(tsStr) * 1000); | ||
| const tooLate = closesAt != null && Number.isFinite(tsMs) && tsMs > closesAt; |
There was a problem hiding this comment.
The slow-delivery grace this refines is unreachable.
The timer resolves the game at exactly closesAt, so a message sent 12:04:59 and delivered 12:05:02 hits db.isResolved(...) first and is clowned regardless of its own ts. Either resolve after a short grace period, or accept an entry whose ts <= closesAt even when resolved.
| const neededGames: Game[] = weekday === 3 ? ['boom', 'hadeda', 'wednesday'] : ['boom', 'hadeda']; | ||
| const neededGames = neededGamesForDate(date); | ||
|
|
||
| // Settle any window whose deadline passed while no timer was live (e.g. after a restart). |
There was a problem hiding this comment.
This new await yields before the entryFor duplicate check below.
Two messages from the same user delivered in the same tick both see priorEntry === null, so both incrementCount, both addPlacement, and both get a check mark. counts[date][game] then exceeds the entrant count, contradicting docs/OPERATIONS.md ("counts[date][game] therefore equals the number of entrants").
| * How long a game's tally window stays open, measured from its first valid entry. | ||
| * When it closes, every unique entrant is given a unique random point value in 1..n. | ||
| */ | ||
| export const ENTRY_WINDOW_MS = (() => { |
There was a problem hiding this comment.
Tally window can outlive the noon window: a first :hadeda-boom: at 12:57 opens a window closing 13:02, but anyone posting at 13:00+ is clowned by the !inWindow guard first.
README and docs/OPERATIONS.md both describe the tally window as authoritative. Intended, or should the window be clamped to 12:59:59?
|
Good mechanic, but it needs a rebase and three fixes before I can merge.
|
zkrige
left a comment
zkrige
left a comment
There was a problem hiding this comment.
Requesting changes. The test coverage on this is genuinely thorough (tsc clean, 70/70 pass). Five issues, one behavioural.
src/features/boom/store.ts:462 — the first entrant of a day is exempt from the entry deadline. duePending skips any date where !hasAnyEntry(date), so a lone late entrant is still recorded, and scheduleSettle then computes a 0ms delay and settles on the next tick. A second player one second behind hits isResolved and gets :clown_face:. Identical circumstances, opposite outcomes, decided by macrotask ordering.
src/features/boom/store.ts:426 — resolveGame's deadline guard only covers the empty case (!entrants.length && nowMs < windowSettlesAtMs), so a game with entrants resolves at any nowMs, including mid-window. Every current caller is gated, so it is latent, but if nowMs lands just before windowSettlesAtMs (clock step-back, early libuv timer) the played games resolve and the unplayed ones do not, so announceDay can never fire and settleDay has already deleted the per-date timer.
src/features/boom/index.ts:282 — inWindow changed from inNoonWindow to inEntryWindow, but the !inWindow clown still runs before the !isWorkday branch at 290. A 💥 posted on a Saturday at 12:30 used to get "Boom isn't played today"; it now gets :clown_face: and no explanation. README.md and docs/TESTING.md in this diff still promise the notice.
.env.example — BOOM_ANNOUNCE_GRACE_MS has no callers left in src/ but is still in the example; the new BOOM_ENTRY_WINDOW_MS is documented in README.md:72 and docs/CONFIG.md:16 but never appears there. An operator copying the example gets one dead knob and misses the live one.
docs/OPERATIONS.md:121 — "Legacy podium helpers (getPlacements, placementsCount, PODIUM_WEIGHTS) remain only to score dates before random_scoring_from". PODIUM_WEIGHTS is used, but getPlacements, placementsCount, addPlacement, incrementCount and getCounts have zero callers in src/ (tests only) — pre-cutover scoring goes through the private computePodium. Either delete the methods or correct the doc.
Records the decision to select the 3-2-1 podium or the randomised point distribution once at registration, install exactly one orchestration, and stamp each date with the mode that scored it so a flag change never re-scores a day already played.
…uctor addEntry and addPlacement each name the mode they record, so the Store needs no mode argument and a historical date written through the legacy path can never be stamped random. Records which existing store tests the deleted cutover logic invalidates.
addEntry stamps random and addPlacement stamps legacy, so the write path names the mechanism and the Store needs no mode argument. Removes the constructor cutover the stamp supersedes, and restores getPodiumMessages, recordedDates and hasAnyPlacement for the podium orchestration.
The app_mention handler is identical between the podium and randomised orchestrations except for which catch-up it calls, so it takes that as an argument and serves both.
index.ts becomes the router that owns the Store. The announce path moves to random/announce.ts to keep every file under the size ceiling.
legacy/index.ts is master's orchestration with four adjustments: the export rename, the Store passed in, import depths, and the app_mention handler replaced by the shared one. rules.ts regains inNoonWindow, isWorkdayDate and noonWindowEndMs, which the podium path needs.
Copied verbatim from origin/master with one line added to each cfg literal. Editing anything else in these files would void the guarantee that rolling the flag back reproduces current behaviour.
Adds the boom section to ARCHITECTURE.md, documents the flag and both mode-specific knobs, and rewrites the OPERATIONS.md and TESTING.md claims about the deleted one-way cutover.
The project had neither a linter nor a coverage reporter, so nothing enforced method size, complexity or nesting. eslint.config.js encodes complexity 10, max-depth 2, max-lines 300, max-lines-per-function 25 and max-params 4. They are errors on src/env.ts and src/features/boom, warnings elsewhere, so the untouched chat/ debt surfaces without making the lint script permanently red. legacy/index.ts is exempt by design: it is a verbatim copy of the pre-flag orchestration whose equivalence is asserted by diff against master and by master's own suite, so refactoring it would void the rollback guarantee. @vitest/coverage-v8 is pinned to the installed vitest major. Also deletes a dead Date.now() timing pair in src/ai/openai.ts whose value was never logged.
Splits four oversized modules and flattens the functions that breached complexity, length, nesting or parameter count. No observable behaviour changes: all 101 tests pass unmodified, including master's own podium suites that assert legacy mode still matches the pre-flag build. store.ts 726 -> 239, split as Store extends PodiumStore extends StoreBase with the types and pure helpers in store-data.ts. Every method keeps its name and this-binding, so no caller changed. getPodiumMessages turned out to be a verbatim duplicate of earliestMessagesByUser plus a slice, and computePodiumFromMessages rebuilt the same earliest-per-user map; both collapse into earliestPerUser. rules.ts 298 -> 188, with the JSONC scanner in jsonc.ts and the holiday loader in holidays.ts. The rewritten stripJsonc was checked against the original over 200,000 random strings from its own alphabet, zero mismatches. random/index.ts 257 -> 20, split into handler.ts, settle.ts, medals.ts, io.ts and a rewritten announce.ts. The message handler was CC 28 in 52 lines, the worst function in the codebase; it is now classify -> catchUp -> dispatch with one guard per outcome. env.ts loadConfig was CC 21 in 65 lines; it is now a spread of four slice loaders, with the SLACK_BOT_TOKEN throw still ordered ahead of the BOOM_SCORING throw. leaderboard.ts extracts the Block Kit builders; rendered output is byte-identical. addPlacement and addEntry took 5 parameters; the trailing ts and channel_id collapse into one MessageRef object. That is the only signature change, and the only edit to the frozen legacy suites beyond selecting legacy mode. Operational reasoning removed from comments, recorded here: - Medals are marked done only once every reaction lands, because awards are flushed first; a crash in between would lose them permanently. The next catch-up retries until they stick, and an already_reacted error is success. - pendingMedals is snapshotted before settling, so it holds only medals orphaned by an earlier crash rather than ones this pass will apply. - A day can be settled yet unannounced, because awards are flushed before the post; without the final catch-up loop a transient postMessage failure would drop the results and the crown for good. - The crown is persisted only after Slack accepts the post, so a failure leaves no record of a crown nobody saw. - A day older than the retry window stays settled but silent: posting a three-week-old podium is worse than never posting it. - The announcing set stops a timer firing mid-await from double-posting. - One settle timer per date, because the window is fixed at 12:00-12:05 local, so every game for a date settles at the same instant and the deadline never moves. - Needed games nobody entered settle empty, so a day never stalls on an unplayed one. - The weekend notice is once per date, not once per poster. - Out-of-window messages are clowned before any store read, so non-workdays never mutate state. A message reaching the in-window clown branch arrived after its game settled, so the grace period has already passed. - addEntry records the entry and bumps the tally in one call, so concurrent deliveries from one user cannot both count. redelivery is a Slack retry; duplicate is a repeat post and is ignored entirely. - Daily results render display names, not <@id>, so listed users are not notified. - The sweep recovers windows abandoned by a restart even when the channel is quiet. app.client is absent in test harnesses, where catch-up is driven by incoming messages instead.
A DRY review found that both earlier splits were cutting files to satisfy a line-count ceiling rather than following a seam, which is the failure mode the ceiling exists to prevent. random/ goes from six files back to three. Nothing outside the directory imported anything but registerRandomBoom, so every other export existed only because the file had been cut up. io.ts was 9 lines used at three sites in settle.ts; medals.ts had one importer. createSettler, createBoom and createAnnouncer were one-line factories with one call site each, and the Announcer record duplicated Settler's store pointer, so db was reachable three ways. The Store inheritance chain goes back to one class. PodiumStore.computePodium was called from Store.scoreFor and Store touched this.data directly at fourteen sites, so the protected field encapsulated nothing. The stateless reads move to store-data.ts as free functions over StoreData. Also removed: getLatestCrown and the latestCrownMs monotonic-clock helper that existed only to serve it, both reachable only from tests; addPlacement's arrival-order branch, whose only production caller always passes a message; weekKeyForRange, a duplicate of weekKeyFor differing by a zone that cannot change the ISO week of a bare date; and env.ts's four Pick<Config> aliases, which annotated returns that loadConfig already enforces. byEarliestTs drops its numeric tier. Slack timestamps are fixed-width SSSSSSSSSS.uuuuuu, so lexicographic order is already chronological, and the float lost the last microsecond digit anyway. parseSlackTs goes with it. inAllowedChannel was defined identically three times. Two now share util/channels.ts; legacy/index.ts keeps its own copy because that file is a frozen equivalence proof. jsonc.ts was proposed for deletion on the grounds that neither committed holiday file contains a comment. That premise was wrong: both feature suites write fixtures with a // comment and a trailing comma, and one of them is frozen. It stays. Net -118 lines across src/features/boom, src/env.ts and src/util. All files under the ceiling, 100 tests green, zero lint errors.
2 participants
No description provided.