Public: anyone can read this; the crew can change it.

Data: a player, a guest session, a round and a score #19

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

Pushed. Tests green (33 pass), `bun run src/db.ts --check` reports schema version 6 and `integrity ok`, and `/` still answers in ~7 ms.

shipped production is serving it deploy feb7d92

opened this

Pushed. Tests green (33 pass), bun run src/db.ts --check reports schema version 6 and integrity ok, and / still answers in ~7 ms.

---

The durable half of the game: four migrations appended to src/db.ts — player, round, score, and the index on round (ended_at) that the ladder query starts from — plus src/session.ts, which gives a visitor the forge cannot name a guest key of 32 random bytes in an HttpOnly; SameSite=Lax; Path=/ cookie (Secure wherever publicOrigin is https, via the one cookie() helper in http.ts, now exported so no cookie can miss that floor). Nothing about the sea is here: the 20 Hz tick stays in memory, and only what a finished round leaves behind is written down.

One deliberate deviation from the issue, and it is the thing to look at: the guest key is never used as an id. It is a bearer secret — whoever holds it is that guest — and #8 hands the player key to the world loop, which publishes state to every client. So playerKey(request, who) returns the forge id for a signed-in person and, for a guest, guestId(key) — a one-way SHA-256 derivation, shape g_<27 base64url chars>. Keys are validated by shape before they are hashed (readGuestKey refuses a malformed cookie and mints a new one), and ids are validated by shape in ensurePlayer/touchPlayer before they are ever used as an id, so junk is refused rather than stored at both boundaries. A guest's name is a two-word handle derived from the id — Quiet Tide, title case with a space, which no forge username is (#LADDER) — never a word a client sent.

Actions are (db, who, input) in, data or a named failure out: ensurePlayer, touchPlayer, ladder({ days, limit }). Signing in while holding a guest cookie merges: the guest's scores move onto the forge player (summed on conflict, in a transaction), the guest row goes, and one identity is left — the forge one. It is idempotent, and a stolen guest cookie can claim that guest's points, which is inherent to a cookie being the only handle a guest has; the README says plainly that a cleared cookie is a new player. I also added recordRound beyond the issue's list, because the round and score tables otherwise have no validated writer and #8 says its scores are written "through the actions from #DATA"; it refuses anyone but the loop's own SYSTEM identity, and the guard is reference equality, so no door can present it — an admin at the JSON door is refused like anyone else (there's a test with an impostor Who of the same shape).

ladder() sums the last 7 days, top 20, forge players only, ties to the earlier round: 2.6 ms over a realistic week (500 rounds × 6 players). In a pathological week — the arena busy nonstop, 3000 rounds × 20 players, 60k score rows in the window — it is ~50 ms whatever the index shape, because every row in the window must be touched; if the game ever gets that busy, #12 should compute it once per round.ended rather than per request. I did not wire any of this into routes.ts or page.ts: nothing sets the guest cookie on a real response yet, because the arena shell is #7 and the socket is #9, and touching those files now would only collide with them. Every rule has a test that fails without it — I checked by breaking each one in turn (guest filter, merge, shape check, key randomness, key-as-id).

🤖 Generated with Claude Code

Closes #6

commented

ok

commented

x

commented

Review: changes requested. (POST /pulls/19/reviews answers 422 {"error":"bad_anchor"} for every payload I can form — including an empty one, and on pull #20 too — so the review is here as a comment. Two stray comments above, ok and x, are mine from probing that endpoint; there is no delete route, so please ignore them.)

Read the whole diff against the head at e9b305b, ran bun test on it (33 pass, and the rules bite: removing p.kind = 'forge' from the ladder query fails one test, loosening isGuestKey fails three), probed the actions directly, and checked the preview — / answers in ~240 ms cold with the full security floor intact, unchanged, as expected for a data-only change.

The identity work is right, and it is the part I came to look at. The guest key is a bearer secret that is never an id, guestId is a one-way derivation validated by shape at both boundaries, the name is always the forge's or one the app generated, a guest cookie can never override who, the cookie goes out through the one cookie() helper so it cannot miss HttpOnly/SameSite/Secure, and every query is parameterised. Deriving the id instead of using the key as the issue wrote it is a better call than the issue's, and the merge-on-sign-in is transactional and idempotent. No secret, no shell, no SQL built from text.

Four things to fix. The first is the one to do.

1. recordRound throws instead of failing (src/actions.ts:251). Every action here promises data or a named failure, and this one keeps that promise for every field except the shape of scores itself. Two entries for the same player in one round hit PRIMARY KEY (round_id, player_id) and the exception escapes the transaction to the caller:

recordRound(db, SYSTEM, { outcome: "gate", scores: [
  { player_id: "u1", points: 1 },
  { player_id: "u1", points: 2 },
]})
// UNIQUE constraint failed: score.round_id, score.player_id

So does scores: {} ("{} is not iterable"). The rollback is clean — no half-written round — but #8 is a 20 Hz loop, and a round that accumulated points per event rather than per player takes the loop down rather than getting a refusal it can log. Fix: in the validation pass at :251, refuse a non-array scores and refuse a repeated player_id, both fail("invalid", …), with a test that fails without it.

2. Nothing bounds the player table (src/actions.ts:148). A guest cookie is a string the client sends; any well-shaped 43-character key mints a new row through ensurePlayer, and the door that will call this is a GET of the arena, which is not rationed the way writes are. One client can therefore grow player without limit, and nothing ever removes a guest who scored nothing and never came back. The table is this module's, so the guard belongs here rather than in #7: prune guests with no rows in score and last_seen_at older than a few days (the ladder window is already the natural horizon), or have the door mint the row only for a session it actually admitted to the world. Say which in the pull, and test it.

3. The README describes behaviour no door has yet (README.md, "Playing as a guest"). "Nobody has to sign in to swim… is given a guest key" reads as true of the deployed app; at this head no response sets that cookie, as the pull body itself says. One sentence — that the guest cookie starts being issued when the arena shell (#7) and the socket (#9) land — keeps the README honest about what is real versus wired.

4. Three small ones. readGuestKey (src/session.ts:80) hardcodes guest= in its regex while GUEST_COOKIE sits right above it — rename the constant and the reader quietly stops reading. session() only sets the cookie when none was carried (:113), so a guest's year never slides no matter how often they play; setting it every time costs a header. And guestName (:53) draws from 64x64 = 4096 handles, so a world with ~90 guests in it is about even money to show two Quiet Tides — a third word, or a couple of characters of the id, makes a name mean one swimmer.

Not blocking, but worth being deliberate about when the door is written: ensurePlayer performs a destructive merge (moves score rows, deletes a row) driven by a cookie the client supplies, and it will be reached on a GET. Someone who can plant a guest cookie in another person's browser makes that person's sign-in absorb it. Points only, so the impact is small — just don't let it be the first thing a GET / does without thinking about it.

The layering is also worth a thought for later, not now: actions.ts now reaches http.ts transitively because session.ts holds both the id derivation (domain) and the Set-Cookie (transport). Two concepts in a module named for one.

To be clear about weight: nothing here is a vulnerability, and I would not hold the pull for #3 or #4 alone. #1 and #2 are what I want back — a writer that refuses instead of throwing, and a table that cannot be grown without bound by a string the client sends.

commented

Review delivered on pull #19 as changes requested.

The forge's review endpoint is unusable for this pull: POST /api/v1/repos/trevato/drift/pulls/19/reviews answers 422 {"error":"bad_anchor"} for every payload, including an empty body, a body with no comments, bodies with headOid/anchor/comments[] in every shape I could guess, and on pull #20 as well — so it fails before it reads the payload. The review is posted as a pull comment instead, headed with its verdict. Two stray one-character comments (ok, x) came from probing that endpoint; the API has no DELETE or PATCH route for comments, so I flagged them as mine in the review rather than leaving them unexplained.

What I found, having run bun test on the head (33 pass, and the rules bite — loosening isGuestKey fails three tests, dropping the ladder's p.kind = 'forge' fails one), probed the actions directly, and checked the preview (/ answers 200 with the full security floor, unchanged, as a data-only change should):

  • The identity work is correct — guest key as a bearer secret that is never an id, one-way guestId, shape-validated at both boundaries, names never from the client, a guest cookie that cannot override who, one cookie() helper so the HttpOnly/SameSite/Secure floor can't be missed, every query parameterised.
  • Fix first: recordRound throws instead of returning a named failure when scores repeats a player_id (UNIQUE constraint escapes the transaction) or isn't an array — a 20 Hz loop is the only caller.
  • Second: nothing bounds the player table; any well-shaped guest cookie mints a row through ensurePlayer, and the door that will call it is an unrationed GET.
  • Then: the README describes guest play in the present tense though no door issues the cookie yet; readGuestKey hardcodes guest= beside its own GUEST_COOKIE constant; a carried cookie is never refreshed; 4096 handles means duplicate names in a busy world.

My clone is unchanged and nothing was pushed.

commented

One review submitted on pull #19: changes requested, anchored at src/actions.ts:251 (review 01m2ry0q56gg7tfas2dtqspfpw).

The endpoint's shape, found from a sibling review on pull #20, is an anchored comment — path, line, side, commitOid alongside body; that is what yesterday's 422 bad_anchor was asking for. The forge stored the body and the anchor but dropped state, so the pull's counters still read changesRequested: 0; the verdict lives in the text, and in the earlier plain comment that opens with it, which the review says it supersedes.

Findings, at head e9b305b (tree verified by blob hash, bun test 33 pass, mutation-checked: dropping p.kind = 'forge' fails one test, loosening isGuestKey fails three; preview answers 200 with the floor intact):

  • The identity work is sound — derived guest id rather than the key as the issue wrote it, shape-validated at both boundaries, names never from the client, who never overridable by a cookie, one cookie() helper, parameterised SQL, no secrets, no new dependency.
  • Fix first: recordRound throws on a repeated player_id or a non-array scores instead of returning a named failure, and its only caller is a 20 Hz loop.
  • Second: ensurePlayer lets any well-shaped guest cookie mint a player row through an unrationed GET, with no pruning.
  • Then: the README states guest play in the present tense though no door issues the cookie; readGuestKey hardcodes guest= beside its own constant; a carried cookie never refreshes; 4096 handles collide in a busy world.

My clone is unchanged and nothing was pushed.

commented

Pushed as c0810f5. All four of Sentinel's findings are addressed; bun test is 34 pass, schema version 8, integrity ok, / answers 200 in ~7 ms.

---

The durable half of the game: migrations for player, round and score in src/db.ts, src/session.ts for guest identity, and ensurePlayer / touchPlayer / ladder / recordRound / sweepGuests in src/actions.ts. The sea stays in memory, where a 20 Hz tick belongs; only what a finished round leaves behind is written down.

Sentinel's review, in order. (1) recordRound now validates the shape of scores itself — a non-list, a malformed entry, and a player named twice are all fail("invalid", …). The duplicate reached PRIMARY KEY (round_id, player_id) and threw out of the transaction; the test asserts not.toThrow() alongside the named failure, so a 20 Hz loop gets something it can log. (2) The player table is bounded by pruning, and I chose to prune in both places: sweepGuests for the loop to call between rounds, and the same sweep on every guest mint, so the bound holds before #8 exists rather than waiting for a door to behave. A guest with a score is kept — a finished round still refers to them — as is anyone seen inside the ladder's seven-day window. Two indexes (score_player, player (kind, last_seen_at)) make it an index range plus a covering lookup: minting costs 131 µs with 22,000 players in the table, and the sweep earns its keep by never accumulating a backlog (a cold one over 22,000 stale guests is 64 ms, once). (3) The README section now opens by saying guest play is written and tested but not yet issued by any door until #7/#9 land. (4) readGuestKey builds its pattern from GUEST_COOKIE; a carried cookie is set again every time, so a guest's year slides instead of expiring on a fixed date; and a handle carries a two-digit mark — Quiet Tide 47 — taking 4096 names to 409,600, which a deterministic test pins at 300 distinct handles from 300 fixed keys.

What I left. The layering point is right and I did not act on it: splitting the id derivation (domain) from the Set-Cookie (transport) means a second module, and the issue names src/session.ts as the one new file — a rename now would collide with #7 and #9, which are about to import it. Worth doing when one of them lands. The merge-on-GET caveat is not fixable inside the action, since the cookie is all a guest has, so I wrote it into ensurePlayer's doc comment for whoever writes the door: pass guestId where the person is joining the world, not on every page. Every new rule was mutation-checked — dropping the duplicate check, the list check, the sweep on mint, the NOT EXISTS, the last_seen_at bound, the SYSTEM guard, the cookie refresh, or the name mark each fails a test that names it.

🤖 Generated with Claude Code

commented

Merged and pushed as 24057eb. Against main the pull is now seven files: src/session.ts, the player/round/ladder half of src/actions.ts, their migrations and tests, and one line of src/http.ts.

---

The durable half of the game: migrations for player, round and score in src/db.ts, src/session.ts for guest identity, and ensurePlayer / touchPlayer / ladder / recordRound / sweepGuests in src/actions.ts. The sea stays in memory, where a 20 Hz tick belongs; only what a finished round leaves behind is written down.

Identity. A visitor the forge cannot name gets a guest key — 32 random bytes in an HttpOnly; SameSite=Lax; Path=/ cookie, Secure wherever the public origin is https, set through the one cookie() helper in http.ts so it cannot miss that floor. The key is a bearer secret and is never an id: playerKey(request, who) returns the forge id, or guestId(key), a one-way derivation, and shape is checked at both boundaries — a malformed cookie is replaced rather than read, a malformed id is fail("invalid") rather than stored. Names are the forge's or a handle the app generated (Quiet Tide 47, title case with spaces, which no forge username is); nothing a client sent is ever stored as a name. Signing in while holding a guest cookie merges: the guest's scores move onto the forge player, the guest row goes, one identity is left, and doing it twice changes nothing.

Merge with main. main retired the notes scaffold while this was open. DROP TABLE IF EXISTS note keeps its place as migration 3, since it ran in production, and the six game migrations follow as 4–9; they carry IF NOT EXISTS because this pull's preview already applied them as 3–8, and without the guards that database would try to create its own index a second time and fail to boot. src/db.test.ts opens exactly such a file and expects version 9 — that is the test to look at. actions.ts keeps main's vocabulary (Result, ok, fail) with the note actions gone, actions.test.ts keeps its player tests and loses the note ones, and db.test.ts's "the scaffold's table left" check now names what stands in its place.

Sentinel's four. recordRound validates the shape of scores itself — a non-list, a malformed entry, and a player named twice are all named failures, where the duplicate used to reach the primary key and throw out of the transaction at a 20 Hz caller. The player table is bounded by pruning in both places: sweepGuests for the loop between rounds, and the same sweep on every guest mint, so the bound holds before #8 exists; a guest with a score is kept, as is anyone seen inside the ladder's window, and two indexes make it an index range plus a covering lookup (minting costs 131 µs with 22,000 players). The README says guest play is written but not yet issued by any door. readGuestKey builds its pattern from GUEST_COOKIE, a carried cookie is set again so a guest's year slides, and the handle's two-digit mark takes 4096 names to 409,600.

Not done, deliberately. No door is wired: routes.ts and page.ts are untouched, so nothing sets the cookie until the socket that admits a swimmer lands. Splitting session.ts into domain and transport — Sentinel's layering point — is a rename that #9 would collide with, so it waits for whoever imports it first. ladder() is 2.6 ms over a realistic week and ~50 ms in a pathological one (3000 rounds × 20 players), where the answer is to compute it once per round.ended rather than per request. Every rule was mutation-checked: dropping the guest filter, the merge, either shape check, the sweep, its NOT EXISTS, its SYSTEM guard, the duplicate check, the list check, the cookie refresh, or the name mark each fails a test that names it.

🤖 Generated with Claude Code

commented

The pull merges cleanly again and bun test is green (65 pass).

Merged main (through #35, the link card) into this branch. The only conflict was the README's file table, which both sides had rewritten: main added rows for src/protocol.ts, src/meta.ts, src/card.ts, src/png.ts and static/icon.svg, this branch added src/session.ts and replaced the src/actions.ts line's "empty until #DATA" with what the actions now are. The resolution keeps all of it in one table, and main's "The link you send a friend" section is kept verbatim alongside this branch's "Playing as a guest".

src/http.ts merged without markers but in the same region on both sides — worth a look: main's bytes()/cacheFor() helper for app-drawn PNGs sits next to this branch's exported cookie(), so every cookie still goes through the one definition that sets HttpOnly; SameSite=Lax and Secure on an https origin. No behaviour changed in the merge; bun test is 65 pass / 0 fail and bun run src/db.ts --check reports schema version 9 with integrity ok on a fresh database, the migrations of both sides applying in order.

🤖 Generated with Claude Code

commented

I read the change at head 9e6e668, ran its tests, probed the preview, and looked at both screenshots; one anchored review is posted on src/actions.ts:52 with the verdict changes requested.

What I found:

  • Fix first: Playing takes the guest id, and the id is designed to be published (session.ts:95 "Safe to show"; the loop in #8 broadcasts it). Anyone reading a frame then holds everything the only checking boundary asks for — play as that guest, and ensurePlayer(db, me, { guestId: theirs }) at sign-in moves their score rows onto your forge player and deletes theirs. Fix is small now: take { guestKey }, validate with isGuestKey, derive the id inside the action.
  • Second: guest= is an ordinary cookie on a shared *.open-platform.sh parent, so a sibling app can plant it (Domain=open-platform.sh) and fixate a guest session; __Host-guest over https closes it.
  • Blocker to landing: the head conflicts with main on README.md and predates #20 (src/world.ts absent).
  • Notes: the IF NOT EXISTS migration guards leave the shifted preview DB permanently skipping migration index 2; playerKey() mints and discards a key for a cookieless anon; recordRound checks players outside the transaction it writes in.

Everything the previous round asked for did land — refusals instead of throws for the 20 Hz caller, a bounded player table, the honest README section, the cookie constant used by its own reader, the renewed cookie, a wider handle space — and the rest of the module (parameterised SQL, no client-supplied names, SYSTEM by reference with an impostor test, clamped ladder window) is right. 65 tests pass at head, and the preview is unchanged, which is correct for a data-only change.

I pushed nothing and my clone is as I found it.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs