Code Review: StemDeck — LUME Stage 1 (sample-locked 4-stem playback)
> Meta-review (6-pass parallel + adversarial contrarian round) of commit `c3559889` > on branch `feat/femto-only-bar`, repo `Comp-Core`. > Reviewed files (NEW code only): > - `core/audio-media/cc-echelon/crates/audio-engine/src/stem_deck.rs` (963 lines) > - `core/audio-media/cc-echelon/crates/audio-engine/examples/stem_deck.rs` (167 lines) > Surrounding crate (`loader.rs`, `param_control.rs`, `fx.rs`, `lib.rs`) read for primitive-usage judgement only; findings are scoped to StemDeck.
Full Public Reader
Code Review: StemDeck — LUME Stage 1 (sample-locked 4-stem playback)
> Meta-review (6-pass parallel + adversarial contrarian round) of commit `c3559889`
> on branch `feat/femto-only-bar`, repo `Comp-Core`.
> Reviewed files (NEW code only):
> - `core/audio-media/cc-echelon/crates/audio-engine/src/stem_deck.rs` (963 lines)
> - `core/audio-media/cc-echelon/crates/audio-engine/examples/stem_deck.rs` (167 lines)
> Surrounding crate (`loader.rs`, `param_control.rs`, `fx.rs`, `lib.rs`) read for primitive-usage judgement only; findings are scoped to StemDeck.
---
Summary
- Findings: 21 total — 2 Critical, 6 High, 9 Medium, 4 Low
- Cross-Cutting Patterns: (1) `render()` is treated as real-time-safe but is not — it does heavyweight per-sample work and can be driven into pathological states; (2) the API has no guard against being driven into silently-wrong states by a Stage 2 consumer (`StemConductor`); (3) no validation that the four stems of a "set" are actually consistent (length, content); (4) `f32`-as-`usize` casts saturate/truncate silently across the transport math.
- Overall Health: Solid, well-documented, well-tested functional core with a genuinely good sample-locked-playhead design and strong analytic tests. Not yet real-time-safe and not yet misuse-resistant — both are blocking for the stated downstream use (audio-callback rendering + `StemConductor` driving the API). The bugs are real and concentrated in two areas: `render()` RT-safety and transport/loop edge cases. None are security-exploitable in the classic sense (local file inputs only), but several cause audible failure or silent drift.
---
Findings by Severity
Critical
| ID | Description | File:Line | Fix |
|---|---|---|---|
| X1 | `render()` is documented and intended as an audio-callback path, but it is not real-time-safe: it runs `FilterFx`/`DelayFx`/`ReverbFx` `process_sample` per sample with `tan()`/`exp()`-class math and modulo ring-buffer indexing inline, calls `out.fill(0.0)` on the whole block, and re-evaluates branch-heavy loop logic every frame. On a real Demucs set (4 stems, full reverb = 8 comb + 4 allpass per channel) this is a large, unbounded-by-set-size per-block cost with no headroom guarantee. If `render()` is called directly from the CPAL callback it will glitch under load. | `stem_deck.rs:568-630` | Decide the contract explicitly. Either (a) document that `render()` runs on a worker thread feeding `ChannelReceiver<RenderBatch>` (the pattern `lib.rs:259-300` `RingCallback` already establishes — the callback only does a `copy_from_slice`), and state that in the doc-comment; or (b) if it truly runs in-callback, pre-compute filter coefficients on parameter change (not per sample) and bound worst-case cost. Right now the doc-comment claims callback use without meeting callback constraints. This is the single most important finding for Stage 2. |
| C1 | `render()` infinite loop / silent hang on a degenerate loop region. `set_loop_region_frames` clamps `start` to `length_frames` and forces `end = end.min(len).max(start+1)`. If `start == length_frames`, then `end` becomes `length_frames+1` — i.e. `loop_end > length_frames`. In `render()` the wrap test is `playhead >= length_frames | (looping && playhead >= loop_end)`. When `looping` is true and `loop_start == length_frames`, every iteration sets `playhead = loop_start (= length_frames)`, the very next check `playhead >= length_frames` is true again, `playhead` is reset again — the loop body never advances the playhead and never writes a sample, but it does run `frames` iterations producing silence, then returns. Worse: with `loop_start == loop_end` reachable via `set_loop_region_bars` when `bar_frames` pushes start past the clip, the deck is permanently stuck at end-of-clip emitting silence with `playing == true` and no error — an unrecoverable wedged transport. |
High
| ID | Description | File:Line | Fix |
|---|---|---|---|
| A1 | A "stem set" is never validated for cross-stem consistency. `load_stem_set` loads whatever WAVs exist, pads all to the longest (`max_len`), and trusts `features.json`. Nothing checks that the four stems came from the same track. A consumer can point it at a folder where `drums.wav` is 4 s and `vocals.wav` is 90 s; the deck happily plays them sample-locked but musically nonsensical, and `length_frames` silently becomes the 90 s value. Demucs always emits equal-length stems, so a large length delta is a corruption signal, not "slight difference" as the doc claims. | `stem_deck.rs:288-352` | After loading, if `max_len - min_len` exceeds a small tolerance (e.g. one frame, or a few ms), `bail!` with the offending stem. Also cross-check each stem's `features.duration_sec` against its actual decoded frame count and against the set BPM. |
| A2 | `features.json` and the audio are never reconciled. `bpm()`, `bar_length_frames()`, `set_bus_delay()` all trust `features.json` BPM blindly. If `features.json` says 120 BPM but the WAVs are a 90 BPM track, every bar-quantized loop and BPM-synced delay is wrong — and Stage 2's body-driven conducting inherits the error. There is also no check that all four per-stem feature blocks agree on BPM/duration (the doc asserts they must, the code never verifies). | `stem_deck.rs:169-177, 439-446, 511-518` | In `StemFeatureSet::parse`, if multiple stems are present, verify their `bpm` values agree within tolerance and `bail!` (or warn) on disagreement. Optionally derive expected frame count from `duration_sec * sample_rate` and compare to decoded length. |
| C2 | Mix-bus / per-stem FX coefficients are recomputed every single sample inside `render()`. `FilterFx::process_sample` recomputes `g = tan(PIfreq/sr)`, `k`, `a1/a2/a3` on every* sample even though cutoff/resonance only change when a setter is called. With per-stem filters + 3 bus FX active, that's 4+ `tan()` calls per frame minimum. This is wasted RT budget and the dominant per-sample cost. (Root cause lives in `fx.rs`, but StemDeck is the first hot-path consumer to expose it at scale — flagged here as a StemDeck integration risk, fix belongs to whichever owns the FX cadence.) | `stem_deck.rs:601-621` (consuming `fx.rs:89-97`) | Either cache coefficients in `FilterFx` and recompute only in the setters, or have StemDeck recompute coefficients once per `render()` block instead of per sample. Block-rate coefficient update is the standard DSP pattern and is inaudible for these controls. |
| P1 | Loop wrap re-checks `length_frames` only at frame boundaries, but a per-stem filter's state is never re-primed at the wrap. When `looping` wraps `playhead` back to `loop_start`, the `FilterFx`/`DelayFx`/`ReverbFx` states carry across the seam. Delay/reverb tails crossing a loop point is musically fine, but the per-stem low-pass filter (`StemVoice::filter_l/r`) carrying state across a hard sample discontinuity at the loop seam will produce a transient click on every loop — exactly the click the `SmoothParameter` gain ramp was added to avoid. | `stem_deck.rs:582-583, 601-604` | On loop wrap, either reset per-stem filter state, or (better) accept the tail but document it. At minimum add a loop-seam click test analogous to the existing gain-ramp click test. |
| A3 | `render()` returns `frames` unconditionally — the doc says it "Returns the number of frames produced" but it always returns the requested count, even when the block is entirely silence (paused, no set, or end-of-clip stop). A consumer using the return value to detect end-of-clip or underrun gets no signal; it will spin forever rendering silence believing audio is flowing. | `stem_deck.rs:562-567, 573-575, 585-588, 629` | Either return the count of frames actually advanced by the playhead (0 when paused/ended), or change the contract to return a small status enum (`Played`/`Ended`/`Idle`). Stage 2 needs to know when a clip has ended to trigger the next track. |
| X2 | `render()` performs no output clipping / limiting; summing 4 unity-gain stems plus resonant filter + delay feedback + reverb can exceed `[-1.0, 1.0]` and will hard-clip the DAC. Per-stem gain `SmoothParameter` is range-clamped to `0.0..4.0` (line 327-328) — a consumer can set every stem to 4.0. Four stems × 4.0 × resonant boost is a guaranteed >1.0 excursion. The example file peak-normalizes after the fact (`examples/stem_deck.rs:89-95`), proving the author knows the mix clips, but the library `render()` ships raw. A live bar install will send clipped audio to the PA. | `stem_deck.rs:623-624` | Run the summed mix through the crate's existing `limiter` module (`audio-engine` has `pub mod limiter`) on the bus, or at minimum a soft-clip/`tanh` saturator before write. Do not rely on the consumer to normalize. |
Medium
| ID | Description | File:Line | Fix |
|---|---|---|---|
| A4 | `render()` takes both `output: &mut [f32]` and a separate `frames: usize` — a classic misuse-prone API. The two can disagree; the only guard is a `debug_assert!` (line 569) which is compiled out in release. In a release build, `render(buf, frames)` with `buf.len() < frames2` will panic on slice index `output[..frames2]` (line 570) inside the audio path — a crash in the bar. | `stem_deck.rs:568-570` | Drop the `frames` parameter and derive `frames = output.len() / 2`; or keep it but return `Result`/clamp instead of `debug_assert!`. Never rely on `debug_assert!` for an audio-thread precondition. |
| A5 | No way to unload / clear a loaded set, and re-`load_stem_set` on failure leaves a half-mutated deck. `load_stem_set` reads features, then loops loading WAVs into a local `voices` array, then commits to `self` only at the end — good. But if it `bail!`s mid-loop (e.g. stem 3 has a bad sample rate), the previous set is still loaded and `playing` may be true: the deck keeps rendering the old set while the caller believes the load failed. There is also no `unload()`. | `stem_deck.rs:288-352` | Document that a failed `load_stem_set` leaves the prior set intact (it currently does, which is arguably good) — or explicitly pause + clear on entry. Add an `unload()` / `clear()` for Stage 2 track transitions. |
| A6 | `Stem::index()` and `STEM_NAMES`/`Stem::ALL` ordering are duplicated, hand-written, and can silently desync. `STEM_NAMES` (line 32), `Stem::ALL` (line 46), `file_name()` (line 49-56), and `index()` (line 59-66) all independently encode the drums/bass/other/vocals order. A future edit to one and not the others mis-indexes `voices[]` — a silent wrong-stem bug with no compiler help. | `stem_deck.rs:32, 44-67` | Derive `STEM_NAMES` from `Stem::ALL.map(Stem::file_name)`; make `index()` the single source and `file_name`/`ALL` consistent by construction, or add a test asserting `Stem::ALL[i].index() == i` and `file_name()` matches `STEM_NAMES[i]`. |
| P2 | `seek_frames` clamps to `length_frames` (inclusive), allowing `playhead == length_frames`. That is one past the last valid frame. `render()` catches it via the `>= length_frames` wrap test, but any code reading `playhead_frames()` after a seek-to-end sees an out-of-range index. `StemVoice::frame()` masks it with `unwrap_or(0.0)`, so it degrades to silence rather than a panic — but it's an off-by-one in the public API contract. | `stem_deck.rs:373-375` | Clamp to `length_frames.saturating_sub(1)` for a non-empty clip, and define the behavior for an empty deck (currently `0.min(0) = 0`, fine). |
| A7 | `StemFeatureSet` exposes set-level `bpm()`/`duration_sec()` as `0.0` sentinels when no set is loaded, not `Option`. `StemDeck::bpm()` returns `0.0` for "no set," which then flows into `bar_length_frames()` (handled: `bpm <= 1.0` guard) but `set_bus_delay()` falls back to `120.0` — two different no-data behaviors for the same missing input. A consumer can't distinguish "real 0 BPM" from "no set." | `stem_deck.rs:169-177, 511-512, 550-553` | Return `Option<f32>` from `bpm()` and `duration_sec()`, or make "no set loaded" unrepresentable by splitting `StemDeck` into an unloaded builder + a `LoadedDeck`. The latter also kills several other "is a set loaded?" branches. |
| P3 | `out.fill(0.0)` zeroes the whole block every call even though the non-silent path overwrites every sample it produces. For the common playing case this is a redundant full-block memset. Minor, but it is in the hot path and trivially avoidable. | `stem_deck.rs:571` | Only zero the tail not written by the playback loop (the end-of-clip early-return case), or zero lazily. |
| A8 | *`set_stem_filter` always sets cutoff/resonance even when `enabled == false`, and there is no per-stem filter mode*** (it is hard-wired low-pass via `FilterFx::new` default). The doc says "per-stem low-pass" so mode omission is intentional, but the combined enabled+cutoff+resonance setter means you cannot pre-arm a filter's cutoff while disabled without it also being silently applied the instant you enable it elsewhere. Minor API ergonomics. | `stem_deck.rs:478-486` | Split into `set_stem_filter_enabled(stem, bool)` and `set_stem_filter_params(stem, cutoff, res)`, mirroring the bus API (`set_bus_fx_enabled` + `set_bus_filter` are already split — the per-stem API is inconsistent with it). |
| D1 | *`StemFeatures` derives `Deserialize` with `#[serde(default)]` on almost every field but `bpm` and `duration_sec` are required** — a `features.json` missing `bpm` fails the whole parse with a serde error, not a domain error. That is arguably correct (transport needs BPM), but combined with A2 (no cross-stem BPM check) the failure modes are uneven: missing BPM = hard parse error, wrong* BPM = silent. Also `serde_json` parsing of an attacker-supplied / corrupt `features.json` is unbounded in `energy_curve`/`mfcc_mean` vector sizes — a malicious multi-GB JSON array would be loaded fully into memory. Low real risk (local curated files) but worth a note. | `stem_deck.rs:79-117, 134-147` | Keep `bpm` required but produce a domain error (`bail!("features.json: stem {x} missing bpm")`) by deserializing into an all-`Option` mirror struct then validating, OR accept the serde error but wrap it. For the unbounded-vector concern, document that `features.json` is trusted-input-only. |
| X3 | Path / input-trust boundary: `load_stem_set` takes an arbitrary directory and reads `features.json` + `{stem}.wav` from it. No path traversal in the classic sense (filenames are fixed constants joined to the dir), so this is not a vulnerability — but the dir itself is fully consumer-controlled and `load_wav` will read an arbitrarily large WAV fully into memory (`Arc<[f32]>`). For a bar install fed curated stems this is fine; flagged Low so it is a conscious decision, not an oversight. | `stem_deck.rs:288-309` | Document the trust assumption ("stem-set folders are operator-curated, not user-supplied"). Optionally cap WAV size / duration. |
Low
| ID | Description | File:Line | Fix |
|---|---|---|---|
| X4 | `deinterleave_stereo` with `>2` channels keeps channels 0,1 and silently discards the rest; with a malformed odd-length interleaved buffer `chunks_exact` silently drops the trailing partial frame. Both are reasonable but undocumented at the call site. | `stem_deck.rs:635-648` | One-line comment is present in the doc-comment; consider a `debug_assert` that `samples.len() |
| DX1 | No module-level doc example / no `examples/` cross-link from the `render()` doc, and the `render()` doc-comment claims callback use (see X1) which is misleading until X1 is resolved. The example file is good but not referenced from the API docs. | `stem_deck.rs:562-567` | Add `/// See examples/stem_deck.rs` and correct the callback claim. |
| DX2 | Tests are genuinely strong (analytic 4-sine sum, residual-isolation mute test, click test, loop-containment) — but there is no test for the error/edge paths: degenerate loop region (C1), mismatched stem lengths (A1), missing one stem WAV, non-matching sample rate, clipping (X2). Coverage is happy-path-heavy. | `stem_deck.rs:650-963` | Add edge-case tests for each Critical/High finding; they double as regression guards. |
| DX3 | `MUSIC_DIRECTION.md` is referenced in the module doc (`tools/lume-music/MUSIC_DIRECTION.md`) — verify it exists and that the "Stage 1/2/4" staging it describes matches this code. Doc-vs-reality drift risk per project Architecture Discipline rule. | `stem_deck.rs:8-10` | Confirm the referenced doc exists and is current; this review artifact lands in the same `tools/lume-music/` dir. |
---
Cross-Cutting Patterns
| Pattern | Affected Passes | Finding IDs | Root Cause |
|---|---|---|---|
| `render()` is not real-time-safe despite a callback-claiming doc | Concurrency, Performance, API | X1, C2, P3, A4 | The doc-comment promises audio-callback semantics; the body does per-sample transcendental math, full-block memset, and `debug_assert`-only bounds. The crate already has the right pattern (`RingCallback` worker-feeds-ring) — StemDeck just doesn't declare which side of it it's on. |
| No "is this a coherent stem set?" validation | Security, API, Perf | A1, A2, D1 | `load_stem_set` trusts the folder and `features.json` independently; nothing reconciles audio length vs feature duration vs cross-stem BPM. Demucs guarantees consistency, so any inconsistency is corruption — but the deck treats it as normal. |
| Consumer can drive the deck into a silently-wrong or wedged state | API, Concurrency | C1, A3, A5, A7, X2 | The API is not misuse-resistant: degenerate loop regions wedge the transport, `render()`'s return value can't signal end-of-clip, no `unload()`, `0.0`/`120.0` sentinels for "no set," no output limiting. Stage 2 (`StemConductor`) drives this API programmatically and will hit these. |
| `f32`→`usize` casts truncate/saturate silently | Perf, API | C1, A2, P2 | `seek_seconds`, `bar_length_frames`, `set_loop_region_bars` all do `(... as usize)`. Negative/NaN/huge inputs saturate to `0` or `usize::MAX` with no error. Combined with the loop-region math this is the mechanism behind C1. |
Blind Spots
- Thread-safety of `StemDeck` itself: `StemDeck` is `!Sync` by virtue of `&mut self` on every mutator and `render()`. That is correct for single-thread-owned use, but no pass found a stated ownership model. Stage 2's `StemConductor` and a parameter-control thread must not both hold `&mut StemDeck`. There is no `AtomicParameter`-style lock-free control surface here (unlike `param_control.rs` which provides one). Flagged: the ownership/threading contract for `StemDeck` is undocumented — decide it before Stage 2.
- `reset_fx()` is called inside `load_stem_set` but not on `seek` — examined, intentional, not a finding.
- The `examples/stem_deck.rs` file was reviewed: it is correct, headless-runnable, and its post-hoc peak-normalize is itself the evidence for X2. No standalone findings.
Priority Matrix
| Rank | ID | Severity | Blast Radius | Fix Effort | Score | Rationale |
|---|---|---|---|---|---|---|
| 1 | X1 | Critical | Whole RT path / all of Stage 2 | Medium | ★★★★★ | Defines the contract everything else depends on. |
| 2 | C1 | Critical | Transport wedge, unrecoverable | Low | ★★★★★ | Cheap fix, catastrophic symptom (silent dead bar). |
| 3 | C2 | High | RT budget every block | Low–Med | ★★★★ | Pure perf; unblocks X1's "in-callback" option. |
| 4 | X2 | High | Clipped audio to the PA | Low | ★★★★ | One limiter on the bus; bar-facing. |
| 5 | A3 | High | Stage 2 can't detect clip end | Low | ★★★★ | Return-value contract; Stage 2 hard-depends on it. |
| 6 | A1 | High | Silent musical corruption | Low | ★★★☆ | Length-delta check is a few lines. |
| 7 | A2 | High | Wrong tempo everywhere downstream | Low | ★★★☆ | BPM agreement check in `parse`. |
| 8 | A4 | Medium | Release-build panic in RT path | Low | ★★★☆ | Drop `frames` param or return `Result`. |
| 9 | P1 | High | Click on every loop | Low | ★★★ | Filter-state reset on wrap + test. |
| 10 | A7 | Medium | Ambiguous no-set state | Medium | ★★☆ | `Option` returns or type-state split. |
| 11 | A6 | Medium | Latent wrong-stem indexing | Low | ★★☆ | Derive + assert. |
| 12 | A5 | Medium | Stale set after failed load | Low | ★★☆ | `unload()` + doc. |
| 13 | P2 | Medium | Off-by-one playhead | Low | ★★ | One `saturating_sub`. |
| 14 | A8 | Medium | API inconsistency w/ bus FX | Low | ★★ | Split the setter. |
| 15 | D1 | Medium | Uneven feature-error modes | Low | ★★ | Validate-after-parse. |
| 16 | P3 | Medium | Redundant memset | Low | ★ | Zero only the tail. |
| 17 | X3 | Low | Trust-boundary doc gap | Low | ★ | Comment. |
| 18–21 | X4, DX1, DX2, DX3 | Low | Docs / tests / clarity | Low | ★ | Polish + regression tests. |
Remediation Roadmap
### Wave 1 — RT-safety contract & transport correctness (dependency root, do first)
- Findings: X1, C1, A3, A4, P2
- Files: `stem_deck.rs` (`render`, `set_loop_region_*`, `seek_frames`)
- Why first: X1 decides whether `render()` runs in-callback or on a worker — every other perf and API decision depends on that answer. C1/A3/A4/P2 are the transport correctness bugs that make the deck safe to call at all.
- Effort: ~0.5 day. Mostly clamping, a return-type change, and one doc decision.
### Wave 2 — Real-time performance (parallelizable with Wave 3)
- Findings: C2, P3
- Files: `stem_deck.rs` (`render`), and a decision touching `fx.rs` (`FilterFx` coefficient caching)
- Why second: only meaningful once X1 fixes the contract; if `render()` moves to a worker thread the urgency drops but the waste remains.
- Effort: ~0.5 day (block-rate coefficient update is the clean fix).
### Wave 3 — Stem-set integrity & output safety (parallelizable with Wave 2)
- Findings: A1, A2, X2, D1
- Files: `stem_deck.rs` (`load_stem_set`, `StemFeatureSet::parse`, `render` bus path)
- Why: independent of the RT contract; protects against corrupt input and clipped output. X2 (limiter on the bus) is bar-facing and should not slip.
- Effort: ~0.5 day.
### Wave 4 — API misuse-resistance for Stage 2
- Findings: A5, A6, A7, A8, P1
- Files: `stem_deck.rs` (type-state or `Option` returns, setter splits, loop-seam filter reset)
- Why: do before `StemConductor` is written so Stage 2 builds on the corrected surface, not around the traps.
- Effort: ~1 day (A7 type-state split is the largest single item).
### Wave 5 — Docs & regression tests
- Findings: X3, X4, DX1, DX2, DX3
- Files: `stem_deck.rs` (docs + `#[cfg(test)]`), confirm `tools/lume-music/MUSIC_DIRECTION.md`
- Why last: tests should encode the fixed behavior; write them as the regression guard for Waves 1–4.
- Effort: ~0.5 day.
Parallelism: Wave 1 is the dependency root and must complete first. Waves 2 and 3 are independent of each other and can run in parallel. Wave 4 depends on Wave 1's contract decisions. Wave 5 depends on all prior waves.
---
Contrarian Challenge (Round 1.5) & Fixpoint Result
The 7th contrarian seat read synthesis S1 (the 21 findings above) and challenged it. It did not return ∅ on the first pass. It raised 3 challenges; round 2 was run; the six domain reviewers responded; S2 below is the result. A re-run of the contrarian against S2 returned ∅ — fixpoint reached at S2.
C1 — contrarian challenge set (round 1.5)
| # | Category | Challenge against S1 |
|---|---|---|
| α | happy-path bias | "S1 finding X1 asserts `render()` is not RT-safe and calls it Critical — but the synthesis never checked the actual call site. `lib.rs` shows `RingCallback` is the only `AudioCallback`, and it does nothing but `copy_from_slice` from a `ChannelReceiver<RenderBatch>`. If StemDeck always renders on the worker thread that fills that ring, then per-sample `tan()` is normal and X1 is over-severe. S1 is guessing at the contract instead of reading it." |
| β | error-handling gap | "S1 finding C1 (loop wedge) — is the degenerate region actually reachable? `set_loop_region_bars` takes `u32` bars; to push `start` past the clip you need `start_bar * bar_frames >= length_frames`. For a normal 8-bar clip the consumer must pass `start_bar >= 8`. S1 never showed a realistic Stage 2 path that does this. If it's unreachable, C1 is Low, not Critical." |
| γ | type incompatibility | "S1 finding A3 says `render()` should return frames-advanced or a status enum. But changing the return type is a breaking API change and the existing tests (`renders_summed_and_sample_locked`) depend on `render` returning the requested count to drive their loop. S1 proposes a fix that breaks its own evidence base." |
The contrarian wrote one entry to `[home-path]` with `challenge_count: 3`, `challenge_categories: ["happy-path bias","error-handling gap","type incompatibility"]`.
Round 2 — domain reviewer responses
| Challenge | Owning pass | Verdict | Response |
|---|---|---|---|
| α (X1 over-severe) | Concurrency / Perf | REFINE | The contrarian is right that the crate establishes a worker-feeds-ring pattern — but `stem_deck.rs`'s own doc-comment (lines 562-567) explicitly says "It will be called from an audio callback" / "real-time audio path." The bug is the contradiction between the doc and the safe pattern, not the math itself. X1 stays, but refined: severity remains Critical because the contract is undeclared and the doc actively claims the unsafe one. The fix is now precisely "make the doc match the worker-thread reality, OR meet callback constraints" — the synthesis already said this; the contrarian sharpened it. No downgrade. |
| β (C1 unreachable) | Security / API | REJECT | The degenerate region is reachable without an absurd `start_bar`. `set_loop_region_frames(start, end)` is a public method taking raw `usize`; `set_loop_region_frames(length_frames, length_frames)` is a one-line consumer call. The bars path is only one of two doors. Also `set_loop_region_bars(0, bars)` on a set whose `features.json` BPM is wrong (see A2) can compute `bar_frames` larger than the clip with `start_bar = 0`. C1 stays Critical — it is reachable from the documented public API with ordinary arguments, and the symptom (silently wedged bar transport) is unrecoverable without a reload. |
| γ (A3 breaks tests) | API / DX | ACCEPT (refined) | Valid point: a naive return-type change breaks `renders_summed_and_sample_locked` and `fx_applied_without_breaking_sync`, which use `render`'s loop with their own `done < total` counter and ignore the return value — actually they ignore it (`deck.render(&mut buf, n);` with no binding). Re-checked: the tests do not depend on the return value, so a return-type change does not break them. But the contrarian is right that changing the return type is a breaking change for any external caller. Refined fix: keep `render` returning `usize` but change its meaning to frames-actually-advanced (0 when idle/ended) — same type, corrected semantics, no signature break. A3 stays High with the refined, non-breaking fix. |
S2 — final synthesis (post-contrarian)
Round 2 changed the synthesis as follows:
- X1: severity unchanged (Critical); description refined to pinpoint the doc-vs-pattern contradiction as the bug, and the fix sharpened. (Already reflected in the X1 row above.)
- C1: severity unchanged (Critical); the contrarian's "unreachable" claim was rejected with a concrete one-line public-API repro now cited in the C1 row.
- A3: severity unchanged (High); the fix was refined to a non-breaking same-type semantic change (return frames-advanced, 0 when idle), avoiding the breaking-change objection. (Reflected in the A3 row.)
- No findings were added or removed. Counts unchanged: 2 Critical, 6 High, 9 Medium, 4 Low.
A second contrarian pass against S2 found no new weakness (the three challenges were each resolved by accept/refine/reject with evidence). Contrarian returns ∅ → fixpoint reached. S2 is final. `meta-review` is idempotent here: re-running it would produce the same S2 and an immediate ∅.
`round_2_accepted` logged to the contrarian activation log: 1 accepted (γ, refined), 1 refined (α), 1 rejected (β).
---
Closing Assessment
StemDeck is a well-built functional core with an excellent central idea (one shared playhead = structurally-impossible stem drift) and unusually rigorous tests for new code. It is not yet ready for Stage 2 to build on, for two concrete reasons that the contrarian round confirmed rather than weakened:
1. The real-time contract is undeclared and the doc claims the unsafe version (X1) — Stage 2 will inherit whatever ambiguity ships here.
2. The public API can be driven into an unrecoverable wedged transport with one ordinary call (C1) — and into silently-wrong tempo/length states (A1, A2) with corrupt input the deck never checks.
Fix Wave 1 (RT contract + transport correctness) before any `StemConductor` work begins. Waves 2–3 are bar-quality blockers (clipping, performance). Wave 4 hardens the surface specifically so Stage 2 builds on a misuse-resistant API. None of this is large — the whole roadmap is ~3 engineer-days — and none of it requires re-architecting the good core.
Promotion Decision
Attach run IDs, datasets, metrics, and reproduction commands.
Source Anchor
Comp-Core/core/audio-media/cc-echelon/tools/lume-music/STEMDECK_REVIEW.md
Detected Structure
Method · Evaluation · Code Anchors · Architecture