fix(client): round durationMs to integer ms before leaderboard submit (#86) #89

Merged

bosun merged 1 commit from i/86-durationms-int into main 2026-06-22 13:30:47 +02:00

Conversation 1 Commits 1 Files changed 2 +42 -5

shipwright commented 2026-06-22 13:24:31 +02:00

Owner

Copy link

What + why

Every solo leaderboard submit has been failing in prod (round-11) with a "BAD JSON" banner. Engineer's #86 probe (live-curl ground-truth) pinned it: the client POSTs durationMs as a sub-ms float — soloElapsedMs = t - soloStartMs, where t is a requestAnimationFrame DOMHighResTimeStamp — and the server's int64 json.Decode rejects a fractional number outright. The decode error is surfaced verbatim to the banner via submitLeaderboard's !res.ok path.

Not a #80 regression. #80's synchronous inert-on-commit was functioning as designed; the submit was reverting to 'entering' on a genuine server error each time, which is visually identical to the original duplicate-submit bug but a different root cause. (My earlier code-reading hypothesis — "the BAD JSON string is the server's error body, not a client parse-throw" — held; Engineer confirmed + pinned the exact field.)

Fix (one line) — and the decision tree

Math.round(t - soloStartMs) at the solo elapsed-stamp.

• Why client-side, not server-side: durationMs is milliseconds — conventionally an integer; sub-ms precision is spurious. The client is the drift. The score screen already Math.floors to seconds for display, so rounding loses nothing observable.
• Why not loosen the server decode (Engineer offered a tolerant float→int64 leg, Bosun declined, I agree): strict-decode is the drift-detector that caught this. A tolerant decode would silently accept future client drift instead of failing loud. Same shape as the #80 content-dedup decline — substrate-strictness as a discipline-pin. If a future client legitimately needs sub-ms timing, that's a deliberate contract change, not a silent tolerance.

Harness (#81), AC#4 — the gap that let this masquerade as #80

The suite's initials-submitting node uses holdSubmit (routes the endpoint to hang → stable 'submitting'); it never exercised the server-resolves-with-error path — which is exactly the path the operator hit, and the reason the green 14/14 read as "#80 fixed" while prod was broken. New test (#86: server-error resolves → revert to entering) routes the submit to a 400 and asserts the client reverts to 'entering' (retry-enabled, initials preserved) and a Re-Enter genuinely re-submits — the operator's loop. Mutation-proven against submitLeaderboard's revert (flip it to stay 'submitting' → the test reds).

Also folded in a small robustness fix: the TABLE loop now polls-until-true instead of reading after a fixed 40ms. A cold CI run (fresh container, first navigation) jittered the read window and flaked the pre-existing commit → submitting row. CI runs every suite cold, so a fixed delay is a latent red; polling is deterministic. Folded in here (not deferred) because it's this PR's own CI gate that the flake threatens.

Verification

• npx tsc --noEmit clean.
• Full nav suite 15/15 green, run 3× (incl. the two added/changed) — no flake after the poll hardening.
• Mutation-proof on the error-resolution row (above).

Honest verification ceiling

The Math.round line itself is not directly unit-covered by the nav harness: the elapsed-stamp (main.ts solo block) only runs on a real playing → gameover transition (soloStartMs set during play), which the harness deliberately bypasses via the enterInitials seam. Asserting Math.round's output would mean either testing JS's Math.round (pointless) or driving a real top-out game (the gameplay-gating the #81 harness was explicitly designed to avoid). So per the no-placebo-test discipline I added no synthetic test for the rounding itself — Engineer's live-curl is the empirical proof (durationMs:1000 → decode passes; durationMs:1000.5 → "bad json"), and the harness contribution is the error-resolution guard that closes the actual coverage gap. The real-flow confirmation is the on-device / live ceiling: needs the round-12 redeploy + an operator solo submit to confirm scores save end-to-end.

What this PR does NOT do

• Does not touch the server decode (intentional — see decision tree).
• Does not add a touch back-affordance or any #85 work (separate PR, separate substrate-of-record).

Closes #86

🤖 Generated with Claude Code

## What + why Every solo leaderboard submit has been failing in prod (round-11) with a **"BAD JSON"** banner. Engineer's #86 probe (live-curl ground-truth) pinned it: the client POSTs `durationMs` as a **sub-ms float** — `soloElapsedMs = t - soloStartMs`, where `t` is a `requestAnimationFrame` `DOMHighResTimeStamp` — and the server's `int64` `json.Decode` rejects a fractional number outright. The decode error is surfaced verbatim to the banner via `submitLeaderboard`'s `!res.ok` path. **Not a #80 regression.** #80's synchronous inert-on-commit was functioning as designed; the submit was reverting to `'entering'` on a genuine server error each time, which is *visually identical* to the original duplicate-submit bug but a different root cause. (My earlier code-reading hypothesis — "the BAD JSON string is the server's error body, not a client parse-throw" — held; Engineer confirmed + pinned the exact field.) ## Fix (one line) — and the decision tree `Math.round(t - soloStartMs)` at the solo elapsed-stamp. - **Why client-side, not server-side:** `durationMs` is milliseconds — conventionally an integer; sub-ms precision is spurious. The client is the drift. The score screen already `Math.floor`s to seconds for display, so rounding loses nothing observable. - **Why *not* loosen the server decode** (Engineer offered a tolerant float→int64 leg, Bosun declined, I agree): strict-decode is the **drift-detector that caught this**. A tolerant decode would silently accept future client drift instead of failing loud. Same shape as the #80 content-dedup decline — substrate-strictness as a discipline-pin. If a future client legitimately needs sub-ms timing, that's a deliberate contract change, not a silent tolerance. ## Harness (#81), AC#4 — the gap that let this masquerade as #80 The suite's `initials-submitting` node uses `holdSubmit` (routes the endpoint to **hang** → stable `'submitting'`); it **never exercised the server-resolves-with-error path** — which is *exactly* the path the operator hit, and the reason the green 14/14 read as "#80 fixed" while prod was broken. New test (`#86: server-error resolves → revert to entering`) routes the submit to a `400` and asserts the client reverts to `'entering'` (retry-enabled, initials **preserved**) and a Re-Enter genuinely re-submits — the operator's loop. **Mutation-proven** against `submitLeaderboard`'s revert (flip it to stay `'submitting'` → the test reds). Also folded in a small robustness fix: the TABLE loop now **polls-until-true** instead of reading after a fixed 40ms. A cold CI run (fresh container, first navigation) jittered the read window and flaked the pre-existing `commit → submitting` row. CI runs every suite cold, so a fixed delay is a latent red; polling is deterministic. Folded in here (not deferred) because it's *this PR's own CI gate* that the flake threatens. ## Verification - `npx tsc --noEmit` clean. - Full nav suite **15/15 green**, run 3× (incl. the two added/changed) — no flake after the poll hardening. - Mutation-proof on the error-resolution row (above). ## Honest verification ceiling The `Math.round` line itself is **not** directly unit-covered by the nav harness: the elapsed-stamp (`main.ts` solo block) only runs on a real `playing → gameover` transition (`soloStartMs` set during play), which the harness deliberately bypasses via the `enterInitials` seam. Asserting `Math.round`'s output would mean either testing JS's `Math.round` (pointless) or driving a real top-out game (the gameplay-gating the #81 harness was explicitly designed to avoid). So per the no-placebo-test discipline I added **no** synthetic test for the rounding itself — **Engineer's live-curl is the empirical proof** (`durationMs:1000` → decode passes; `durationMs:1000.5` → "bad json"), and the harness contribution is the error-resolution guard that closes the actual coverage gap. The real-flow confirmation is the **on-device / live ceiling**: needs the round-12 redeploy + an operator solo submit to confirm scores save end-to-end. ## What this PR does NOT do - Does not touch the server decode (intentional — see decision tree). - Does not add a touch back-affordance or any #85 work (separate PR, separate substrate-of-record). Closes #86 🤖 Generated with [Claude Code](https://claude.com/claude-code)

shipwright added 1 commit 2026-06-22 13:24:31 +02:00

fix(client): round durationMs to integer ms before leaderboard submit (#86) ... ba51786eee

Every solo leaderboard submit was failing in prod with a "BAD JSON"
banner. Root cause (Engineer #86 probe, live-curl ground-truth): the
client POSTed durationMs as a sub-ms FLOAT — soloElapsedMs = t -
soloStartMs where `t` is a requestAnimationFrame DOMHighResTimeStamp —
and the server's int64 json.Decode rejects a fractional number outright
(decode error surfaced verbatim to the banner via submitLeaderboard's
!res.ok path). Not a #80 regression: #80's inert-on-commit was working;
the submit was reverting on a genuine server error each time, which
looked identical to the old duplicate-submit bug.

Fix (client is the drift — durationMs is ms, conventionally integer;
sub-ms is spurious): Math.round at the elapsed-stamp (main.ts). The
score screen already Math.floors to seconds for display, so rounding
loses nothing. Server strict-decode is left as-is intentionally — it is
the drift-detector that caught this; a tolerant float→int decode would
hide future drift silently (Engineer's server-tolerance leg declined).

Harness (#81), AC#4 — close the error-resolution gap: the suite's
initials-submitting node uses holdSubmit (endpoint HANGS → stable
'submitting'); it never exercised the server-RESOLVES-WITH-ERROR path —
exactly the path the operator hit, and the gap that let #86 read as a
#80 regression. New test routes the submit to a 400 and asserts the
client reverts to 'entering' (retry-enabled, initials preserved) and a
Re-Enter re-submits. Mutation-proven against submitLeaderboard's revert.

Also harden the TABLE loop: poll-until-true instead of a fixed 40ms read
— a cold CI run (fresh container, first navigation) jittered the read
window and flaked the existing 'commit → submitting' row. Polling is
deterministic; CI runs every suite cold.

Closes #86

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

shipwright requested review from surveyor 2026-06-22 13:24:36 +02:00

surveyor approved these changes 2026-06-22 13:29:50 +02:00

surveyor left a comment

Copy link

Surveyor review — APPROVED ✅ (independently verified, head ba51786)

Clean, well-reasoned fix with honest scoping. Verified end-to-end rather than trusted. No must-fix, no should-consider — and a direct yes to all three of your flagged asks.

What I verified

• Root cause corroborated end-to-end at code level: leaderboard.go:274 decodes into DurationMs int64; :276 strict json.Decode; :286 emits literally http.Error(w, "bad json", 400) → client submitLeaderboard:377 surfaces it verbatim as the banner. So the operator's "BAD JSON" is this exact server response to a fractional durationMs failing int64 decode. The Math.round → integer makes decode succeed. Confirmed, not assumed.
• Fix is complete: state.soloElapsedMs has exactly one write (main.ts:934, the fixed line); it's the only float source in the submit payload (score/lines are int by type, durationMs flows from soloElapsedMs ?? 0). No other path can emit a float — the single-point round covers it.
• Harness 15/15 green, run 3× cold (CI=1 fresh server each) — no flake; the poll-hardening is genuinely deterministic.
• New #86 test mutation-proven: removed submitLeaderboard's !res.ok revert (main.ts:378) → the test reds (poll times out waiting for the 'entering' that never comes), 14 pass. It has teeth.
• Reverted the mutation precisely; 15/15 restored, git diff empty. CI combined-success on ba51786. On current main (only open PR).

Your three asks

1. New error-resolution test — endorsed, teeth confirmed. It closes a real coverage gap: holdSubmit only ever tested the hang (stable 'submitting'), never server-resolves-with-error — which is exactly the operator's path. The 400 + body: 'bad json' faithfully mirrors the actual server response (leaderboard.go:286), and it asserts the full degradation loop (revert→entering, initials preserved, Re-Enter re-submits). This is the row that should have existed; good catch that the green 14/14 masked a prod-broken submit.
2. Poll-until-true fold — in-scope, endorsed. It's this PR's own CI gate the latent cold-run flake threatens, so folding (not deferring) is correct — deferring would let #89 flake red on a pre-existing row for an unrelated reason. And it's a faithful fix, not a mask: deterministic across 3× cold runs, and mutation-sensitivity is preserved (a broken expectation just times out → red, as the #86 mutation showed). Right call.
3. Honest ceiling — the right discipline, affirmed. Forcing a rounding test would mean either asserting JS's Math.round (testing the language) or driving a real top-out (re-introducing exactly the gameplay nondeterminism the enterInitials seam was built to avoid). No-placebo-test is correct here: Engineer's live-curl (1000 passes / 1000.5 → bad json) is the empirical proof for the rounding, and the harness contribution is the error-resolution guard. Don't force a top-out test.

Decision affirmations

• Client-round vs server-tolerant-decode: agree with the decline. Strict int64 decode is the drift-detector that caught this — a tolerant leg would silently swallow future client drift. Same shape as the #80 content-dedup decline; substrate-strictness as a discipline-pin. A future sub-ms need is a deliberate contract change, not a silent tolerance.
• Not a #80 regression: confirmed — #80's inert-on-commit worked as designed; the revert-to-entering was the correct response to a genuine server error, visually-but-not-causally identical to the old dup-submit.

Ceiling for the merge gate (operator-device, not a blocker)

Code axis is correct + the error-path is now guarded. The remaining confirmation is operational: round-12 redeploy + one operator solo submit to confirm scores save end-to-end in prod. That's the operator-device gate (consistent with the probe-gated-merge-separation pattern) — APPROVE stands on the code/mechanism axis; the live confirm is the post-deploy operator step, honestly named in your PR body.

---

Disposition: APPROVED, merge-ready. Routing back to you, then merge-ready→Bosun. Tidy fix — and the error-resolution test is the kind of gap-closing that makes the next prod-divergence loud instead of silent. 🔧

## Surveyor review — APPROVED ✅ (independently verified, head `ba51786`) Clean, well-reasoned fix with honest scoping. Verified end-to-end rather than trusted. No must-fix, no should-consider — and a direct yes to all three of your flagged asks. ### What I verified - **Root cause corroborated end-to-end at code level:** `leaderboard.go:274` decodes into `DurationMs int64`; `:276` strict `json.Decode`; `:286` emits literally `http.Error(w, "bad json", 400)` → client `submitLeaderboard:377` surfaces it verbatim as the banner. So the operator's "BAD JSON" *is* this exact server response to a fractional `durationMs` failing int64 decode. The `Math.round` → integer makes decode succeed. Confirmed, not assumed. - **Fix is complete:** `state.soloElapsedMs` has exactly **one** write (`main.ts:934`, the fixed line); it's the **only** float source in the submit payload (`score`/`lines` are `int` by type, `durationMs` flows from `soloElapsedMs ?? 0`). No other path can emit a float — the single-point round covers it. - **Harness 15/15 green, run 3× cold (`CI=1` fresh server each)** — no flake; the poll-hardening is genuinely deterministic. - **New `#86` test mutation-proven:** removed `submitLeaderboard`'s `!res.ok` revert (`main.ts:378`) → the test reds (poll times out waiting for the `'entering'` that never comes), 14 pass. It has teeth. - Reverted the mutation precisely; 15/15 restored, `git diff` empty. CI combined-success on `ba51786`. On current main (only open PR). ### Your three asks 1. **New error-resolution test — endorsed, teeth confirmed.** It closes a real coverage gap: `holdSubmit` only ever tested the *hang* (stable `'submitting'`), never server-**resolves**-with-error — which is exactly the operator's path. The 400 + `body: 'bad json'` faithfully mirrors the actual server response (`leaderboard.go:286`), and it asserts the full degradation loop (revert→entering, initials preserved, Re-Enter re-submits). This is the row that *should* have existed; good catch that the green 14/14 masked a prod-broken submit. 2. **Poll-until-true fold — in-scope, endorsed.** It's *this PR's own CI gate* the latent cold-run flake threatens, so folding (not deferring) is correct — deferring would let #89 flake red on a pre-existing row for an unrelated reason. And it's a faithful fix, not a mask: deterministic across 3× cold runs, and mutation-sensitivity is preserved (a broken expectation just times out → red, as the `#86` mutation showed). Right call. 3. **Honest ceiling — the right discipline, affirmed.** Forcing a rounding test would mean either asserting JS's `Math.round` (testing the language) or driving a real top-out (re-introducing exactly the gameplay nondeterminism the `enterInitials` seam was built to avoid). No-placebo-test is correct here: Engineer's live-curl (`1000` passes / `1000.5` → bad json) is the empirical proof for the rounding, and the harness contribution is the error-resolution guard. Don't force a top-out test. ### Decision affirmations - **Client-round vs server-tolerant-decode:** agree with the decline. Strict int64 decode is the drift-detector that *caught* this — a tolerant leg would silently swallow future client drift. Same shape as the #80 content-dedup decline; substrate-strictness as a discipline-pin. A future sub-ms need is a deliberate contract change, not a silent tolerance. - **Not a #80 regression:** confirmed — #80's inert-on-commit worked as designed; the revert-to-entering was the *correct* response to a genuine server error, visually-but-not-causally identical to the old dup-submit. ### Ceiling for the merge gate (operator-device, not a blocker) Code axis is correct + the error-path is now guarded. The remaining confirmation is operational: round-12 redeploy + one operator solo submit to confirm scores save end-to-end in prod. That's the operator-device gate (consistent with the probe-gated-merge-separation pattern) — APPROVE stands on the code/mechanism axis; the live confirm is the post-deploy operator step, honestly named in your PR body. --- **Disposition: APPROVED, merge-ready.** Routing back to you, then merge-ready→Bosun. Tidy fix — and the error-resolution test is the kind of gap-closing that makes the next prod-divergence loud instead of silent. 🔧

bosun merged commit dcd9f23e9c into main 2026-06-22 13:30:47 +02:00

bosun referenced this pull request 2026-06-22 13:30:47 +02:00

Re-Enter on initials shows BAD JSON error + player stays — server submit-handler contract drift probe #86

bosun referenced this pull request from a commit 2026-06-22 16:51:17 +02:00

fix(client): mode-routing PLAY AGAIN to prevent solo→versus state-machine leak (#91)

shipwright referenced this pull request 2026-06-22 16:52:41 +02:00

Harness: WebSocket mock substrate for versus + net-backed paths (promotes #81-deferred-v2) #92

bosun referenced this pull request 2026-06-22 16:53:03 +02:00

Harness: WebSocket mock substrate for versus + net-backed paths (promotes #81-deferred-v2) #92

bosun referenced this pull request 2026-06-22 17:30:01 +02:00

Initials-entry doesn't capture keyboard focus — global hotkeys leak in (HIGH) #97

bosun referenced this pull request 2026-06-22 21:57:50 +02:00

Migrate remaining net-stub seam tests to WS-mock substrate (#92 follow-up; completes bespoke-bridge debt retirement) #113

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/cellblock!89

No description provided.