[Tim020]

What

• DELETE /api/v1/show now requires an authenticated admin (@api_authenticated @require_admin), matching show creation.
• Debug mode is opt-in: --debug defaults to off (previously on unless running as a PyInstaller bundle, including the Docker image). serve_traceback=False is set explicitly so stack traces are never returned, even with --debug. check_origin is untouched. README / server/README.md / client-v3/README.md now say to start the backend with --debug for development.
• Request size metrics (measurement only, no limits): Prometheus histograms digiscript_http_request_body_bytes{handler,method} (requests with a body; labelled by handler class, never raw path) and digiscript_ws_message_bytes, buckets powers of 4 from 1 KiB to 256 MiB, served at /debug/metrics. Histograms are module-level (registered once per process). HTTP is observed from DigiScriptServer.log_request; WebSocket via a small mixin that wraps WebSocketController.on_message (one base-class line, no change to the on_message body). README has a short note and a histogram_quantile example. Intended to tune size limits later from real data.
• client-v3: a WebSocket close with code 1009 (message too big) now backs off exponentially and shows one toast, instead of reconnecting every second (the error counter only incremented on onerror). This is independent of any server limit: Tornado's default 10 MiB WebSocket cap can already trigger a 1009 close (e.g. a full-state Yjs frame for a very large script).

No Docker changes and no HTTP/WebSocket size limit changes in this PR (Tornado defaults remain).

Tests

• Show deletion: unauthenticated and non-admin rejected, admin allowed, current show still 400.
• Debug option defaults to off, no traceback returned to clients even with debug enabled, body size observed for requests with a body and not for body-less ones (test_digi_server.py), WebSocket message size observed (test_ws_controller.py), client-v3 1009 backoff (useWebSocket.closeTooBig.test.ts).
• pytest 1052 passed, ruff clean, client-v3 lint/typecheck/vitest clean (217 tests), Playwright chromium 225 passed.

🤖 Generated with Claude Code

[github-actions]

Client Test Results

98 tests   98 ✅  0s ⏱️
 7 suites   0 💤
 1 files     0 ❌

Results for commit 1748bd9.

♻️ This comment has been updated with latest results.

[github-actions]

Client V3 Test Results

223 tests   223 ✅  1s ⏱️
 18 suites    0 💤
  1 files      0 ❌

Results for commit 1748bd9.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files      1 suites   1m 3s ⏱️
1 055 tests 1 055 ✅ 0 💤 0 ❌
1 060 runs  1 060 ✅ 0 💤 0 ❌

Results for commit 1748bd9.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

225 tests   225 ✅  2m 25s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit 1748bd9.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

225 tests   225 ✅  2m 22s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit 1748bd9.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA reviewed: 3dce9d07cddebf082d73f357107be4a648cb104f

Verdict: Changes needed before merge — two Important issues, both empirically confirmed, that undercut the two headline fixes in this PR (the 1009 backoff, and bounded metric cardinality). Everything else is polish/test-hardening.

Previous round follow-up: N/A — this is round 1, no prior threads.

SonarQube: Quality Gate passed. All 5 gate conditions pass (new reliability/security/maintainability ratings, 0% new duplication, 100% hotspots reviewed). 0 new issues, 0 security hotspots, 0 bugs/vulnerabilities/code smells on new code. new_coverage is not a gate condition on this project so the 0.0% figure in the bot comment (no coverage report uploaded for this run) does not block merge, but it's worth noting nothing enforces it either. Source: SonarCloud anonymous API (public project), cross-checked against the bot's PR comment — analysis confirmed fresh (exact head-SHA match).

New findings: 2 Important, 7 Suggestion. All anchored inline below.

Strengths:

• The show-DELETE auth fix is correct and verified fail-closed for malformed/expired JWTs and deleted users, with the right decorator order (@api_authenticated → @require_admin → @requires_show → @no_live_session) matching the existing post handler's convention.
• All 11 pre-existing test_shows.py delete regression tests still pass unmodified under the new decorators — solid implicit proof an admin can still delete a show.
• serve_traceback=False correctly overrides Tornado's debug-mode default and is covered by a genuinely behavioral test (not just an implementation check).
• The --debug default flip to False doesn't break anything in CI/E2E/Docker — grepped for IS_FROZEN/--debug usage repo-wide.
• The HTTP metric's handler label is correctly bounded to the class name (not raw path), and that's specifically tested.
• The frontend 1009 test is well-designed DAMP coverage for what it does check (toast-once + 2s→4s backoff doubling) — it just doesn't yet cover the auth-reset interaction below.

🤖 Generated with Claude Code

[Tim020]

Review round 1 addressed in 19c012c (all 9 inline threads replied to and resolved). Summary: 1009 backoff now persists across re-authentication; method label bounded; WS size mixin hardened; test assertions tightened and added. Declined one sub-item (try/except around the metric observation) with reasoning in-thread. No Docker or size-limit changes, per decision. Local: pytest 1055, ruff, client-v3 lint/typecheck/vitest 220, Playwright chromium 225 all pass.

[Tim020]

Review round 2

Head SHA reviewed: 19c012c70aa4d4f50528d24bb220dc62127d7d42

Verdict: Changes needed — one new Important regression introduced by this round's own fix, otherwise all round-1 findings are genuinely resolved with real code + tests (not just claims).

Previous round follow-up: 9/9 verified fixed. Every round-1 thread (2 Important, 7 Suggestion) now has a real code change and an accompanying test, checked directly against the diff since last round — not just taken on the author's word:

• Both Important items (1009 backoff never escalating; unbounded method metric label) are fixed and covered by new tests that reproduce the original failure scenario.
• All 7 Suggestions are fixed: toast exception safety, the metrics mixin's three latent footguns (silent no-op → now raises, double-counting → re-entrancy guard, char-vs-byte counting → fixed), the loose assertIn assertion, the missing decorator-order test, the brittle AST test, and the missing normal-close regression test.
• One (the end-to-end oversized-message test) was addressed with a reasonable, explicitly-documented tradeoff — it proves the connection terminates but not the specific 1009 code, since the author found that unreliable to assert — treated as resolved, not disputed.

SonarQube: Quality Gate still passed on this head (re-checked directly via the SonarCloud anonymous API): all 5 conditions pass, 0 new issues, 0 security hotspots.

New findings: 1 Important, 2 Suggestion — all introduced by this round's own changes, not missed-earlier items.

Strengths:

• Genuinely good engineering in the fixes, not just the minimum to pass review: tooBigCount cleanly separated from errorCount with a 30s stable-connection reset, a correct re-entrancy guard for the metrics mixin, and a custom assertion message instead of just silencing the brittle AST test.
• All CI checks (pytest, vitest, both Playwright browsers, typecheck, lint) are green on this head.
• The new useWebSocket.closeTooBig.test.ts cases are well-targeted at exactly the scenarios the review raised (auth-success-in-between, throwing toast, normal close).

Note, not a blocking finding: the legacy client/ (Vue 2) still has no 1009 handling at all — this predates this round and is below the bar for a "missed in earlier round" item at Suggestion level, but worth a follow-up given this codebase's usual both-clients parity requirement.

🤖 Generated with Claude Code

[Tim020]

Review round 2 addressed in ce676e3: all 3 inline threads replied to and resolved (reconnect re-sync regression fixed, stability timer and size-limit control case covered). Not changed: the legacy client/ (Vue 2) has no 1009 handling; it has no Yjs collaborative editing and only sends small messages, so it cannot trip the limit today (follow-up if parity is wanted). Local: pytest 1055, ruff, client-v3 lint/typecheck/vitest 223, Playwright chromium 225 pass.

[Tim020]

Review round 3

Head SHA reviewed: ce676e3ea0ecfc5f68b0a1e9317b9a8821acd197

Verdict: Changes needed — one round-2 finding is not actually fully resolved yet (test doesn't exercise what it claims), everything else from round 2 is genuinely fixed. No new production-code issues found in this round's diff.

Previous round follow-up: 2 fixed, 1 fix incomplete (caught and corrected after a second look — see below).

• Fixed: the wasErrored/re-sync regression — now attempts = Math.max(errorCount, tooBigCount) drives both the toast and the live-session re-sync, with a new test proving getShowSessionData() runs and the toast reads "after 1 attempt" following a pure-1009 reconnect.
• Fixed: the oversized-message test now has a proper control case (a message just under the cap, followed by a ping, asserting the connection is still open) before the oversized write, so the close is actually attributable to the size cap.
• Fix incomplete: the new forgets earlier 1009 closes once a connection has stayed up for 30s test doesn't actually have an earlier 1009 close before the 30s wait — tooBigCount is 0 going in either way, so the test passes even with the reset line deleted from the stableTimer callback (confirmed by removing it and re-running). I replied on the thread with a corrected version that actually drives a nonzero count into the wait. The companion test for the "not cleared under 30s" direction is fine and does catch a real regression.

SonarQube: Quality Gate still passed on this head (re-checked via the SonarCloud anonymous API): all 5 conditions OK, 0 new issues.

New findings: 0 Critical/Important/Suggestion in production code from this round's own diff. One trivial, non-blocking style nit: the new Python control-case test imports tornado.gen for gen.sleep(0.2), while the test directly above it in the same file already uses asyncio.sleep(0.05) — using asyncio.sleep here too would be more consistent and drop the extra import. Not worth a separate thread for something this small.

Strengths:

• The attempts fix is correct for every combination (both zero, either nonzero, both nonzero) and matches the same Math.max combinator already used for the reconnect delay — good internal consistency.
• The live-session re-sync test correctly doesn't wait for WS_AUTH_SUCCESS, since getShowSessionData() is a REST call, not a WS round-trip — accurately modeled.
• CI and the SonarCloud gate remain green on this head (E2E was still in progress at last check, not a concern for this review).

🤖 Generated with Claude Code

[Tim020]

Review round 3 addressed in the latest commit. Verified the claim: the 30s stability test started from a zero 1009 count, so it passed even with the reset removed. It now drives a 1009 close first, reconnects, stays up 30s, closes again and expects the 2s delay (not 4s); I confirmed it fails when the reset line is removed. Also switched the control-case test to asyncio.sleep and dropped the unused tornado.gen import. No production code changed. Local: pytest 1055, ruff, client-v3 lint/typecheck/vitest 223. The non-admin delete test still asserts exactly 401 and will need updating when #1458 lands.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Tim020]

Review round 4

Head SHA reviewed: 1748bd92124b916c3f345a5f79b06d962e3eb89d

Verdict: Ready to merge. This round's diff is test-only (2 files, ~10 lines) and closes out the last open item from round 3; no production code changed, so no new regression risk.

Previous round follow-up: 1 fixed (the only item still open).

• The forgets earlier 1009 closes once a connection has stayed up for 30s test now does an initial 1009 close (driving tooBigCount to 1) before the 30s wait, then checks the second 1009 close gets the minimum 2s delay rather than the 4s a non-cleared count would produce. Traced through manually: this version genuinely fails if the tooBigCount = 0; reset line were removed, unlike the previous version. Matches the corrected test I proposed in round 3 almost verbatim.
• Bonus: the round-3 tornado.gen → asyncio.sleep style nit (mentioned in the summary, not worth its own thread) was picked up too, and asyncio was already imported so no new import issue.

All other round-1/2/3 findings remain verified fixed from prior rounds (9 + 1 + 2 = 12 total across the PR's lifetime), with no new activity on those threads this round.

SonarQube: Quality Gate still passed on this head (re-checked via the SonarCloud anonymous API): all 5 conditions OK, 0 new issues.

New findings: none. Diff this round is test-only and was traced by hand rather than via subagent, given its size (2 files, one test rewritten, one import swapped) — both changes check out correctly.

Strengths: this PR has had a genuinely good iterative loop — every round's fixes were real code changes with tests that actually reproduce the original failure, including self-correcting when a test didn't cover what it claimed. CI is green (aside from E2E still in progress, unrelated to this round's test-only changes) and the Sonar gate has stayed clean across all 4 rounds.

🤖 Generated with Claude Code