Mission Control Conversation Code Review

Date: 2026-04-07 Type: Code Review Reviewer: Luci (with Opus subagent council) Ticket: MC-291

Scope

Full review of the MC ticket conversation system: - mission-control/app.py — Flask routes, SSE, ticket/comment API, WebSocket handler - mission-control/models.py — SQLite models, ticket CRUD, session management - mission-control/mc_interactive.py — WebSocket interactive Claude sessions - mc_pickup.py — Ticket dispatcher + background worker spawning - mission-control/static/chat.js — Browser chat UI (SSE + WebSocket)

Council

Gemini: Rate-limited (429 — MODEL_CAPACITY_EXHAUSTED). No findings.
Codex: CLI syntax issue prevented execution. No findings.
Opus subagent: Completed full review. Confirmed all critical findings, added 2 new important ones.

Summary

19 issues found across 6 categories. 3 CRITICAL, 10 IMPORTANT, 6 MINOR.

Tickets Created

Ticket	Severity	Category	Title
MC-294	IMPORTANT	Security	SQL injection risk in models.py get_luci_activity() f-string queries
MC-295	CRITICAL	Race Condition	_next_identifier() not atomic — duplicate MC- IDs possible
MC-296	IMPORTANT	Reliability	mc_interactive.py recursive _run_turn() can stack overflow
MC-297	IMPORTANT	Architecture	Duplicate stream parsing code in mc_pickup.py (main + fallback)
MC-298	IMPORTANT	Performance	SSE clients queue grows unbounded — no cleanup on disconnect
MC-299	CRITICAL	Bug	models.py get_db_stats() uses vconn after close
MC-300	IMPORTANT	Architecture	api_add_comment() 170+ lines needs decomposition
MC-301	IMPORTANT	Performance	work_ticket() fetches ALL tickets to find one — O(N)
MC-302	IMPORTANT	Security	No auth on upload endpoint
MC-303	MINOR	Reliability	Fire-and-forget threads swallow errors silently
MC-304	IMPORTANT	Performance	ticket_messages table grows unbounded — no retention
MC-305	MINOR	Security/UX	chat.js missing markdown rendering for agent responses
MC-306	CRITICAL	Race Condition	queued_messages drain race — messages can be lost
MC-307	MINOR	Architecture	Tool context extraction duplicated across files
MC-308	MINOR	Performance	Every model function opens/closes a new DB connection
MC-309	IMPORTANT	Race Condition	Lock file written AFTER Popen — TOCTOU race
MC-310	IMPORTANT	Race Condition	Double-pickup window for needs_input tickets
MC-311	IMPORTANT	Reliability	stderr pipe never drained — potential deadlock
MC-312	IMPORTANT	Security	Hardcoded fallback auth tokens in source code

Priority Recommendations

Fix first (CRITICAL): 1. MC-299 — vconn use-after-close (trivial 1-line fix) 2. MC-295 — identifier race condition (moderate — atomic INSERT) 3. MC-306 — queued message loss (moderate — row-level deletes)

Fix soon (HIGH-IMPACT IMPORTANT): 4. MC-310 — double-pickup race (atomic compare-and-swap) 5. MC-294 — SQL injection (parameterized queries) 6. MC-304 — ticket_messages index + retention

Fix when convenient: 7-17: Architecture improvements, minor reliability, and UX enhancements