Reviewing changes

What a reviewer of this repo — human, CodeRabbit, or any other AI agent — must verify before a PR merges, and just as importantly, what not to flag. The machine-enforced subset lives in .coderabbit.yaml (path instructions + pre-merge checks); this page is the authority that config encodes, written for a reader. When a new invariant earns a place here, it lands in both files in the same PR — a guideline the bot doesn’t enforce, or a bot rule no document explains, is how the two drift apart.

The house review method, in one paragraph: verify claims against the code, not the diff description — cite file:line. A finding must name a concrete failure scenario (“inputs/state → wrong output”), not a vibe. Prefer the smallest fix that closes the failure. A reviewer finding you disagree with is answered on the thread with the reason, never ignored — a reasoned decline on the record is a valid outcome (see CONTRIBUTING and AGENTS.md’s CodeRabbit gate).

Section 1. The invariants a review protects

These are the properties this repo has been burned by, ordered roughly by blast radius. Each one names where it is enforced and what a violation looks like in a diff.

1. The contestant secrecy boundary. A contestant-reachable path must never obtain a quiz answer key (ctf:quiz:key), a classic flag (ctf:classic:flag, ctf:classic:flagnorm), or unpurchased hint text (ctf:classic:hints). The boundary is pinned independently at four layers — store, route, view-model, markup — and the tests inject a fake secret and assert it never renders. In a diff, the red flags are: a store function on the contestant path issuing a command against a secret key; a route serializing an admin-shaped object; a type change that lets AdminQuestion/ AdminChallenge flow where Question/Challenge is expected. Anything that weakens one layer “because another layer still catches it” is a finding — the four layers are deliberate redundancy.

2. Lua is the scoring authority. The grading scripts (SUBMIT_SCRIPT in classic-store.ts, GRADE_SCRIPT in quiz-store.ts, AWARD_SCRIPT in ai-store.ts) decide points; the JS pre-checks are advisory early-outs. Review implications: moving an already-solved guard, attempt cap, cooldown check, or price read out of the script into JS is a correctness regression, not a refactor. A change to script text needs tests that pin the semantics (the HEXISTS … == 1 polarity, the >= cap comparison, which key each counter is keyed by), not just command ordering — those live in src/lib/__tests__/*.lua.upstash.test.ts, which execute the real scripts against a real Redis in CI; a script change with no change there is the finding. And never case-fold inside Lua — string.lower is ASCII-only and diverges from the JS normalizer, producing challenges nobody can solve. Normalization is JS-only, applied identically on authoring and submission.

3. Fail direction is a per-gate decision, not a default. The direction each gate takes on a storage error is chosen, documented, and tested — a diff that flips one silently is a bug even when it looks like hardening:

Every row carries its anchors so the claim is checkable against the code — a row whose direction cannot be traced to an implementation and a test is a row to distrust. Issue #232 tracked the rows below until each gained a test anchor; the boundary-instant row’s shared corpus (test/fixtures/window-corpus.json) is what closed it.

Gate Direction on error Why Implemented · tested
Freeze / scoring-window reads (app, scorer, sync) OPEN a Redis blip must not drop live submissions (ADR 32) classic-store.ts + quiz-store.ts gate reads · classic-store.grade.test.ts / quiz-store.grade.test.ts fail-OPEN pins; sync/src/redis.js and scorer/src/store.js isPaused both log the failure and fail open · redis.test.js / store.test.js
Grading lookups (does this challenge/question exist, what is its key) CLOSED don’t grade what you can’t verify classic-store.ts submitFlag / quiz-store.ts answerQuestion lookup paths · error paths exercised in the grade test files
Admin allowlist reads (requireAdmin) CLOSED an unreachable datastore denies, never grants admin-auth.ts (the fail-closed branch is commented) · admin-auth.test.ts
Jeopardy visibility (is this challenge hidden, ADR 68) CLOSED a challenge that cannot be proven visible is treated as not existing: a hidden one must never be graded, sold a hint, or have its bought hint text returned classic-store.ts submitFlag and hint-store.ts revealHint (a failed records read is unavailable / a failed reveal, before the script runs); SUBMIT_SCRIPT / REVEAL_SCRIPT decide from the record they read; /api/hints withholds Jeopardy text on a failed read · classic-store.grade.test.ts (“fails closed when the records cannot be read”), hint-store.test.ts (“refuses (closed) when the records cannot be read…”), app/api/hints/__tests__/route.test.ts (“returns no Jeopardy text when the challenges cannot be read”)
Hint purchase (spend) CLOSED never charge on uncertainty hint-store.ts revealHint (comments its direction) · hint-store.test.ts (“fails CLOSED (rejects, never charges) when the settings read errors”)
Team registration window read OPEN a Redis blip must not itself block registration; the join/create Lua script still validates every real invariant atomically team-store.ts isRegistrationClosed (comments its direction, logs and fails open on both a thrown transport error and a resolved per-command error) · team-store.test.ts (“fails OPEN … and logs when … transport failure” / “… per-command error”)
Scheduled-window boundary instant (the exact </<= at start and end) N/A — cross-reader agreement a flip surviving one reader’s own tests must still fail via the shared corpus schedule-window.ts / scorer/src/store.js / sync/src/redis.js outsideWindow · test/fixtures/window-corpus.json run verbatim by schedule-window.test.ts, store.test.js, redis.test.js
Freeze at an instant (a Secure Development score judged at its comment’s time, ADR 67) N/A — cross-reader agreement; the scorer’s policy read fails OPEN a score posted during a freeze must stay refused after it lifts, and one posted before it must count when read during it; the read is open for the freeze read’s reason pausedAt in the same three readers · test/fixtures/pause-interval-corpus.json run verbatim by the same three suites; scorer/src/store.js readScoringPolicy logs and returns no policy on error · store.test.js; the verdicts · serve.test.js and acceptance-scorer.sh against a real Redis
Scoring start present? (no/unparseable scoringStartsAt = not launched, #464) CLOSED (value) an event nobody launched scores nothing; distinct from the freeze READ, which still fails open outsideScoringWindow in the same three readers · test/fixtures/scoring-window-corpus.json run verbatim by the same three suites
Launch lock (module pages and APIs before launch, #464) CLOSED a secrecy boundary over challenge text: a failed settings read is “not launched” for a non-admin, a failed admin check is “not admin”; distinct from the freeze READ, which still fails open launch.ts getLaunchAccess (comments its direction) · launch.test.ts (“fails CLOSED for a non-admin when the settings read throws” / “treats an admin check that throws as not-admin”); launch-guard-coverage.test.ts fails if a module page or route lacks the guard
sync poll-state read (ctf:sync:state, and the legacy state.json seed during migration, ADR 64) HOLD an empty cursor would re-ingest every score comment, undoing per-contestant resets and zeroing /admin’s counters; holding loses nothing (the comments stay on GitHub) sync/src/index.js loadDurableState (retries each poll interval; a file that exists but cannot be read, or cannot be moved aside once copied into Redis, holds too; only ENOENT counts as “no file”). Its repair lines name the field and the value’s kind, never the value, which came from Redis or a hand-edited file · sync/test/state-redis.test.js (“an unreadable Redis at startup holds the poller”, “a per-command error reply … is a failed read”, “a legacy state file that exists but cannot be read holds the poller”)
ctf-setup.sh check_step CLOSED a gh error is never “already satisfied” setup/ctf-setup.sh check_step · setup/test/ctf_setup.bats (“fails closed when gh api errors”)
Audit-log writes best-effort an audit failure is logged, never fails the request that already committed writeAudit in each api/admin/* route · route tests assert success without a completed audit write

4. Tests must be able to fail. The recurring failure mode in this repo is the vacuous pass: an assertion satisfiable whether or not the code under test works. Concrete shapes to reject in review: a leak test that scans for tokens never seeded through any source the code reads; a regex assertion satisfied by static page copy; a bats [[ ]] or ! … | grep mid-test (non-gating — the decisive check must be the test’s last statement, in a form that exits nonzero); an exploit test that “passes” because the target was never up (the vacuous-zero gate exists for exactly this). When a test accompanies a fix, ask: what single-token mutation of the fix would this test miss?

5. The score-comment trust chain. The <!-- ctf-score: {json} --> marker is trust-authoritative: sync ingests it as real points. It must only ever come from the judge’s own output (CTF_OUT_DIR, outside the PR checkout), posted only when the scorer step succeeded. Never post checkout-derived content as the bot; never gate the final-comment step on if: always() without an outcome check. Either reopens the score-forge.

6. Three-reader lockstep. effectivePaused/outsideWindow, and the scoring window’s outsideScoringWindow (a start is required: no start means not launched, #464), are implemented independently in apps/web/src/lib/schedule-window.ts, scorer/src/store.js, and sync/src/redis.js (registration windows in team-store.ts). They read the same ctf:admin:settings fields and must agree — a diff touching one reader touches all three or explains why not. Don’t DRY them into a shared package (ADR 32: three separately-deployed services); the right agreement mechanism is a shared test corpus.

7. Allowlist, never sweep. Anything that serializes state for export — the event archive bundle, settings pickers, log lines — enumerates known fields, so a future key is excluded by default rather than leaked by default. In a diff: a SCAN ctf:*, an object spread where a field list should be, or a “drop these fields” denylist are all findings. Corollary for CodeRabbit’s own config: every path_filters entry stays negated — a single bare pattern flips the whole filter into an allowlist and silently skips review of everything else (this happened; see the comment in .coderabbit.yaml).

8. Destructive paths validate first, then destroy. Replace-all import, master reset, force-disband: the guard (live check, confirmation, payload validation) runs before anything is wiped, so a bad input fails with nothing destroyed. A diff that moves validation after a destructive step — or adds a destructive step before the existing guard — is a P1 finding. UI side: a destructive control needs an explicit confirmation gating the network call, and the test proves the call cannot fire without it.

9. Presence is not discoverability. A shipped feature needs a visible entry point, and its test asserts the link/nav/button exists — not merely that the route responds. This repo has shipped invisible-but-working features past green tests three times.

10. The app reads no build-time config. Config v2 (#386) deleted event.yaml and its EVENT_CONFIG_B64 bake: the app now reads its two bootstrap identities, GITHUB_ORG and ADMIN_LOGINS, from the process environment at runtime (bootstrap-env.ts), and everything else — the event’s name and branding, which Secure Development targets run — is a runtime /admin setting in the ctf:admin:settings hash. A review should verify: no process.env read reaches a client component (a client bundle must never see who the admins are, and process.env there is empty or misleading anyway); an empty ADMIN_LOGINS fails CLOSED in admin-auth.ts/isAdminLogin — nobody is an admin, not “no allowlist configured, let runtime grants decide”; an empty GITHUB_ORG degrades gracefully in the app (apps.ts renders a bare repo name instead of a fork link) but sync’s loadConfig(env) throws on it instead, because for sync a missing org is a genuine misconfiguration, not “nothing to poll” — the two components deliberately disagree; runtime /admin settings are read once per request through the cache()-memoized snapshot (getAdminSettingsSnapshot in enabled-modules.ts, imported by resolved-modules.ts, enabled-apps.ts and site.ts), not a second independent read that could disagree with the first on an unlucky Redis blip; and / is never statically prerendered (CI’s .next/server/app/index.html must-not-exist check, ci.yml’s app job) — a prerendered / would bake this request-time read into static HTML that never updates again without a rebuild. The last three of those are machine-enforced as well: .coderabbit.yaml carries path instructions for bootstrap-env.ts and the snapshot readers, and for apps/web/src/**/*.tsx (the client-bundle read and the prerendered /).

11. The public surface is a named list, not a shape. Exactly eight routes under /api answer without a session or a verified launch token, and each is on the list for its own stated reason — a ninth does not inherit an exemption by resembling one that has it; it needs its own case. Seven are read-only, policy or public-by-design content out and never facts in, nothing secret in the response: GET /api/public/scoring, GET /api/ai/launch-key, GET /api/board/items, GET /api/sponsors/logo/[id], GET /api/attachments/[id], GET /api/event/logo and GET /api/event/icon. One is a POST that exists before identity: POST /api/stats/visit (the approximate, no-PII per-country reach counter, always 204, whose own header comment documents that it is not a security boundary). The pre-event password check (POST /api/gate) that used to be the second is gone (#464); a route that brings back a pre-identity credential check is a new case, not a restoration. /api/auth/* is better-auth’s and is outside this list. Anything else under /api that answers without getSession, requireAdmin or a verified launch token is the finding. board/items (from the #207 redesign) serves the public leaderboard’s row expansion, and like the board it is public only once the event is launched — before that it answers 403 not-launched to anyone but an admin. Who solved what is already on the board as counts, the items are built only from the contestant-safe listers (ids, labels, banked points — never a flag, hint, or key), and the login list is capped at a team roster’s size so it reads like the board, not like a scrape. launch-key is the sharpest test of the “nothing secret” half: it exists to publish the launch token’s public key, kid, and algorithm so an external integrator can verify a token without holding a credential, and the finding to watch for is the private half (or any per-challenge signing key) drifting into that same payload — the ai contract test’s import ban is what makes that a compile-time impossibility rather than a promise. sponsors/logo/[id] (issue #405, ADR 57) is the newest, and its own reason is a privacy one rather than a launch-time one: it exists so a contestant’s browser NEVER fetches a sponsor’s logo from the sponsor’s own CDN, which would leak that contestant’s IP to the sponsor on every page load. It validates id against SPONSOR_ID_RE before any Redis command, serves only PNG/WebP bytes that already passed the store’s magic-byte sniff at upload (never SVG, which could execute script if opened directly), and carries nothing an organizer did not deliberately upload to be shown publicly — no flag, key, or contestant data has anywhere to ride in this payload. The finding to watch for here is the same shape as launch-key’s: anything beyond those bytes and their content-type metadata leaking into the response, or the id check moving after a Redis read. attachments/[id] (issue #186, ADR 61) serves a classic challenge’s uploaded file to exactly the viewers who can read that challenge’s page — and a signed-out visitor can read a launched challenge, which is why it needs no session. It answers 403 not-launched before launch (admins pass as a preview) before it looks the attachment up, then asks classicVisibility, the same answer the page gives, so a story lock or a disabled module applies to downloads too; a hidden, unknown or malformed id is one bodiless 404. The bytes are always served as a download (application/octet-stream, Content-Disposition: attachment, nosniff, private, no-store), never rendered from the box’s origin. event/logo and event/icon (issue #529, ADR 65) serve the event’s own hero logo and favicon, and are public before launch on purpose: the landing page and every tab show them. They are the sponsor logo’s shape with no id at all — a fixed slot per route, so there is nothing to validate before the Redis read — and serve only PNG/JPEG/WebP bytes an organizer uploaded to be shown, already sniffed by magic number at upload (never SVG). The finding to watch for is the same as for the sponsor logo: anything beyond those bytes and their content-type metadata in the response.

12. requireAdmin gates per-contestant data by default; self-service reads are the one carve-out, and it’s narrow. A route returning points, attempts, hint spend, team membership, answer keys, flags, or metrics needs requireAdmin unless it reads only the row belonging to the caller who is asking — and “the caller” has to be established cryptographically, not trusted from the request: a verified launch token’s sub, or a session login, never a client-supplied id, header, or query parameter standing in for one. GET /api/ai/state is the shape that qualifies — it derives its subject from a signature-verified token (verifyLaunchToken(...).claims.sub) before it reads anything. A route that reads “my own row” by trusting an unverified field isn’t self-service; it’s the admin gate with the identity check removed, and it still needs requireAdmin or a real bound identity to pass review.

13. A catch logs a label, never the caught object. The grading paths call upstashEval with the submitted flag or answer — and, for classic, the stored flag’s comparison form — as ARGV, so a driver that decorates its rejection with the request it failed on (command, body, cause) turns one console.error("…", err) into the event’s flags in a log the organizer may already have shipped somewhere. No error shape reachable today carries them; the invariant is that nothing prevents a future one from doing so, and logs cannot be un-shipped. Every store hands the logger errorLabel(err) from apps/web/src/lib/error-label.ts (the admin routes import the same function as adminErrorLabel; error-label.test.ts fails on any second copy of its body in apps/web/src) — name and message, capped, no stack, no own properties, "non-Error throw" for anything that is not an Error — and nothing else. That holds for every server-side catch in apps/web/src, not only the stores: the leaderboard overlays, challenges.ts, activity-log.ts, the launch lock and the API routes hand over the same label. In a diff, the red flag is a raw err (or a .reason, String(err), new Error(String(err)), JSON.stringify(err)) as a console.* argument. The tests reject with an Error carrying a planted flag in command/cause and assert two things: the flag appears in no logged argument, AND no logged argument is instanceof Error — the second is load-bearing, since JSON.stringify(new Error("x")) is "{}" and the first check alone passes against the unfixed code (#241, #244); apps/web/src/lib/__tests__/log-redaction.ts holds that assertion for every suite to share. The same rule governs the import validators’ messages (*-io.ts), which go back to the admin client verbatim: an error names the indexed position and the rule (questions[3].correct[1], “must be one of this question’s choice ids”), never the submitted value or a key name (#500).

Section 2. What not to flag

Deliberate decisions, each with the ADR that settled it. Re-raising one without new evidence is noise; a reviewer who disagrees argues against the ADR, not the code.

Section 3. Where the enforcement lives