Collaborative script editing: backend salvage + fix-up (Phase 1)

[Tim020]

Summary

Phase 1 of the collaborative script editing rebuild (issue #889). Ports the CRDT backend from the abandoned feature/collaborative-editing branch (Vue 2 attempt, PR #905, never merged) onto current dev, and closes the outstanding backend issues a two-pass review found on that branch but that were never actually fixed.

Full design/rationale: plans/COLLABORATIVE_EDITING_V3_PLAN.md (gitignored — ask if you want it pasted).

Why this exists / why the old branch stalled: checked out feature/collaborative-editing in an isolated worktree and found its 221 collab backend tests pass clean against current dev — the backend design is sound. But a follow-up review of PR #905 found ~40 issues, including a critical one (every keystroke did a full delete+insert on the shared Y.Text, defeating CRDT merge semantics — concurrent editors would clobber each other on every keypress). A fix plan for all 40 was drafted but git history shows none of the 6 batches were ever committed. That, not just design churn, is why it never shipped.

Given that, and that the feature is being rebuilt for client-v3 (Vue 3) only per the new plan, this PR:

• Salvages the framework-agnostic backend (room manager, Y.Doc↔DB conversion, WebSocket protocol) via a 3-way merge against dev's 236 commits of drift (the controller directory moved to controllers/api/v1/ in the interim)
• Folds in every still-open backend fix from that review directly, rather than deferring to a follow-up pass
• Adds the script_drafts table + sessions.is_cutting column migration
• Closes several test coverage gaps the review flagged (corrupt payload handling, live-session guards on Yjs ops, revision-switching room handoff, stale draft cleanup, checkpoint-due detection)

The Vue 3 frontend integration is not in this PR — it's tracked as later phases in the plan doc. This PR adds backend surface only; without a frontend sending JOIN_SCRIPT_ROOM, none of the new WebSocket ops are reachable, but it does touch shared, already-live code paths (revision guards, the script PATCH helpers, session model) so the full suite (not just collab tests) needs to stay green.

What's fixed from the original review

• YJS_SYNC step=2 had no error handling around apply_update (crashed the WS coroutine on a malformed payload)
• Checkpoint atomic-write could corrupt a good file on partial write, or leak .yjs.tmp files on rename failure
• close_active_room() exceptions inside on_close's finally block went unhandled and unlogged
• discard_active_room errors weren't surfaced to the HTTP DELETE caller (silent divergence)
• Checkpoint failures didn't back off — would retry every 30s forever, flooding logs
• ROOM_CLOSED broadcast swallowed all exceptions with a bare except: pass
• DISCARD_SCRIPT_DRAFT was incorrectly blocked during a live session (it's meant to be the one recovery path if a live session starts mid-draft)
• The load-revision guard only checked the in-memory room, not a DB-persisted draft that survived a server restart
• Added a cycle/dangling-pointer guard to the linked-list→Y.Doc walk (dev had already independently fixed the dangling-pointer half via a composite FK constraint — nice surprise, found while merging models/script.py)
• "Insufficient permissions" was duplicated 6× across ws_controller.py; centralized to a constant

Also found and fixed one incidental bug while adding test coverage: collab WS handlers call get_logger().trace(...), but the custom TRACE log level is only registered by main.py at process start — the test harness never imports main.py, so every test that actually reached these lines raised AttributeError. This is why the coverage was missing in the first place. Fixed by registering TRACE in test/conftest.py too.

Automated review response (2026-09-08)

A pr-review-toolkit bot review raised 5 Critical / ~9 Important / ~9 Suggestion findings. Full triage in plans/collab_v3_pr1359_review_response.md (gitignored — ask if you want it pasted). Verified and fixed:

• YJS_UPDATE/YJS_SYNC step 2 had no role check — any joined viewer, not just an approved editor, could mutate the shared Y.Doc directly
• CueAssociation.group_id/sort_order were silently dropped on every collaborative line edit that touched an already-grouped/ordered cue
• The Y.Doc build-from-DB path had no error handling, unlike its corrupted-draft-file sibling — a malformed revision would crash JOIN_SCRIPT_ROOM uncaught
• SAVE_SCRIPT_DRAFT/DISCARD_SCRIPT_DRAFT called the (deliberately-raising) draft-cleanup helper unguarded, after already telling clients the save/discard succeeded
• Found independently while checking the review: the 3-way merge had silently dropped a dev-side hardening fix (self.on_close() on a dead WS write) — restored
• save_draft's broadcast delta was captured after the deleted_line_ids clear, so the clear never reached other clients
• _has_line_changed never compared page, so a page-only line move left the DB's ScriptLine.page stale

One Critical finding (a claimed TOCTOU race in editor/cutter mutual exclusion) was traced line-by-line and verified not to occur in the current code — no await actually yields control between the check and the commit in either handler. Deferred to Phase 4 (where the mutual-exclusion model gets hardened anyway) rather than patched speculatively. Reasoning and remaining deferred/rejected items are in the ledger doc.

Test plan

• Full backend suite: 861/861 passing
• ruff check / ruff format --check: clean
• Alembic migration generated via --autogenerate against a purpose-built pre-collab baseline DB (verified diff contains exactly the new table + column + index, nothing else)
• Full Playwright E2E suite (chromium + firefox, run separately — this branch is backend-only so neither run touches collab code paths, but both confirm no regression in existing REST/WS contracts)
• Manual smoke test of the new endpoints once a frontend exists to drive them (Phase 2+)

🤖 Generated with Claude Code

https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8

[github-actions]

Client V3 Test Results

39 tests   39 ✅  0s ⏱️
 4 suites   0 💤
 1 files     0 ❌

Results for commit 82b096d.

♻️ This comment has been updated with latest results.

[github-actions]

Client Test Results

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

Results for commit 82b096d.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

  1 files    1 suites   2m 29s ⏱️
863 tests 863 ✅ 0 💤 0 ❌
868 runs  868 ✅ 0 💤 0 ❌

Results for commit 82b096d.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

221 tests   221 ✅  2m 15s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit 82b096d.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

221 tests   221 ✅  2m 13s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit 82b096d.

♻️ This comment has been updated with latest results.

[Tim020]

🤖 Automated Review (Claude Code)

This is an automated review generated by Claude Code using its pr-review-toolkit skill — four specialized review agents (general code review, test coverage, silent-failure/error-handling, and type-design) ran in parallel against this PR's diff. It is not a human review; treat findings as a starting point for discussion, and verify anything before acting on it.

Session: https://claude.ai/code/session_017Nhmed96rrh8ZF5dK7hLGY

---

Critical Issues (recommend fixing before merge)

1. No authorization on JOIN_SCRIPT_ROOM / YJS_UPDATE
server/controllers/ws_controller.py:763-782, 899-937
An unauthenticated client can join a room and receive the full script. The viewer/editor role distinction is tracked (room.clients) but never enforced on write paths — a viewer, or an unauthenticated socket, can mutate the shared Y.Doc, and that mutation is later persisted by whichever editor next saves. REQUEST_SCRIPT_EDIT does check write access, but joining and writing bypass it entirely.

2. CueAssociation.group_id / sort_order silently dropped on every collaborative line edit
server/utils/script/ydoc_to_lines.py:337-344
The changed-line migration path deletes the old CueAssociation and recreates it, copying only revision_id/line_id/cue_id. The PATCH endpoint this is supposed to mirror (script.py:507-516) also copies group_id and sort_order. Any grouped or manually-ordered cue permanently loses that data the first time an editor touches the line it sits on. The test fixture (test_ydoc_to_lines.py:838-874) seeds the association without those fields, so nothing catches it.

3. Y.Doc build/join path has no error handling, unlike the rest of this PR
server/controllers/ws_controller.py:989-990 → server/utils/script_room_manager.py:7605-7616 (_load_or_build_doc)
Unlike the corrupted-draft-file recovery path just above it (which is well guarded), the normal "build from DB" branch is unwrapped, as is the call site. A malformed line/association raises straight out of on_message, which has no top-level try/except — the joining client hangs forever with no COLLAB_ERROR and no server-side context in the log.

4. WS SAVE_SCRIPT_DRAFT / DISCARD_SCRIPT_DRAFT don't guard _delete_draft, even though the HTTP equivalent does
server/utils/script_room_manager.py:7432 (save_room), call sites server/controllers/ws_controller.py:1154,1163
_delete_draft's own docstring says failures must propagate so callers know the discard didn't fully succeed. The HTTP DELETE endpoint (draft.py:230-238) honors that and returns 500. The WS handlers don't: save_room already broadcasts "saved" to every client before the unguarded _delete_draft call, so if cleanup then fails, everyone believes the save succeeded while the revision silently stays locked (blocking future edit/cuts/revision operations) with no error ever surfaced.

5. Editor/cutter mutual exclusion is a TOCTOU race, not an enforced invariant
server/controllers/ws_controller.py:600-680
REQUEST_SCRIPT_EDIT/REQUEST_SCRIPT_CUTS both do check-then-commit with awaits in between the read and the write. Exclusivity currently holds only because no yield point happens to land between check and write today — there's no DB CheckConstraint and no lock. Contrast with RoomManager.get_or_create_room, which solves the structurally identical "don't let two concurrent handlers both act on a stale check" problem with an explicit, documented asyncio.Lock.

---

Important Issues

• Regression — dead WS-client cleanup silently removed. The WebSocketClosedError handler in write_message (ws_controller.py:953-964) used to call self.on_close() to deregister a dead client; that call is gone, but the log message still says "closing". A stale client now logs an error on every future broadcast, forever, with no remediation.
• deleted_line_ids broadcast bug — script_room_manager.py:339-357 captures the post-save state vector after clearing the array, so the delta sent to clients never actually carries the clear. Every connected client keeps stale entries, which is the exact staleness scenario the surrounding comment warns about.
• Page-move bug — _has_line_changed (ydoc_to_lines.py:161-207) never compares page, so moving a line to a different page via the Y.Doc leaves ScriptLine.page stale in the DB, which can then orphan its cue association during Pass-2 cleanup (:446).
• close_active_room() race — mutates self._room outside _room_lock, called concurrently from three places, and can't simply be locked without deadlocking its own caller at get_or_create_room:512 (script_room_manager.py:664-694).
• Zero test coverage for the new DELETE /api/v1/show/script/draft REST endpoint, and for the linked-list cycle/dangling-pointer guards in line_to_ydoc.py.
• "Corrupt payload" WS tests only mock the exception, they don't send real garbage bytes through real pycrdt.apply_update. Verified empirically that pycrdt doesn't always raise on garbage input — a structurally-valid-but-meaningless update could sail through uncaught in production despite these tests passing.
• No end-to-end two-socket convergence test exists for the core CRDT promise — two real clients concurrently editing and converging to the same state.
• PR description's test-plan numbers don't match reality: claimed "847 pre-existing + 12 new = 859"; actual measured result is 706 baseline → 860 (+154 new tests). Not a coverage problem (more tests than claimed, not fewer), but the description wasn't checked against a real run.
• RoomManager.discard_room/discard_active_room query ScriptDraft with no .where() clause — silently picks an arbitrary row if more than one ever exists, trusting an unenforced "at most one draft globally" assumption.

---

Suggestions

• Checkpoint write (_write_checkpoint_sync) doesn't loop on os.write()'s return value — a short write would still corrupt the checkpoint despite the new atomic-rename logic.
• Checkpoint backoff advances the retry clock on failure but still logs a full traceback every 30s on a sustained fault (e.g. disk full) — no escalation or de-dup.
• Dead code: RoomManager.cleanup_stale_drafts duplicates logic already inlined in app_server.py; RoomManager.stop() is defined but never called, so a dirty room isn't checkpointed on graceful shutdown.
• _reject_script_room_op helper is defined but bypassed at 4 of its own call sites in favor of inline duplicate dicts.
• Private fields/methods (room._dirty, room._last_checkpoint, rm._checkpoint_room) are reached across class boundaries instead of through public accessors that already exist for the success path (mark_checkpointed()).
• ScriptDraft.last_editor_id is documented, migrated, and indexed — but never written anywhere.
• Two straggler string literals ("No show loaded", "No active revision") sit right next to the PR's newly-centralized error constants but weren't added to it.
• ScriptRoom.add_client(role: str = "editor") defaults to the privileged role and is reused for demotion (STOP_SCRIPT_EDIT) — should be a Literal/enum with a dedicated set_role/demote method.
• _show_changed's message says the draft was "discarded" when it's actually checkpointed (saved to disk) — misleading, and the revision remains locked afterward until someone finds the DELETE endpoint.

---

Strengths

• 4 of the 6 explicitly-claimed bug fixes verified solid: apply_update error handling on YJS_SYNC step 2, the on_close finally-block guard, HTTP-surfaced discard errors, and the ROOM_CLOSED broadcast's per-recipient error handling. The "Insufficient permissions" string centralization also fully landed (verified only one unrelated docstring occurrence remains repo-wide).
• RoomManager.get_or_create_room's double-checked locking against concurrent room creation is genuinely well done, with a docstring explaining exactly why the lock is needed.
• The TRACE-log-level registration fix in test/conftest.py is a real, verified unlock — reverting it locally reproduces silent timeouts across ~9 collab tests that were previously passing for the wrong reason.
• Crash-recovery tests (corrupt draft file → DB rebuild, stale-draft cleanup in both directions) and checkpoint-due-detection tests are behavior-focused and would genuinely fail on regression, not just exercise happy paths.
• The linked-list cycle/dangling-pointer defensive guards in line_to_ydoc.py are the right instinct for a hand-rolled linked-list walk — they're just currently untested.

---

Findings are ranked by severity within each section but not otherwise deduplicated across the four agents beyond what's shown above. File:line references were verified by the reviewing agents against the PR branch at the time of review; re-verify against HEAD if the branch has moved on.

[Tim020]

Thanks for the review — went through all five Critical findings and the highest-value Important ones, verifying each against the actual code before acting. Fixed in 32d4238:

Fixed (verified real):

• C1 (write-side): YJS_UPDATE/YJS_SYNC step 2 had no role check — any joined viewer, not just an approved editor, could mutate the shared Y.Doc directly, bypassing REQUEST_SCRIPT_EDIT's RBAC gate entirely. Added a room.clients.get(self) == "editor" check before applying updates. (The read-side half of this finding — unauthenticated JOIN_SCRIPT_ROOM — was checked separately and found consistent with a pre-existing, systemic pattern: ScriptController.get() also lacks @api_authenticated. Not changed this round.)
• C2: CueAssociation.group_id/sort_order were silently dropped on every collaborative line edit that touched an already-grouped/ordered cue — real data loss. Fixed + regression test added (the old fixture never set these fields, which is why it had zero coverage).
• C3: The Y.Doc build-from-DB path had no error handling, unlike its corrupted-draft-file sibling branch — a malformed revision would crash JOIN_SCRIPT_ROOM uncaught. Wrapped and surfaced a COLLAB_ERROR instead.
• C4: SAVE_SCRIPT_DRAFT/DISCARD_SCRIPT_DRAFT called the (deliberately-raising) draft-cleanup helper unguarded, after already telling clients the save/discard succeeded — a cleanup failure left the revision reporting itself locked forever with no explanation. Both call sites now guard it and surface a clear error.
• Two Important-severity bugs: save_draft's broadcast delta was captured after deleted_line_ids was already wiped, so the wipe never reached other clients; and _has_line_changed never compared page, so a page-only line move left the DB stale.

Investigated, not fixed — C5 (TOCTOU race on editor/cutter exclusion): traced both handlers line-by-line. There's no await that actually yields control back to the event loop between the mutual-exclusion check and the commit in either REQUEST_SCRIPT_EDIT or REQUEST_SCRIPT_CUTS as currently written (the nested awaits either hit an uncontended lock's fast path or wrap functions with no internal await at all). Verified this doesn't reproduce today. Deferring the hardening (an explicit lock) to Phase 4, where the whole exclusivity model is being rebuilt for the CRDT frontend anyway, rather than patching around a race that isn't currently reachable.

Also found independently while checking the review (not something it flagged): the earlier 3-way merge had silently dropped a dev-side hardening fix — write_message's WebSocketClosedError handler no longer called self.on_close(), leaking dead clients in the room roster forever. Restored.

Deferred with reasons (test-coverage suggestions, two-socket convergence tests, an un-scoped select(ScriptDraft) query that only matters if multiple orphaned drafts ever coexist): see plans/collab_v3_pr1359_review_response.md for the full ledger and reasoning.

Backend suite: 861/861 passing (was 860). ruff clean. Full Playwright E2E suite green (chromium + firefox, run separately — this branch is backend-only, so neither run touches the new collab code paths).

[sonarqubecloud]

Quality Gate passed

Issues
24 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud