[Tim020]

Summary

• WS: malformed frames (non-object, non-string OP) ignored; leader ops and LIVE_SHOW_JUMP_TO_PAGE validate DATA instead of raising; script-room ops tolerate non-object DATA; update_session handles a deleted user.
• Logging: the existing Enable Log Redaction setting is now the single switch. On: WS messages are logged as op name and size only, and REST bodies have sensitive keys (password, tokens, secret, ...) redacted recursively plus decorator paths. Off: raw content is logged for debugging.
• Default change: log_redaction now defaults to on (was off), and log_level defaults to INFO (was DEBUG). Installs that already have a saved value keep it; only fresh installs or installs with no saved value change. Documented in docs/pages/user_config.md.

Already on dev (re-verified): REFRESH_CLIENT ownership rules, leader/auth gating, admin-only ws/sessions.

Tests

WS and REST log tests for redaction on/off, malformed DATA, deleted user, defaults. Full pytest, ruff and the full Playwright chromium suite pass.

🤖 Generated with Claude Code

[github-actions]

Client Test Results

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

Results for commit 946c193.

♻️ 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 946c193.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files      1 suites   57s ⏱️
1 076 tests 1 076 ✅ 0 💤 0 ❌
1 081 runs  1 081 ✅ 0 💤 0 ❌

Results for commit 946c193.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

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

Results for commit 946c193.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

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

Results for commit 946c193.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA reviewed: 603542e517e72453f55624ba68090559245b923e

Verdict: changes needed. One Critical regression (reproducible crash in request-body logging on a common, documented auth flow) should be fixed before merge; everything else is non-blocking polish.

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

SonarQube: Quality gate PASSED (status OK) at commit 10c39b19 (the gate hasn't re-run against 603542e yet). All rated conditions passed (new reliability/security/maintainability rating 1, 0% new duplication, 100% hotspots reviewed, 0 open hotspots). Two new CRITICAL code smells (python:S3776, cognitive complexity) were reported at 10c39b19:

• server/utils/web/base_controller.py — on_finish (21/15)
• server/controllers/ws_controller.py — _handle_session_op (22/15)

The follow-up commit 603542e ("Reduce cognitive complexity of REST body logging and WS session op dispatch") extracts _redaction_enabled/_loggable_raw_body/_loggable_body out of on_finish, and _handle_leader_op/_handle_jump_op/_live_show_session out of _handle_session_op. Reading both functions post-refactor, this should resolve both findings, but it hasn't been confirmed against a fresh SonarCloud analysis of this head yet — worth checking the gate on the next CI run.

New findings: 1 Critical, 3 Important, 4 Suggestion — all anchored to diff lines (listed inline below).

Strengths:

• The WS hardening itself is solid and well-targeted: malformed frames, non-dict DATA, wrong-typed fields, and a concurrently-deleted user are all handled with warn-and-drop instead of crashing the connection, and the isinstance(x, bool) guards correctly anticipate Python's bool-is-int quirk.
• New WS tests exercise both the malformed-input and deleted-user cases directly against real state (checking latest_line_ref is unchanged, checking the connection stays responsive) rather than just checking for "no exception."
• log_redaction.py is a clean, well-isolated unit with accurate RST docstrings and non-mutating semantics backed by a dedicated unit test file.
• Docs were updated for the new user-facing setting, per CLAUDE.md's requirement to update docs/ alongside behavior changes.

🤖 Generated with Claude Code

[Tim020]

Review round 1 addressed in abd282e: all 8 inline items fixed (replied in each thread). The S3776 findings were already fixed in 603542e; SonarCloud reports 0 new issues. Settled decision unchanged: log_redaction is the single switch (raw logging when off, default on).

[Tim020]

Review round 2

Head SHA reviewed: abd282e3db178844b39fb4b205c828e684540407

Verdict: changes needed — one Important finding remains in the new fix-up commit itself (see below); everything from round 1 is resolved.

Previous round follow-up: 8/8 verified fixed, all threads already resolved by the author with a matching code change and, in most cases, a new regression test:

• Critical (JSONPatch crash on absent optional path) — fixed, redact-by-key now runs first and the patch application is wrapped in try/except JSONPatchError.
• Important (malformed-WS-frame logging lost raw content + exception detail) — fixed, exception text is now always included and raw/size-only content follows the redaction setting.
• Important (docs didn't mention Log Level=DEBUG requirement) — fixed, both the prose and the Log Viewer Sources bullet updated; the rot-prone sentence reworded to be timeless.
• Important (untested "saved value survives upgrade" claim) — fixed, new TestSavedLoggingSettingsAreKept seeds the settings file before app construction and asserts both log_redaction and log_level survive load.
• 4 Suggestions (conflated BEGIN_INTERVAL warning, silent DATA coercion in script-room ops, untested bool actId/length, missing confirmPassword in SENSITIVE_KEYS) — all fixed, each with a matching test added.

I re-verified all 8 against the current code (not just the author's replies) and confirmed via the green CI run on this commit (Python Test Results, Ruff, SonarCloud Code Analysis all passed).

SonarQube: Quality gate PASSED (status OK), freshly re-analyzed against this exact head (analysisDate matches commit abd282e3's timestamp). All 5 rated conditions pass, and the 2 cognitive-complexity code smells noted in round 1 (already fixed by 603542e, prior to this commit) are confirmed gone — codeSmells: 0, 0 open issues of any kind.

New findings: 1 Important (in the fix-up commit itself), 0 unanchored.

Strengths: every round-1 finding was fixed precisely as suggested (or better — e.g. the BEGIN_INTERVAL test now asserts no Interval row was created, not just "no exception"), each with a new regression test, in a single tightly-scoped commit. Good discipline.

🤖 Generated with Claude Code

[Tim020]

Review round 2 addressed in 47b5cef (1 inline item, fixed; thread replied to and resolved). SonarCloud was already clean. Settled decision unchanged: log_redaction is the single switch.

[Tim020]

Review round 3

Head SHA reviewed: 47b5cef0de99bdd1aae3f6867e55f24c8a914df2

Verdict: ready to merge. No open findings remain across all three rounds.

Previous round follow-up: 1/1 verified fixed (round 2's Important finding). apply_path_redactions (server/utils/web/log_redaction.py) now guards isinstance(body, (dict, list)) before touching the body, and applies each decorated path as its own single-op JSONPatch with its own try/except JSONPatchError, so a missing path no longer blocks redaction of the others. I independently re-ran both failure scenarios from round 2 (a bare-string body, and a two-path patch with one path missing) directly against the real jsonpath library and confirmed both now behave correctly; the PR's own new tests (test_json_string_body_does_not_break_logging, TestApplyPathRedactions) cover the same cases. All 9 findings raised across rounds 1–2 are now fixed and verified — no open threads.

SonarQube: Quality gate PASSED (status OK), freshly re-analyzed against this exact head (47b5cef0...). 0 bugs, 0 vulnerabilities, 0 code smells, 0 open issues of any kind.

New findings: none. CI is green on every check relevant to this change (Python Test Results, Ruff, SonarCloud Code Analysis, CodeQL, check-migrations all passed); the Playwright E2E jobs were still mid-run at review time but this change is backend-only with no client-facing surface touched, so they aren't expected to be affected — worth a final glance once they complete.

Strengths: three clean, tightly-scoped fix-up commits in a row, each addressing exactly what was raised with a dedicated regression test and nothing extra. The final apply_path_redactions helper is a well-isolated, well-tested unit (own docstring, own test module) rather than inline complexity bolted back onto on_finish.

🤖 Generated with Claude Code

[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