[Tim020]

Summary

• New server/utils/web/app_metrics.py: digiscript_ws_connections, digiscript_ws_messages_total{op} (bounded; unknown OPs reported as OTHER), digiscript_collab_rooms, digiscript_show_session_active, digiscript_info{version}.
• collab_rooms and show_session_active are read live at scrape time (RoomManager registers itself and its app), so they cannot drift on any close path.
• ws_connections is incremented in open() and decremented exactly once in on_close() (guarded by _close_handled plus a per-handler counted flag).
• Replaced silent swallows in ws_controller (YJS invalid base64, YJS_UPDATE apply failure), script_room_manager (checkpoint cleanup, save-progress broadcast) and logs_viewer with contextual logging (revision, user/ip, sizes only; no payloads).

Testing

• Full pytest and full Playwright suite (225 passed) green.

🤖 Generated with Claude Code

[github-actions]

Client Test Results

102 tests  ±0   102 ✅ ±0   0s ⏱️ ±0s
  9 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit d106e36. ± Comparison against base commit 5998f6a.

♻️ This comment has been updated with latest results.

[github-actions]

Client V3 Test Results

227 tests  ±0   227 ✅ ±0   0s ⏱️ ±0s
 20 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit d106e36. ± Comparison against base commit 5998f6a.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files  ± 0      1 suites  ±0   1m 20s ⏱️ +13s
1 241 tests +21  1 241 ✅ +21  0 💤 ±0  0 ❌ ±0 
1 272 runs  +29  1 272 ✅ +29  0 💤 ±0  0 ❌ ±0 

Results for commit d106e36. ± Comparison against base commit 5998f6a.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

227 tests  ±0   227 ✅ ±0   2m 24s ⏱️ +3s
 14 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit d106e36. ± Comparison against base commit 5998f6a.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

227 tests  ±0   227 ✅ ±0   2m 14s ⏱️ -4s
 14 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit d106e36. ± Comparison against base commit 5998f6a.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA: 51ddfb00faf2974a2bb4db11207aece2262690de

Verdict: Ready to merge after considering the two Important findings below (neither is a correctness bug in production today; both are design/coverage gaps worth a decision). No blocking issues.

Previous round follow-up: N/A — first round, no prior threads.

SonarQube: Quality Gate PASSED. All conditions pass (new reliability/security/maintainability ratings A, 0% duplication, 100% hotspots reviewed, 0 new lines needing coverage). 4 new issues reported, all MINOR/MAJOR code smells, none gate-blocking:

• 2 are in server/utils/web/app_metrics.py (unnecessary list() calls on WeakSets) — covered in inline comments below.
• 2 are in server/test/digi_server/test_logger.py (lines 94, 123 — prefer assertLessEqual; use monkeypatch instead of manually restoring sys.excepthook). That file is not part of this PR's diff (gh pr diff 1467 doesn't touch it) — Sonar is comparing against the eventual main reference, so these pre-exist on the base branch feature/obs-capture-stdlib-logs. Flagged here for awareness only; not actionable in this PR.

New findings: 3 Important, 6 Suggestions. No Critical issues.

Strengths:

• The stated goal (convert silent except: pass swallows to contextual logging) is executed thoroughly and correctly: the original exception still propagates everywhere it must (_write_checkpoint_sync's raise), no new log line leaks a JWT, Yjs payload, or script content, and severity choices (WARNING for YJS_SYNC/YJS_UPDATE, DEBUG for the higher-frequency YJS_AWARENESS) are deliberate and tested.
• Metrics design is solid: label cardinality on digiscript_ws_messages_total is provably bounded (bounded_op + _KNOWN_OPS, backed by test_known_ops_cover_every_handler for the dispatch-table half), the double-close guard (_counted_connection + _close_handled) is correct and well tested (double-close, close-without-open, reconnect all covered), and the gauges reading live state at scrape time avoid drift.
• Test design shows real care: _write_checkpoint_sync's cleanup test triggers a genuine EBADF rather than mocking it away, and the metrics tests correctly use before/after deltas (with an explicit comment acknowledging cross-test global-registry contamination) rather than asserting absolute values.

🤖 Generated with Claude Code

[Tim020]

Review round 1 follow-up, all in b2c8981:

• Important, _live_session_value aborts on first failure / warning unhelpful: fixed. Each app is checked independently; failures are logged with app type and id; NaN only if nothing was live and at least one app failed (a live session elsewhere still reports 1). Tests added for both cases.
• Important, _count_rooms unguarded: fixed. Each manager is guarded and logged. Test scrapes generate_latest() with a raising manager and a failing app and asserts the scrape succeeds with the other series intact.
• Important, logs_viewer logged everything at DEBUG: fixed. StreamClosedError is DEBUG (client disconnect); any other exception is WARNING with traceback. Both covered by tests (the old DEBUG-only test was replaced).
• Important, tautological _KNOWN_OPS test: fixed. The test now parses the controller source with ast and asserts every string compared against ws_op is in _KNOWN_OPS, so a new elif ws_op == "X" branch fails it.
• S7504 unnecessary list() on both WeakSets: fixed.
• Use get_sync("current_show"): fixed.
• Module docstring "registered once": fixed; it now distinguishes the singleton metric objects from the per-instance weak sets.
• Comment ordering above _KNOWN_OPS: fixed.
• register_app_info() import side effect in ws_controller: moved into app_metrics module init (idempotent). It no longer depends on any controller import.
• register_application() in RoomManager.__init__: partially addressed. DigiScriptServer is outside this PR's ownership, so it stays in RoomManager.__init__ with a comment explaining why. Hook I would add: in DigiScriptServer.__init__ (app_server.py), call app_metrics.register_application(self) (and drop the call from RoomManager).
• Sonar smells in test_logger.py: not in this diff (belongs to Capture asyncio/alembic/warnings logs and install crash hooks #1466), ignored.

Local: full pytest 1191 passed, ruff check and format clean.

[Tim020]

Review round 2

Head SHA: 01e64f4c73675da227278e1acb66a715212ee611

Verdict: Ready to merge. No blocking issues; one minor Suggestion below, genuinely optional.

Previous round follow-up (all in commit b2c8981):

• Verified fixed (8): _live_session_value no longer aborts on first failure (now checks every app, logs with type+id, NaN only when nothing confirmed); _count_rooms now guards each manager individually (fixes the Important finding and the Sonar list() smell in one change); get_sync("current_show") adopted; module docstring now distinguishes singleton metrics from per-instance weak-set registration; register_app_info() moved out of ws_controller.py into app_metrics.py itself; the _SESSION_OPS/_KNOWN_OPS comment reordered; logs_viewer's except block split into StreamClosedError (DEBUG) vs. everything else (WARNING + traceback); the tautological _KNOWN_OPS test rewritten to parse the controller source via ast and catch any new elif ws_op == "X" branch. Each has an inline reply citing the exact fix and new/updated test, and all 8 threads are resolved.
• Accepted with explanation, left open for you (1): register_application() staying in RoomManager.__init__ rather than moving to DigiScriptServer.__init__ — the added comment's reasoning (scope + "every real app builds a RoomManager") is sound, so I replied accepting it, but left the thread unresolved since it's your call whether the explanation closes it.
• Needs human decision: none.

SonarQube: Quality Gate PASSED. Fresh analysis confirmed against head SHA 01e64f4c... (analysed 2026-10-22T22:32:36Z). All conditions pass, and 0 new issues (down from 4 last round — both app_metrics.py smells fixed above; the two test_logger.py smells were never part of this PR's diff and aren't reported against this PR anymore). 0 security hotspots.

New findings: 1 Suggestion, anchored to code the round-1 fix introduced (not a missed round-1 item).

Strengths:

• The round-1 follow-up comment itemized every finding with what changed and where, which made this round's verification straightforward and accurate.
• Two of the fixes (the _count_rooms guard, the ast-based _KNOWN_OPS test) also added direct regression tests proving the fix works (test_failing_gauge_callbacks_do_not_break_the_scrape literally scrapes generate_latest() with an injected failure and asserts the other series survive), not just a code change taken on faith.
• test_one_failing_app_does_not_hide_a_live_session_elsewhere is a nice touch — it runs both WeakSet iteration orders via subTest to make sure the fix isn't order-dependent.

🤖 Generated with Claude Code

[Tim020]

Round 2: the optional suggestion on _live_session_value (confirmed False vs an unrelated failure) was kept as NaN by design, with the rule now documented and pinned by a test in 1d93a1e. See the inline reply. Full pytest 1236 passed, ruff check and format clean.

[Tim020]

Round 3: register_application moved from RoomManager.__init__ to DigiScriptServer.__init__ in d106e36 per Tim's decision (see the inline reply). Full pytest 1241 passed, ruff check and format clean.

[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 3

Head SHA: d106e36c82ebb7e54811f2a8f2947735018472da

Verdict: Ready to merge. No open findings, no new issues.

Previous round follow-up (2 threads, both now resolved):

• Verified fixed: register_application() moved from RoomManager.__init__ to the end of DigiScriptServer.__init__ (app_server.py:347-352), exactly as suggested in round 1 — confirmed by two new tests (test_server_registers_itself_for_app_metrics, test_room_manager_registers_itself_but_not_the_application) that directly assert the split: a RoomManager registers itself but not its application.
• Dispute resolved in the author's favour: my round-2 Suggestion about _live_session_value returning NaN when a confirmed False from one app coexists with an unrelated app's failure. The counter-argument is correct — the gauge's contract is "confirm no live session anywhere," and a False from one app can't establish that while another app's state is genuinely unknown, so NaN is the right conservative answer, not a bug. The docstring now states the rule explicitly and test_live_session_value_rule_pinned pins all 6 cases (including both orderings of the mixed outcome) as a regression guard. Accepted and resolved.

SonarQube: Quality Gate PASSED. Fresh analysis confirmed against head SHA d106e36... (analysed 2026-10-22T23:01:31Z). All conditions pass, 0 new issues, 0 hotspots.

New findings: None. The incremental diff since round 2 (app_server.py, script_room_manager.py, app_metrics.py, plus the three test files) is exactly the two fixes above, each backed by a new, specific test — nothing else changed in the files this PR owns.

Strengths:

• Both fixes this round are minimal and surgical: the registration move touches only the two __init__ methods involved, and the docstring/test addition for _live_session_value doesn't change behaviour at all — it just makes an already-correct rule explicit and regression-proof.
• test_live_session_value_rule_pinned is a good regression-test pattern: it enumerates the full truth table (confirmed-true, confirmed-false, failed, empty, and both orderings of mixed outcomes) in one table-driven test rather than scattering it across ad hoc cases.

🤖 Generated with Claude Code