From f5b3870bca938a7a37a60d1ff991e9ec2f941f41 Mon Sep 17 00:00:00 2001 From: slitty-codes Date: Sat, 30 May 2026 22:02:55 +1200 Subject: [PATCH] =?UTF-8?q?audit:=20full=20bug=20+=20architecture=20review?= =?UTF-8?q?=20(2026-05-30)=20=E2=80=94=2014=20confirmed,=205=20P1,=2039=20?= =?UTF-8?q?arch=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 30-agent workflow (7 bug-hunt + 7 architecture dimensions) with independent adversarial verification on every candidate. Two themes worth fixing as units: staged-run lifecycle robustness (cancel/cleanup/terminalizers) and chain-of-custody integrity (parser drops, validator self-corruption, citation-relevance). Co-Authored-By: Claude Opus 4.7 --- audits/full-review-2026-05-30.md | 485 +++++++++++++++++++++++++++++++ 1 file changed, 485 insertions(+) create mode 100644 audits/full-review-2026-05-30.md diff --git a/audits/full-review-2026-05-30.md b/audits/full-review-2026-05-30.md new file mode 100644 index 00000000..26084d37 --- /dev/null +++ b/audits/full-review-2026-05-30.md @@ -0,0 +1,485 @@ +# ASA — Full Bug & Architecture Review + +_2026-05-30 · Opus 4.8 (1M) · adversarially-verified multi-agent review_ + +**Bottom line.** ASA is structurally healthy where it counts. The product's central invariant — Phase 1 measurements are authoritative, Phase 2 never overrides them — is enforced *structurally* (separate tables, separate write-paths), not merely by convention, and the adversarial pass refuted 2 of 16 candidate bugs, so the confirmed list below is trustworthy rather than padded. The defects cluster into two stories worth fixing as units, not one-by-one: + + 1. The **staged-run lifecycle** (cancel, cleanup, failure-handling) was built solidly for the *measurement* stage, and the later stages never inherited the same discipline — so they orphan multi-GB subprocesses on cancel, strand attempts in `running` forever on a setup error, and race the artifact cleaner. + 2. The **chain of custody** — the thing the product actually sells — carries the most validator bugs: a parser that silently drops prompt-blessed fields, key/BPM extractors that self-corrupt or check only the first mention, and a citation check that validates *existence* but not *relevance*. + +Nothing here is a rewrite. Most P1s are localized, and several of the highest-leverage fixes are one-liners. + +## How to read this + +This review ran as a 30-agent workflow: **7 bug-hunt dimensions** and **7 architecture dimensions** fanned out in parallel, and then **every candidate bug was handed to an independent skeptic agent whose job was to *refute* it** against the real source. Only findings that survived verification appear under "Confirmed"; the ones that didn't are listed under "Refuted" with the reason, so the rest earns your trust. + +Severities: **P0** crash/data-loss/security · **P1** wrong result or leak in normal use · **P2** real but edge-case · **P3** latent/minor. Where verification corrected the finder's severity, both are shown (`Pn→Pm`). + +**Totals:** 14 confirmed bugs (5×P1, 6×P2, 3×P3), 2 refuted, 39 architecture findings. + +--- + +## 1. Executive summary + +### The five P1s (full detail in §2) + + 1. **Cancel doesn't cancel (#1, `server.py:2557`).** Only the measurement subprocess is registered for termination; pitch-note (Demucs+torchcrepe, ~2–4 GB) and MT3 (multi-GB) run via blocking `subprocess.run` and survive interrupt/delete for up to 600 s / 1800 s. The UI reports the run terminated while the RAM stays pinned — and the orphan later resurrects its own interrupted DB row after its backing artifacts were deleted. + 2. **The parser breaks the citation spine (#2, `backendPhase1Client.ts:935`).** Four reverb/vocal subfields the Phase 2 prompt is told to cite are dropped during Phase 1 parsing, so legitimate recommendations citing them fail the existence check. The product's core promise is undercut by a silent field-drop — exactly the camelCase tripwire CLAUDE.md warns about. + 3. **Interpretation strands forever (#3, `server.py:2008`).** If artifact resolution fails before the Gemini call, the attempt stays `running` with no terminalizer and no reaper; the UI polls indefinitely. Its siblings (pitch-note, MT3) already guard this — interpretation never got the same treatment. + 4. **Polling never times out (#4, `analyzer.ts:260`).** `monitorAnalysisRun` has no wall-clock budget and the per-poll fetch has no deadline; a hung backend hangs the client until the tab closes. + 5. **SSRF DNS-rebinding TOCTOU (#5, `url_ingest.py:159`).** Validation and fetch resolve the hostname independently, so an attacker-controlled DNS record can pass the guard and then point the fetch at an internal address. The guard is otherwise strong; this is the one real hole. + +The most *interesting* lower-severity catches: `normalizeKey` self-corrupts `"major"`→`"majoror"` and fires a **false** key-contradiction error at users (#11, one-char fix); PLR is computed by subtracting **dB** LUFS from a **linear** true-peak amplitude (#9, unit mismatch); and `truePeak` rounded to one decimal silently hides inter-sample overs on the default path (#8) — a *confidently wrong* number, which violates the "ground truth" promise more than an honest low-confidence one would. + +### Cross-cutting themes (the real signal) + + 1. **Staged-run lifecycle robustness** — #1, #3, #6, #7 plus the operational arch findings (cleanup is run-state-blind, no global cap on concurrent heavy subprocesses, the admission-control 429 is dead code so the queue is unbounded). Root cause: terminalization/registration discipline that exists for `measurement` was never extended to the peer stages. Fix the pattern once and most of these close together. + 2. **Chain-of-custody integrity** — the product thesis has the densest bug cluster: #2 (dropped fields), #11/#14 (validator self-corruption / first-match-only), and the structural finding that citations are checked for **existence, not relevance** (a rec can cite any present-but-unrelated measurement and pass) while the whole validator is **advisory-only and never gates** storage or rendering. + 3. **Frontend resilience** — there is **no React error boundary anywhere**, so one throw or chunk-load failure in lazy `AnalysisResults` blanks the entire UI; combine with #4 (no polling timeout) and #10 (retry handlers swallow failures into an unhandled rejection) and a small set of fixes hardens the UX disproportionately. + 4. **DSP honesty** — #8 and #9 are "confidently wrong" measurements. Under invariant #1 these are worse than hedged uncertainty and deserve priority over their P2 label. + 5. **No executable contract guard** — the Phase 1 contract lives in *three* hand-maintained representations with no generated source of truth and **no cross-app test**; CI gates only a subset of the suite (full vitest, chord-theory coverage, real-backend e2e never run); and the real streamed-subprocess path is *mock-branched away from every test*. The rename tripwire bites precisely because nothing executable defends it. + +### What I'd fix first + + 1. **Today, trivial + high-leverage:** the latent `NameError` in the eviction loop (#12 — a live landmine that throws the moment that branch runs), the `normalizeKey` typo (#11 — it currently shows users false contradiction errors), and adding a React error boundary. + 2. **One reliability PR:** register + terminalize *all* subprocess stages (#1), add the interpretation/measurement terminalizers (#3, #6, #7), and give polling a wall-clock timeout (#4). + 3. **Restore the dropped subfields (#2)** and add the cross-app contract test so the rename class of bug becomes executable rather than a comment. + 4. **SSRF (#5):** resolve once and pin the IP through to the fetch (the v2 fix the docstring already anticipates). + 5. **Schedule deliberately (larger):** the god-object splits (`AnalysisResults.tsx`, `server.py`) and moving citation validation from existence toward relevance. + +### Credit where due + +The audit also confirmed what's solid, which is worth stating: invariant #1 is enforced by construction, the intentional cross-boundary dupes (the MIME map) actually agree today, the admin-key seam and hosted/local isolation check out, and the stage-queue is correct-by-construction. And the verification pass earned its keep — it caught two plausible-but-wrong findings (a "citation mirror divergence" and a "stderr secret leak") and refuted them against the source before they reached this page (§4). + +--- + +## 2. Confirmed bugs + +### 1. [P1] Interrupt/delete leaves pitch-note and MT3 subprocesses running (multi-GB RAM orphaned up to 30 min) + +`apps/backend/server.py:2557` · Backend concurrency & resource lifecycle · verification confidence: **high** + +**What's wrong.** The interrupt route (line 2557) and delete route (line 2606) both call `_interrupt_active_child_processes(run_id)`, which only terminates processes in `_ACTIVE_CHILD_PROCESSES`. Only the measurement subprocess is ever registered there (via `_run_streamed_subprocess` -> `_register_active_child_process(stage_key='measurement')`). The pitch-note stage (`_execute_pitch_note_attempt`, `subprocess.run(... timeout=600)` at server.py:1302) and the MT3 stage (`_execute_mt3_attempt`, `subprocess.run(... timeout=1800)` at server.py:1455) use plain blocking `subprocess.run` and are never registered. So on interrupt/delete the DB rows flip to 'interrupted' and the route returns 202 with `stagesTerminated`, but the Demucs+torchcrepe (~2-4GB) or MT3 JAX/t5x (multi-GB) subprocess keeps running and holding memory+CPU until its own 600s/1800s timeout. delete_run (analysis_runtime.py:1578) additionally unlinks the source-audio artifact file out from under the still-running child, and the child's later `record_artifact`/`complete_*` writes target a now-deleted/interrupted run. + +**Trigger.** POST /api/analysis-runs/{run_id}/interrupt or DELETE /api/analysis-runs/{run_id} while the run's pitch/note (stem separation) or MT3 stage is mid-flight. The subprocess survives the cancel for up to 600s (pitch_note) or 1800s (MT3), pinning multiple GB of RAM the user believed they freed. + +**Fix.** Register the pitch-note and MT3 subprocesses in `_ACTIVE_CHILD_PROCESSES` so `_interrupt_active_child_processes` can terminate them: either route both through a `Popen`-based helper that calls `_register_active_child_process(run_id, 'pitchNote'|'mt3', process)`/`_unregister_active_child_process` (mirroring `_run_streamed_subprocess`), or wrap the existing `subprocess.run` calls in a `Popen`+`communicate(timeout=...)` and register/unregister around them. Then `stagesTerminated` is truthful and the memory is actually reclaimed on cancel. + +**Why it's real (verifier).** Verified against the code. (1) Only the measurement subprocess is registered in _ACTIVE_CHILD_PROCESSES: server.py:835-840 routes it through _run_streamed_subprocess, which calls _register_active_child_process(run_id, 'measurement', process) at line 510 using a Popen handle. (2) Pitch-note (_execute_pitch_note_attempt) at server.py:1302 and MT3 (_execute_mt3_attempt) at server.py:1455 use plain blocking subprocess.run(timeout=600/1800) with no Popen handle ever exposed or registered. (3) Both interrupt (2557) and delete (2606) call _interrupt_active_child_processes(run_id) (214-226), which only iterates _ACTIVE_CHILD_PROCESSES and terminates registered processes via _terminate_process (real SIGTERM->SIGKILL at 200-211). Unregistered pitch-note/MT3 children are never reached, so stagesTerminated returns [] for them while the child keeps running. (4) No cooperative cancellation: the worker loops (_pitch_note_worker_loop 2150, _mt3_worker_loop 2188) blockingly run _execute_*_attempt via asyncio.to_thread in the SAME process (so the in-memory registry is the correct mechanism — proven by measurement working); the only is_run_interrupted checks (1146, 2076) are stage-boundary/post-completion gates, not mid-subprocess. The orphaned Demucs+torchcrepe (~2-4GB, confirmed by the docstring at 1239-1242) or MT3 JAX/t5x (multi-GB, docstring 1396-1398) child pins RAM until its own 600s/1800s timeout — memory the user believes freed. (5) State-corruption aggravator on the interrupt path: interrupt_run (1469) only flips the attempt to status='interrupted' and sets preferred_pitch_note_attempt_id=NULL (1542), AND deletes source_audio+stem artifact FILES from disk (1551-1554) out from under the still-running child. When that orphan finishes it calls complete_pitch_note_attempt (936), whose ONLY guard is row existence (950 raises KeyError only if the row is gone) — the interrupt did not delete the row, so it unconditionally UPDATEs status='completed' (959) and resets preferred_pitch_note_attempt_id=attempt_id on the run (968-975), resurrecting the interrupted attempt and undoing the NULL — after its backing artifacts were deleted. The child can also insert orphan stem rows via record_artifact (server.py:1327). On delete_run (analysis_runtime.py:1558+) every artifact including source audio is unlinked (1581) mid-read. The pitch-note stage is a standard staged-pipeline stage (not an opt-in like MT3), so the trigger is routine. P1 is justified: multi-GB RAM survives a cancel the UI reports as succeeded, plus persisted state-machine corruption on the canonical interrupt/delete path. The proposed fix (register both subprocesses via a Popen helper mirroring _run_streamed_subprocess) is correct. + + +### 2. [P1] Frontend Phase 1 parser silently drops 4 prompt-blessed reverb/vocal subfields, breaking the citation-existence check + +`apps/ui/src/services/backendPhase1Client.ts:935` · Frontend↔backend contract integrity · verification confidence: **high** + +**What's wrong.** parseOptionalReverbDetail (lines 935-944) reconstructs reverbDetail field-by-field and forwards only rt60/isWet/tailEnergyRatio/measured — it drops perBandRt60 and preDelayMs. parseOptionalVocalDetail (lines 946-961) likewise drops stemEnergyRatio and stemOtherCorrelation. All four are emitted by the backend (analyze_detection.py:590-591 for reverb; :879-886 for vocal) and are explicitly registered as valid Phase-1 citation targets: phase2Validator.ts PHASE1_NEW_FIELD_PATHS lists 'reverbDetail.perBandRt60' and 'reverbDetail.preDelayMs' (lines 105-106), and the Phase 2 system prompt instructs Gemini to cite all four verbatim (prompts/phase2_system.txt:124,130,190,191,211,213 — e.g. line 191 "reverbDetail.preDelayMs ... 'Set Reverb Predelay to 42 ms'"). The frontend citation check builds its allowlist by walking the PARSED Phase1Result (phase2Validator.ts:331 collectPhase1FieldPaths -> walkForPaths), so any field the parser dropped is absent from the allowlist. The same parser feeds both the legacy (backendPhase1Client.ts:176) and the canonical staged path (analysisRunsClient.ts:382 parseCanonicalMeasurementResult). The backend's own mirror validator (server_phase2.py:2699 _validate_phase2_citation_paths) walks the RAW measurement payload, where these keys are present, so the two checks — designed to be byte-for-byte equivalent (server_phase2.py:2597 docstring) — disagree. + +**Trigger.** Run any track through Phase 2 where Gemini follows the prompt and emits a recommendation citing reverbDetail.preDelayMs (or .perBandRt60, vocalDetail.stemEnergyRatio, .stemOtherCorrelation) in its phase1Fields array. The backend accepts the citation (no warning); the frontend validatePhase1FieldCitations raises a false MISSING_CITATION ERROR (phase2Validator.ts:465-476): 'phase1Fields entry "reverbDetail.preDelayMs" ... does not match any path present in the Phase 1 payload. Do not invent field paths.' A legitimate, blessed measurement citation is flagged as invented, breaking the chain-of-custody report on a correct recommendation. + +**Fix.** In parseOptionalReverbDetail, forward perBandRt60 (object of {low,lowMids,highMids,highs} numbers, or null) and preDelayMs (toNumber). In parseOptionalVocalDetail, forward stemEnergyRatio and stemOtherCorrelation (both toNumber, nullable). This restores the four prompt-blessed paths into the parsed Phase1Result so collectPhase1FieldPaths includes them and the frontend citation allowlist re-agrees with the backend. Add a fixture covering a reverbDetail.preDelayMs citation to lock the regression. + +**Why it's real (verifier).** Verified end-to-end against actual code. (1) Backend emits all four fields on the TOP-LEVEL detail objects: analyze_detection.py:590-591 (reverbDetail.perBandRt60, reverbDetail.preDelayMs) and :879-886 (vocalDetail.stemEnergyRatio, vocalDetail.stemOtherCorrelation). (2) The frontend parser drops them: parseOptionalReverbDetail (backendPhase1Client.ts:935-944) reconstructs reverbDetail field-by-field forwarding only rt60/isWet/tailEnergyRatio/measured; parseOptionalVocalDetail (:946-961) forwards only hasVocals/confidence/vocalEnergyRatio/formantStrength/mfccLikelihood. The UI types DO declare all four (measurement.ts:562/569/584/593), so they are legitimately expected — the drop is a bug, not by-design. (3) Both transport paths use these parsers: legacy parsePhase1Result wires them at lines 652-653; the canonical staged path calls parseCanonicalMeasurementResult (line 382) -> parsePhase1Result, applied at analysisRunsClient.ts:355 to snapshot.stages.measurement.result. (4) The citation allowlist walks the PARSED object: phase2Validator.ts:396 collectPhase1FieldPaths(phase1) -> walkForPaths (line 337) enumerates only keys present on the parsed Phase1Result, and projectPhase1FromRun (line 238-244) spreads the already-stripped measurement into the object fed to validatePhase2Consistency (analyzer.ts:297, App.tsx:966). (5) A cited dropped path raises a hard ERROR: phase2Validator.ts:465-476, type MISSING_CITATION / severity 'ERROR', message 'does not match any path... Do not invent field paths', and it runs on every modern run via the isNewShapePhase2 gate (line 250-251) which fires whenever any rec exposes phase1Fields (the v2 schema always does). (6) The backend mirror disagrees: server_phase2.py:2699-2700 collects allowed paths from _normalize_measurement_result_for_gemini(raw) where these four keys ARE present (normalization only renames spectral fields, not reverb/vocal subfields), and it is WARNING-only for *missing* paths — so it stays silent on these four, while its docstring (line 2602-2603) claims byte-for-byte equivalence with the frontend. (7) The prompt blesses all four verbatim, including phase2_system.txt:191 'reverbDetail.preDelayMs: 42 -> Set Reverb Predelay to 42 ms'. CONCRETE BITE: Gemini follows the prompt, emits a recommendation citing reverbDetail.preDelayMs (or the other three) in phase1Fields; backend accepts silently; frontend flags it as an invented citation ERROR on the chain-of-custody report — penalizing a correct, measurement-grounded recommendation, which directly undermines PURPOSE.md invariant #2. Adversarial checks ruled out false-positive: the path is NOT guarded elsewhere (no re-derivation of reverb/vocal from raw; projectPhase1FromRun consumes the stripped MeasurementResult), the check IS reached on real runs, and existing tests miss it because validator tests build Phase1Result literals via createBasePhase1 (bypassing the parser, e.g. test at line 1137 passes only because preDelayMs is hand-placed) and the passing stemAnalysis citation tests exercise the pass-through cast path (backendPhase1Client.ts:616-618), not the dropping top-level reconstructors. P1 is correct: no crash/security (not P0), but it corrupts the chain-of-custody report — the product's core deliverable — on correct output and violates a documented validator-equivalence contract, which is more than cosmetic (not P2). + + +### 3. [P1] Interpretation worker strands attempt in 'running' on artifact-resolution failure, UI polls forever + +`apps/backend/server.py:2008` · Error handling & type safety · verification confidence: **high** + +**What's wrong.** `_execute_interpretation_attempt` calls `runtime.get_interpretation_grounding(run_id)` (line 2014), `runtime.get_source_artifact(run_id)` (line 2046, raises KeyError if the source-artifact row is missing), and `runtime.require_local_artifact_path(...)` (line 2047, raises FileNotFoundError when the source-audio file is gone from disk) with NO try/except. The attempt was already flipped to status 'running' by `reserve_next_interpretation_attempt` (analysis_runtime.py:1260-1267). When any of these raise, the exception propagates uncaught to `_interpretation_worker_loop` (server.py:2169-2185), which only logs `[warn] interpretation worker loop failed` and idles -- it never calls `fail_interpretation_attempt`. The attempt is left terminal-less in 'running'. This is asymmetric with the two sibling executors: `_execute_pitch_note_attempt` (line 1256-1260) and `_execute_mt3_attempt` (line 1413-1417) deliberately bind `source_artifact = None` before a try and moved these exact calls INSIDE it, with comments stating the move exists 'so a failure here terminalizes the attempt rather than leaving it stuck running -- which the interpretation-ordering gate would otherwise turn into an indefinite block.' The recovery sweep `recover_incomplete_attempts` (analysis_runtime.py:1433) runs only once at startup (server.py:329-330), so the attempt stays 'running' until a server restart. On the frontend, `monitorAnalysisRun` (analyzer.ts:259-356) loops `while(true)`; `isRunTerminal` (line 140-161) requires interpretation status to be one of completed/failed/interrupted/not_requested -- 'running' is none of these -- and the declared `timeoutMs` option is never consulted, so the UI polls indefinitely in a perpetual 'interpreting' state with no error surfaced. + +**Trigger.** Create an analysis run with interpretation requested. The interpretation attempt is queued behind a slower MT3/pitch-note peer (the ordering gate at analysis_runtime.py:1246-1253 blocks it while those are queued/running). Before the interpretation worker reserves it, the source-audio file is removed from `.runtime/artifacts/` -- e.g. the periodic `cleanup_artifacts` (utils/cleanup.py:43, ttl 24h) deletes any non-`preserved`/`.keep` file older than the TTL while leaving the SQLite run row intact, or an operator clears the artifacts dir. When the worker then reserves the attempt (status->running) and calls `require_local_artifact_path`, it raises FileNotFoundError; the attempt strands in 'running' and the client poll loop never terminates. + +**Fix.** Wrap the body of `_execute_interpretation_attempt` (the `get_interpretation_grounding`/`get_source_artifact`/`require_local_artifact_path` calls and the request dispatch) in try/except, mirroring `_execute_pitch_note_attempt`: on exception call `runtime.fail_interpretation_attempt(attempt_id, error={'code': 'INTERPRETATION_FAILED', 'message': str(exc), 'retryable': True, 'phase': ERROR_PHASE_GEMINI}, ...)` so the stage terminalizes and the run becomes terminal. Separately, give the frontend poll loop a wall-clock deadline using the already-declared `timeoutMs` so a stranded backend can't hang the UI indefinitely. + +**Why it's real (verifier).** Verified the full chain in the actual code. (1) `_execute_interpretation_attempt` (server.py:2008-2110) calls `runtime.get_interpretation_grounding` (2014), and for non-stem_summary profiles `runtime.get_source_artifact` (2046) and `runtime.require_local_artifact_path` (2047) with NO try/except. (2) Those three raise: `get_source_artifact` raises KeyError when the source-artifact row is missing (analysis_runtime.py:497); `require_local_artifact_path` raises FileNotFoundError when the file is not a local file (analysis_runtime.py:540); `get_interpretation_grounding` raises KeyError on missing run/measurement rows (573). (3) The attempt is already flipped to 'running' by `reserve_next_interpretation_attempt` (analysis_runtime.py:1260-1267) before the executor runs. (4) `runtime.fail_interpretation_attempt` is invoked only at server.py:2097, on the `execution["ok"] == False` branch — a raised exception jumps past it. (5) `_interpretation_worker_loop` (server.py:2169-2185) catches the exception only to `print('[warn] interpretation worker loop failed')` and `asyncio.sleep`; it never terminalizes the attempt, which strands in 'running'. (6) The asymmetry is explicit and intentional in the two siblings: `_execute_pitch_note_attempt` (1252-1265) and `_execute_mt3_attempt` (1413-1428) bind `source_artifact = None` before a try and moved these exact calls inside it, with comments stating the move exists 'so a failure here terminalizes the attempt rather than leaving it stuck running — which the interpretation-ordering gate would otherwise turn into an indefinite block.' The interpretation stage — the very one the gate (analysis_runtime.py:1239-1267 blocks queued interpretation while MT3/pitch-note are queued/running) protects — was left unhardened. (7) The only recovery, `recover_incomplete_attempts` (analysis_runtime.py:1433, flips 'running'→'interrupted'), runs once at process startup (server.py:329-330, worker.py:14-15); grep found no stale/stuck/heartbeat/timeout sweeper, so the attempt stays 'running' until a restart. (8) Frontend confirmed: `monitorAnalysisRun` (analyzer.ts:259-356) loops `while(true)` with no wall-clock check; `isRunTerminal` (140-161) requires interpretation status ∈ {completed,failed,interrupted,not_requested} — 'running' (and its `publicStatus` mapping 'running' in stage_status.py) is excluded — so it returns false forever; the declared `timeoutMs` option (analyzer.ts:31) is never read anywhere in the file (grep confirms a single hit at the declaration). Net: an artifact-resolution failure permanently hangs the UI in 'interpreting' with no error surfaced, recoverable only by server restart. Trigger is reachable via operator clearing `.runtime/artifacts/`, hosted-profile artifact eviction, or the 24h `cleanup_artifacts` TTL (utils/cleanup.py:43; only `.keep`/`preserved` exempt) deleting source audio while the SQLite row persists — the ordering gate widens the window by delaying interpretation behind a slow MT3 peer. P1 is appropriate: indefinite client hang with no surfaced error, no self-heal short of restart; the maintainers' own sibling-stage hardening with comments naming this exact deadlock corroborates both reachability and severity. + + +### 4. [P1] monitorAnalysisRun polls forever — no wall-clock timeout, and per-poll fetch has no deadline + +`apps/ui/src/services/analyzer.ts:260` · Frontend async / polling / cleanup · verification confidence: **high** + +**What's wrong.** The polling loop `while (true)` in monitorAnalysisRun (lines 260-356) only exits when isRunTerminal(snapshot) becomes true or throwIfUserCancelled throws. There is no maximum-elapsed guard. The `timeoutMs` field is declared in AnalyzeAudioOptions (line 31) and passed in from App.tsx (handleStartAnalysis line 672, `timeoutMs: activeTimeoutMs`) but `grep -n timeoutMs analyzer.ts` shows it is referenced ONLY at the declaration — it is never read in the loop. The waiter `delay(ms, signal)` (lines 46-65) resolves on the timer or rejects only on abort; it has no deadline either. The per-poll request getAnalysisRun → fetchJson (httpClient.ts:4) likewise honors only `init.signal` and has no per-request timeout. The AbortController is aborted only in handleStopAnalysis (App.tsx:770), i.e. exclusively on a manual user Stop. Consequently, if the backend leaves a stage non-terminal (worker dies without marking the stage failed, stage stuck in 'running'/'queued'), or if a poll fetch hangs with the TCP connection open but no response, the UI polls/hangs indefinitely with isAnalyzing stuck true and no error ever surfaced — the user has no automatic recovery and must click Stop. + +**Trigger.** Start an analysis; backend worker crashes or wedges after enqueue so e.g. stages.interpretation.status stays 'running' permanently (or a single GET /api/analysis-runs/{id} poll hangs without responding). monitorAnalysisRun never breaks out and never times out; the spinner runs forever. + +**Fix.** Thread the existing `timeoutMs` (deriveAnalyzeTimeoutMs already computes it) into monitorAnalysisRun: record a start timestamp, and at the top of each loop iteration (and/or inside delay) throw a BackendClientError('BACKEND_TIMEOUT', ...) once Date.now() - start exceeds the deadline. Additionally give the per-poll getAnalysisRun fetch its own AbortController-backed timeout (as postBackendMultipart already does in backendPhase1Client.ts:351-352) so a single hung socket cannot stall the loop. + +**Why it's real (verifier).** REAL. Traced the full path. `monitorAnalysisRun` (apps/ui/src/services/analyzer.ts:260-356) is `while(true)` with exactly two exits: `isRunTerminal(snapshot)` (line 338) and abort via `throwIfUserCancelled`/`delay`. Neither is wall-clock bounded. `timeoutMs` is declared in AnalyzeAudioOptions (line 31) and passed from App.tsx (line 672, `timeoutMs: activeTimeoutMs`), but a repo-wide grep confirms it appears in analyzer.ts ONLY at the declaration — it is never read in the loop. `delay()` (46-65), and the per-poll path getAnalysisRun (analysisRunsClient.ts:153-166, passes only `signal: options.signal`) → fetchJson (httpClient.ts:4-22, only `init.signal`) → buildConfiguredRequestInit (config.ts:137-154, injects headers only, NO signal/AbortSignal.timeout) all lack any per-request deadline. The AbortController fires solely in handleStopAnalysis (App.tsx:770) — a manual user Stop; `analysisStartedAtRef` drives only the elapsed-time display counter (App.tsx:362-374), never an abort, and there is no setTimeout-based auto-abort anywhere in App.tsx. That the mechanism exists and was deliberately omitted from the run path is proven by the legacy multipart path postBackendMultipart (backendPhase1Client.ts:345-352), which wires the same deriveAnalyzeTimeoutMs value to `setTimeout(() => controller.abort(), timeoutMs)`. No analyzer test asserts any monitor timeout (timeoutMs/BACKEND_TIMEOUT refs live only in backendPhase1Client.test.ts). + +Reachability (tightened): a fully-dead backend self-heals — the next fetch throws TypeError, converted to NETWORK_UNREACHABLE BackendClientError (httpClient.ts:12-19) which propagates to the catch at analyzer.ts:357 and exits the loop. The genuine, defensible trigger is: backend alive and still serving GETs but a stage row never reaches terminal. The authors' own comment at server.py:1419-1423 concedes a stage can 'escape to the worker loop stuck in running ... indefinitely (until restart recovery)'; recover_incomplete_attempts (analysis_runtime.py:1433-1467) is startup-only and only UPDATEs `WHERE status = 'running'`, so a `queued` stage orphaned by worker death is never recovered even on restart (and `queued`/`running` are non-terminal in isRunTerminal, lines 145-158). Every such poll returns a valid non-terminal snapshot, so the loop spins forever with isAnalyzing stuck true and no error surfaced. Secondary prong: an established-but-silent socket on a single GET has no client deadline (effectively unbounded UX, ultimately OS/browser-bounded in minutes). + +Severity P1: hosted profile is a documented target (CLAUDE.md) with no guaranteed restart-recovery within a session; stage-stuck-non-terminal + zero client-side defense = stuck user, spinner forever, no error, no auto-recovery, manual Stop the only escape. Backend mitigations are partial and self-admittedly incomplete, and the caller-computed timeoutMs is silently dropped — an intent violation, not a missing nicety. Proposed fix (thread timeoutMs into the loop as a wall-clock deadline + give the per-poll fetch its own AbortController timeout mirroring postBackendMultipart) is correct. + + +### 5. [P1] SSRF DNS-rebinding TOCTOU: validation and fetch resolve the hostname independently + +`apps/backend/url_ingest.py:159` · Security · verification confidence: **high** + +**What's wrong.** fetch_url_to_bytes() validates the host by calling socket.getaddrinfo(hostname) inside _assert_host_is_public() (line 281), then hands the original URL string to requests.get() (line 162), which performs its OWN, second DNS resolution. The two lookups are not pinned to each other, so an attacker who controls a domain's authoritative DNS (TTL=0, round-robin) can return a public A record for the validation lookup and a private/loopback/metadata address for the fetch lookup — exactly the SSRF the guard exists to stop. I confirmed the guard logic itself is otherwise sound: IPv6 ULA fd00::/8, IPv6 loopback ::1, link-local fe80::, IPv4-mapped ::ffff:127.0.0.1 / ::ffff:169.254.169.254, decimal (2130706433) and hex (0x7f000001) IPv4 literals are ALL blocked (getaddrinfo normalizes the literals and _is_non_public_address flags every category). So the only surviving bypass is the time-of-check/time-of-use gap, which the module docstring (lines 13-16, 104-110) explicitly acknowledges as unmitigated ('out of scope for v1'). Acknowledged-in-a-docstring is not guarded. The route is NOT profile-gated (server.py:2317), so it is reachable in both local and hosted modes. + +**Trigger.** POST /api/analysis-runs with form field url=http://rebind.attacker.example/x.flac, where rebind.attacker.example serves TTL=0 DNS that returns a public IP on the first lookup (passes _assert_host_is_public) and 169.254.169.254 on the second lookup (the requests.get fetch). On a cloud VM this exfiltrates IAM/instance credentials from the metadata endpoint; on any host it reaches internal services. + +**Fix.** Pin the validated IP through to the connection instead of re-resolving. Resolve once, pick a vetted IP, and connect to that IP with a custom requests HTTPAdapter (or urllib3 PoolManager) that overrides DNS, preserving the original Host header / SNI for TLS (do NOT substitute the IP into the URL string — that breaks certificate validation). Alternatively, after connect, re-validate the actual peer socket's getpeername() IP against _is_non_public_address before reading the body and abort on mismatch. Apply on every redirect hop too (redirects are already disabled, which is good). + +**Why it's real (verifier).** CONFIRMED REAL — DNS-rebinding TOCTOU in url_ingest.fetch_url_to_bytes. The mechanism is exactly as claimed: line 159 calls _assert_host_is_public(parsed.hostname), which resolves via socket.getaddrinfo (line 281) and validates every returned A/AAAA record. Line 162 then passes the ORIGINAL url string to requests.get(url, ...), and urllib3 performs a second, independent name resolution at connect time. There is no DNS pinning between the two: grep across server.py/server_upload.py/url_ingest.py shows zero mount()/HTTPAdapter, no custom PoolManager, and no post-connect getpeername() re-check. So a domain whose authoritative DNS returns a public A record on the first lookup and 169.254.169.254 (or a private/loopback address) on the second passes the guard yet is fetched against the private target. Redirects are disabled (allow_redirects=False, line 173), which correctly closes the redirect-based bypass — leaving rebinding as the genuine residual path, not a duplicate of an already-blocked vector. + +I independently verified the static guard is otherwise sound, so the TOCTOU is the only surviving bypass and is therefore material: _is_non_public_address returns True for 169.254.169.254 (link-local/metadata), 127.0.0.1, ::1, fd00::1 (IPv6 ULA), fe80::1, ::ffff:169.254.169.254 (IPv4-mapped), 10.0.0.5, and 100.64.0.1 (CGNAT, explicitly handled); and getaddrinfo normalizes the decimal literal '2130706433' to 127.0.0.1 (blocked). All literal/encoding bypasses are closed. + +NOT a false positive despite the docstring: the rubric's "by-design/documented" exclusion covers documenting INTENDED, CORRECT behavior. Here the docstring (lines 13-16, 104-110, 270-273) documents a KNOWN-BUT-UNREMEDIATED exploitable gap ("out of scope for v1"). An acknowledgment comment does not remediate a vulnerability — the attack still works at runtime, so it remains a real defect. + +Reachability: @app.post("/api/analysis-runs") (server.py:2273) has no auth/profile dependency in its signature; url is a plain Form field, url_provided routes to _create_analysis_run_record_from_url → fetch_url_to_bytes(url) at line 646 with the user-supplied string flowing straight through. The 40-test tests/test_url_ingest suite passes and, tellingly, must patch socket.getaddrinfo and requests.get SEPARATELY (lines 104/111 vs 242+/252/263), corroborating that the two resolutions are unlinked. Live, exercised code. + +Severity — qualified P1: impact is trust-boundary dependent. In the default LOCAL single-user profile the URL-submitter is the operator, so a rebind to loopback/metadata is self-SSRF with near-zero impact and no IAM metadata endpoint on a dev laptop. The P1 lives in HOSTED multi-tenant deployment on cloud infra, where a rebind to 169.254.169.254 exfiltrates instance/IAM credentials across a real boundary. docs/PUBLIC_HOSTING_FOUNDATION.md confirms hosted is a genuine, intended deployment target (so this is not dead scaffolding), though no Dockerfile/compose/fly/render manifest is present yet — hosted is foundational/in-progress, not yet shipped to cloud. Net: P1 conditional on hosted deployment landing on metadata-bearing infrastructure; it degrades toward P2 under the local-only deployment that actually runs today. Proposed fix (resolve-once, pin the vetted IP via a DNS-overriding HTTPAdapter preserving Host/SNI, or re-validate getpeername() before reading the body, applied per hop) is the correct remediation. + + +### 6. [P2] Interpretation attempt stuck 'running' forever on any pre-Gemini exception (no terminalizer) + +`apps/backend/server.py:2008` · Backend concurrency & resource lifecycle · verification confidence: **high** + +**What's wrong.** `_execute_interpretation_attempt` has no try/except around its setup: `get_interpretation_grounding(run_id)` (line 2014, raises KeyError if rows missing), `get_source_artifact`+`require_local_artifact_path` (lines 2046-2050, raises KeyError/FileNotFoundError), and `_resolve_interpretation_profile_config` (line 2063) all run before the Gemini call's own try/except (which is inside `_run_interpretation_request_with_profile_config`). `buildPrompt(...)` (server.py:1797) and `_genai.Client(...)` (1804) also sit before that inner try. Any exception from these propagates out of `_execute_interpretation_attempt`, through the worker's `await asyncio.to_thread(...)`, into `_interpretation_worker_loop`'s bare `except Exception` (server.py:2183) which only logs+sleeps. `fail_interpretation_attempt` is never called, so the attempt the worker reserved as 'running' stays 'running' indefinitely. There is no stale-running reaper; `recover_incomplete_attempts` (analysis_runtime.py:1433) only flips 'running'->'interrupted' at process startup, so the run shows perpetually in-progress until restart. Sibling functions `_execute_pitch_note_attempt` (server.py:1257-1260) and `_execute_mt3_attempt` (server.py:1418-1423) carry explicit comments saying they moved `get_source_artifact` inside the try precisely to avoid this; interpretation never got that treatment. + +**Trigger.** Interpretation reserved (status='running'), then `require_local_artifact_path` raises FileNotFoundError because the age-based artifact cleanup (utils/cleanup.py deletes any artifact with mtime > 24h, with no awareness of in-flight stages) swept the source audio while interpretation sat queued during a Gemini outage/backoff or was re-run on demand; or `get_interpretation_grounding` KeyErrors; or hosted-mode `resolve_local_path` returns None. + +**Fix.** Wrap the body of `_execute_interpretation_attempt` (from `get_interpretation_grounding` onward, including buildPrompt/Client construction paths) in a try/except that, on any exception, calls `runtime.fail_interpretation_attempt(attempt_id, error={...retryable...})` before returning — matching the defensive pattern already used in `_execute_pitch_note_attempt` and `_execute_mt3_attempt`. + +**Why it's real (verifier).** VERIFIED REAL. `_execute_interpretation_attempt` (server.py:2008) has no enclosing try/except around its setup: `get_interpretation_grounding` (2014), `get_source_artifact` (2046), `require_local_artifact_path` (2047), and the unguarded second `_resolve_interpretation_profile_config` (2063). The only try that terminalizes is inside `_run_interpretation_request_with_profile_config`, which begins at server.py:1815 — AFTER buildPrompt (1797) and `_genai.Client(...)` (1804), and after all the setup calls above. `_run_interpretation_request` (1606) wraps only the profile-config ValueError; it does nothing for the source-artifact calls that precede it at 2046-2047. + +Confirmed each cited call raises: get_source_artifact -> KeyError (analysis_runtime.py:491,497), require_local_artifact_path -> FileNotFoundError (540), get_interpretation_grounding -> KeyError (573). + +Confirmed the stuck-state mechanism end to end: reserve_next_interpretation_attempt sets status='running' (analysis_runtime.py:1262-1263) and never checks on-disk artifact presence; an escaping exception bubbles through `await asyncio.to_thread(_execute_interpretation_attempt, ...)` into _interpretation_worker_loop's bare `except Exception` (server.py:2183-2185) which only prints+sleeps — fail_interpretation_attempt is never called. The reserve query only re-picks status='queued' rows (1244), so a stuck 'running' row is never retried. recover_incomplete_attempts flips 'running'->'interrupted' ONLY at process startup (called server.py:329 and worker.py:14); grep found NO periodic stale-running reaper. Run shows perpetually in-progress until restart. + +Primary reachable trigger is `require_local_artifact_path` -> FileNotFoundError (540), not the grounding KeyError. reserve checks no disk state, and `cleanup_artifacts` (server.py:352 _artifact_cleanup_loop, while-True periodic) deletes any artifact with mtime older than 24h; _is_exempt (utils/cleanup.py:36-40) only spares `.keep` files and `preserved/` paths — zero in-flight-stage awareness — so source audio for an interpretation re-run/queued during a Gemini outage gets swept. Hosted-mode resolve_local_path -> None (538-539) is a second reachable trigger needing no delete-race. (The grounding KeyError is the weakest trigger: reserve JOINs measurement_outputs requiring mo.status='completed' at 1243-1245, so run/measurement rows existed at reserve time and would only vanish in a narrow delete-race.) + +Decisive corroboration: the two sibling executors carry explicit comments stating get_source_artifact was moved INSIDE the try precisely to avoid leaving the attempt stuck 'running' (pitch_note server.py:1257-1260, mt3 1418-1423), each with a try/except calling fail_*_attempt (1355-1371). And tests/test_server.py:5677 (test_source_artifact_failure_terminalizes_attempt) plus :5646 (mt3 timeout) assert exactly this terminalization for _execute_mt3_attempt — 'The key assertion: terminal, NOT stuck running.' There is no equivalent guard or test for _execute_interpretation_attempt; the authors hardened two of three siblings and left interpretation exposed. + +Severity P2 is right: liveness/stuck-state, recoverable on restart, not data loss, requires artifact-missing-at-execute-time rather than the happy path (above P3 given documented sibling precedent and reachable cleanup/hosted triggers; below P1 since it is not the common path and self-heals on restart). Proposed fix (wrap body from get_interpretation_grounding onward in try/except -> fail_interpretation_attempt) matches the established sibling pattern. + + +### 7. [P2] Measurement stage stuck 'running' forever if source artifact unresolvable before subprocess + +`apps/backend/server.py:1129` · Backend concurrency & resource lifecycle · verification confidence: **high** + +**What's wrong.** `_execute_measurement_run` calls `runtime.get_source_artifact(run_id)` (line 1129, raises KeyError) and `runtime.require_local_artifact_path(...)` (line 1130, raises FileNotFoundError) BEFORE the subprocess and outside any try/except. The only error handling for the reserved measurement job is in `_execute_reserved_measurement_job` (server.py:1193), whose try catches solely `UnsupportedPitchNoteModeError`; the subsequent `_execute_measurement_run(...)` call (line 1222) is not guarded. An exception from those two lines therefore propagates to `_measurement_worker_loop`'s bare `except Exception` (server.py:2145) which only logs+sleeps. `fail_measurement` is never called, so the row reserved as 'running' by `reserve_next_measurement_run` (it won't be re-picked) is wedged in 'running' until restart recovery. Because pitch/note and interpretation are gated on measurement completion, the entire downstream run is blocked. This is the same documented hazard the sibling stages explicitly guarded (server.py:1257-1260, 1418-1423) but measurement did not. + +**Trigger.** Measurement reserved as 'running', then `require_local_artifact_path` raises because the source artifact path doesn't resolve to an existing local file (hosted-mode `artifact_storage.resolve_local_path` returns None, or a DB/disk inconsistency), or `get_source_artifact` raises KeyError on a missing artifact row. + +**Fix.** Move the `get_source_artifact`/`require_local_artifact_path` calls in `_execute_measurement_run` inside a try/except (or wrap the `_execute_measurement_run(...)` invocation in `_execute_reserved_measurement_job`) that calls `runtime.fail_measurement(run_id, error={...})` on exception, mirroring the sibling stages so a missing/unresolvable source terminalizes the stage instead of wedging it. + +**Why it's real (verifier).** REAL, verified end-to-end in code. (1) `_execute_measurement_run` (server.py:1129-1133) calls `runtime.get_source_artifact(run_id)` (raises KeyError — analysis_runtime.py:491/497) and `runtime.require_local_artifact_path(...)` (raises FileNotFoundError when the local file is absent — analysis_runtime.py:539-540, since `resolve_local_path` returns `Path(storage_ref)` without an existence check, artifact_storage.py:91-94) OUTSIDE any try/except. (2) Its sole caller `_execute_reserved_measurement_job` (server.py:1186-1230) wraps only `resolve_measurement_flags` in a try that catches solely `UnsupportedPitchNoteModeError` (line 1197); the `_execute_measurement_run(...)` invocation at line 1222 is unguarded. (3) The exception escapes to `_measurement_worker_loop`'s bare `except Exception` (server.py:2145) which only logs+sleeps — `fail_measurement` is never called. (4) The row was flipped queued→running by `reserve_next_measurement_run` (analysis_runtime.py:766-767), whose selector is `WHERE mo.status='queued'` (line 757), so it is never re-picked → wedged in 'running'. (5) `recover_incomplete_attempts` (analysis_runtime.py:1433-1443) would flip it to 'interrupted' but runs ONLY at server/worker startup (server.py:329, worker.py:14), not periodically — so the wedge persists for the whole server uptime, exactly as the claim ("until restart recovery") states. (6) Downstream is gated on `mo.status='completed'`: pitch/note (analysis_runtime.py:912) and interpretation (line 1245), so the entire run is blocked. + +Reachability is a LIVE trigger, not hypothetical: the hourly `cleanup_artifacts` loop (server.py:355, interval 3600s, default ttl_hours=24) deletes any artifact file older than 24h by mtime with NO run-status check (utils/cleanup.py:52-75), removing only files (not DB rows). A measurement run backlogged past 24h loses its source file → `require_local_artifact_path`'s `.is_file()` check fails → FileNotFoundError → wedge. The FileNotFoundError path is the strong trigger; KeyError (get_source_artifact) needs genuine DB corruption since cleanup doesn't delete rows. + +Proof of intent + asymmetry: the two sibling stages explicitly moved these exact calls inside try/except with comments naming this precise failure (pitch/note server.py:1257-1265; MT3 server.py:1418-1428: "escape to _mt3_worker_loop stuck in 'running' ... block this run's interpretation indefinitely (until restart recovery)"), and each has a dedicated passing test `test_source_artifact_failure_terminalizes_attempt` (e.g. Mt3ExecutorTests at test_server.py:5677, which I ran: OK). The measurement stage has NEITHER the guard NOR the test — its only failure test (test_server.py:4340) covers PITCH_NOTE_MODE_UNSUPPORTED, the one path that IS caught; the other measurement test (line 4320) mocks out `_execute_measurement_run` entirely and never exercises the source path. Severity P2 (not P1/P3): a realistic scheduled-cleanup trigger, silent whole-run block (stage stuck 'running' with no surfaced error), recoverable only on restart — the same hazard class the maintainers rated worth guarding for the siblings. The proposed fix (wrap the calls / the `_execute_measurement_run` invocation in a try that calls `fail_measurement`) correctly mirrors the sibling pattern. + + +### 8. [P1→P2] truePeak rounded to 1 decimal silently drops inter-sample overs on the default analysis path + +`apps/backend/analyze_core.py:252` · DSP & numeric correctness · verification confidence: **high** + +**What's wrong.** analyze_true_peak returns round(true_peak, 1) where true_peak is a LINEAR amplitude (empirically verified: Essentia TruePeakDetector's 2nd output is the oversampled signal, so max() yields 0.5 for a 0.5-amplitude sine, 1.2 for an over). Rounding a near-full-scale linear amplitude to 1 decimal is catastrophically lossy: any true over in the range (1.0, 1.05] — i.e. 0 to +0.42 dBTP, the single most common over magnitude in mastered material — rounds to exactly 1.0. The frontend objective-loudness guardrail (apps/ui/src/services/loudnessGuardrails.ts:22,63) flags an over only when truePeak > TRUE_PEAK_OVER_LINEAR (1.0), so a real over of e.g. 1.04 becomes 1.0, is not > 1.0, and the MISSING_LOUDNESS_ACTION violation never fires — Phase 2 is then not required to recommend a limiter/ceiling. This is the default ('standard') path (analysis_runtime.py:290); the --fast path rounds to 6 decimals (analyze_fast.py:113) and is unaffected, so the two paths also disagree on precision for the same input. + +**Trigger.** Upload any mastered track whose oversampled true peak lands in (1.0, 1.05] (0 to +0.42 dBTP) via the default standard analysis run. Verified numerically: round(1.02,1)=round(1.04,1)=1.0, which is not > 1.0, so the over-detection guardrail is defeated; round(1.05,1)=1.1 fires correctly, so only the small-over band is silently lost. + +**Fix.** Round truePeak with enough precision to preserve sub-0.1 overs, matching the fast path: return {"truePeak": round(true_peak, 6)} (or at least 4 decimals). The frontend over threshold is 1.0 exact, so the measurement must retain resolution finer than 0.1 to be usable. + +**Why it's real (verifier).** The defect is real and reachable on the default path. Verified against actual code: + +1. truePeak is a LINEAR amplitude, not dBTP. Confirmed by JSON_SCHEMA.md:183 ("linear amplitude proxy (rounded)"), the fixture EXPECTED_TRUE_PEAK=1.0 for a full-scale signal (test_audio_fixture.py:109), and loudnessGuardrails.ts:6-10 docstring ("1.0 == 0 dBFS full scale; > 1.0 == inter-sample over"). So rounding a near-full-scale linear amplitude to 1 decimal is genuinely lossy: round(1.04,1)=1.0. + +2. analyze_core.py:252 returns round(true_peak, 1) on the standard path. Reachable: analysis_runtime.py:290 defaults analysis_mode to "standard"; the non-fast branch in analyze.py:1597 calls analyze_true_peak(stereo). The --fast path (analyze_fast.py:113) rounds to 6 decimals, so the two paths disagree on precision for the same input, as claimed. + +3. The frontend over-threshold is exactly 1.0: loudnessGuardrails.ts:22,63 fires TRUE_PEAK_OVER only when truePeak > TRUE_PEAK_OVER_LINEAR (1.0). round(1.04,1)=1.0 is not > 1.0, so the defect is dropped. + +4. The clipping detector does NOT cover the gap. analyze_core.py:1041,1049 computes clippedSampleCount from RAW (non-oversampled) sample peaks (np.abs(stereo).max(axis=1) >= 0.9999). An inter-sample over in (1.0, 1.05] is precisely the case where the oversampled true peak exceeds the highest discrete sample, so a true-peak-limited master (sample ceiling ~-0.3 dBFS, raw peak ~0.966) yields clippedSampleCount=0 AND truePeak rounded to 1.0 — both defects absent. validateLoudnessActionPresence (phase2Validator.ts:516) returns [] when defects.length===0, so MISSING_LOUDNESS_ACTION never fires. The two detectors are genuinely independent in this band; truePeak is the only line of defense and rounding defeats it. + +Refutation attempts all fail: not guarded by clipping detection (independent, raw-sample based), reachable on the default path, and not by-design (the schema and frontend treat truePeak as a meaningful over-detection signal with a 1.0 threshold finer than the 0.1 stored resolution). + +Severity downgraded P1 -> P2: the bite is a SUPPRESSED WARNING (phase2Validator.ts:548 severity:'WARNING') in a defense-in-depth net on a narrow input band (0 to +0.42 dBTP). The primary measurement is still emitted (just imprecise), clipping detection is intact, and Phase 2 still receives full audio and can recommend a limiter on its own judgment — no corrupted authoritative value and no confidently-wrong recommendation, just a missing nudge. The proposed fix (round to 6 decimals) is correct and trivial. + + +### 9. [P2] PLR computed by subtracting dB LUFS from LINEAR true-peak amplitude (unit mismatch) + +`apps/backend/analyze_core.py:633` · DSP & numeric correctness · verification confidence: **high** + +**What's wrong.** analyze_plr returns round(true_peak_value - lufs_value, 2) as the Peak-to-Loudness Ratio. PLR is a dB quantity defined as dBTP_peak - LUFS, but true_peak here is a LINEAR amplitude (~0.5-1.2, confirmed empirically) while lufsIntegrated is dBFS (~-9 to -14). Subtracting linear amplitude from dB is dimensionally invalid: it only approximates the real PLR when truePeak is near 1.0 (because 20*log10(1.0)=0). For a track peaking at -6 dBFS (truePeak~0.5) at LUFS -12, the code yields 0.5-(-12)=12.5 instead of the correct 20*log10(0.5)-(-12)=6.0 dB — off by ~6.5 dB. The frontend treats this value as dB and drives advice from it: mixDoctor.ts:249/253 prints 'PLR ${plr} dB is below/exceeds target' and penalizes dynamics, so a genuinely dynamic track is mislabeled over-limited (or vice versa), inverting the recommendation. The bug is mirrored in the frontend fallback estimatePlr (mixDoctor.ts:118), so it is consistent but consistently wrong. + +**Trigger.** Analyze any track whose true peak is meaningfully below 0 dBFS (e.g. an unmastered/dynamic mix peaking at -6 dBFS, truePeak~0.5). PLR is reported ~6 dB too high, pushing mixDoctor's dynamics scoring and 'add bus glue/saturation' vs 'ease limiter' advice the wrong way. + +**Fix.** Convert true peak to dBTP before the subtraction: plr = 20*log10(true_peak_value) - lufs_value (guarding true_peak_value > 0). Apply the identical change to estimatePlr in apps/ui/src/services/mixDoctor.ts so the fallback stays byte-identical, and update the JSON_SCHEMA.md plr description. + +**Why it's real (verifier).** REAL. analyze_plr (apps/backend/analyze_core.py:633) returns round(true_peak_value - lufs_value, 2). The arguments are passed straight through at analyze.py:1631 (full) and analyze.py:1470 (fast) as result.get("truePeak") and result.get("lufsIntegrated") with no dB conversion between. + +Unit mismatch is incontrovertible: (a) In fast mode, truePeak = np.max(np.abs(stereo)) (analyze_fast.py:112) is by definition a LINEAR amplitude; (b) JSON_SCHEMA.md:183 explicitly documents truePeak as "linear amplitude proxy (rounded)", while lufsIntegrated is LUFS/dB (JSON_SCHEMA.md:185-186); (c) the full path uses Essentia es.TruePeakDetector() whose linear output is confirmed by the golden fixture tests/fixtures/golden/phase1_default.json: truePeak=1.0, lufsIntegrated=-5.6, plr=6.6 — i.e. 1.0-(-5.6)=6.6. A full-scale master reads truePeak=1.0 (linear), not 0 dBTP, and the two paths agree on units, killing the "Essentia returns dBTP" escape. The correct dB PLR at truePeak=1.0 would be 20*log10(1.0)-(-5.6)=5.6 — off by exactly 1.0 dB even on this normalized track. The error is systematic and one-directional: bias = truePeak_linear - 20*log10(truePeak_linear) is >= 1.0 dB for every truePeak <= 1.0, always inflating PLR. + +Reachable and consumed as dB: MeasurementDashboard.tsx:1221 calls generateMixDoctorReport(phase1), which at mixDoctor.ts:245-254 compares plr against profile.targetPlrRange (genuine dB ranges in genreProfiles.ts, e.g. [6,10], [14,24]) and renders user-facing advice "PLR ${plr} dB is below/exceeds ... ease limiter" vs "add gentle bus glue or saturation". The inflated PLR systematically pushes the dynamics verdict toward "too-dynamic", inverting the bus-glue/limiter recommendation for unmastered/dynamic tracks (worst case ~+6.5 dB at truePeak~0.5). The frontend fallback estimatePlr (mixDoctor.ts:118) replicates the same wrong subtraction; the audit comment at mixDoctor.ts:102-111 only blesses byte-identical CONSISTENCY with the backend, never dimensional correctness, so it confirms rather than refutes the defect. + +P2 is right: systematic, user-facing advisory corruption that can invert a dynamics recommendation (undermining the measurement-grounded thesis and also leaking into the Phase 2/Gemini payload since plr is a top-level field), but it lives in the client mixDoctor heuristic, not Phase 1 ground truth, and is not a crash/data-loss/security issue. Proposed fix (20*log10(true_peak) - lufs, guarding true_peak>0, mirrored in estimatePlr, plus a JSON_SCHEMA plr note) is correct. + + +### 10. [P2] Retry handlers have try/finally with no catch: backend failure on retry is swallowed and escapes as an unhandled promise rejection + +`apps/ui/src/App.tsx:787` · Frontend async / polling / cleanup · verification confidence: **high** + +**What's wrong.** handleRetryPitchNoteExtraction (App.tsx:777-843) and handleRetryInterpretation (App.tsx:845-912) are `async` and structured `try { await createPitchNoteTranslationAttempt(...)/createInterpretationAttempt(...); await monitorAnalysisRun(...) } finally { setIsAnalyzing(false); ... }` with NO catch block. monitorAnalysisRun never rethrows (its own catch calls onError and returns — analyzer.ts:357-359), so the only throw source in the try is the awaited create* call, which rejects with a BackendClientError whenever the backend is down or returns a non-ok status (fetchJson throws on !response.ok, httpClient.ts:39-51). When that rejects: (1) the finally resets isAnalyzing so the spinner vanishes, but nothing calls setError — the user gets no error feedback, the retry silently no-ops; (2) the rejected promise returned by the async handler is invoked directly as an onClick/onRetry callback (AnalysisStatusPanel props are typed `() => void`, wired at App.tsx:1363-1364 and onClick at AnalysisStatusPanel.tsx:323-335), so nobody awaits or .catch()es it → it becomes an unhandled promise rejection. handleStartAnalysis (App.tsx:745-748) has exactly the catch+setError these two lack, confirming the omission is a bug rather than intentional. + +**Trigger.** Run an analysis where pitch/note or interpretation fails, then click 'Retry' on AnalysisStatusPanel while the backend is unreachable or returns 500. createPitchNoteTranslationAttempt/createInterpretationAttempt rejects: the spinner clears with no error shown, and an 'Uncaught (in promise) BackendClientError' is logged. + +**Fix.** Add a `catch (rawError)` to both retry handlers mirroring handleStartAnalysis (lines 745-748): map to an Error, call setError(err.message) and setErrorRetryable(err instanceof BackendClientError && err.details?.retryable === true), skipping USER_CANCELLED. This both surfaces the failure and consumes the rejection. + +**Why it's real (verifier).** Verified every link in the claimed chain against the actual code: + +1. handleRetryPitchNoteExtraction (App.tsx:787-842) and handleRetryInterpretation (App.tsx:856-911) are both `async` with `try { await create*Attempt(...); await monitorAnalysisRun(...) } finally { setIsAnalyzing(false); abortControllerRef=null; setElapsedMs(0); }` and NO catch. Confirmed by reading both bodies in full. + +2. JS semantics: try/finally with no catch does not swallow — the finally runs, then the rejection propagates out of the async function as a rejected promise. + +3. The only throw source reaching that try is the awaited create*Attempt call. createPitchNoteTranslationAttempt (analysisRunsClient.ts:168-186) and createInterpretationAttempt (213-231) both `await fetchJson(...)`, which throws BackendClientError on network failure (httpClient.ts:12-19), non-JSON (28-37), and `!response.ok` (39-51). monitorAnalysisRun does NOT rethrow: its entire body is wrapped in `try {...} catch (error) { onError(mapBackendError(error)); }` (analyzer.ts:259/357-359), and onError is a setError closure (signature analyzer.ts:240) that returns normally. + +4. The finally only resets isAnalyzing/refs/elapsed; it never calls setError — confirmed at App.tsx:838-841 and 907-910. Contrast handleStartAnalysis (App.tsx:745-748) which has the correct `catch (rawError) { setError(err.message); setErrorRetryable(...) }`, confirming the omission is a bug, not intent. Both retry handlers share the omission, so a fix touches both. + +5. The rejected promise is unhandled: AnalysisStatusPanel retry props are typed `() => void` (lines 26-29), wired at App.tsx:1363-1364, and invoked as `onClick={stage.onRetry}` (AnalysisStatusPanel.tsx:364). React discards the returned promise. Confirmed NO global unhandledrejection handler exists anywhere in apps/ui/src (grep returned NO_GLOBAL_HANDLER), so the rejection surfaces as "Uncaught (in promise) BackendClientError". + +Reachability: the retry button only renders when the stage status is failed/interrupted (App.tsx:1363-1364) and error.retryable !== false (AnalysisStatusPanel.tsx:340-344). So the trigger requires a prior stage failure, then a click while the backend is down or returns 500 at retry time — a real but second-order error-recovery edge path. + +Severity nuance: the harm is narrower than "silent no-op with no feedback." Because the rejected POST never calls setAnalysisRun, analysisRun still holds the prior snapshot showing the failed stage and the retry button, so the user retains implicit "still failed, try again" feedback. What is actually missing is the explicit error banner (the setError path) plus an unhandled console rejection. No crash, no data loss (an unhandled rejection does not crash a React app). This sits on the P2/P3 boundary; P2 is defensible weighting the unhandled-rejection + missing-explicit-banner combination on a user-facing retry control. + + +### 11. [P2] normalizeKey self-corrupts "major" to "majoror", so a valid "Maj" abbreviation triggers a false key-contradiction ERROR + +`apps/ui/src/services/phase2Validator.ts:741` · Chain-of-custody validation logic · verification confidence: **high** + +**What's wrong.** normalizeKey() runs `.replace(/maj/g, 'major')` with a bare (unguarded) pattern. Because the string "major" already contains the substring "maj", this turns "major" into "majoror" (verified by execution: normalizeKey('C Major') === 'c majoror'). The minor branch correctly guards itself with `/min(?!or)/g`, but the major branch does not. Phase 1 emits keys as `"{key} {scale.capitalize()}"` e.g. "C Major" (analyze_core.py:139), which normalizes to 'c majoror'. The extraction regex extractKeyFromPhase2 (line 699) explicitly accepts the abbreviated quality forms 'maj'/'Maj'. A Phase 2 mention of "C Maj" normalizes to 'c major' (the /maj/ match expands the abbreviation correctly), which does NOT equal Phase 1's corrupted 'c majoror'. validateKeyConsistency then emits a NUMERIC_OVERRIDE ERROR claiming Phase 2 'contradicts' Phase 1 even though "C Maj" and "C Major" are the same key. The whole purpose of this validator is to flag genuine contradictions; here it raises a user-visible chain-of-custody ERROR on a non-contradiction, and the corruption is silent. + +**Trigger.** Phase 1 key = "C Major" (or any major key); Gemini emits the abbreviated major form, e.g. trackCharacter/styleProfile.description containing "C Maj" (a form the extraction regex explicitly accepts). validateKeyConsistency raises a spurious 'key contradiction' ERROR. + +**Fix.** Mirror the working minor branch: change `.replace(/maj/g, 'major')` to `.replace(/maj(?!or)/g, 'major')`. The two preceding no-op lines (`/major/g -> 'major'`, `/minor/g -> 'minor'`) can be dropped. After this, "C Major" and "C Maj" both normalize to 'c major' and match, while a genuinely different key (e.g. "D Maj" vs "C# Major") still mismatches and errors correctly. Add a unit test asserting normalizeKey('C Major') === normalizeKey('C Maj'). + +**Why it's real (verifier).** Verified by reading code and executing the exact regex chain. phase2Validator.ts:741-750 normalizeKey() runs `.replace(/maj/g,'major')` unguarded (line 747), while the minor branch is guarded `.replace(/min(?!or)/g,'minor')` (line 748). Execution confirms: normalizeKey('C Major') -> 'c majoror' (the literal 'maj' inside 'major' is re-expanded), normalizeKey('C Maj') -> 'c major'; the two do NOT match. Phase 1 emits major keys as 'C Major' (analyze_core.py:138, f"{key} {scale.capitalize()}"). extractKeyFromPhase2 (line 699) explicitly lists maj/Maj as accepted forms, and its regex captures 'C Maj' from a Gemini sentence (verified). I ran the full validateKeyConsistency path end-to-end: Phase1='C Major' + Phase2 mentioning 'C Maj' (the SAME key) produces a NUMERIC_OVERRIDE ERROR ('contradicts Phase 1 key'), whereas a genuine contradiction (D Maj vs C Major) still errors correctly. That ERROR is user-visible — Phase2ConsistencyReport.tsx:27/85 renders ERROR severity with text-error and counts userErrorCount, mounted on AnalysisResults.tsx — so this raises a spurious chain-of-custody ERROR on a non-contradiction, violating the validator's purpose (PURPOSE.md invariant #2). The existing key-validation tests (phase2Validator.test.ts:177,197) pass (ran them: 4 passed) but never exercise the major-abbreviation-vs-full case: the pass-test uses guarded minor ('F minor' both sides), the error-test uses different note letters ('F minor' vs 'A major') so corruption is irrelevant. Thus the bug is latent and uncaught. The asymmetric guard (min guarded, maj not) plus the two no-op lines 745-746 confirm an authoring oversight, not by-design. Severity P2 (not P1): the trigger is conditional on Gemini choosing the abbreviated major form 'C Maj' over the more-common full 'C Major' (if Gemini writes the full form, both sides corrupt identically to 'c majoror' and match — no false positive), and it produces a false ERROR rather than suppressing a real contradiction or crashing. The proposed fix `/maj(?!or)/g` mirrors the working minor branch and is correct. + + +### 12. [P3] Cache-eviction loop references undefined _FILE_CACHE_LOCK/_FILE_CACHE_TTL_SECONDS (latent NameError) + +`apps/backend/server.py:254` · Backend concurrency & resource lifecycle · verification confidence: **high** + +**What's wrong.** `_cache_temp_file` (line 254-256), `_pop_cached_temp_file` (262), and `_evict_expired_cache_entries` (275) reference `_FILE_CACHE_TTL_SECONDS` and `_FILE_CACHE_LOCK`, but neither symbol is defined or imported anywhere in apps/backend (grep across all backend .py files finds only these use-sites). `_evict_expired_cache_entries` is invoked by `_evict_loop` (line 285). In production `_evict_loop` is never scheduled (`include_cache_eviction=False` at both server.py:335 and worker.py:17), so it is dead today, but the legacy `_start_cache_eviction` hook (server.py:312) still starts it and these three helpers read as production-intended temp-file-cache plumbing. If `_evict_loop` is ever enabled or any of these helpers is wired onto a request path, the first call raises NameError. + +**Trigger.** Call `_start_cache_eviction()` (or set `include_cache_eviction=True`) so `_evict_loop` runs; after 300s it invokes `_evict_expired_cache_entries`, which raises NameError on `_FILE_CACHE_LOCK`. Likewise any caller of `_cache_temp_file`/`_pop_cached_temp_file`. + +**Fix.** Either define the missing module-level constants (`_FILE_CACHE_LOCK = threading.Lock()`, `_FILE_CACHE_TTL_SECONDS = `) next to `_TEMP_FILE_REGISTRY`, or delete the three dead helpers and the `_evict_loop`/`_start_cache_eviction` machinery (and the test that exercises it) since the temp-file registry they manage is unused on the product path. + +**Why it's real (verifier).** CONFIRMED real, latent NameError. Empirically verified at apps/backend/server.py:251-285. (1) Symbols genuinely undefined: grep across all backend .py finds `_FILE_CACHE_TTL_SECONDS`/`_FILE_CACHE_LOCK` ONLY at the four use-sites (server.py:254,255,262,275); they are never assigned or imported. Loading the module confirms hasattr(server,'_FILE_CACHE_LOCK')==False and hasattr(server,'_FILE_CACHE_TTL_SECONDS')==False, while _TEMP_FILE_REGISTRY==True — the omission is specific, not a misread. (2) No escape hatch: server.py has no `import *` and no globals()[...] dynamic injection, and the import lists are all explicit named imports. The sibling helpers the claim cites as defined (_current_time def@375, _cleanup_temp_path imported@63) do exist, so only these two constants are missing. (3) NameError proven to fire: calling server._evict_expired_cache_entries() directly raises `NameError: name '_FILE_CACHE_LOCK' is not defined`. (4) Latency confirmed: `python -m py_compile server.py` succeeds because the undefined names live in function bodies, never run at import — so the server boots fine and the bug is dormant. (5) Reachability matches the claim exactly: _cache_temp_file/_pop_cached_temp_file have ZERO callers (pure dead code); _evict_loop runs only via include_cache_eviction=True (hard-coded False at the two production sites server.py:335 and worker.py:17) or via the legacy _start_cache_eviction hook (server.py:312). The only thing that touches _start_cache_eviction is tests/test_cleanup.py:196, which patches asyncio.create_task to immediately coro.close() the _evict_loop coroutine (lines 183-186) and asserts only co_name=='_evict_loop' (line 200) — the loop body never steps, so the NameError never fires; tests.test_cleanup passes (6 tests OK). Verdict: a genuine guaranteed-NameError defect on undefined module symbols, but unreachable on the product path today. P3 is correct — low-severity latent/dead-code-adjacent bug. The proposed fix (define the two constants next to _TEMP_FILE_REGISTRY, or delete the dead helpers + _evict_loop/_start_cache_eviction machinery and its test) is appropriate. + + +### 13. [P3] SSRF guard leaks resolved internal IP back to the caller (DNS/network-mapping oracle) + +`apps/backend/url_ingest.py:305` · Security · verification confidence: **high** + +**What's wrong.** When _assert_host_is_public rejects a host, UrlBlockedPrivateHostError is raised with the message f"Hostname resolves to non-public address '{ip}'..." embedding the exact resolved private/internal IP. server.py:_url_ingest_error_response (line 686) returns that message verbatim in the client-facing error envelope. This turns the SSRF guard into an internal-DNS oracle: an attacker can enumerate how internal hostnames resolve (e.g. confirm db.internal -> 10.x, or map a target's private subnet) purely from the 400 response body, even though the fetch is correctly blocked. + +**Trigger.** POST /api/analysis-runs with url=http://internal-service.corp.example/a.flac that resolves to an RFC1918 address; the 400 URL_BLOCKED_PRIVATE_HOST body reveals the resolved 10.x/172.16.x/192.168.x address. + +**Fix.** Make the client-facing message generic ('Hostname resolves to a non-public address; only public hosts are accepted.') and log the specific IP only server-side. The module's own docstring already warns 'do not embed... internal hostnames' (lines 92-93, 120-121) — honor it here. + +**Why it's real (verifier).** Confirmed reachable end-to-end. apps/backend/url_ingest.py:305-308 raises UrlBlockedPrivateHostError with message f"Hostname resolves to non-public address '{ip}'..." where `ip` (line 297) is the actual resolved private/internal address (ipaddress.ip_address of sockaddr[0] from socket.getaddrinfo). server.py:691 sets the client-facing envelope to "message": str(exc) with no sanitization between raise and response, and server.py:2394-2395 routes UrlIngestionError to that handler. The trigger is a real public param: url: str | None = Form(None) at server.py:2276 on POST /api/analysis-runs; server.py:646 calls fetch_url_to_bytes(url) → _assert_host_is_public(parsed.hostname) (url_ingest.py:159) before any HTTP request, so a URL whose host resolves to RFC1918 yields a 400 body containing the exact 10.x/172.16.x/192.168.x address. + +Not by design / not guarded elsewhere: the module's own base-class docstring (url_ingest.py:90-93) states the message "is safe to surface to the client; do not embed secrets or internal hostnames" — line 306 violates that stated contract. The existing contract test (tests/test_server.py:4696-4720) asserts only on status_code (400) and error.code (URL_BLOCKED_PRIVATE_HOST); it does NOT assert the IP appears in the message (its mock string "10.0.0.5 is private" is incidental), so nothing pins this leak as intended. + +Rebutting the obvious refutation ("attacker supplied the hostname, so they could resolve it themselves"): the SSRF threat model is split-horizon/internal-only DNS — the server resolves names like db.internal to private IPs that an external attacker cannot map from their own vantage. The server is acting as a resolver oracle for names the attacker can't otherwise enumerate. A coarse yes/no "resolves-to-private" bit already leaks via the distinct error code (URL_BLOCKED_PRIVATE_HOST 400 vs URL_FETCH_FAILED 502 from gaierror at lines 282-285); the marginal leak from line 306 is the specific IP value, enabling subnet/host mapping beyond that bit — exactly what the proposed generic-message fix closes. + +Severity correctly P3: the fetch is correctly blocked (allow_redirects=False, pre-fetch SSRF check), nothing is exfiltrated from the internal host, and exploitation requires the attacker to already be probing internal names. It is reconnaissance-aiding information disclosure, not a fetch bypass — should not be inflated to P2 despite "SSRF" in the title. + + +### 14. [P2→P3] extractBPMFromText returns only the FIRST BPM match, silently dropping a later contradicting mention + +`apps/ui/src/services/phase2Validator.ts:649` · Chain-of-custody validation logic · verification confidence: **high** + +**What's wrong.** extractBPMFromText collects all regex matches via matchAll but returns only `parseFloat(matches[0][1])` (the comment even says 'Return the first BPM found'). Each Phase 2 text field therefore contributes at most one BPM mention to validateBPMConsistency. If a single field states the correct tempo first and then a contradicting one (e.g. "This 126 BPM track modulates to feel like 140 BPM in the bridge"), only 126 is checked; 126 agrees with Phase 1 so no violation fires, and the 140 contradiction is never seen — a silent chain-of-custody false-negative. This is asymmetric with the key path: extractKeyFromPhase2 (line 703) iterates `for (const match of matches)` and pushes EVERY match, so multi-mention key contradictions are caught. BPM is the only one that drops trailing mentions. + +**Trigger.** Gemini emits two BPM figures in one text field (trackCharacter, sonicElements.kick/grooveAndTiming, styleProfile.description/generationPrompt) where the first matches Phase 1 and a later one contradicts it by > 2 BPM. The contradiction passes validation undetected. + +**Fix.** Return all matches from extractBPMFromText (e.g. `return matches.map(m => parseFloat(m[1]))`) and have extractBPMFromPhase2 push one mention per value, or change extractBPMFromPhase2 to iterate all matches per field exactly as extractKeyFromPhase2 already does. Then validateBPMConsistency checks every mention against the tolerance. + +**Why it's real (verifier).** The defect exists exactly as described. extractBPMFromText (apps/ui/src/services/phase2Validator.ts:642-653) collects all regex matches via `matchAll` but returns only `parseFloat(matches[0][1])` (line 649; the comment says "Return the first BPM found"). extractBPMFromPhase2 (lines 589-636) therefore contributes at most one BPM mention per scanned field, so validateBPMConsistency (lines 562-584) never sees a trailing contradicting BPM within a single field. The asymmetry with the key path is genuine: extractKeyFromPhase2 (lines 695-736) iterates `for (const match of matches)` (line 704) and pushes every match, so a "extract the primary BPM by design" defense does not hold. I empirically traced the claim's example string "This 126 BPM track modulates to feel like 140 BPM in the bridge" through the actual regex: 2 matches [126, 140]; the function returns 126 (diff 0 from Phase 1=126, no violation), and the trailing 140 (diff 14, well over BPM_TOLERANCE=2.0 at line 48) is silently dropped. The path is live (validateBPMConsistency → validatePhase2Consistency → Phase2ConsistencyReport.tsx) and nothing else re-checks numeric BPM — the backend mirror _validate_phase2_citation_paths only validates citation-path *existence*, not values. + +Severity corrected to P3, not P2, for three reasons: (1) This is a defense-in-depth *surfacing* validator rendered hideWhenClean, not an enforcement gate — Phase 1 stays authoritative in the payload regardless, so invariant #1 is not violated at the data level; the only cost is one class of prose contradiction going unwarned. (2) The scanner is already a known-incomplete heuristic (only 5 fields: trackCharacter, sonicElements.kick/grooveAndTiming, styleProfile.description/generationPrompt; does not scan confidenceNotes; simple `\d+ BPM` regex that misses "tempo of 140" etc.), so "first match per field" is one more hole in a porous net rather than a regression from a complete check. (3) The proposed fix (check all mentions) carries its own real downside: prompts/phase2_system.txt line 143 explicitly invites legitimate multi-tempo prose ("ritardando/accelerando, DJ-tool tempo blends, any tempo-modulated section"), so a trailing second BPM is frequently a legitimate modulation description, not a contradiction — checking all mentions would raise spurious ERROR-level warnings on exactly that prose. The trigger also requires a specific conjunction: two BPMs in the same scanned field, agreeing-first, contradicting-second, >2 BPM apart. (Note: prompt lines 442-444 mandate double/half-time discussion in confidenceNotes, but extractBPMFromPhase2 does not scan confidenceNotes, so that mandate does not feed this bug and is excluded from the justification.) Baseline tests are green (npx vitest tests/services/phase2Validator.test.ts -t "BPM"); the closest existing test at line 153 puts the contradicting value in a separate field from the agreeing one, so it does not exercise the same-field drop. + + +--- + +## 3. Architecture & structural findings + +### Module boundaries & god-objects + +- **[high/L] AnalysisResults.tsx is a 1,938-line single-component god-object; its section model already names the split seams** + - _Evidence:_ apps/ui/src/components/AnalysisResults.tsx: the AnalysisResults function spans lines 626–2564 (~1,938 LOC) with 16 props and 6 hoisted useState maps (openArrangement/openSonic/openMix/openPatch/showSources/appliedIds, lines 644–657). The derived view-models are ALREADY extracted and unit-tested in analysisResultsViewModel.ts (buildArrangementViewModel L375, buildSonicElementCards L602, buildMixChainGroups L970, buildPatchCards L1146; covered by tests/services/analysisResultsViewModel.test.ts). What remains inline is ~1,700 lines of JSX rendering (~401 tag-bearing lines in the 870–2564 return body) plus 5 inline helper components (AppliedCheckbox L203, Collapsible L236, SourcesToggle L248, MetaBadgeList L404, ResultsSectionHeader L346). The navEntries array (L819–863) is an explicit 1:1 section model — Style/Setup/Layout/Routing/Warp/Audio/Arrangement/Session/MT3/Stem Notes/Sonic/Mix Chain/Patches/Measurements — and several of those sections are already delegated to child components (SessionMusicianPanel L1916, Mt3TranscriptionPanel L1936, MeasurementDashboard L2547), proving the pattern. Testability gap is the same root cause: the only unit-tested surface of this file is 3 tiny exported pure helpers (getInterpretationSubtitle, getPhase2NavDisabledReason, toggleOpenKeySet — tests/services/{interpretationSubtitle,phase2NavReason,analysisResultsUi}.test.ts); the 1,700-line render body is untestable because Vitest runs node-only (no jsdom, CLAUDE.md tripwire #5), so every Style/Sonic/Mix/Patch rendering branch is uncovered. + - _Recommendation:_ Extract one section component per navEntries entry that is still rendered inline (StyleProfileSection, SonicElementsSection, MixChainSection, PatchesSection, ArrangementSection, plus the smaller Setup/Layout/Routing/Warp/Audio blocks), co-located under components/analysisResults/. Critically, pair extraction with relocating each section's open-state: today openSonic/openMix/openPatch/openArrangement/showSources are hoisted into the parent, so a naive extraction drills ~10 props per child. Instead follow the pattern the already-extracted children use — they take raw phase1/phase2 + narrow scalars (apiBaseUrl, runId) and NONE receive the hoisted maps (verified at L1916, L2547) — and move each section's collapse state into its own component (or a tiny AnalysisResultsContext). AnalysisResults then becomes a ~250-line layout shell that maps view-models to sections. This unblocks per-section unit tests on the extracted projection-to-render boundary without fighting the existing builder/view-model split. +- **[high/L] server.py fuses a ~2,000-line stage-execution engine with a ~1,700-line HTTP route layer** + - _Evidence:_ apps/backend/server.py is 3,965 LOC. Lines ~180–2213 are a stage-execution engine with no FastAPI route decorators: the child-process registry (_register/_unregister/_terminate/_interrupt_active_child_processes L180–227), subprocess streaming (_run_streamed_subprocess L472, _run_measurement_subprocess L753 = 313 LOC), the stage handlers (_execute_measurement_run L1119, _execute_reserved_measurement_job L1186, _execute_pitch_note_attempt L1233, _execute_mt3_attempt L1389 = 217 LOC, _run_interpretation_request_with_profile_config L1747 = 261 LOC, _execute_interpretation_attempt L2008), and the four worker loops (_measurement/_pitch_note/_interpretation/_mt3_worker_loop L2131–2213). Lines ~2273–3965 are the FastAPI route layer (create_analysis_run L2274, get/delete/interrupt, artifacts, samples, csv, pianoroll, spectral-enhancements, the *-transcriptions/pitch-note/interpretations creators, plus the legacy /api/analyze + /api/phase2 wrappers L3607–3965). The only coupling from engine to app is the startup hook wiring the worker loops (app.on_event('startup'), _create_background_tasks L288). This grain already exists conceptually — worker.py is the documented hosted worker-process entry point (runtime_profile.py / CLAUDE.md hosted seam). + - _Recommendation:_ Extract the engine cluster into a stage_execution.py module (child-process registry + _run_*/_execute_* stage handlers + the worker-loop bodies), leaving server.py as router composition + lifecycle wiring. This is with-grain: it mirrors the existing analyze.py→analyze_*.py and server.py→server_phase1/2/upload/samples splits, and gives worker.py a clean module to import instead of reaching into server.py. The four worker loops (L2131–2213) are near-identical (reserve→to_thread(execute)→idle→catch) differing only in the reserve method and execute handler — collapse them into one parameterized loop driven by a small (reserve_fn, execute_fn, label) table to remove the duplication while you move them. +- **[medium/M] server.py re-exports ~90 symbols from server_* modules purely for test back-compat — the module split is real but the test contract still points at server.py** + - _Evidence:_ apps/backend/server.py L55–148: three import blocks each tagged '# noqa: F401 — re-exported for test backward compat' pull ~90 symbols back from server_upload, server_phase1, and server_phase2 (e.g. _build_phase1, _normalize_run_snapshot, _parse_phase2_result, _collect_phase2_shape_issues, _get_audio_mime_type). Tests consume them through the server namespace, not the owning module: tests/test_server.py references server._build_phase1 (46×), server._parse_phase2_result family (22×), etc. (confirmed by `grep -ohE 'server\.[a-zA-Z_]+' tests/test_server*.py | sort | uniq -c`). So the domain modules exist and are imported correctly by production code (no circular import back to server.py — verified), but the test boundary lags: the symbols can't be removed from server.py until tests repoint. A spot check also found many re-exported symbols with ZERO test consumers (LEGACY_ENDPOINT_SUNSET, _MultipartTrackSizeCounter, _scope_header_value, _normalize_run_snapshot, _build_measurement_provenance, _round_timing_value, _format_timing_summary_value, _build_descriptor_hooks, _coerce_enum_fields, _normalize_and_salvage_phase2_result, apply_live12_catalogue_gates — 0 hits each) — pure dead re-export surface. + - _Recommendation:_ Sequence, don't big-bang: (1) repoint test imports to the owning modules (from server_phase1 import _build_phase1, etc.), then (2) delete the re-export blocks. The zero-consumer symbols above are a free immediate deletion from the re-export lists with no test change. This makes the already-real module split visible at the test boundary so future readers stop treating server.py as the home of these functions. Not high-severity — production layering is already correct; this is test-coupling hygiene. +- **[medium/M] server_phase2.py mixes HTTP/Gemini orchestration with a 350-line hand-rolled schema validator, ~70 type-guard predicates, and LLM grammar post-processing** + - _Evidence:_ apps/backend/server_phase2.py is 2,826 LOC spanning four unrelated responsibilities: (a) HTTP/Gemini orchestration + prompt building (_build_phase2_prompt L791, _build_stem_summary_prompt L831, _resolve_interpretation_profile_config L942); (b) a complete hand-written structural validator — _collect_phase2_shape_issues L1947 is 351 LOC, supported by ~70 _is_* predicates (L1097–1435: _is_project_setup, _is_track_grounding, _is_ableton_recommendation_item, _is_valid_phase2_shape, …) and the catalog validator _load_live12_device_catalog L122; (c) salvage/normalization (_normalize_and_salvage_phase2_result L1796, _sanitize_optional_phase2_fields L1458, _repair_return_track_context L1706); (d) natural-language grammar post-processing on LLM output (_to_gerund L1018, _fix_by_gerund_in_text L1038, _fix_grammar_in_record L1057, _apply_phase2_grammar_fixes L1072). The grammar fixers are the sharpest cohesion smell — gerund-correcting prose has no business living in an HTTP router module. + - _Recommendation:_ Extract the validation/normalization core into phase2_schema.py: the ~70 _is_* predicates + _collect_phase2_shape_issues + _normalize_and_salvage_phase2_result + the grammar fixers. server_phase2.py keeps Gemini transport, prompt assembly, and route-handler glue. This is with-grain (same domain-module pattern as the rest of apps/backend) and gives the validator its own testable home; tests currently reach these through server.* re-exports (see the re-export finding), so repoint them to phase2_schema at the same time. +- **[low/S] Backend Phase 2 citation check intentionally mirrors the frontend validator — drift risk on a documented mirror, not duplication to merge** + - _Evidence:_ apps/backend/server_phase2.py _validate_phase2_citation_paths L2666 re-implements the citation-existence half of apps/ui/src/services/phase2Validator.ts (validatePhase1FieldCitations). This is DELIBERATE and documented: CLAUDE.md tripwire #8 and apps/backend/ARCHITECTURE.md 'Phase 2 citation-path verification' both state the backend mirror is API-consumer defense-in-depth so a non-browser client can't silently accept invented citations. Note the scope is narrow: the backend mirrors only the citation-existence check (WARNING-only on validationWarnings), NOT the full consistency validator (validatePhase2Consistency at phase2Validator.ts L221, which also does BPM/key/LUFS/genre/numeric-bounds checks). A parallel intentional mirror exists for MIME resolution: audio_mime.py CANONICAL_AUDIO_MIME_BY_EXT ↔ audioFile.ts AUDIO_EXTENSION_MIME_TYPES (tripwire #9). + - _Recommendation:_ Do NOT consolidate — the duplication is an intended cross-runtime safety boundary. Instead reduce drift risk: add a shared contract fixture (a JSON list of the canonical Phase 1 citable field paths, and the MIME-by-extension map) checked by a parity test on both sides, so the two implementations can't silently diverge. This respects the documented seam while closing the only real risk (the two copies drifting). Low severity: both mirrors are small, documented, and currently consistent. + +### Layer integrity (3-layer thesis) + +- **[high/M] Citation contract validates path EXISTENCE, not RELEVANCE — a recommendation can cite any unrelated-but-present measurement and pass both validators** + - _Evidence:_ Both the backend (`_collect_measurement_field_paths`/`_walk_measurement_paths`, server_phase2.py:2596-2755) and the frontend (`collectPhase1FieldPaths`/`walkForPaths`, phase2Validator.ts:331-370) build an allow-set of *every* dotted path in the measurement payload — including intermediate container keys (`spectralBalance`, `kickDetail`, `bpm`). A citation is accepted iff `normalized in allowed` (server_phase2.py:2651) / `allowed.has(normalized)` (phase2Validator.ts:465). Nothing links the cited path to the recommendation's actual claim: a reverb-decay card can cite `bpm`, or a kick EQ card can cite `stereoDetail`, and both pass `UNRESOLVED_CITATION_PATH`/`MISSING_CITATION`. This is the exact 'fake a citation that passes validation' vector. PURPOSE.md invariant #2 ('every recommendation must cite the *specific* measurement(s) that justify it') is therefore only partially machine-checked — presence is checked, justification is not. The diversity heuristic (`validateCitationDiversity`, threshold 0.6) and `validateNewFieldCoverage` catch *gross* gaming (same anchor on >60% of cards) but not per-card mismatch. + - _Recommendation:_ Add a relevance layer rather than relying on existence alone. Pragmatic first step: a category->expected-measurement-family map (e.g. category DYNAMICS expects citations under {crestFactor, saturationDetail, lufs*, dynamic*}; STEREO expects stereoDetail.*; EQ expects spectral*/kickDetail/bassDetail) emitting a WARNING-grade `CITATION_OFF_DOMAIN` when a card cites zero paths in its plausible family. This reuses the existing `infer_domain`/category machinery already present for the recommendation scorer. Keep it WARNING-only to respect invariant #1 (never reject). It closes the largest believability gap in the chain of custody. +- **[medium/M] Chain-of-custody validator is advisory/display-only — `passed:false` never gates storage or rendering** + - _Evidence:_ On the backend, all checks funnel into `validation_warnings` which is attached to `diagnostics` (server.py:1941-1960) and returned alongside an *unmodified* `interpretationResult` (server.py:1969-1973); the warnings never drop/rewrite recommendations and never fail the request. On the frontend, `validatePhase2Consistency` produces a `ValidationReport` whose `passed` flag is consumed only by `Phase2ConsistencyReport.tsx` (rendered with `hideWhenClean`, AnalysisResults.tsx:1086) — I grepped all consumers and found no branch where `report.passed === false` or `errorCount>0` blocks the recommendation surface from rendering. A Gemini response that overrides BPM in prose, or cites only invented paths, still renders its device cards to the user; the violation appears in a separate diagnostics panel. This is an intentional reading of PURPOSE.md ('describe the contradiction, do not pick a winner'), but it means the entire validator battery is a *trust/transparency* surface, not an *enforcement* gate — worth stating explicitly because it is easy to mistake the elaborate validator code for a hard boundary. + - _Recommendation:_ Make the design intent explicit in code and docs: rename/annotate so future maintainers don't assume the validator blocks bad output. Then decide per-violation-type whether display-only is correct. `NUMERIC_OVERRIDE` (Phase 2 prose contradicting a measured BPM/key) is a candidate for stronger treatment — e.g. visually flag the offending card inline (not just the side panel) so the contradiction travels with the recommendation the user is about to act on. Keep recommendations rendering (invariant #1 forbids silent suppression) but raise the salience of ERROR-grade violations at the point of use. +- **[medium/S] Citation-path walker is duplicated across the Python/TS boundary with no shared-source or golden-parity guarantee** + - _Evidence:_ `_walk_measurement_paths`/`_collect_measurement_field_paths` (server_phase2.py:2596-2634) is a hand-maintained 'faithful port' / 'byte-for-byte equivalent' (per its own docstring) of `walkForPaths`/`collectPhase1FieldPaths` (phase2Validator.ts:331-370). The two implementations encode identical, subtle semantics: register intermediate keys, register array paths, and for arrays-of-objects descend into `prefix.key` without an `[i]` index. Sync is enforced only by parallel, independently-written unit tests (apps/backend/tests/test_phase2_citation_paths.py vs the TS tests) and a comment — there is no shared fixture, golden file, or generated artifact that fails when one side drifts. The same duplication exists for the whole Phase 2 shape: server_phase2.py's `_is_*` type-guards (lines 1097-1455) explicitly 'mirror isPhase2Result() in geminiPhase2Client.ts'. If the measurement schema gains a new container shape (e.g. nested arrays-of-arrays), the two walkers can silently diverge and the backend/frontend will disagree on which citations are 'invented' — the backend passing a citation the frontend flags, or vice versa. + - _Recommendation:_ Pin the two walkers with a shared cross-language fixture: a committed JSON corpus of (measurement-fragment -> expected-path-set) cases consumed by *both* the Python test and a Vitest test, so a divergence fails CI on whichever side drifts. This respects the documented duplication-by-design (the contract is deliberately mirrored) while removing the silent-drift risk. Lower-effort stopgap: cross-reference the two test files and add a CHANGED-TOGETHER tripwire entry in CLAUDE.md alongside the existing audio_mime.py duplication tripwire (#9), since this is the same class of deliberately-mirrored contract. +- **[medium/M] server_phase2.py is a 2,827-line god-module spanning six distinct responsibilities** + - _Evidence:_ apps/backend/server_phase2.py (2827 lines) co-locates: (1) the response JSON schema (PHASE2_RESPONSE_SCHEMA + STEM_SUMMARY_RESPONSE_SCHEMA, lines 218-669); (2) the full `_is_*` structural type-guard battery mirroring the TS validator (lines 1097-1455); (3) an English grammar post-processor with an irregular-verb table and noun denylist (`_to_gerund`/`_fix_by_gerund_in_text`/_GERUND_IRREGULARS, lines 967-1095); (4) enum coercion + array salvage (`_coerce_enum_fields`, `_salvage_phase2_array_items`, lines 1585-1900+); (5) the authoritative-measurement overwrite + citation-path validator (lines 2394-2755); and (6) prompt assembly (`_build_phase2_prompt`/`_build_stem_summary_prompt`, lines 791-852). This cuts against the documented domain-module split (CLAUDE.md 'Recent Refactors': monoliths intentionally split into focused modules). The grammar post-processor in particular is an unrelated NLP concern wedged into the validation/transport module, and the schema-as-Python-dict duplicates contract knowledge that also lives in JSON_SCHEMA.md and src/types. Cohesion is low and the file is a merge-conflict magnet for any Phase 2 work. + - _Recommendation:_ Extract along the existing grain into peer modules without changing behavior: `phase2_schema.py` (the two response schemas), `phase2_shape_guards.py` (the `_is_*` battery), `phase2_grammar.py` (the gerund post-processor — it is self-contained and trivially testable in isolation), and `phase2_grounding.py` (authoritative-measurement overwrite + citation-path walker/validator). Leave prompt assembly + the parse/normalize orchestration in server_phase2.py. This matches the precedent set by phase2_catalogue_gates.py (already split out) and the analyze_*.py / server_*.py splits, and shrinks the citation/override-enforcement logic into a module small enough to reason about as the security-critical seam it is. +- **[low/S] Invariant #1 is enforced structurally (separate tables/write-paths), not merely conventionally — this is a strength worth preserving as a named seam** + - _Evidence:_ apps/backend/analysis_runtime.py stores each layer in a physically separate SQLite table: measurement in `measurement_outputs` (written only by `complete_measurement`/`fail_measurement`, lines 791-835), pitch/note in `pitch_note_translation_attempts`, interpretation in `interpretation_attempts` (written only by `complete_interpretation_attempt`, line 1277), and MT3 in `mt3_attempts`. The run snapshot (`get_run`, lines 701-728) sources `stages.measurement.result` *only* from `measurement_outputs.result_json` and hard-tags it `"authoritative": True`. `get_interpretation_grounding` (lines 546-607) reads the measurement row read-only and hands it to Gemini as `measurementResult`. I traced every writer of the measurement table and found no path where interpretation/pitch-note/MT3 output is written back into it. The single place Gemini echoes measured numbers back (`styleProfile.authoritativeMeasurements.{bpm,key,timeSignature}`) is force-overwritten with the real Phase 1 values by `_finalize_style_profile_authoritative_measurements` (server_phase2.py:2394-2432), emitting an `AUTHORITATIVE_MEASUREMENT_OVERRIDDEN` warning on mismatch rather than trusting Gemini. So invariant #1 holds by table topology + write-method partitioning, the strongest enforcement available. + - _Recommendation:_ Keep this as-is; do not let a future 'simplification' merge the per-layer tables into one row with a JSON blob, which would reopen the override surface. Document the table-partition + the single force-overwrite point in docs/ARCHITECTURE_STRATEGY.md as the *mechanism* that enforces invariant #1, so it is treated as load-bearing rather than incidental. Consider an architectural-fitness test that asserts no symbol outside `complete_measurement`/`_update_measurement_row` writes `measurement_outputs.result_json`. +- **[low/S] transcriptionDetail/transcription stripping from the measurement row is a correct but fragile single-point layer guard** + - _Evidence:_ `complete_measurement` (analysis_runtime.py:799-809) pops `transcriptionDetail` and `transcription` from the measurement payload so Layer-2 (pitch/note) and MT3 content cannot leak into the authoritative measurement record — the comment explicitly cites the `ASA_ENABLE_MT3=1` env hook in analyze.py as a leak source it is defending against. This is the right call and preserves the layer boundary, but it is enforced at exactly one location by string-keyed `dict.pop`. If analyze.py later emits another Layer-2/Layer-3-flavored key (e.g. a new transcription sub-namespace or a Gemini-derived descriptor), it will silently flow into `measurement_outputs.result_json` and be served as `authoritative: True` unless someone remembers to add another pop here. The CLAUDE.md note (analysis_runtime.py:685 reference) shows this coupling is already load-bearing for the pianoroll route. + - _Recommendation:_ Invert the guard from a denylist (pop known-bad keys) toward an allowlist or an explicit assertion: after stripping, assert the measurement payload's top-level keys are a subset of a known measurement-key set (the same EXPECTED_TOP_LEVEL_KEYS the analyze tests already maintain), logging a loud warning on any unexpected key rather than silently storing it. This turns a future Layer-boundary leak into a visible diagnostic instead of a silent authoritative-data contamination, at minimal cost and without changing the happy path. + +### Contract surface, duplication & dead code + +- **[high/L] Phase1 cross-boundary contract has no generated/shared source of truth — three hand-maintained representations kept in sync by convention** + - _Evidence:_ The Python→TS Phase1 contract is replicated by hand in three independent places with no codegen bridging them: (1) backend hand-builds dicts with string-literal camelCase keys scattered across apps/backend/analyze_core.py (e.g. "bpmConfidence": at L114/125), apps/backend/analyze.py (L1473, L1876), and apps/backend/analyze_fast.py (L45) — no Pydantic/TypedDict/dataclass models (grep for BaseModel/pydantic/TypedDict/@dataclass in analyze*.py returns nothing); (2) the 755-line hand-written interface set in apps/ui/src/types/measurement.ts; (3) a *separate* hand-rolled runtime guard in apps/ui/src/services/backendPhase1Client.ts (parsePhase1Result L536, parseBackendAnalyzeResponse L172, parseOptionalMelodyDetail, etc.) — no zod/io-ts/ajv in apps/ui/package.json. Enforcement is partial and asymmetric: apps/backend/tests/test_analyze.py EXPECTED_TOP_LEVEL_KEYS (L39) is TOP-LEVEL ONLY — it never descends into nested objects, so the bulk of the payload (kickDetail.*, spectralBalance.*, stereoDetail.*, rhythmDetail.* — most of measurement.ts) is uncovered there. apps/backend/tests/test_phase1_golden.py guards more but is backend-only and shallow: the golden fixture tests/fixtures/golden/phase1_default.json stores just 3 meta-keys (topLevelKeys, topLevelTypes, coreValues) — coreValues is a *curated allowlist* of dotted paths (compare() at L175-194), not a deep structural snapshot, and it never validates the TS side. Net: cross-boundary nested-field agreement rests entirely on discipline. CLAUDE.md Tripwire #3 documents exactly this hazard ("A rename in analyze.py without a matching update in types.ts is undetectable by either type system"). + - _Recommendation:_ Establish one machine-checked source of truth for the nested payload rather than three hand-synced copies. Lowest-friction within the existing grain: (a) emit a JSON Schema from a backend golden run and generate apps/ui/src/types/measurement.ts via openapi-typescript/quicktype in CI, OR (b) keep hand-written types but add a CI step that validates a recorded analyze.py payload against the TS types (e.g. ts-json-schema-generator + ajv) so a Python rename or a forgotten TS field fails a test. Minimally, extend the golden/coreValues allowlist to cover the high-value nested detail objects the UI renders, and add a check that parsePhase1Result and measurement.ts cover the same field set. Do not introduce a heavy runtime-validation framework on the request path — the gap is build-time contract drift, not request-time safety. +- **[medium/S] Deterministic recommendation engine (abletonDevices.ts) is dead code on the product path, yet CLAUDE.md documents it as 'shipped'** + - _Evidence:_ apps/ui/src/data/abletonDevices.ts (309 lines: BAND_DEVICES map, FX_RULES, getInstrumentRecommendations, getFXRecommendations, getSecretSauce) is imported by ZERO product components and ZERO frontend tests (grep across apps/ui/src and apps/ui/tests). Its only consumer anywhere is the research harness bridge apps/backend/scripts/emit_deterministic_recs.ts (L31 imports from ../../ui/src/data/abletonDevices.ts), and even that uses only getFXRecommendations + getInstrumentRecommendations — getSecretSauce is fully dead. Product recommendations come entirely from Gemini (Phase2Result). apps/backend/NEEDS.md (L165-180) already flags this and warns that 'a score-driven improvement to abletonDevices.ts would not reach any user … harness-gaming, not a product improvement.' Compounding the risk: CLAUDE.md 'Backport Candidates' (L294) states 'Most of the original sonic-architect-app port (genre profiles, Ableton device mappings, mix doctor, eight detectors) is shipped' — the literal 'Ableton device mappings' module is NOT shipped to any user, so the doc misleads a maintainer into thinking this path is live. + - _Recommendation:_ Decide and record the module's status, then make code+docs agree. Either (1) treat it explicitly as a research-only baseline: move it under a clearly research-scoped location (or annotate the file header that it is harness-only and unwired), and correct the CLAUDE.md 'shipped' line to scope it to the Gemini path; or (2) if a deterministic free-tier path is genuinely wanted, wire it into the UI. Until one is chosen, the file is a standing trap that invites harness-gaming changes (PURPOSE.md decision #5). At minimum, fix CLAUDE.md L294 so it does not claim a dead module is shipped, and delete the unused getSecretSauce export. +- **[medium/M] Two independent Live 12 device catalogs on the chain-of-custody path are not reconciled (prompt catalog vs validator allowlist)** + - _Evidence:_ Gemini is *told* which devices exist via apps/backend/prompts/live12_device_catalog.json (loaded in server_phase2.py _load_live12_device_catalog L122-129, LIVE12_DEVICE_CATALOG L210; top-level dict with 2 keys), but Gemini's output is *validated* against a different file, data/live12_catalogue.json (repo root, generated by scripts/build_live12_catalogue.py from upstream gluon/AbletonLive12_MIDIRemoteScripts; top-level dict with 8 keys), loaded by live12_catalogue.py and consumed by phase2_catalogue_gates.py (apply_live12_catalogue_gates L304) to gate Phase 2 device/parameter names. These two catalogs have different shapes, different generators, and different lifecycles, and nothing reconciles them: build_live12_catalogue.py does not touch the prompt JSON (grep for 'prompts/' returns nothing), and no script generates the prompt catalog (scripts/ has only replay_catalog_validation.py). A third hand-authored device list (abletonDevices.ts) adds to the divergence. Consequence on the product's core invariant: if the prompt catalog lists a device the validator catalogue lacks (or vice-versa), a valid Gemini recommendation gets flagged as invented, or an invalid device name slips the gate — directly affecting PURPOSE.md invariants #2/#3 (citation + Ableton specificity). + - _Recommendation:_ Make the prompt catalog derive from (or be checked against) the generated validator catalogue so the device set Gemini is told about and the set the validator accepts cannot drift. Concretely: add a generation/check step (extend scripts/build_live12_catalogue.py or add a small reconciler) that asserts every device named in prompts/live12_device_catalog.json exists in data/live12_catalogue.json, run in CI. If the two must stay shaped differently for prompt-token reasons, at least gate on the device-name set equality. This is the higher-leverage half of the 'three catalogs' observation — the prompt↔validator pair sits on the request path. +- **[medium/L] server.py remains a 3,965-line god-object despite the documented router split** + - _Evidence:_ apps/backend/server.py is 3,965 lines with ~100 route decorators and 74 function defs (vs the next-largest product files analysis_runtime.py 2,112 and analyze.py 1,959). The documented split into server_phase1/server_phase2/server_upload/server_samples is *partial*: server.py imports those modules with '# noqa: F401 — re-exported for test backward compat' (L55, L71, L100) and pulls server_samples via plain `import server_samples` (L150) rather than composing FastAPI APIRouters. So the extraction moved some symbols out but server.py still owns the bulk of the routes AND re-exports the extracted ones to keep tests importing from server. This concentrates the upload pipeline, run/stage orchestration glue, Phase 1 normalization, MT3 worker loop (_execute_mt3_attempt/_mt3_worker_loop per CLAUDE.md), and legacy compat wrappers in one file — the single highest-churn merge-conflict surface and hardest unit to reason about. CLAUDE.md flags the split as a deliberate target shape ('resist consolidating it back'), so the issue is that the split is incomplete, not that it should be undone. + - _Recommendation:_ Continue the existing split in the same grain rather than reversing it: convert server_phase1/phase2/upload/samples into real APIRouter modules that own their routes and are composed via include_router, and have tests import handlers from those modules so server.py can drop the F401 re-export shims. Pull the MT3 worker loop and the staged-run orchestration glue into their own modules (peers of analysis_runtime.py). Target shrinking server.py toward a thin app-composition + router-registration file. Do this incrementally, one router at a time, gated by tests/test_server.py contract tests. +- **[low/S] Dead/test-only frontend modules; one (fieldAnalytics.ts) is documented as wired but has zero product consumers** + - _Evidence:_ Four UI modules have no product import (grep across apps/ui/src): apps/ui/src/components/EQSpinner.tsx, apps/ui/src/hooks/useCpuMeter.ts, apps/ui/src/components/waveformPlayerUtils.ts, apps/ui/src/services/fieldAnalytics.ts. Of these, waveformPlayerUtils.ts and fieldAnalytics.ts are kept alive ONLY by their own unit tests (tests/services/waveformPlayerUtils.test.ts, tests/services/fieldAnalytics.test.ts) — passing tests give false confidence that they're load-bearing. EQSpinner.tsx and useCpuMeter.ts have no references at all (incl. tests). Notably, CLAUDE.md describes fieldAnalytics.ts as live instrumentation — 'fieldAnalytics.ts + diagnosticLogs.ts: Instrumentation hooks and diagnostic-log capture for the request panel' — but only its paired sibling diagnosticLogs.ts is actually wired (imported by App.tsx, DiagnosticLog.tsx, analyzer.ts, ConfidenceBandBadge.tsx); fieldAnalytics's exports (analyzeFieldUtilization, generateUtilizationMarkdown, getAllPhase1Fields) are consumed by nothing but its test. waveformPlayerUtils/useCpuMeter date to an unrelated 2026-03 commit ('feat: add confidence calibration script'), suggesting they were stranded by a later refactor of WaveformPlayer.tsx. + - _Recommendation:_ Decide wire-or-remove per module. fieldAnalytics.ts: either surface the field-utilization report in a dev/diagnostic panel (it computes a genuinely useful PURPOSE-aligned metric — which Phase 1 fields Phase 2 actually cites) OR delete it and its test; and fix the CLAUDE.md line that mis-pairs it with diagnosticLogs as wired. EQSpinner.tsx, useCpuMeter.ts, waveformPlayerUtils.ts: delete with their tests unless a near-term consumer is planned. Tests that exist solely to keep an otherwise-orphaned module 'covered' should be removed with the module, not used as a reason to keep it. +- **[low/S] Legacy no-op field dsp_json_override is dead plumbing threaded through all three layers** + - _Evidence:_ dsp_json_override is documented in CLAUDE.md ('accepted by the server but ignored … legacy field; don't repurpose it') and is genuinely inert — apps/backend/server.py accepts it on two endpoints (L3610, L3655) and immediately discards it (`_ = dsp_json_override` at L3630, L3685). But the dead field is still wired end-to-end: apps/ui/src/services/backendPhase1Client.ts (L477-488) still constructs it and appends it to the multipart FormData, and ~20 sites in apps/backend/tests/test_server.py pass dsp_json_override=None. So a reader on each layer encounters a parameter that does nothing. + - _Recommendation:_ Low priority, but when the legacy POST /api/analyze compat wrappers are next touched, remove the field outright: drop the Form param + the `_ =` discards in server.py, stop appending it in backendPhase1Client.ts, and delete the test arguments. It is pure decommissioning with no behavior change. Not worth a dedicated change in isolation; fold into the next edit of that compat surface. +- **[low/S] Positive: intentional dupes hold and there are no accidental backend orphans** + - _Evidence:_ Verified the two documented contracts are intact rather than drifted. (1) The intentional MIME dupe agrees exactly: apps/backend/audio_mime.py CANONICAL_AUDIO_MIME_BY_EXT and apps/ui/src/services/audioFile.ts AUDIO_EXTENSION_MIME_TYPES carry the identical 5 entries (.mp3→audio/mpeg, .wav→audio/wav, .flac→audio/flac, .aiff/.aif→audio/aiff) — no extra/missing extensions on either side, so the deliberately-duplicated cross-boundary map (CLAUDE.md Tripwire #9) has not drifted. (2) mixDoctor.ts is a clean Layer-3 client computation, not a layering violation: it reads Phase 1 fields only (phase1.spectralBalance.*, phase1.plr) and even documents that its plr fallback mirrors analyze_core.py rather than re-deriving a measurement (L104-113), honoring PURPOSE.md invariant #1. (3) Backend orphan sweep (inbound-import count excluding self/tests/scripts) shows a floor of 1 across all top-level product .py modules — every low-inbound module is either a router-split piece (server_*, analyze_*) or a documented research harness (*_evaluation.py, *_report_html.py), so there is no accidental unreachable backend code. genreProfiles.ts (frontend mix targets, consumed only by mixDoctor.ts) and backend genreDetail (detected genre) are distinct purposes, not a duplication. + - _Recommendation:_ No action required — recorded so the review is explicit about what was checked and found healthy. When adding a new ingested audio format, remember the MIME map must be edited on both sides together (Tripwire #9). Keep new backend modules reachable from the router or a documented harness so the clean orphan-free state is preserved. + +### Testability & coverage + +- **[high/M] No cross-app contract test: the frontend/backend field-rename gap is documented as a tripwire but has zero executable guard** + - _Evidence:_ The backend pins its output keys in apps/backend/tests/test_analyze.py (EXPECTED_TOP_LEVEL_KEYS, line 39) and value-snapshots a committed golden at apps/backend/tests/fixtures/golden/phase1_default.json (test_phase1_golden.py). The frontend declares the consuming shape independently in apps/ui/src/types/measurement.ts (Phase1Result, line 630). The two are NEVER cross-checked: grep across apps/ui for `phase1_default | JSON_SCHEMA | EXPECTED_TOP_LEVEL | golden` returns only prose/comment mentions (e.g. measurement.ts:722, loudnessGuardrails.ts:7, analysisRunsClient.ts:248) — no test parses the backend golden and asserts it conforms to Phase1Result, and no backend test reads the UI types. CLAUDE.md tripwire #3 explicitly states a one-sided rename is 'undetectable by either type system' because Python emits camelCase directly with no conversion layer — yet nothing tests it. Compounding this, the ~25 Playwright smoke stubs hand-write their own AnalysisRunSnapshot literals (every `page.route(...).fulfill` in tests/smoke/*.spec.ts, e.g. upload-phase1.spec.ts:113-190; upload-phase1-midi.spec.ts has 8 inline snapshot stubs) with no shared factory, so when the real backend shape drifts the mocks don't, and smoke stays green against a stale contract. + - _Recommendation:_ Add a frontend contract test that imports the committed backend golden (apps/backend/tests/fixtures/golden/phase1_default.json — the one artifact both sides could share) and asserts it satisfies Phase1Result / MeasurementResult (compile-time `satisfies` or a runtime shape check). Separately, extract the smoke-stub snapshot literals into one shared factory in tests/smoke/fixtures/ so a single edit updates every stub and the snapshot shape is asserted against the typed AnalysisRunSnapshot once. This closes the exact gap tripwire #3 names. +- **[high/M] CI gates a strict subset of the tests that exist: full vitest, chordTheory coverage, and the real-backend e2e harness never run in CI/verify** + - _Evidence:_ The verify gate is `npm run lint && npm run test:unit && npm run build && npm run test:smoke` (apps/ui/package.json:21), and CI runs exactly `npm run verify` (.github/workflows/ci.yml:25). `test:unit` is `vitest run tests/services` (package.json:15) — scoped to ONE directory. The full `npm test` (`vitest run`, package.json:14) is wired into no workflow and no script. Consequences I confirmed: (1) tests/utils/chordTheory.test.ts is substantive logic coverage (analyzeChord, Roman-numeral/function mapping) for src/utils/chordTheory.ts, which IS shipped (imported by src/components/HarmonyLanes.tsx) — but it lives outside tests/services so it is never executed by verify or CI; it can go red and still ship. (2) The local-only real-backend integration harness (scripts/test-e2e-integration.sh, test:e2e:integration) is confirmed absent from both .github/workflows files — so the only path that boots a real backend against the real UI never gates a PR. (3) `npm run lint` is `tsc --noEmit` over an include of only `src` + `vite.config.ts`, with `tests` explicitly excluded (apps/ui/tsconfig.json:32-37) — so no smoke/service/e2e .ts file is ever type-checked; a type error in a Playwright spec surfaces only at runtime. There is also no ESLint/Prettier config anywhere (confirmed: no .eslintrc/eslint.config/.prettierrc). + - _Recommendation:_ Make the CI gate match the test surface: switch verify's `test:unit` to full `npm test` (or add a `vitest run` step) so tests/utils and any future top-level tests gate; add a `tsconfig.test.json` (or extend include) so test files are type-checked in `lint`; and either add the integration harness as a CI job or document explicitly that it is a local-only pre-merge step so its absence is a decision, not an accident. Headline framing for the team: CI currently gates a subset, and the subset boundary (`tests/services` only) is invisible until something outside it breaks. +- **[high/M] The real streamed-subprocess path (threaded Popen stdout reader) is branched away from every test by a mock-detection hack — the JSON contract's stream-assembly is untested** + - _Evidence:_ server.py _run_streamed_subprocess branches at line 480 on `getattr(subprocess.run, '__module__', '').startswith('unittest.mock')`: when subprocess.run is mocked (i.e. in unit tests) it takes a simple capture_output=True path; production takes the threaded Popen path at lines 503+ (two daemon threads reading process.stdout/stderr into chunk lists, _read_subprocess_stream). So tests exercise the easy branch and the real stream-assembly code never runs under unittest. This matters specifically for the task's 'subprocess JSON contract under malformed output' concern: I confirmed the JSON *parser* that handles bad output IS covered (server.py:974 `json.loads(stdout)` → JSONDecodeError → ANALYZER_INVALID_JSON, shared by both legacy /api/analyze and the canonical run path via _run_measurement_subprocess). But the threaded reader that *produces* stdout — the only place truncation, partial reads, or stdout/stderr interleaving bugs could originate — is exactly what the mock branch skips. The parser is tested; the assembler is not. Only the local-only e2e harness (not in CI, per the finding above) ever runs the threaded path. + - _Recommendation:_ Test the production reader without mocking subprocess.run: invoke _run_streamed_subprocess against a tiny real helper script (a few lines of python -c that prints known stdout, interleaves stderr, exits nonzero, and one variant that emits malformed JSON) so the Popen+threads path and its handoff to the JSONDecodeError handler are both exercised in CI. The mock-branch can stay as a fast path, but at least one test must take the real branch. +- **[medium/M] Canvas rendering: 9 canvas components, color/scale logic inlined and unexported, smoke asserts only element presence — pixel/mapping regressions ship green** + - _Evidence:_ Nine components draw via getContext('2d') (ChromaHeatmap, RetroVisualizer, Sparkline, SpectralEvolutionChart, SpectrogramViewer, TranscriptionPianoroll, TranscriptionPianorollBlock, sessionMusician/PianoRollCanvas, IdleValuePropPanel). Vitest runs in `node` with no canvas, so none of the drawing is unit-tested. The one smoke spec that touches a canvas (tests/smoke/transcription-pianoroll.spec.ts) asserts only `canvas.toBeVisible()` and the aria-label (lines 231-233) plus the textual header — never a pixel. The load-bearing mapping logic is inlined and unexported: velocityToColor in TranscriptionPianoroll.tsx (lines 36-47, with explicit branches for v<=0, v<64 out-of-spec grey, and the cyan→yellow ramp) and the pitch-row inversion / cell-size math in the draw() loop (lines 82-99). Because it is a private function inside the component, it cannot be unit-tested in node, so a wrong color ramp, inverted pitch axis (the 'lowest pitch at bottom — Ableton convention' comment at line 85 is an untested invariant), or off-by-one cell geometry passes every gate. For a tool whose value is a faithful visual of measurements, a silently wrong heatmap erodes the trust PURPOSE.md centers on. + - _Recommendation:_ Extract the pure mapping functions (velocityToColor, midi→pitch-name, pitch-row→y, value→color scales) out of each canvas component into plain modules under src/services/ or src/utils/ and unit-test them in node (boundary velocities, out-of-spec inputs, the bottom=lowest-pitch invariant). Leave the imperative ctx calls thin. For at least the transcription pianoroll, add a Playwright `toHaveScreenshot` to catch gross drawing regressions. This converts 'untestable in node' into 'mostly tested in node' without fighting the canvas. +- **[medium/S] Stage-queue concurrency is correct-by-construction but has no concurrent-double-claim regression test** + - _Evidence:_ The stage reservations in analysis_runtime.py use the safe pattern — `UPDATE measurement_outputs SET status='running' WHERE id=? AND status='queued'` guarded by a `cursor.rowcount > 0` check (reserve_measurement_run:734-745, reserve_next_measurement_run:747-779; pitch-note/mt3/interpretation reservations follow the same shape at 898, 923, 1060, 1085, 1217, 1264), on a WAL connection (line 90). That is atomic and correct. But the tests in test_analysis_runtime.py only drive these reservations sequentially (lines 337-528 call reserve_next_* one at a time and assert state transitions / returned options) — there is no test that fires two reservations concurrently and asserts exactly one wins. test_worker.py covers wiring (imports, role flags, recovery) but not contention. The exposure is hosted multi-process (SONIC_ANALYZER_PROCESS_ROLE=worker, multiple workers) — in local mode work runs in a single asyncio loop so the race cannot manifest there; this is a test gap, not a live bug. + - _Recommendation:_ Add one regression test that opens two runtime handles to the same SQLite db and invokes reserve_next_measurement_run() from two threads against a single queued row (or back-to-back with a yield), asserting exactly one returns the job and the other returns None. It pins the rowcount-guard invariant before the hosted worker path scales, and documents the atomicity contract the SQL relies on. +- **[low/S] v8 coverage is configured but never enforced — no threshold and verify/CI never request it, so service-layer coverage (incl. the phase2Validator chain-of-custody guards) can erode silently** + - _Evidence:_ vitest.config.ts (lines 8-12) configures v8 coverage with include `src/services/**/*.ts` and text/html reporters — but neither `verify` nor CI passes `--coverage` (package.json:21 verify chain; ci.yml:25), and there is no `thresholds` block. So coverage is collectible on demand but gates nothing. This is the honest read on the 'validator edge cases' dimension: phase2Validator.test.ts (57k) and loudnessGuardrails.test.ts exist and the validators (validateBPMConsistency, validateLUFSConsistency, validateLoudnessActionPresence, etc.) are the runtime enforcers of PURPOSE.md invariants #1/#2/#4 — but file size is not evidence of branch coverage, and with no threshold, a future validator branch (a new guardrail case) can be added with no test and CI stays green. The chain-of-custody guards are precisely where silent coverage erosion is most costly. + - _Recommendation:_ Add a `coverage.thresholds` floor to vitest.config.ts scoped to src/services (start at the current measured number, ratchet up) and run `vitest run tests/services --coverage` in the verify chain so a drop fails the gate. Optionally lift the floor specifically for phase2Validator.ts / loudnessGuardrails.ts, since those encode non-negotiable invariants. Pair with the full-vitest change above so tests/utils logic counts too. + +### Frontend state management & resilience + +- **[high/M] No React error boundary anywhere — a throw or chunk-load failure in lazy AnalysisResults blanks the entire UI** + - _Evidence:_ grep for componentDidCatch/getDerivedStateFromError/ErrorBoundary/errorElement across apps/ui/src returns zero hits (React ^19.0.0, which has no hook equivalent — a class boundary is required). main.tsx:17-21 renders with no boundary at root. App.tsx:1407-1451 lazy-loads AnalysisResults (the 2564-line results surface — the single largest crash surface in the app) via React.lazy (App.tsx:72-76) inside a whose fallback (1408-1419) covers ONLY the pending state — it catches neither a render throw from AnalysisResults nor a rejection of the dynamic import() (e.g. a flaky network fetching the code-split chunk). Both blank the page today. The codebase already knows the seam is missing and pays for it with hand-rolled crash defense leaking into call sites: App.tsx:962-971 wraps validatePhase2Consistency in try/catch with the literal comment "a throw here is on the render path with no error boundary above , so it would blank the entire UI"; analyzer.ts:299-303 duplicates the same try/catch around the same pure validator. This is the structural tell — an absent boundary forcing defensive try/catch into unrelated logic. + - _Recommendation:_ Add a class ErrorBoundary (or adopt react-error-boundary) at two levels: one at root in main.tsx wrapping , and one wrapping the + block in App.tsx so a results-render crash or chunk-load rejection degrades to an inline error card with a reset/retry, not a white screen. Once the boundary exists, the two scattered try/catch blocks around validatePhase2Consistency can be removed — the seam, not the call site, should own crash containment. This is squarely within the documented grain (no new framework, no profile branching). +- **[high/L] App-level run orchestration is a bespoke, untestable concurrency state machine hand-rolled from four coordinating refs + one overloaded boolean** + - _Evidence:_ App.tsx coordinates live-run identity and lifecycle through four refs doing manual concurrency control: currentRunIdRef (273), ignoredRunIdsRef:Set (274), previousRunRef (289), completionRef (290-293), plus a single shared abortControllerRef (272). isAnalyzing (241) is one boolean conflating three distinct operations — initial run (handleStartAnalysis, 526), retry-pitch-note (handleRetryPitchNoteExtraction, 777), retry-interpretation (handleRetryInterpretation, 845) — and all three share that one abortControllerRef: each assigns abortControllerRef.current (lines 547, 781, 849). A retry kicked off while a prior op is in flight overwrites the controller, orphaning the first abort handle — a real race, not just untidiness. 22 useState + 8 useRef live in one 1476-line component. The logic is structurally untestable: the only "App orchestration" test (tests/services/appInterpretationLogging.test.ts) does NOT exercise behavior — it readFileSync's App.tsx and asserts the source string does NOT contain "message: 'AI interpretation in progress.'". That a regression guard had to be written as a source-text grep is direct evidence the orchestration is trapped in inline closures with no seam to test against. + - _Recommendation:_ Extract a useAnalysisRun hook that owns the run-identity refs (currentRunId/ignoredRunIds/previousRun/completion), the abort lifecycle (one controller per in-flight op, not one shared slot), and the onRunUpdate→setLogs wiring. App.tsx keeps presentational state; the hook becomes unit-testable in the node Vitest env and the source-grep test can become a behavioral one. Note for the 'reusable vs bespoke' question: monitorAnalysisRun in analyzer.ts IS reusable (3 callers) — it is only the App-level glue wrapping it that is bespoke. +- **[medium/M] Three near-identical run/retry handlers duplicate the full setIsAnalyzing/AbortController/onRunUpdate skeleton** + - _Evidence:_ handleStartAnalysis (App.tsx:526-755), handleRetryPitchNoteExtraction (777-843), and handleRetryInterpretation (845-912) repeat the same scaffold: new AbortController → abortControllerRef.current = controller → setIsAnalyzing(true) → setError(null)/setErrorRetryable(false) → analysisStartedAtRef.current = Date.now() → call monitorAnalysisRun with a near-identical onPhase1Complete/onPhase2Complete/onError/onRunUpdate quartet (the onRunUpdate closure that does currentRunIdRef.current=update.runId; setActiveRunId; setAnalysisRun; previousRunRef.current=update.snapshot is copied verbatim across all three) → identical finally block resetting isAnalyzing/abortControllerRef/analysisStartedAtRef/elapsedMs. The two retry handlers' onRunUpdate bodies (823-834, 892-903) are byte-for-byte the same. This shares a root cause and a fix with the concurrency-refs finding above. + - _Recommendation:_ Fold into the same useAnalysisRun extraction: a single startOrResume(runFactory) primitive that owns the setIsAnalyzing/abort/started-at scaffold and the shared onRunUpdate wiring, parameterized only by which attempt-creating call (createAnalysisRun vs createPitchNoteTranslationAttempt vs createInterpretationAttempt) runs first. Removes ~120 lines of triplicated glue and guarantees the three paths can't drift. +- **[medium/S] Estimate effect re-POSTs the entire audio file on model-dropdown changes that cannot affect the estimate** + - _Evidence:_ The estimate useEffect (App.tsx:316-360) lists selectedModel in its dependency array (line 360). estimateAnalysisRun (analysisRunsClient.ts:89-91) appends the full File to FormData ('track', file) on every call. But selectedModel is only forwarded to the estimate when interpretation will run — interpretationModel: interpretationWillRun ? selectedModel : undefined (App.tsx:338). So whenever AI interpretation is off (or not configured), changing the model dropdown still refires the effect and re-uploads the whole track to /api/analysis-runs/estimate with zero effect on the returned estimate. For a 100 MiB file that is a full redundant upload per dropdown change. (The effect already guards with an isCancelled flag, so it is not a correctness bug — it is wasted bandwidth/work from a dependency that is dead in the common path.) + - _Recommendation:_ Gate the selectedModel dependency on whether it can influence the estimate — either drop selectedModel from the dep array and rely on interpretationWillRun (which already flips when the model matters), or compute the effective estimate-model (interpretationWillRun ? selectedModel : null) and depend on that. Verify against the estimate contract that model never changes timing when interpretation is off before removing. +- **[low/M] Analysis-run results are not URL-addressable; a reload silently discards a completed run** + - _Evidence:_ View routing is split across two mechanisms, neither of which persists run state. main.tsx:9-10 selects the root component from a query param (resolveAppView(window.location.search) → App vs DenseDawConcept). Within App, the upload→estimate→analysis→results progression is pure conditional view-state driven by analysisRun/phase1ForRender (App.tsx:945-951, 1407). activeRunId (250) and the full snapshot live only in React state — there is no write of runId to the URL and no read-back on mount. Since the backend persists runs in SQLite and exposes GET /api/analysis-runs/{run_id} (analysisRunsClient.ts:153-166), a completed analysis is fully recoverable server-side, but a browser refresh drops it entirely and returns the user to the empty upload screen. + - _Recommendation:_ Reflect activeRunId into the URL (history.replaceState / ?run=) when a run is created, and on mount rehydrate via getAnalysisRun when the param is present. This is a small, grain-respecting enhancement that reuses the existing run client — do NOT introduce a router framework for it (PURPOSE.md §5 cautions against complexity-for-its-sake; the conditional view-state is appropriate for this app's surface). + +### Hosted/local runtime seams + +- **[medium/M] Stage-interruption registry (`_ACTIVE_CHILD_PROCESSES`) is in-process — a local single-process assumption that breaks the api/worker seam** + - _Evidence:_ `server.py:175` `_ACTIVE_CHILD_PROCESSES: dict[tuple[str,str], Popen]` is a module-global populated only where subprocesses are spawned, inside the worker loops (`_register_active_child_process` at `server.py:510`, spawn at `server.py:503`). `DELETE /api/analysis-runs/{run_id}` (`server.py:2570`) calls `_interrupt_active_child_processes(run_id)` (`server.py:2606`, def at 214) to abort active stages. But in hosted mode the api/worker roles are *separate processes*: `_start_background_tasks` (`server.py:323`) gates worker loops behind `should_start_in_process_workers(...)`, which returns False for the `api` role (`runtime_profile.py:42-50` — only `worker` role gets them in hosted). So the API process serving DELETE has an empty registry while the real subprocess lives in the worker process. Compounding it: the worker loops do NOT poll for interruption mid-flight — `is_run_interrupted` is only checked *after* the subprocess returns (`server.py:1146`, `server.py:2076`, both downstream of the blocking `subprocess.run`/`_run_measurement_subprocess` call). The PID-kill is the only in-flight abort path, and it no-ops cross-process. There is also no `FOREIGN KEY`/`ON DELETE CASCADE` in the schema (confirmed: zero matches in `analysis_runtime.py`), and `delete_run` (`analysis_runtime.py:1558`) just DELETEs the rows. Net effect on first hosted deploy: DELETE during a running stage returns `stagesTerminated: []` (a lie), the worker runs the full DSP/Demucs pipeline to completion against now-deleted rows, and `record_artifact` re-inserts orphaned `run_artifacts` rows + files under a run that no longer exists. Works today only because local `all` role keeps spawn+kill in one process. + - _Recommendation:_ Move interruption to a DB-mediated signal rather than an in-process PID table: have `delete_run`/`interrupt_run` write a tombstone/`interrupted` status (the `interrupt_run` path at `analysis_runtime.py:1470` already sets status='interrupted'), and have each worker loop poll that status between subprocess phases and on a watchdog so it can `_terminate_process` its own child and skip the write-back. Keep the in-process registry as a fast-path for the local `all` role, but make DB state the source of truth for cross-process correctness. Treat `_interrupt_active_child_processes` returning `[]` as 'no local child' rather than 'nothing was running.' +- **[medium/S] Every stage requires a materialized local file via `require_local_artifact_path` — `resolve_local_path` is the real (and only) hosted-storage extension point, but it's undocumented as such** + - _Evidence:_ The `ArtifactStorage` Protocol (`artifact_storage.py:16`) exposes only `store_bytes`/`store_file`/`delete`/`resolve_local_path` — no streaming/`get_bytes` read API. Consequently all seven stage consumers depend on a local-disk path: `require_local_artifact_path` (`analysis_runtime.py:537`, which hard-asserts `local_path.is_file()`) is called at `server.py:1077, 1130, 1262, 1425, 1679, 2047, 3206` for measurement (`analyze.py `), pitch/note (Demucs), MT3, samples, and spectral-viz — all of which shell out to subprocesses that can only read from disk (`./venv/bin/python analyze.py …` at `server.py:772/1268/1430`). The CLAUDE.md/`artifact_storage` framing says callers 'do not assume disk paths forever,' but in practice the entire execution model does. Related (same root cause, benign): transient scratch dirs are created with raw `tempfile.mkdtemp(dir=str(runtime.runtime_dir))` (`server.py:1296, 1485`) and raw `open()`/`Path.write_text` (`server.py:1511, 3218`) — these bypass `artifact_storage`, but only for ephemeral subprocess I/O before the result is ingested via `record_artifact`; persistent artifacts still route through the seam. A remote-storage backend is forced to implement `resolve_local_path` as download-to-temp-and-cache, which is a legitimate design — it's just not written down anywhere, so a future implementer may instead try to add streaming methods no consumer can use. + - _Recommendation:_ Document `resolve_local_path` in `artifact_storage.py` as the deliberate localization seam: a non-filesystem backend implements it by fetching to a process-local temp/cache and returning that path, precisely because stages exec subprocesses. State that worker-local scratch (`runtime_dir`) is expected to exist even when artifacts live remote, so the tempdir usage is intended, not a leak. Do NOT add streaming read methods — there is no consumer for them. This is a docs-only change that prevents a future mis-refactor against the grain. +- **[medium/M] Hosted runtime profile + worker-process split are unexercised end-to-end — no deploy manifest, no integration coverage boots the api/worker topology** + - _Evidence:_ Repo-wide search for `Dockerfile*`/`*compose*`/`Procfile`/`fly.toml`/`render.yaml` returns nothing — there is no manifest that runs `SONIC_ANALYZER_PROCESS_ROLE=worker` (`worker.py`) alongside an `api` process. `worker.py` is imported by nothing but its own `__main__` and `tests/test_worker.py` (grep for `import worker` outside tests: empty). `tests/test_runtime_profile.py` and `tests/test_worker.py` unit-test the *switchboard* (`should_start_in_process_workers`, role resolution) but nothing boots the two-process split together. The seams themselves (`runtime_profile.py`, `auth_context.py`, `artifact_storage.py`, `worker.py`) are coherent and low-churn (last touched in PR #11 / #37). The risk is not that the design is wrong — it's that the cross-process correctness gaps (findings #1 and #4) cannot be caught by the current suite because nothing runs hosted mode the way it would actually deploy. The first real hosted bring-up is where they surface. + - _Recommendation:_ Within the existing grain (these seams are intentional per CLAUDE.md 'Recent Refactors — don't undo'), add one hosted-topology integration test that boots an `api`-role app and a `worker.py` process against a shared `SONIC_ANALYZER_RUNTIME_DIR`, runs a real `/api/analysis-runs` job to completion, and asserts a DELETE-while-running actually interrupts. That single test would have caught finding #1. This is the seam's missing testability layer, not a reason to remove the seam. +- **[low/S] Artifact cleanup loop is not role-gated — every API replica runs an independent cleaner, applying the `ARTIFACT_CLEANUP_MAX` safety budget per-process** + - _Evidence:_ `_artifact_cleanup_loop` is started unconditionally inside `@app.on_event('startup')` (`server.py:343`), with no `process_role`/profile guard (unlike the worker loops and `recover_incomplete_attempts`, which are gated at `server.py:325-339`). It only fires under uvicorn, so `worker.py` (which runs `asyncio.run(_run_worker_service())` and never boots the FastAPI app) correctly does NOT run cleanup — the loop is API-process-only. But in a multi-replica hosted API deployment, each replica runs its own `cleanup_artifacts` (`utils/cleanup.py:43`) over the same shared artifacts dir on its own timer, and the `ARTIFACT_CLEANUP_MAX` abort budget (`utils/cleanup.py:63`, default 100) is evaluated independently per process — so the intended safety ceiling on a single sweep is effectively multiplied by replica count, and replicas race over the same expired files (the per-file `FileNotFoundError` handler at `utils/cleanup.py:77` absorbs the races, so it's safe but wasteful/confusing). Harmless in local single-process mode. + - _Recommendation:_ Gate `_artifact_cleanup_loop` behind a single designated role (e.g. only the `worker` role, or an explicit `should_run_cleanup(profile, role)` predicate in `runtime_profile.py` mirroring the existing `should_*` switchboard), so exactly one process owns cleanup. Keeps local behavior identical (the `all` role still runs it) while making the hosted ceiling meaningful. +- **[low/S] Admin-key surface and seam isolation verified sound — local path stays clean of hosted concerns (null-result confirmation of the stated invariant)** + - _Evidence:_ Checked the dimension's core invariant ('local mode unaffected by hosted concerns; indirection respected everywhere') and it holds, with the exceptions above. (1) Artifact indirection: write (`record_artifact` → `store_file`, `analysis_runtime.py:1364`; source ingest → `store_bytes`, `:283`), read (`resolve_artifact_local_path` → `artifact_storage.resolve_local_path`, `:532`), and delete (`delete_run`/`interrupt_run` → `artifact_storage.delete`, `:1554/1578`) all route through the seam; the `path` column consistently stores the opaque `storage_ref`, never re-derived as a disk path. The only bypass is the benign transient scratch noted in finding #2. (2) Admin surface is tight: `admin_key_matches` (`auth_context.py:71`) uses `hmac.compare_digest`, returns False when unset/blank, and is wired into exactly one route — DELETE (`server.py:2593`) — deliberately scoped out of `source-audio` re-serve (documented at `server.py:2706`); admin path returns `RUN_NOT_FOUND` to block run-ID enumeration (`server.py:2588`). (3) Import hygiene: `analysis_runtime.py` (the core run-store) imports only `artifact_storage` + `mt3_transcription`, never `auth_context`/`runtime_profile`/`worker` — so the data layer carries zero hosted coupling; `auth_context`/profile resolution live only at the `server.py` route boundary and no-op in local (`resolve_api_user_context` returns the `local-dev` `UserContext` when `should_require_authenticated_user` is False, `auth_context.py:35-40`). The seam boundary is placed correctly. + - _Recommendation:_ No change required. Recorded as a positive control so the review reflects that the indirection and admin gating were audited and pass — the findings above are the specific, bounded exceptions, not a systemic breakdown. + +### Operational & scalability + +- **[high/S] Dead admission-control code; the MEASUREMENT_QUEUE_FULL 429 is unreachable and the queue is unbounded** + - _Evidence:_ `AnalysisRuntime.__init__` takes `max_pending_per_stage: int = 4` and stores it (analysis_runtime.py:75-79); `_count_active_measurement_runs` (analysis_runtime.py:1889) computes queued+running depth. Grep confirms NEITHER is ever called — both are dead. `create_run` (analysis_runtime.py:260) inserts unconditionally and never raises for depth (no `raise RuntimeError`/QUEUE_FULL anywhere in the module). Yet the create route wraps the call in `except RuntimeError -> 429 MEASUREMENT_QUEUE_FULL` (server.py:2430-2439). So there is no backpressure: a client (or the legacy compat wrappers) can enqueue unlimited runs, each of which pins a heavy subprocess when picked up. The except branch is also a mislabel trap — SQLite lock contention surfaces as `sqlite3.OperationalError`, not `RuntimeError`, and any unrelated stray `RuntimeError` from `create_run` would be reported to the user as 'measurement queue full'. + - _Recommendation:_ Pick one and do only it: either wire `max_pending_per_stage` into `create_run` (consult `_count_active_measurement_runs`, raise a typed QueueFull error that the route maps to 429) to give real admission control; OR delete the unused counter, the unused ctor arg, and the unreachable/mislabeling `except RuntimeError` branch. Given PURPOSE #5, deleting the dead code is the lower-risk default unless hosted backpressure is an imminent need. +- **[medium/M] Artifact cleanup is run-state-blind and DB rows/result blobs are never reaped — two-sided lifecycle leak** + - _Evidence:_ utils/cleanup.py `cleanup_artifacts` globs `artifacts_dir.rglob('*')` and unlinks any file with mtime > ttl_hours (default 24h); it never opens SQLite and has no notion of run state. `_is_exempt` only spares `*.keep` files and a `preserved/` subdir, but grep confirms NOTHING in product code ever writes `.keep` or a `preserved/` path for an active run (the only matches are a docstring, a CSS class in beat_report_html.py, and comments). Artifacts are stored flat as `artifacts_dir/{artifact_id}{suffix}` (artifact_storage.py:108), so the rglob catches source audio, stems, spectrograms, and samples alike. The scheduled loop `_artifact_cleanup_loop` (server.py:352) runs `cleanup_artifacts` hourly but never reaps DB rows. The ONLY code that deletes `analysis_runs`/`measurement_outputs` rows and their inline `result_json`/interpretation blobs is `delete_run` (analysis_runtime.py:1558), called from exactly one place — the owner/admin-gated DELETE route (server.py:2607). Forward consequence: completed-run rows and their large inline JSON blobs (full Phase 1 payload, interpretation results) accumulate in SQLite forever. Backward consequence: a run that sits queued/running > 24h (backlog or a stuck stage) has its source-audio file unlinked out from under it; `require_local_artifact_path` (analysis_runtime.py:537) then raises FileNotFoundError. Every completed run older than 24h becomes a half-broken results page — DB results still return, but spectrogram/stem/sample artifact endpoints 404. + - _Recommendation:_ Make cleanup run-state-aware on the same TTL clock, in-grain with the existing seams: (1) reap whole terminal runs past TTL by calling the existing `delete_run` from `_artifact_cleanup_loop` (this also bounds SQLite row/blob growth); and (2) exempt artifacts belonging to non-terminal runs (measurement/stage status in queued/running) from the mtime sweep so a backlogged run can't lose its source audio. Keep the `ARTIFACT_CLEANUP_MAX` safety cap. This extends the artifact_storage / delete_run boundary rather than adding a new subsystem. +- **[medium/S] No global cap on concurrent heavy subprocesses across the four worker loops — peak memory is the unbounded sum** + - _Evidence:_ Four independent always-on loops poll and launch subprocesses with no shared gate: `_measurement_worker_loop`, `_pitch_note_worker_loop`, `_interpretation_worker_loop`, `_mt3_worker_loop` (server.py:2131-2211), started together via `_create_background_tasks` (server.py:288). Each reserves one job and runs a subprocess: measurement via `_run_streamed_subprocess` (Essentia), pitch/note via `subprocess.run(timeout=600)` which the code comments at server.py:1240 note loads '~2-4GB' Demucs/torchcrepe, and MT3 via `subprocess.run(timeout=1800)` documented at server.py:1395 as 'multi-GB JAX/t5x' weights. The mt3 and pitch_note stages are deliberately gated only on measurement completion so they run CONCURRENTLY (analysis_runtime.py:996-1000), and the measurement loop can independently reserve a SECOND run's pass while those are in flight. Nothing bounds the sum of resident memory. This bites a single local user with two queued tracks and mt3 enabled — so it is not a hypothetical-scale concern. + - _Recommendation:_ Add one shared bounded `asyncio.Semaphore` (or a small threading semaphore around the `asyncio.to_thread` subprocess launches) sized to the box's memory budget, acquired by all four executors before spawning their heavy subprocess. This preserves the intentional subprocess-isolation design and the staged-peer concurrency; it only caps simultaneity. Make the bound an env-tunable constant alongside `WORKER_IDLE_SECONDS`. +- **[low/S] SQLite-as-queue: hot status columns are unindexed, so every poll is a full scan + sort** + - _Evidence:_ The schema in `_ensure_schema` (analysis_runtime.py:95-258) creates no secondary indexes (grep for CREATE INDEX returns none). The four `reserve_next_*` queries each filter on `status='queued'`, JOIN on run_id, and `ORDER BY created_at ASC LIMIT 1` (analysis_runtime.py:758, 913, 1075, 1254). With `WORKER_IDLE_SECONDS = 0.25` (server.py:159) the four loops collectively issue ~16 such scan+sort queries per second, indefinitely. On the tiny tables of a fresh local DB this is negligible, but it compounds directly with the never-reaped-rows leak (finding above): as `measurement_outputs`/`interpretation_attempts` grow without bound, these full scans degrade and the per-poll write lock is held longer. + - _Recommendation:_ Add covering indexes on the reservation predicates, e.g. `(status, created_at)` on measurement_outputs, pitch_note_translation_attempts, interpretation_attempts, and mt3_attempts, plus `(run_id)` where stage tables are joined. Cheap and idempotent in `_ensure_schema`. Low priority on its own; promote it to be done together with the row-reaping fix since that is what gives it teeth. +- **[low/S] SQLite WAL queue is correct but write-serialized and process-local; the hosted multi-writer / fairness assumptions are undocumented** + - _Evidence:_ `_connect` opens per-call connections with `journal_mode=WAL`, `synchronous=NORMAL`, and `busy_timeout=5000ms` (analysis_runtime.py:87-93). The reserve-then-update pattern (SELECT queued + conditional UPDATE guarded by `WHERE status='queued'`, checking `cursor.rowcount`) is a correct optimistic claim that is safe under multiple writers. But WAL still serializes writers, so this scales only to a few worker processes hitting the one `.runtime/analysis_runs.sqlite3` file; the runtime_dir is a single shared file with no per-host sharding. Reservation is strict global FIFO (`ORDER BY created_at`) with no per-owner fairness — under hosted multi-user load one user's backlog blocks all others. `runtime_profile.py` already defines the api/worker process split that this contention model depends on, but neither the WAL write-serialization ceiling nor the FIFO fairness limitation is documented as a migration trigger. + - _Recommendation:_ Do not build a fair scheduler or swap the datastore now (PURPOSE #5). Instead document, at the `AnalysisRuntime` class and in docs/ARCHITECTURE_STRATEGY.md, that the queue is a single-file WAL SQLite suitable for one host with a small fixed worker-process count, and name the concrete migration trigger (move the queue to Postgres / a real broker, or add `SKIP LOCKED`-style claiming) for when hosted concurrency exceeds a few writers or per-tenant fairness becomes required. Treat the existing runtime_profile seam as the insertion point. + +--- + +## 4. Refuted / by-design (verified NOT real) + +### [P2→none] Backend citation mirror rejects legitimate transcriptionDetail.* citations the frontend accepts (allowed-set divergence) + +`apps/backend/server.py:1900` · Chain-of-custody validation logic + +**Original claim.** The backend mirror _validate_phase2_citation_paths is called with `measurement_result` (the persisted measurement row) plus `mt3_result`, but NOT `pitch_note_result`. The persisted measurement row has had `transcriptionDetail` stripped (analysis_runtime.py:802 `measurement_result.pop('transcriptionDetail', None)`), and the pitch/note translation result is held separately as `grounding['pitchNoteResult']` (analysis_runtime.py:586; bound at server.py:2016) and only forwarded to Gemini under OPTIONAL_PITCH_NOTE_TRANSLATION_RESULT_JSON. So the backend's `allowed` set (built only from `measurement_result` normalized + `mt3_result` namespace) lacks every `transcriptionDetail.*` path. The frontend, by contrast, re-merges the pitch-note result back into Phase1Result as `{ ...measurement, transcriptionDetail: pitchNote }` (analysisRunsClient.ts:243), so collectPhase1FieldPaths DOES include `transcriptionDetail`/`transcriptionDetail.averageConfidence`, and CONFIDENCE_PAIRS (phase2Validator.ts:160) explicitly expects `transcriptionDetail` citations. A `transcriptionDetail.*` citation is therefore legitimate and prompt-invited, yet the backend emits a spurious UNRESOLVED_CITATION_PATH warning ('does not resolve to any path present in the authoritative measurement payload') on the validationWarnings channel — the very channel CLAUDE.md designates as the safety net for non-browser API consumers, who would be told a valid transcription citation is invented. + +**Why it was refuted.** The mechanical gap the claim describes is real but the defect it asserts is not reachable and not a contract break. The claim's load-bearing premise — that a `transcriptionDetail.*` citation is "legitimate and prompt-invited" — is refuted by the actual prompt and data flow. + +WHAT IS TRUE (the mechanical half): The backend mirror `_validate_phase2_citation_paths` (server_phase2.py:2666) builds its `allowed` set from `_normalize_measurement_result_for_gemini(measurement_result)` ∪ the `transcription.mt3.*` namespace (lines 2699-2708). `measurement_result` has had `transcriptionDetail` stripped by design (analysis_runtime.py:802, "Layer 2 (pitch/note translation) concern"). `pitch_note_result` is available at the call site (server.py:2016) but is NOT passed into the validator (server.py:1900-1904 passes only `interpretation_result, measurement_result, mt3_result=mt3_result`). The frontend, by contrast, merges `{ ...measurement, transcriptionDetail: pitchNote }` (analysisRunsClient.ts:242-243; `pitchNote` is the unwrapped inner object stored at server.py:1319+1349-1351), so `collectPhase1FieldPaths` (phase2Validator.ts:333) does include `transcriptionDetail`/`transcriptionDetail.averageConfidence`, and CONFIDENCE_PAIRS (phase2Validator.ts:160) maps `transcriptionDetail`→`transcriptionDetail.averageConfidence`. So the two mirrors genuinely diverge on `transcriptionDetail.*`. + +WHY IT IS NOT A REAL P2 DEFECT: +1. UNREACHABLE TRIGGER. The warning only fires on Gemini-emitted `phase1Fields`. The prompt restricts citations to AUTHORITATIVE_MEASUREMENT_RESULT_JSON (phase2_system.txt lines 95 and 240). Pitch-note data is forwarded under a different block, OPTIONAL_PITCH_NOTE_TRANSLATION_RESULT_JSON, as the *unwrapped* inner object (top-level keys `averageConfidence`/`noteCount`/`dominantPitches`). The literal token `transcriptionDetail` appears 0 times in phase2_system.txt (verified via grep -c), and it is absent from the citable-namespace table (lines 119-138). The prompt's only invitation to cite outside the authoritative block is `transcription.mt3.*` (server_phase2.py:806-813), not `transcriptionDetail`. Gemini therefore has no in-context signal to write `transcriptionDetail.averageConfidence` and is explicitly told not to cite outside the authoritative payload. (The `noteCount`/`averageConfidence` references in the prompt are `melodyDetail.noteCount`, a real authoritative field, not the pitch-note one.) +2. THE WARNING IS LITERALLY TRUE. Its message says the path "does not resolve to any path present in the authoritative measurement payload" — and `transcriptionDetail` is intentionally not authoritative (PURPOSE invariant #1, Layer 1 vs Layer 2). The backend emits no false statement. +3. ACKNOWLEDGED DESIGN, NOT A CONTRACT BREAK. The backend docstring (server_phase2.py:2679-2681) explicitly declares itself "coarser than the frontend's validatePhase1FieldCitations ... the frontend remains authoritative." It is WARNING-only, never rejects, never raises (per the code and CLAUDE.md tripwire #8). A divergence the code documents as intentional, on a non-failing advisory channel, with an unreachable trigger, is a latent code-quality nicety at most (P3/low if one wanted to flag the mirror imperfection) — not a reachable P2. The proposed fix (fold `pitch_note_result` into `allowed`) is a defensible hardening, but its absence does not produce the asserted spurious warning in practice. + +### [P3→none] Full untruncated Gemini exception string is stored in client-visible diagnostics.stderr + +`apps/backend/server.py:1990` · Security + +**Original claim.** On a Gemini generation failure, _run_interpretation_request_with_profile_config captures error_msg = str(exc) and, while the user-facing message field is truncated to 200 chars (line 1996), the FULL untruncated exception string is placed into diagnostics.stderr (line 1990). That diagnostics object is persisted on the interpretation attempt and surfaced in the run snapshot the owner polls. The google-genai SDK normally carries GEMINI_API_KEY in the x-goog-api-key request header (not the URL), so this is defense-in-depth rather than a confirmed key disclosure — but an arbitrary upstream/SDK exception string is reflected to the client without sanitization, which is a latent leakage channel for whatever the SDK chooses to put in an exception (request metadata, file URIs, internal paths). + +**Why it was refuted.** The claim's stated mechanism is factually wrong on both counts. (1) NOT untruncated: at server.py:1990 `stderr=error_msg` is passed to `_build_diagnostics`, which routes it through `_safe_snippet(stderr)` (server_phase1.py:518). `_safe_snippet` (server_phase1.py:355-365) returns `snippet[:MAX_SNIPPET_LENGTH]` where `MAX_SNIPPET_LENGTH = 2000` (server_phase1.py:18) — so the exception is capped at 2000 chars, not "FULL untruncated." (2) NOT `diagnostics.stderr`: there is no such field. The value is stored under the key `stderrSnippet` (server_phase1.py:518). The headline API-key-disclosure risk is absent: the Gemini client is built with header-based auth (`_genai.Client(api_key=api_key)` at server.py:1804-1805; google-genai places this in the `x-goog-api-key` header, not the URL), which the claim itself concedes. Moreover, `stderrSnippet` is a deliberate, tested part of the diagnostics contract — tests/test_server.py asserts raw subprocess stderr like "segfault in essentia" and "partial stderr" surface there by design (lines 2241, 2454, 2515, 2575); a Gemini exception flowing through that same truncated channel is consistent with the established design, not a new defect. The recipient is the authenticated run owner viewing their own run. The only non-zero-merit residual is that google-genai exception contents are not controlled by this project (unlike analyze.py's own prints), so in principle an SDK exception could carry incidental metadata — but the claim demonstrates no concrete sensitive value reaching the client ("whatever the SDK chooses to put" is hypothetical), so it does not clear the "concrete path where it bites" bar. CWE-209-flavored hardening at most, not a reachable defect. + +--- + +## 5. Appendix — coverage & method + +**Bug-hunt dimensions:** backend concurrency & resource lifecycle · frontend async/polling/cleanup · chain-of-custody validation · frontend↔backend contract integrity · security (SSRF, upload limits, auth, path traversal, secret leakage) · DSP & numeric correctness · error handling & type safety. + +**Architecture dimensions:** module boundaries & god-objects · layer integrity · contract/duplication/dead-code · testability & CI · frontend state management · hosted/local seams · operational & scalability. + +**Method:** each finder read source directly (no speculation), capped at its 6 highest-certainty findings; each bug was then independently verified by a skeptic that defaulted to *not-real* unless it could trace a concrete biting path, and was permitted to run existing tests read-only to confirm reachability. + +**Not examined (out of scope):** applying fixes; Phase 2 prompt/catalog tuning; the render-gated recommendation campaign; `packages/loudness-spectro-wasm` (off the product path); live Gemini behavior; visual/pixel correctness of canvas renders; performance profiling under real load.