From f0d5d231714e2bbec12478bd61727dacac6be9b8 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 16:43:24 -0400 Subject: [PATCH 1/5] =?UTF-8?q?docs(q-w0):=20T1=20DSP=20audit=20=E2=80=94?= =?UTF-8?q?=2011=20triaged=20findings,=20SOLA=20engine=20sound;=20stereo?= =?UTF-8?q?=20splice=20decorrelation=20is=20the=20headline?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/product/audit-notes/q-w0-t1-dsp.md | 279 ++++++++++++++++++++++++ 1 file changed, 279 insertions(+) create mode 100644 docs/product/audit-notes/q-w0-t1-dsp.md diff --git a/docs/product/audit-notes/q-w0-t1-dsp.md b/docs/product/audit-notes/q-w0-t1-dsp.md new file mode 100644 index 0000000..86f34d7 --- /dev/null +++ b/docs/product/audit-notes/q-w0-t1-dsp.md @@ -0,0 +1,279 @@ +# Q-W0 Track 1 — DSP / audio algorithm-quality audit (findings) + +Static analysis only; no code changed. Surfaces per the track brief: `src/vst/pitch_shift.{h,cpp}` +(highest priority — GA correlation-aligned SOLA rewrite + GA2 prime + GA3 freeze, never audited), +`src/vst/sampler_core.{h,cpp}`, `src/peaks.*`, `src/wav_trim.*`, the capture/tail paths +(`src/capture.cpp`, `src/capture_realtime.cpp`, `src/realtime_record.h`), `src/vst/master_gain.*`, +`src/vst/velocity_curve.*`. + +Dispositions follow Q-11 (SETTLED): default document-and-defer; a bounded SOLA fix is weighed +before any technique replacement; technique replacement is Daniel's decision at triage. Note on +"assigned wave": **no structural wave Q-W1..Q-W6 opens the `src/vst/` DSP files** (they target +`core/json`, `bank_panel`, `main`, `actions`, `persist`, registration tables) — so any DSP finding +triaged fix-now is remediated in Q-W0 itself or folded in as a new point before Q-W1 begins. + +I cannot listen; every artifact below is stated as mechanism + predicted audible consequence. +Perceptual materiality is Daniel's call. + +--- + +## Overall verdict on the pitch engine (Q-11 framing) + +The correlation-aligned SOLA in `pitch_shift` is **not a reinvented wheel in the pejorative +sense** — single-tap SOLA with normalized cross-correlation splice alignment, parabolic sub-sample +peak refinement, and amplitude-complementary raised-cosine fades *is* an established time-domain +technique family (SOLA/TD-PSOLA lineage), and the implementation is unusually well-defended: +normalized (not raw) correlation, ratio-scaled fade lengths with a drain-headroom derivation, +prime-with-real-content onset, frozen-writer tail, filled-span clamping, and double-before-int64 +clamps at the overflow-prone spots. The RT discipline holds throughout: `process()` does no +allocation, no locks; the splice burst is bounded and fires once per splice cadence, not per frame. +**No technique replacement (phase-vocoder / WSOLA) is warranted on this evidence.** The findings +below are bounded-fix candidates and documented limits within the existing approach, exactly the +Q-11 escalation ladder's first rung. + +--- + +## Findings + +### T1-01 — Stereo Preserve: per-channel independent splice alignment decorrelates L/R +- **Location:** `src/vst/sampler_core.cpp` `Voice::advanceFrame` (Preserve branch, ~577–589) + + `src/vst/pitch_shift.cpp` `PitchShifter::splice`. +- **Mechanism:** a stereo Preserve voice owns two `PitchShifter`s, each running its **own** + correlation search on its own channel's PCM. `bestLag + frac` differ per channel at every splice, + so after the first splice the two read taps sit at different ring positions — an inter-channel + time offset of up to ±`maxLag` (= window/4 ≈ **12.5 ms** at the 50 ms window), re-drawn at every + splice (cadence ≈ window/|ratio−1| frames). The splice *schedules* also diverge (delay drift + depends on tap position), so L and R fade at different times. +- **Predicted artifact:** on genuinely stereo captures played through Preserve off-root: stereo + image wander / widening that changes at the splice cadence, and comb-filter coloration on any + mono sum. Correlated stereo content (the common case for a captured bus) is the worst case. + Dual-mono (mono sample in stereo bus) is unaffected — the code correctly mirrors one shifter. +- **Severity:** **High** (Preserve is the product-default pitch engine and the output bus is + permanently stereo with channel mode auto-defaulting from the capture — this hits the flagship + path on stereo material). +- **Disposition proposal:** this is the strongest candidate for a **bounded SOLA fix** (Q-11 rung + 1): link the channels — run the correlation search once (on the L+R mid signal, or on L as + master) and apply the same `bestLag + frac` and splice schedule to both channels. Standard + practice for stereo SOLA. It reshapes the `PitchShifter` seam slightly (splice decision must be + computable once and applied to two rings — e.g. a lag-provider hook or a two-channel shifter), + but no technique change and no new dependency. Recommend **fix-now in Q-W0** pending Daniel's + triage call; if deferred, record it as the known stereo-Preserve limitation. + +### T1-02 — Ratio slew mid-fade can drain the outgoing tap past the writer +- **Location:** `src/vst/pitch_shift.cpp` `splice()` fade-length cap (~301–311) + `process()` tap + advance (~360–366). +- **Mechanism:** `fadeLen_` is capped from the drain headroom **at splice time** using the + then-current `ratio_`. The pitch envelope legitimately slews the ratio per frame + (`setShiftRatio` mid-fade). A ratio that **rises** after the splice (pitch-env attack toward a + positive peak, or attack from a negative dip back to base) drains tap B faster than the cap + assumed; the code comment claims the 2-frame margin covers "any realistic per-frame bias", but + the margin is absolute, not slew-proportional: e.g. a splice at ratio ≈ 1 sets + `fadeLen_ = window/4` with no cap (drainRate ≈ 0), and a pitch-env attack ramping to +24 st + (ratio 4) within those ~12 ms drains tap B ≈ 3·window/4 — far past the `dLow` ≈ window/4 + headroom. Tap B laps the parked/advancing writer and reads ring-length-stale content at up to + ~half fade gain. +- **Predicted artifact:** a periodic click/garble burst at the splice cadence during fast upward + pitch-envelope ramps on Preserve voices. Only reachable with the AD pitch envelope enabled and + steep; base transpositions (constant ratio) are correctly covered by the existing cap. +- **Severity:** Med. +- **Disposition proposal:** document-and-defer (needs pitch-env + Preserve + steep attack to + trigger), with a cheap bounded fix noted for whenever the file is opened: re-tighten `fadeLen_` + when the ratio increases mid-fade (the `freezeTail()` re-anchor block is the exact pattern to + reuse), or clamp tap B's delay to ≥ 2 during a fade. + +### T1-03 — Preserve prime ignores the Trigger play-end bound (short-span rings hold cut content) +- **Location:** `src/vst/sampler_core.cpp` `Voice::start` prime block (~375–400) vs. the GA3 + `feedBound` in `advanceFrame` (~568–572). +- **Mechanism:** the per-frame feed treats `playEnd_` (Trigger) / `frameCount` as source + exhaustion and freezes the writer so "no padding ever enters the ring" (GA3). But `start()` + primes a **full window** from `pcm[q]` bounded only by `frameCount` — not by `playEnd_` — and + pads with zeros past the sample end while declaring the whole window `filled_`. Two + consequences for spans shorter than the 50 ms window: (a) a Trigger zone's ring holds real PCM + **past the user's chosen stop**, which an up-shifted tap can reach and play (transposed) before + `readPos_ ≥ playEnd_` frees the voice; (b) a sample shorter than the window gets zero padding + inside the ring as declared-valid history, so splices can land in silence — a bounded re-entry + of exactly the burst/gap onset artifact GA2/GA3 eliminated, scoped to sub-50 ms material (short + drum one-shots are realistic content). +- **Severity:** Med (bounded to short spans / short samples in Preserve; inaudible for spans ≥ one + window). +- **Disposition proposal:** bounded fix candidate — prime `min(window, span-to-feedBound)` frames + and call `freezeTail()` immediately after prime when the span is shorter than a window (the GA3 + machinery then recycles the real short tail, which is its designed behavior). Small and + contained in `Voice::start`. Recommend fix-now in Q-W0 if Daniel agrees the short-one-shot case + matters; else document-and-defer with this note as the record. + +### T1-04 — No sustain-loop crossfade (hard loop seam) +- **Location:** `src/vst/sampler_core.cpp` `Voice::advanceFrame` loop wrap (~493–498, 610–612). +- **Mechanism:** the sustain loop wraps by subtracting the loop length (phase-preserving) and the + interpolation partner wraps `i1 → loop.start`, giving one-sample continuity only. There is no + crossfade region: unless the user's loop points sit at amplitude/slope-matched positions, every + loop pass produces a step discontinuity — a click at the loop rate. `waveform_view`'s + zero-crossing snap on the loop markers mitigates but does not remove it (zero crossings with + mismatched slopes still click). Established samplers crossfade the loop seam (equal-power over a + user- or fixed-length region). +- **Severity:** Med (musically prominent when it hits, fully user-avoidable with careful loop + placement). +- **Disposition proposal:** document-and-defer — a loop-crossfade is a *feature* (needs a + crossfade-length parameter and UI surface), not a bug fix; wrong scope for a reorg phase. Record + as a known limitation beside the zone-loop spec. + +### T1-05 — Linear interpolation + no band-limiting on repitch (both engines) +- **Location:** `src/vst/sampler_core.cpp` Varispeed read (~607–624); `src/vst/pitch_shift.cpp` + `readTap` (~172–185). +- **Mechanism:** all fractional reads are first-order (linear). Linear interpolation's frequency + response rolls off highs and leaks imaging sidebands (the interpolation image spectrum is + attenuated only ~12 dB/oct); Varispeed up-shifts additionally alias (reading faster than 1× with + no pre-filter folds source content above the post-shift Nyquist back into band). This is classic + hardware-sampler behavior — often accepted, sometimes desired — and both engines share it + consistently. +- **Severity:** Low (quality ceiling, not a defect; deterministic and stable). +- **Disposition proposal:** document-and-defer as a recorded trade-off. If a quality bump is ever + wanted, a 4-point cubic Hermite read is a drop-in bounded upgrade at both call sites (no + structural change); band-limited varispeed is a much bigger lift and not recommended. + +### T1-06 — Correlation search: coarse step 4 can mis-lock on very high fundamentals; maxLag bounds alignment to ≥ ~80 Hz +- **Location:** `src/vst/pitch_shift.cpp` `splice()` search loops (~240–257) and `configure()` + geometry (~68–72). +- **Mechanism:** two documented-by-construction limits. (a) The coarse search samples the + correlation every 4 lags and refines ±3 around the coarse best — full integer coverage only + *near* the coarse winner. For content whose correlation oscillates with period < ~8 samples + (fundamentals above ~5.5 kHz at 44.1k), the coarse grid can alias and lock a non-optimal region; + the splice then lands up to half a period misaligned. (b) `maxLag = window/4` (~12.5 ms) cannot + span a full period below ~80 Hz, so deep-bass fundamentals cannot be period-aligned and splices + degrade toward unaligned OLA there. Both are inherent range/cost trades every SOLA makes; the + in-code comment already states (b). +- **Severity:** Low (edge content: pure tones > 5 kHz, fundamentals < 80 Hz). +- **Disposition proposal:** document-and-defer; record both bounds as the engine's stated + operating range. No change recommended — widening either costs splice-burst CPU linearly. + +### T1-07 — `splice()` up-jump clamp comment contradicts the code (margin direction) +- **Location:** `src/vst/pitch_shift.cpp` ~196–210. +- **Mechanism:** the comment derives the "tight cap" as `filled_ - d - maxLag_ - 2`, then says the + code's `- 1` is "one sample of conservative margin" — but `-1` permits a *larger* jump than + `-2`, i.e. the code is *less* restrictive than the comment's own derivation; the sentence has + the direction backwards. Re-deriving: the deepest probe is the parabola's outer lag at + `d + jump + maxLag + 1` (the interpolator's `i1 = i0 + 1` read-ahead moves *younger*, not + deeper), so the code's `-1` is exactly tight and the comment's `-2` double-counts the + interpolator. No out-of-range read either way; the comment is wrong, not the code. +- **Severity:** Low (doc-only; misleads the next maintainer of a safety-critical clamp). +- **Disposition proposal:** fix-now (comment rewrite, zero behavior change) — fold into whichever + Q-W0 remediation touches `pitch_shift`; if none does, a standalone one-line doc fix in Q-W0. + +### T1-08 — Linear-in-amplitude ADSR decay/release segments +- **Location:** `src/vst/sampler_core.cpp` `AdsrEnvelope::tick` (~131–170). +- **Mechanism:** decay and release ramp linearly in amplitude. Constant-slope amplitude is + constant-dB-rate nowhere: a long release spends most of its wall-clock at perceptually loud + levels then collapses abruptly (in dB terms the curve is logarithmic-late). Classic samplers use + exponential (constant-ratio) segments for decay/release. The evaluator itself is correct and + well-tested (release-from-current-level, hold-0 byte-compat re-dispatch are both right). +- **Severity:** Low (character, not correctness; the perceptual judgment is Daniel's). +- **Disposition proposal:** document-and-defer. An exponential-segment option is a contained + evaluator change but alters every existing instrument's envelope feel — a product decision, not + a Q-W0 cleanup. + +### T1-09 — Takeover-declick: `declickR_` is dead state +- **Location:** `src/vst/sampler_core.cpp` (~598–599, 639–646, 515–518). +- **Mechanism:** both channels deliberately share one blend weight (`declickL_` — commented), but + `declickR_` is still seeded and decayed every frame and never read for output. Dead state that + invites a future L/R-weight divergence bug. The blend itself audits **clean**: `out' = + (1−w)·out + w·ref` is a convex combination for w ∈ [0,1], so `|out'| ≤ max(|out|,|ref|)` — the + rev-2 boundedness claim is mathematically sound, the boundary-frame identity holds, and the + ring-out path on voice end is handled (the peer-path symmetry is present). +- **Severity:** Low (hygiene; no audio effect). +- **Disposition proposal:** fix-now-trivial (delete the field or rename the shared weight) — fold + into any Q-W0 edit of `sampler_core`; not worth its own change otherwise. + +### T1-10 — `planWavTruncate` silently drops chunks located after `data` +- **Location:** `src/wav_trim.cpp` `planWavTruncate` (~151), `capture_realtime.cpp` + `trimAutoTailInPlace`. +- **Mechanism:** the plan truncates the file at `dataByteOffset + keptDataBytes`. Any RIFF chunk + REAPER wrote *after* the data chunk (bext/iXML/smpl orderings vary by writer) is discarded; the + RIFF size is patched consistently so the result is a valid WAV, but metadata is lost without a + trace. The PCM and the trim boundary math themselves audit clean (file-rate-authoritative frame + math, scan confined to the tail region, -72 dB threshold single-sourced from + `kAutoTrimThresholdDb`, one-frame-past-last-audible per spec, no-trim fallbacks total). +- **Severity:** Low (metadata only; audio unaffected; trim is a convenience path). +- **Disposition proposal:** document-and-defer — note the behavior in the header's FORMAT + ASSUMPTION block when the file is next touched. Preserving trailing chunks would complicate the + single-truncating-write design for no audio benefit. + +### T1-11 — `makeUniqueTag` has one-second resolution (collision window) +- **Location:** `src/capture.cpp` (~224–227) and `src/capture_realtime.cpp` (~120–123). +- **Mechanism:** the uniqueness tag is `std::time(nullptr)` — 1 s resolution. Two captures of the + same `baseName` within the same wall-clock second derive the same file stem: the offline path + would overwrite the first render's file and mint two Samples with colliding ids. Reachable in + practice via `batch_capture` driving several short renders back-to-back. The realtime path + can't self-collide (transport exclusivity) but shares the pattern. DSP-adjacent rather than + DSP; recorded here because the capture paths are this track's surface — Track 2 may claim it. +- **Severity:** Low-Med (silent data loss on collision; narrow window). +- **Disposition proposal:** fix-now candidate, trivial: append a per-session monotonic counter to + the tag (both call sites). Belongs wherever Track 2/triage routes capture-path hygiene; Q-W3 + (main/orchestration split) is the nearest wave that opens the extension capture flow, else Q-W0. + +--- + +## Surfaces that came back clean + +- **`src/peaks.*` — clean.** The bin partition `[b·frames/binCount, (b+1)·frames/binCount)` is + exact integer math, remainder-distributing, no dropped tail; overflow guarded; short-buffer + clamped; per-channel with no fold (invariant honored). `columnMinMax` mirrors the partition with + 64-bit products and the enclosing-bin fallback. `lastFrameAboveThreshold` scans backward with a + correct strictly-greater test and no wrap hazard. +- **`src/wav_trim.*` — clean** except T1-10 (metadata note). Chunk walk is bounds-checked and + total; even-byte padding honored; extensible-format float discrimination via the SubFormat GUID + leading tag is correct; LE reads via `memcpy` (no aliasing UB); truncate plan never grows. +- **`src/capture.cpp` (offline) — clean** from the algorithm-quality lens except T1-11. Exact + unrounded bounds, dither forced off (bit-identical repeats), float32-only with ground-truth + format blob, surgical trim-end normalize only in Auto, full snapshot/restore RAII. The + precision-invariant plumbing is disciplined. +- **`src/capture_realtime.cpp` + `src/realtime_record.h` — clean** except T1-11 (shared) and the + already-in-code DAW-verify flags (take/frame-0 alignment assumption for the trim; abort()'s + best-effort finalize racing the flush — both explicitly documented in place, correctly scoped). + The record state machine's decisions are pure and ceiling-bounded; the trim is best-effort and + never eats the range body. +- **`src/vst/master_gain.*` — clean.** Taper endpoints single-sourced; norm-0 true-zero detent + with the finite −60 dB floor; unity at ≈ 0.714 as documented; inverse collapses sub-floor values + to the detent (documented); non-finite input clamped. The dB↔linear math is correct. +- **`src/vst/velocity_curve.*` — clean.** The Fritsch–Carlson tangent is the standard + weighted-harmonic-mean form (w₁ = 2h₂ + h₁, w₂ = h₂ + 2h₁), which bounds m ≤ 3·min(d₁,d₂) — + monotonicity and no-overshoot inside [0,1] hold as claimed; sign-change/flat neighbors pin to 0; + zero-span steps and coincident-X knots are handled; deserialize repairs the invariant + defensively. `eval` once per note-on keeps it off the per-frame path. +- **`sampler_core` voice/steal/mono machinery — clean** (beyond the findings above): the steal + policy is deterministic and as documented; the mono held-stack has correct range guards against + uint8 aliasing, order-preserving removal, per-note velocity for retrigger fallback, and CC 123 + as its only reset path; `soundingNote()` correctly excludes ring-out tails from the Preserve cap + and legato predicates; the two-tier panic semantics are right; both render overloads share one + summation discipline with no allocation; envelope/keymap resolution honors the rate-free-seconds + invariant (frames resolved at keymap build against the live rate — the prior frame-domain + incident is not repeated here). The per-frame `std::pow` when the pitch envelope is active is + bounded and acceptable. +- **`pitch_shift` core machinery — clean** (beyond the findings above): the safe-band geometry, + filled-span clamps, normalized correlation, parabolic sub-sample refinement, complementary + raised-cosine fade (correct for phase-aligned content; the −6 dB midpoint on uncorrelated + content is a documented, benign trade), `freezeTail`'s fade re-anchor continuity, and the + down-shift ring-lap margin all audit sound. Down-shift writer-lap is unreachable for any ratio + above ≈ −109 st; up-shift fade drain is covered to +24 st and beyond by the ratio-scaled cap + (T1-02 is the slew case only). + +--- + +## Summary table + +| ID | Surface | Finding | Severity | Disposition proposal | +|-------|----------------------------------|------------------------------------------------------------|----------|---------------------------------------------------| +| T1-01 | pitch_shift + sampler_core | Stereo Preserve: independent L/R splice alignment | High | Bounded SOLA fix (linked lag) — recommend fix-now in Q-W0; Daniel's call | +| T1-02 | pitch_shift | Ratio slew mid-fade can lap the outgoing tap | Med | Document-and-defer; bounded re-cap noted | +| T1-03 | sampler_core (Preserve prime) | Prime ignores Trigger playEnd / pads short samples | Med | Bounded fix candidate in Q-W0; else defer w/ note | +| T1-04 | sampler_core (loop) | No sustain-loop crossfade (hard seam) | Med | Document-and-defer (feature, not reorg scope) | +| T1-05 | sampler_core + pitch_shift | Linear interp, no band-limiting on repitch | Low | Document-and-defer (recorded trade-off) | +| T1-06 | pitch_shift | Coarse-search HF mis-lock; ≥ ~80 Hz alignment bound | Low | Document-and-defer (stated operating range) | +| T1-07 | pitch_shift | maxJump clamp comment contradicts code | Low | Fix-now (comment-only), in Q-W0 | +| T1-08 | sampler_core (ADSR) | Linear-amplitude decay/release segments | Low | Document-and-defer (product decision) | +| T1-09 | sampler_core (declick) | `declickR_` dead state | Low | Fix-now-trivial, fold into any Q-W0 edit | +| T1-10 | wav_trim | Truncate drops post-`data` chunks (metadata) | Low | Document-and-defer (header note) | +| T1-11 | capture.cpp + capture_realtime | 1 s-resolution unique tag → batch collision window | Low-Med | Fix-now candidate (monotonic counter); route at triage | + +Clean surfaces: `peaks`, `master_gain`, `velocity_curve` (fully); `wav_trim`, offline + realtime +capture paths, and the non-flagged machinery of `sampler_core` / `pitch_shift` (clean with the +noted exceptions above). From dfe6ccbddd5bce8a16852d8c68367d6f1ed744c0 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 16:43:34 -0400 Subject: [PATCH 2/5] =?UTF-8?q?docs(q-w0):=20T2=20architecture=20audit=20?= =?UTF-8?q?=E2=80=94=20wire-cursor=20hardening=20drift,=205th=20JSON=20dec?= =?UTF-8?q?oder,=205x=20readFileBytes,=20rect-type=20zoo;=2011=20triaged?= =?UTF-8?q?=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../audit-notes/q-w0-t2-architecture.md | 333 ++++++++++++++++++ 1 file changed, 333 insertions(+) create mode 100644 docs/product/audit-notes/q-w0-t2-architecture.md diff --git a/docs/product/audit-notes/q-w0-t2-architecture.md b/docs/product/audit-notes/q-w0-t2-architecture.md new file mode 100644 index 0000000..efea37c --- /dev/null +++ b/docs/product/audit-notes/q-w0-t2-architecture.md @@ -0,0 +1,333 @@ +# Q-W0 Track 2 — architecture-smell audit (functional lens) + +Static analysis of the whole `src/` tree (extension + `src/vst/`), 2026-07-28, branch +`pq-w0-audit`. Complement to the grep-verified SOLID audit (§2) and naming audit (§2b) in +`docs/product/code-organization.md` — this track reports the **functional** smells those did not +target: duplicated *algorithms* (not merely duplicated responsibilities), reinvented wheels, +poor abstractions, and leaky pure/shell boundaries. Findings already catalogued there (the four +god-modules, the 4× JSON `Parser`, fat headers, `promptText`/`mintBankId` duplication, namespace +flatness, naming families) are **not restated**; where a finding below touches the same file it +is because the functional mechanism is new. + +Every claim below was verified by grep/read of the actual tree. Line numbers are as of this +audit's snapshot. Wave assignments reference PLAN.md §Q-W1..Q-W6. + +--- + +## Findings + +### T2-01 — 3× copy-pasted length-prefixed wire `Cursor`, with security-hardening drift +**Location:** `src/provenance.cpp:56–137`, `src/assignment_request.cpp:25–121`, +`src/sample_usage.cpp:25–89` (plus a fourth sibling: `src/vst/bank_sync.cpp:11–28` +`parseBankGeneration` re-rolls the same guarded decimal accumulate). + +**Mechanism.** The `':'` ext-state wire idiom ("one grammar across every ext-state +seam", per sample_usage's own comment) is implemented as three near-identical `putField` + +`Cursor` copies — and they have **drifted on the hardening**. The two newer copies +(`assignment_request`, `sample_usage`) carry a 20-digit length cap and an overflow guard +(`len > (SIZE_MAX - digit) / 10 → fail`) plus the subtraction-first bounds check +(`len > s_.size() - start`). The oldest copy (`provenance.cpp:65–82`) has **neither**: a crafted +long digit run wraps `len` silently, and the additive bounds check `start + len > s_.size()` +can itself wrap, letting a wrapped length pass. Downstream, `parseFingerprint` +(`provenance.cpp:197–199`) calls `r.trackGuids.reserve(guidCount)` on an **unbounded** count +parsed by the equally unguarded `fieldSizeT` — a corrupt/crafted `Sample.provenance` string in +the bank JSON can drive `reserve(huge)` into `std::length_error`/`bad_alloc` thrown through the +shell. (`sample_usage` fixed exactly this with its `count > wire.size()/4 + 1` sanity bound, +`sample_usage.cpp:121`; the fix was never backported.) `std::string::assign` clamping keeps the +wrap short of UB, but the parse-integrity promise ("never UB, never a partial value") is upheld +in two copies and eroded in the third — the textbook cost of a duplicated algorithm. + +**Severity:** High (the drift already produced a concrete robustness gap on a persisted, +user-editable input; the class of bug will recur with every new wire seam). +**Disposition:** **fix-now, split:** (a) backport the hardened `field()` + a count sanity bound +to `provenance.cpp` **in Q-W0** — small, pure, existing `provenance_tests` covers round-trip and +malformed-input paths; (b) the structural collapse (one shared `wire` codec module beside +`core/json`, all three seams + `parseBankGeneration` consuming it) belongs to **Q-W1**, which is +already the serialization-extraction wave. Rationale: the hazard is cheap to close now; the +dedup is a relocation-adjacent move that should ride the wave already creating `core/`. + +### T2-02 — a FIFTH hand-rolled JSON decoder the §2 audit did not count +**Location:** `src/tail_control.cpp:88–130` (`valueAfterKey` + `deserializeTailSetting`). + +**Mechanism.** The catalogued DRY violation is "JSON `Parser` duplicated 4×" (`bank_model`, +`bank_book`, `view_mode_model`, `owned_manifest`). `tail_control` carries a fifth, structurally +different JSON decode: a substring-scan reader (`json.find("\"key\"")` → skip ws → parse token). +It is correct for the flat single-object payload it reads (the file argues this honestly), but +it is a fifth place JSON-reading behavior is defined, with different tolerance semantics (a key +found inside a *string value* would match — impossible today only because the writer is its own +sole producer). If Q-W1 extracts `core/json` from the four `Parser`s and misses this site, the +"one JSON path" goal is silently not achieved. + +**Severity:** Med (no live bug; a completeness gap in the already-planned fix). +**Disposition:** **fix-now, folded into Q-W1** — add `tail_control` to the Q-W1 consumer list +explicitly. Rationale: zero extra cost when `core/json` lands; a stray fifth decoder afterward +would be a defect of the wave. + +### T2-03 — `readFileBytes` hand-rolled five times, both sides of the artifact split +**Location:** `src/capture.cpp:232`, `src/capture_realtime.cpp:329` (as `readAllBytes`), +`src/ingest.cpp:86` (comment admits: "of capture.cpp's readFileBytes"), +`src/vst/reasampler_processor.cpp:72`, and inline in `src/vst/reasampler_editor.cpp:749–756`. + +**Mechanism.** The identical ifstream-binary-ate/tellg/read whole-file loader exists five times +(two spellings, one anonymous inline). Well past extract-on-third-occurrence, and the copies +already disagree cosmetically (name, empty-on-failure comment placement) — the next divergence +will be behavioral (e.g. one copy gaining a size ceiling the others lack). + +**Severity:** Med. +**Disposition:** **fix-now, folded into Q-W1** — a trivial pure `readFileBytes` helper in the +`core/` utility home Q-W1 creates; both CMake targets link it. Rationale: five occurrences of a +ten-line function is pure debt with a zero-risk fix, but creating its home is exactly Q-W1's +job — doing it days earlier in the flat tree would just move the file twice. + +### T2-04 — the growing `GetProjExtState` read loop, three copies, pure half only half-used +**Location:** `src/persist.cpp:140–157` (`getProjExtStateString`), +`src/vst/reaper_bridge.cpp:93–107` (self-described "mirrors persist.cpp's growing strategy"), +`src/usage_scan.cpp:168–181` (self-described "the persist.cpp idiom"). + +**Mechanism.** The grow-buffer-until-it-fits retry loop over `GetProjExtState` is implemented +three times, in three TUs, on both sides of the split. The fiddly part — interpreting the int +return against the filled buffer — is *already extracted pure* as +`bridge_marshal::decodeGetProjExtState`, but only `reaper_bridge` consumes it; `persist` and +`usage_scan` interpret `rv` inline with their own conventions (persist: `rv <= 0` → absent; +bridge: return AND non-empty buffer). The absent-vs-truncated-vs-empty semantics are precisely +the kind of edge that drifts when defined thrice. `usage_scan`'s copy is prune-safety-adjacent +(an unreadable usage record must abort the prune) — its read loop deserves the tested pure +decode, not an inline reimplementation. + +**Severity:** Med. +**Disposition:** **fix-now, assigned to the downstream wave that opens `persist`** +(Q-W4 per the current wave map; whichever wave splits `persist.cpp` is the moment). Generalize +the retry policy (next-capacity/done decision) into `bridge_marshal` (or its `core/` successor) +and make all three loops consume it. Rationale: touching persist's session machinery outside +its own wave risks the highest-traffic shell for a dedup that has no live bug today. + +### T2-05 — 19 rect structs + ~15 inline point-in-rect predicates across the pure UI family +**Location (structs):** `action_bar.h:61`, `bank_grid.h:23`, `card_drag.h:109`, +`component_geometry.h:28,53,106`, `drag_out.h:40`, `footer_bar.h:41`, `mode_switch.h:21,35`, +`overflow_menu.h:23,39`, `prune_button.h:32,46`, `tab_strip.h:24,51`, `tooltip.h:20`, +`vst/editor_geometry.h:19`, `vst/velocity_curve.h:124`. +**Location (predicates):** inline half-open `px >= r.x && px < r.x + r.width && …` re-typed in +`action_bar.cpp`, `bank_grid.cpp`, `card_drag.cpp` (×2), `component_geometry.cpp` (×2), +`drag_out.cpp`, `footer_bar.cpp`, `mode_switch.cpp`, `overflow_menu.cpp`, `prune_button.cpp`, +`tab_strip.cpp` (×2), `bank_panel.cpp:1049`, plus `vst/editor_geometry.cpp:20`. + +**Mechanism.** §2b.2 catalogued the *naming/collision* half of this (the `footer_bar.h` "NAME +NOTE" hand-checking smell). The functional half is uncatalogued: nineteen structurally identical +axis-aligned `{x, y, w, h}` record types, each with its own hand-typed containment predicate. +Every new pure-UI module re-mints both. This is Daniel's heuristic (b) verbatim: N near-identical +concrete implementations that one shared type collapses at compile time — one `ui::Rect` + one +`contains(Rect, x, y)` free function (both already exist in embryo as `vst/editor_geometry`'s +`Rect`/`contains`), with per-module aliases or thin wrappers only where a struct carries extra +fields (e.g. `TabRect::index`). Zero runtime cost; deletes ~15 chances for the next half-open/ +closed-interval inconsistency to slip in. + +**Severity:** Med. +**Disposition:** **fix-now, folded into Q-W2** (the ui/ relocation wave) — collapsing the type +zoo is nearly free precisely when every one of these files is being moved and re-namespaced; +doing it pre-reorg would churn 19 headers twice. Rationale: same-moment-as-relocation is the +stated principle for renames (§2b intro); it holds identically for type unification. + +### T2-06 — pure-computable layout math stranded in the VST editor shell (the §2 scope gap) +**Location:** `src/vst/reasampler_editor.cpp` — `SampleFaceLayout` (~line 922) and its builder, +`ClusterLayout` (~line 964), `zonesStripArea`/`noteEntryFieldsArea`/`noteEntryFieldRect`/ +`zonesControlPanel`/`zonesDeckArea`/`zonesCurveButton` (lines 992–1042), the channel-toggle +segment rects (~line 1065), banner rect math (~line 1195), among ~49 inline geometry +computations across the 3,065-LOC TU. + +**Mechanism.** The codebase's own grammar homes exactly this class of math in pure modules +(`editor_geometry`, `knob_deck`, `curve_popup`, `capture_browser`, …), yet the editor shell has +accreted a second, untested layout layer: whole named layout structs and pure `Rect → Rect` +functions that take only ints and rects, compiled into the one TU that cannot be unit-tested +without a host window. This is the mirrored form of the pure/shell leak (algorithm math living +untestable in a shell). Note also the audit-scope gap this exposes: §2's god-module catalogue +covered the extension tree only — `reasampler_editor.cpp` (3,065 LOC) and +`reasampler_processor.cpp` (1,164 LOC) repeat the bank_panel pattern on the VST side and appear +in no existing finding. + +**Severity:** Med (no correctness bug found in the stranded math; the cost is untestability and +the growth trajectory — the editor gained ~500 LOC/phase through r11). +**Disposition:** **document-and-defer, with a named reshape:** Q-W0 should surface a downstream +point (the wave that opens `src/vst/`, or a new one) hoisting the Sample-face/Zone-panel layout +into the existing pure homes (`editor_geometry` is the natural owner). Rationale: a hoist is a +behavior-preserving mechanical move best done under the reorg's test discipline, not pre-reorg; +but it must be a recorded point or the layer keeps growing. + +### T2-07 — the extension links the entire voice engine to serialize one preset blob +**Location:** `CMakeLists.txt:383–385` (`instrument_drop` → PUBLIC `sample_map`); +`src/instrument_drop.cpp` includes `vst/sample_map.h`, which pulls `sampler_core.h` → +`pitch_shift.h` + `velocity_curve.h`. + +**Mechanism.** `instrument_drop` (extension side) deliberately reuses +`sample_map::serializeComponentState` so the `.vstpreset` payload and the instrument's own +reader cannot drift — the right DRY call, explicitly documented in CMake. But the shared writer +lives *inside* the module that also owns zone resolution, WAV decode plumbing, and (via header +fan-in) the whole voice engine — so `reaper_reasampler` compiles and links `sampler_core`, +`pitch_shift`, and `velocity_curve` object code it never executes. The abstraction is right; its +*granularity* is wrong: the ComponentState codec is not separable from the engine stack today. + +**Severity:** Low (dead weight in the binary and a misleading dependency edge; no runtime cost — +heuristic (c) is about call chains, which this does not add). +**Disposition:** **document-and-defer to Q-W1/Q-W2 module-homing:** when serialization gets its +`core/` home, split a `component_state` codec module (types + serialize/deserialize only) out of +`sample_map`; both artifacts link the codec, only the VST links the engine. Rationale: purely +structural, zero behavior change, and exactly the kind of module-boundary decision the reorg +waves exist to make once, deliberately. + +### T2-08 — WAV/RIFF byte-format knowledge spread across four modules, two chunk walkers +**Location:** `src/wav_trim.cpp` (canonical parse: `parseWavLayout`/`extractFloatFrames`), +`src/capture_paths.cpp:31–115` (a second, independent RIFF chunk walker for content hashing), +`src/ingest.cpp:108–160` (hand-built 32f WAV writer), `src/capture_realtime.cpp:423` (in-place +RIFF/data size patch). + +**Mechanism.** The tree is disciplined about *decoding* ("no third WAV reader" — sample_map, +editor, processor all route through `wav_trim`), but RIFF *container* knowledge is still minted +per site: `capture_paths` walks chunks with its own tag/size/pad-byte logic to hash `fmt `+`data` +while skipping metadata; `wav_trim` walks the same container shape for layout; `ingest` writes +headers by hand; `capture_realtime` patches sizes by offset. Four places know the RIFF framing +rules (even-byte padding, chunk-header arithmetic); a drift in any one (e.g. pad-byte handling) +would desynchronize hashing from decoding — the dedup-by-hash and null-test invariants both sit +on this. + +**Severity:** Low (all four are currently correct against each other by inspection; the smell is +the maintenance surface, not a live divergence). +**Disposition:** **document-and-defer** — consolidate into a `core/wav` home (walker + layout + +writer + patch) when the reorg assigns module homes. Rationale: pre-reorg consolidation churns +the capture hot path (§3 guardrail) for no functional gain; the reorg wave that relocates +`wav_trim` is the natural moment. + +### T2-09 — the two capture backends' Sample-stamping epilogue is copy-paste with silent divergences +**Location:** `src/capture.cpp:490–537` vs `src/capture_realtime.cpp:505–540`. + +**Mechanism.** The finished-capture metadata stamp — `trackGuids`, `channelCount`, `sampleRate`, +`Master_GetTempo`, the `TimeMap_GetTimeSigAtTime` block, the WAV-aware `hashWavContent` content +hash (comment block duplicated verbatim, ~10 lines), `createdTimestamp` — is written twice, once +per backend. The copies have already diverged in quiet ways: offline passes `proj = nullptr` +(active project) to `TimeMap_GetTimeSigAtTime` while realtime pins `st.proj_`; the sampleRate +fallback logic differs in shape; realtime overrides `lengthSeconds` post-hoc. Some divergence is +semantic (realtime's tail-trim length), but the shared stamp is one concept — a future field +(e.g. a new provenance stamp) must currently be added in two places, and the time-sig +active-project vs pinned-project asymmetry is exactly the kind of drift that produces a +wrong-project stamp during a background-project capture. + +**Severity:** Med. +**Disposition:** **fix-now, folded into Q-W3** (the wave already hoisting capture orchestration +out of `main.cpp` / right-sizing `capture.h`). Extract a `stampCaptureSample(Sample&, const +CaptureRequest&, ReaProject*)` shared helper; the divergent bits (length override) stay in the +realtime caller. Rationale: the fix touches both backend TUs, which Q-W3 opens anyway; doing it +there keeps one review of the precision-invariant-adjacent code. + +### T2-10 — the two thumbnail pipelines' cache-invalidation strategies have drifted +**Location:** `src/bank_panel.cpp:422–483` (extension: `computeThumbnail` + pure +`ThumbnailKey{id, width, generation}` via `bank_grid::thumbnailKeyString`) vs +`src/vst/reasampler_editor.cpp:715–789` (VST: `monoPcmFor` keyed by bare `sampleId`, +`thumbnailFor` keyed by ad-hoc `sampleId + "|" + binCount`, invalidated by wholesale +`clear()` at lines 159–160/894). + +**Mechanism.** The dock panel and the VST browser render the same thumbnails through the shared +`peaks::computeEnvelope`, but the caching layer around it was re-designed independently on each +side: the extension bakes the bank generation into a *pure, tested* key type; the editor +hand-concats a string key with no generation and relies on call-site `clear()`s (bank-refresh, +resize). Both are correct **today** — but correctness on the editor side is distributed across +remembering every clear site, and the bin-clamp guard comment ("computeEnvelope pads binCount > +frameCount…") is duplicated verbatim in both TUs (`bank_panel.cpp:462`, +`reasampler_editor.cpp:780`), marking the copied design. A future refresh path that forgets the +clear shows stale waveforms with no test to catch it. + +**Severity:** Low. +**Disposition:** **document-and-defer** — when T2-06's layout hoist opens the editor, adopt the +pure `ThumbnailKey` (or a shared `thumb_cache` helper) on the VST side. Rationale: no live bug; +unifying cache policy is a natural rider on the editor wave, pointless as standalone churn. + +### T2-11 — ComponentState v1→v11 deserialize chain: sound, but the legacy branches triplicate the shared read +**Location:** `src/vst/sample_map.cpp:775–945` (`deserializeComponentState`). + +**Mechanism.** Audited the full lift chain for functional soundness: the bounded `ByteReader` +latches on truncation, every version's tail fields carry per-field corrupt fallbacks +(previewVelocity → mid default, voiceCount → default-not-clamp, gain → unity, refs → keep-parsed +prefix), and the strict-prefix envelope discipline is honest. **No correctness finding.** The +smell is shape: the v3, v4, and v5 branches each re-implement the mode-byte → marker → idLen/id +→ zones read sequence that the v6+ shared path also implements (three near-copies of the same +cursor walk, lines 806–841 vs 851+), and each new envelope version adds another +`version >= kꞏꞏꞏV*Version` stanza to a function already ~170 lines long. + +**Severity:** Low. +**Disposition:** **document-and-defer, explicitly.** The legacy branches are frozen back-compat +contract code with saved-project blobs as their only callers; rewriting them into a table-driven +lift risks the one thing they must never break, for zero user-visible gain. Record the pattern +so the *next* envelope bump (v12) prefers extending the shared path over minting another branch. +(The unbounded-suffix version-constant naming is §2b territory; not restated.) + +--- + +## Surfaces checked and found clean + +Recorded per the wave's no-silent-omission rule; each was read/grepped this audit. + +- **Pure-module include hygiene, both trees.** Every module CLAUDE.md claims pure was scanned + for REAPER/SWELL/WDL/LICE/VST3-SDK includes: all clean, `src/` and `src/vst/` both. The one + grep hit in `pitch_shift.h` is a comment (the S16 WDL-exclusion note), not an include. The one + cross-tree include (`instrument_drop` → `vst/sample_map.h`) is pure-to-pure — see T2-07 for + the granularity concern; it is not a boundary violation. +- **`bank_sync`** — the generation/consume decision rules are pure, explicit, and exhaustively + commented (rules 1–4); `parseBankGeneration` is overflow-guarded (its duplication is rolled + into T2-01's family, not a separate defect). +- **`bridge_marshal`** — one honest job, done pure, with the S1 string-scan JSON reader + documented as retired (verified: no second JSON parser on the VST side; `sample_map` routes + through `BankBook::deserialize`). +- **The realtime record lifecycle** — *not* an implicit state machine: `RecordPhase` is an + explicit enum with pure per-tick transitions in `realtime_record.h`; `main.cpp` holds only the + handle + project pointer. (Its *residence* in main.cpp is catalogued §2.1; nothing functional + to add.) +- **Project-identity transitions** — `classifyProjectTransition` is pure (capture_paths), the + shell passes `sameProjectObject` as a bool to keep it so; the GUID-primary layering is + decision-tabled in one place. +- **`usage_scan`** — every decision delegated to pure `sample_usage`; container recursion is + depth-bounded with a protect-on-truncation fail-safe; the `std::function` parm-getter + indirection is prune-scan-cold (heuristic (c) satisfied — no hot-path chain). +- **Exception boundaries** — the three `catch (...)` sites (`bank_book.cpp:794` stol guard, + `instrument_drop_win.cpp:74` REAPER-callback boundary, `render_settings.cpp:180` stod guard) + are all documented, narrow, and non-swallowing in intent (each converts to an explicit + failure value). No silent error swallowing found. +- **`FxBypassGuard` (main.cpp) vs `view.cpp` park/restore** — both are snapshot-mutate-restore + over track flags and *look* like a dedup candidate; they are deliberately not one. Different + flag sets, different invariants (precision-neutralization vs Design-View parking), different + failure postures. Duplication of shape, not of concept — correctly left separate. +- **Path resolution** — `capture_paths::resolveBankFile` is the single resolver on both sides + of the split (panel, insert, drag_out, editor, processor). No parallel path logic. +- **WAV decode on the play path** — `wav_trim` is genuinely the only decoder (T2-08 concerns + the *container* knowledge spread, not a second decoder). +- **Draw layer** — the VST editor/embed compile the same `draw_kit`/`theme`/`component_geometry` + the extension uses (verified in CMake + includes); no parallel draw vocabulary grew on the + VST side. +- **Interface cost audit (heuristic (c))** — `ICaptureBackend` is the tree's only virtual + interface; two real implementations, dispatched once per capture (cold). No hot-path virtual + or std::function chain found in `sampler_core`/`pitch_shift`/`sample_map` (all static calls). + No interface-with-one-implementation found anywhere. +- **Boolean-parameter proliferation** — swept `src/` headers for multi-bool signatures; the only + hit is `bank_grid::applyClick(…, bool ctrl, bool shift, …)`, which mirrors physical modifier + keys and reads fine at call sites. Not a finding. + +## Cross-checks against the §2/§2b audits (gaps noted, not restated) + +- §2's evidence base is scoped to the extension tree ("45 files / ~19,800 LOC"); the full tree + is now ~39,000 LOC. The VST shells repeat the god-module pattern uncatalogued + (`reasampler_editor.cpp` 3,065 LOC, `reasampler_processor.cpp` 1,164 LOC) — carried here as + T2-06's scope note so the reorg waves size the `src/vst/` work realistically. +- §2.1's "4× JSON Parser" undercounts by one — T2-02 (`tail_control`). +- §2b.2's shared-rect naming hazard has an uncatalogued functional twin — T2-05. + +## Summary table + +| ID | Finding | Severity | Disposition | Where | +|-------|---------------------------------------------------------------|----------|-------------------|--------------| +| T2-01 | Wire `Cursor` ×3 with hardening drift; provenance unguarded | High | fix-now (split) | Q-W0 backport + Q-W1 dedup | +| T2-02 | Fifth JSON decoder in `tail_control` | Med | fix-now | Q-W1 | +| T2-03 | `readFileBytes` ×5 across both artifacts | Med | fix-now | Q-W1 | +| T2-04 | Growing ext-state read loop ×3; pure decode half-adopted | Med | fix-now | persist's wave (Q-W4) | +| T2-05 | 19 rect structs + ~15 inline point-in-rect predicates | Med | fix-now | Q-W2 | +| T2-06 | Layout math stranded in VST editor shell (+§2 scope gap) | Med | document-and-defer (named reshape) | src/vst wave | +| T2-07 | Extension links voice engine to share the preset serializer | Low | document-and-defer | Q-W1/Q-W2 homing | +| T2-08 | RIFF container knowledge in 4 modules / 2 chunk walkers | Low | document-and-defer | core/wav homing | +| T2-09 | Capture backends' Sample-stamp epilogue copy-paste w/ drift | Med | fix-now | Q-W3 | +| T2-10 | Thumbnail cache-invalidation strategies drifted across split | Low | document-and-defer | editor wave | +| T2-11 | ComponentState legacy lift branches triplicate the shared read | Low | document-and-defer | (pattern note for v12) | From ca086f40093a4c0f6212fdca19127608cfff3b1b Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 16:50:38 -0400 Subject: [PATCH 3/5] =?UTF-8?q?docs(q-w0):=20T3=20env-coupled-constant=20a?= =?UTF-8?q?udit=20=E2=80=94=207=20findings=20(2=20fix-now:=20gain-ramp=20s?= =?UTF-8?q?tep,=20fade-ceiling=20frames);=20persistence=20surfaces=20clean?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../audit-notes/q-w0-t3-env-constants.md | 220 ++++++++++++++++++ 1 file changed, 220 insertions(+) create mode 100644 docs/product/audit-notes/q-w0-t3-env-constants.md diff --git a/docs/product/audit-notes/q-w0-t3-env-constants.md b/docs/product/audit-notes/q-w0-t3-env-constants.md new file mode 100644 index 0000000..993aa15 --- /dev/null +++ b/docs/product/audit-notes/q-w0-t3-env-constants.md @@ -0,0 +1,220 @@ +# Q-W0 Track 3 — env-coupled-constant domain-modeling audit + +Static analysis, 2026-07-28, branch `pq-w0-audit`. Scope: any value stored in an +environment-coupled domain — frames, sample rate, DPI, pixels, tick cadence — that should be +stored **rate-free / device-free and resolved at the point of use** (PLAN.md §Q-W0 env-coupled +bullet; `docs/product/code-organization.md` §2c.3; the load-bearing `sample_map` seconds +invariant). Findings are domain-modeling calls, not "rescale by rate" patches. The judgment bar +applied: a finding requires (a) an env-coupled *stored* domain AND (b) an environment that can +actually change under it. Frame counts computed transiently from seconds at the use site are +correct and are not reported. + +Waves referenced for disposition: Q-W1..Q-W6 open `bank_panel.cpp`, `main.cpp`, `actions.cpp`, +`persist.cpp`, and relocate the clean pure libs — **no downstream wave opens +`reasampler_processor.cpp` / `sampler_core.h` / `reasampler_editor.cpp` for logic change**, so +fix-now findings in those files must be remediated in Q-W0 itself. + +--- + +## Findings + +### T3-01 — master-gain ramp step is a frame-domain constant anchored to 48 kHz + +- **Location:** `src/vst/reasampler_processor.cpp:46-51` (`kGainRampRate = 1.0f / 960.0f`, + `kGainRampSnap`), applied per-sample at `:1069-1085` (stereo) and `:1114-1126` (mono). +- **Stored vs. correct domain:** stored as a **per-sample linear step** — `960` is literally + 20 ms × 48 000 Hz, and the comment says so ("960 samples @ 48 kHz ≈ 20 ms"). The intended + quantity is a **wall-clock ramp time** (~20 ms); the correct model is a seconds/ms constant + with the per-sample step derived from `sampleRate_` at `setupProcessing` — exactly the + pattern the same file already uses two paragraphs away for `kPreserveWindowMs` + (`:618-623`, `:733-735`). +- **What breaks when the environment shifts:** the ramp's wall-clock length halves at 96 kHz + (~10 ms) and quarters at 192 kHz (~5 ms); at 44.1 kHz it stretches to ~21.8 ms. The FB1 + "no zipper" contract degrades silently as the host rate rises. Not persisted, so no on-disk + breakage — but it is a live hardcoded-rate assumption in `src/`, which Daniel's standing + ruling forbids. +- **Severity:** Med. +- **Disposition:** **fix-now, remediated in Q-W0.** Rationale: trivial, isolated, + behavior-identical at 48 kHz; no downstream wave opens this file, so deferring means keeping + a named violation of the no-hardcoded-rate ruling through the whole reorg. Store + `kGainRampSeconds = 0.020`, derive the step from the live rate where `sampleRate_` is set. + +### T3-02 — takeover-declick decay is a per-frame coefficient (documented deliberate) + +- **Location:** `src/vst/sampler_core.h:409-410` (`kDeclickDecay = 0.95`, + `kDeclickFloor = 1e-4`), applied per output frame in `sampler_core.cpp:514-519`, `:643-645`. +- **Stored vs. correct domain:** a per-output-frame exponential coefficient; the implied + wall-clock decay-to-floor is ~4.2 ms at 44.1 kHz and ~1.9 ms at 96 kHz. The strict + domain-model form would be a time constant in seconds resolved to a coefficient at engine + build. +- **What breaks when the environment shifts:** the takeover-blend residue fades ~2× faster at + 96 kHz. Audibly negligible for a declick micro-ramp — and the in-code comment + (`sampler_core.h:397-401`) **already documents this as a deliberate per-frame DSP micro-ramp, + "not a stored wall-clock quantity"**, with the 44.1–96 kHz variance stated and accepted. +- **Severity:** Low. +- **Disposition:** **document-and-defer.** Rationale: the coupling is already an explicit, + written, bounded design decision in the code; converting it buys no audible improvement. + Triage should ratify the in-code note as the record. + +### T3-03 — Trigger-fade UI throw ceiling hardcodes 2 s × 44 100 as `88200.0` frames + +- **Location:** `src/vst/reasampler_editor.cpp:395` + (`constexpr double kFadeMaxFrames = 88200.0`), used by `controlValue`/the commit path to + normalize the Trigger fade-in/out knobs. +- **Stored vs. correct domain:** the fade **storage** domain (int64 SOURCE frames, persisted in + the zones payload) is settled and correct — a source-timeline fact, invariant under project- + rate change (PLAN.md §S15). The *UI ceiling*, however, encodes a wall-clock intent ("2-second + max fade throw") as a frame count at an assumed 44.1 kHz source. `88200` is a rate-derived + literal in `src/`, brushing the no-hardcoded-rate ruling even though it never touches disk. +- **What breaks when the environment shifts:** the environment here is the **source file's + rate**: a 96 kHz capture's maximum fade throw is ~0.92 s; a 22.05 kHz file gets 4 s. The knob's + full-scale meaning silently varies per loaded sample. +- **Severity:** Low (UI-only, not persisted, comment flags it as a "build-time residual — one + place to retune"). +- **Disposition:** **fix-now, remediated in Q-W0.** Rationale: small and contained — replace + with `kFadeMaxSeconds = 2.0` resolved against the loaded source's rate at the two normalize + sites (the editor already threads `frameCount + rate` through the pack/unpack path, + `envelope_edit.cpp:148`); storage domain unchanged. If triage prefers zero UI-feel change, + the fallback is document-and-defer with the comment amended to name the 44.1 k assumption. + +### T3-04 — drop-hint banner duration stored in sync-timer ticks + +- **Location:** `src/vst/reasampler_editor.cpp:2920-2923` (`dropHintTicks_ = 6`), decayed in + `onSyncTimer` (`:244-245`); field at `reasampler_editor.h:457`. +- **Stored vs. correct domain:** a wall-clock intent ("a few seconds of banner") stored as a + **count of `kSyncTimerIntervalMs` ticks** (6 × 500 ms). Correct model: a duration in ms, + ticks derived — or a `GetTickCount`-style deadline like the bank panel's tooltip already + uses. +- **What breaks when the environment shifts:** retuning the sync cadence (a plausible perf + tweak — the 500 ms value is itself a tuning constant) silently changes the banner duration. + The comment does state the coupling. +- **Severity:** Low. +- **Disposition:** **document-and-defer.** Rationale: cosmetic, self-documenting at the single + site, and the cadence and hint decay live three lines apart; a fix is fine to fold in + opportunistically if the file is ever opened, but does not justify a Q-W0 edit on its own. + +### T3-05 — systemic: no DPI/content-scale support in either UI surface + +- **Location:** systemic. VST3 editor: `reasampler_editor.cpp:150-153` (`ViewRect(0,0,840,620)` + default and the size floor at `:816`), all `editor_geometry` / `knob_deck` / `curve_popup` / + `envelope_overlay` px constants (e.g. the 8 px node min-separation, the 28×28 curve button), + cached font sizes in `draw_kit`. Extension side: the LICE-drawn `bank_panel` dock and its + geometry modules. No implementation of VST3's `IPlugViewContentScaleSupport` anywhere in + `src/vst/` (grep: zero hits for content-scale/DPI), no scale factor threaded through the + pure geometry modules. +- **Stored vs. correct domain:** every layout constant is a **physical device pixel** that + silently assumes ~96 DPI. Correct model: logical units × one scale factor resolved at draw + time (the pure geometry modules take widths/heights as parameters already, so a scale factor + threads through cleanly — the constants are centralized, which is the good news). +- **What breaks when the environment shifts:** on a 150–200 % Windows display the editor and + dock render physically small (or get bitmap-stretched by the host, blurring text); hit + targets like the 8 px min node separation shrink below comfortable pointer accuracy. + Usability, not correctness — nothing mis-plays and nothing persisted is wrong. +- **Severity:** Med (usability on modern displays; Windows-only product makes high-DPI common). +- **Disposition:** **document-and-defer.** Rationale: a proper UI-scaling pass is a feature + wave of its own (scale plumbing through ~15 geometry modules + font cache + both shells), + far outside Q-W0's remediation budget; deferral should be recorded as a named future phase, + and Q-W1's relocation of the geometry modules should keep the constants centralized so the + eventual scale factor lands in one place. + +### T3-06 — legacy v3 zone-payload lift divides by the *current* project rate + +- **Location:** `src/vst/sample_map.cpp:601-605` (v3 lift inside `readZonesPayload`), format + note at `sample_map.h:439-456`, `:510-513`. +- **Stored vs. correct domain:** the v3 blobs (Daniel's beta projects) stored wall-clock times + as frames — **the prior incident itself**. The lift converts frames → seconds by dividing by + the live `projectRate` threaded in at read time. That is exact only if the project rate today + equals the rate in effect when the S15/S16 editor wrote the frames; the write-era rate was + never recorded, so a project whose rate changed since lifts skewed times (old/new ratio, + e.g. ~8.8 % for 44.1→48 k). +- **What breaks when the environment shifts:** already broken by construction for + rate-changed-since-write projects; a one-time lift residue, after which v5+ re-saves in + seconds and the skew is frozen in, silently. +- **Severity:** Low (legacy-only, beta-project blobs, envelope-time magnitudes; unrecoverable + in principle — the missing datum was never written). +- **Disposition:** **document-and-defer.** Rationale: no better conversion exists; this is the + documented residue of the incident that motivated the seconds invariant. Worth one sentence + in the code-quality-audit report so the skew is a recorded known, not a mystery bug later. + +### T3-07 — SOLA correlation-segment cap of 512 frames (deliberate CPU bound; cross-ref T1) + +- **Location:** `src/vst/pitch_shift.cpp:72` + (`corrFrames_ = max(1, min(dLow_ - 1, 512))`; rationale comment at `:64-67`). +- **Stored vs. correct domain:** borderline by design. The quantity being bounded is **work per + splice** (multiply-accumulates), which is genuinely frame-domain — a CPU bound *should* be in + frames. The side effect is that the correlation segment's wall-clock span halves at 96 kHz + (512 frames ≈ 11.6 ms at 44.1 k, ≈ 5.3 ms at 96 k), raising the lowest frequency the + alignment search can lock onto at high rates. All other shifter geometry correctly derives + from `kPreserveWindowMs` resolved at the live rate. +- **What breaks when the environment shifts:** alignment quality for low-frequency content + degrades somewhat at high host rates; no correctness or persistence impact. +- **Severity:** Low. +- **Disposition:** **document-and-defer**, and hand to the T1 DSP audit for the quality call. + Rationale: the frame domain is arguably correct for a compute bound; whether 512 is the right + *number* is an algorithm-quality question (T1's territory), not a domain-modeling one. + +--- + +## Surfaces checked clean + +- **`sample_map` v5+ persistence (the reference implementation):** AHDSR + pitch-env times as + SECONDS doubles; no rate constant anywhere in the read/write paths (`kLegacyV3NominalRate` + deliberately does not exist); legacy v3 lift takes the rate as a parameter. Clean. +- **ComponentState envelope v6–v11 fields:** channel mode, assign generation, preview velocity, + voice count/mode/trigger, `masterGainLinear` (dimensionless linear), explicit flag, + `SampleRefs` (paths + root/loop/channels intrinsics), `instanceGuid` — all rate-free or + file-fact domains. Clean. +- **Trigger `fadeInFrames`/`fadeOutFrames`/`startPoint`/`SampleLoop.start/end` persisted as + int64 SOURCE frames:** deliberate, settled source-timeline facts (PLAN.md §S15; + `bank_model.h:66-72` documents the loop rationale) — frames *of the file* are invariant under + project-rate change; the file's own rate is stored alongside and resolved at decode. Correct + domain, not a finding. +- **`trigger_seam`:** frames↔fraction with `startFrame` threaded both directions; the overlay's + fraction domain is expressly rate-invariant. Clean. +- **`kPreserveWindowMs` (50 ms):** resolved to frames against the live host rate at both call + sites (`reasampler_processor.cpp:618-623`, `:733-735`) — the correct pattern, cited here as + the model T3-01 should copy. +- **`pitch_shift` internal geometry:** ring length, fade, lag band, delay band all derived from + the rate-resolved `window_`; ratio-scaled live fade length. Clean (T3-07 cap noted above). +- **Tail system:** `TailSetting.manualMs` persisted in **ms**; `kMaxTailSeconds`/`kMaxTailMs` + wall-clock; trim threshold in **dB** with the linear ratio derived + (`render_settings.h:59-86`); the realtime decay scan resolves frames against the **file's own + authoritative rate** (`capture_realtime.cpp:384-408`). Clean. +- **`wav_trim`:** frame counts are parsed file facts and transient truncate plans. Clean. +- **`bank_model` persisted metadata:** source bounds in seconds + PPQ (both stored, each for + its consumer); loudness in dB; `sampleRate`/`channelCount` are *recorded facts about the + file*, not assumptions; capture tempo + meter stamped at capture time deliberately so the + bars.beats read-out is stable under later project meter changes (`card_meta`). Clean. +- **Ext-state wires** (`banks` JSON, view-mode model, owned manifest, `assignment_request`, + `rsusage_*`): no frame-domain values; the rate field in the assign wire is a recorded fact. + Clean. +- **Envelope schematic (`envelope_overlay`/`envelope_edit`):** param-domain px↔seconds scale + derived from the live rect (`gatePxPerSecond`), sample-length-free; editor time-slider + ceiling is `kEnvTimeMaxSeconds = 2.0` (seconds). Clean (pixel constants themselves fall under + the systemic T3-05). +- **Timers:** bank_panel tooltip delay uses `GetTickCount()` ms against `kTooltipDelayMs = 500` + (wall-clock — the pattern T3-04 should copy); editor sync timer is a 500 ms `SetTimer` + interval (ms, not ticks); the new-content detector is an event diff per tick with no + wall-clock meaning encoded in tick counts; `retireIdleDrain` is idleness-driven, not + time-driven. Clean. +- **`peaks` / `waveform_view` / `master_gain` / `velocity_curve` / `keyboard_strip`:** bins and + columns derived from rects at use; dB↔linear taper and curve math dimensionless; key rects + from the passed strip rect. Clean. + +## Summary + +| ID | Location | Stored domain | Severity | Disposition | +|----|----------|---------------|----------|-------------| +| T3-01 | `reasampler_processor.cpp:46-51` gain-ramp step | per-sample step (20 ms @ 48 k baked in) | Med | **Fix-now (Q-W0)** — store seconds, derive step from `sampleRate_` | +| T3-02 | `sampler_core.h:409-410` declick decay | per-frame coefficient | Low | Document-and-defer — deliberate, already documented in-code | +| T3-03 | `reasampler_editor.cpp:395` fade throw ceiling | 88200 source frames (2 s @ 44.1 k) | Low | **Fix-now (Q-W0)** — seconds ceiling resolved vs. source rate at use | +| T3-04 | `reasampler_editor.cpp:2923` drop-hint duration | sync-timer ticks | Low | Document-and-defer — cosmetic, coupling stated in-code | +| T3-05 | systemic (both UI surfaces) | physical px, ~96 DPI assumed; no content-scale | Med | Document-and-defer — a UI-scaling phase of its own; keep geometry constants centralized through Q-W1 | +| T3-06 | `sample_map.cpp:601-605` v3 legacy lift | frames ÷ *current* project rate | Low | Document-and-defer — unrecoverable legacy residue; record as known skew | +| T3-07 | `pitch_shift.cpp:72` correlation cap | 512 frames (CPU bound) | Low | Document-and-defer — frame domain arguably correct for a compute bound; hand to T1 for the quality call | + +Two fix-now findings (T3-01, T3-03), both assigned to **Q-W0 itself** — no downstream wave +opens those files for logic change. Five deferrals, each with a recorded rationale. The +persistence surfaces — the highest-stakes case — are clean: every wall-clock quantity written +to disk since the S12 remediation is in seconds or ms, and every frame-domain persisted value +is a source-file fact whose rate travels with it. From 2dbefb8c0162bb35a1bd32048171292e2b9cb02f Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 16:50:47 -0400 Subject: [PATCH 4/5] =?UTF-8?q?audit(q-w0-t4):=20sizing=20census=20?= =?UTF-8?q?=E2=80=94=2014=20oversize=20files,=20VST=20side=20unowned;=20pr?= =?UTF-8?q?opose=20Q-W2v=20wave,=208-seam=20bank=5Fpanel,=20ICaptureBacken?= =?UTF-8?q?d=20is=20dead?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/product/audit-notes/q-w0-t4-sizing.md | 399 +++++++++++++++++++++ 1 file changed, 399 insertions(+) create mode 100644 docs/product/audit-notes/q-w0-t4-sizing.md diff --git a/docs/product/audit-notes/q-w0-t4-sizing.md b/docs/product/audit-notes/q-w0-t4-sizing.md new file mode 100644 index 0000000..9634422 --- /dev/null +++ b/docs/product/audit-notes/q-w0-t4-sizing.md @@ -0,0 +1,399 @@ +# Q-W0 Track 4 — structural sizing + placement audit + +Date: 2026-07-28 · Branch: `pq-w0-audit` · READ-ONLY static analysis (no build, no code edits) + +**Method.** Line counts measured with `wc -l` on the worktree; seams derived from function-definition +skeletons (`grep` for top-level definitions + section markers) plus targeted reads. Acceptance bar = +Daniel's three heuristics: (a) more directories a must, files ≤ ~600 lines, SRP applies to files and +namespaces; (b) templates are good where they dedup at compile time; (c) saved CPU beats abstraction — +no dispatch-stack blowouts, prefer static polymorphism where types are compile-time-known. + +**Measured sizes differ from the wave brief in several places** (the tree moved after the brief was +drafted — GA/pS/pS-usage landed): `reasampler_editor.cpp` 3065 (brief said 3035), +`reasampler_processor.cpp` 1164 (1004), `sampler_core.cpp` 968 (1049), `sample_map.cpp` 970 (807), +`sample_map.h` 708 (600), `sampler_core.h` 762 (820), `actions.cpp` 1016 (996), `persist.cpp` 852 +(812). All numbers below are the measured ones. + +--- + +## 1. Oversize census + seams + +Every `.cpp`/`.h` in `src/` (both sides) over ~600 lines, with the *real* responsibility clusters. +Where a file is genuinely one responsibility, I say so and recommend leaving it. + +### 1.1 The four planned splits — do the plan's seams still land sub-600? + +**T4-01 — `src/bank_panel.cpp` (3459; plan assumed 2424).** +The plan's six seams (`panel_render` / `panel_thumbnails` / `panel_audition` / `panel_input` / +`panel_bank_ops` / `panel_window`) no longer all land sub-600 at current size. Tally against the +skeleton: + +| Planned TU | Functions (line spans) | Est. LOC | Verdict | +|---|---|---|---| +| `panel_thumbnails` | `computeThumbnail`/`thumbnailFor` (420–487) | ~130 | fine | +| `panel_render` | `drawCardMeta`/`drawThumbnail` (487–547), kit adapters (547–570), `drawFooter` (685–780), `drawToolbar`/`drawMoreButton`/`drawTooltip` (992–1155), `drawRegionGrid`/`drawCardDropTarget`/`drawRegionHeader`/`drawTabStrip`/`paintPanel` (1334–1646) | ~700 | **over — needs the layout cut below** | +| *(unplanned)* **`panel_layout`** | toolbar/footer/menu rect + row/cluster builders (571–684, 785–991), split geometry + region rects + L7 slot-order display bridge (1156–1333) | ~500 | **new TU required** — this is pure-ish geometry glue, distinct from LICE drawing; extracting it puts `panel_render` at ~550 | +| `panel_audition` | `initPreview`/`startAudition`/`stopAudition`/`deinitPreview` (1874–1965) | ~90 | fine (keep the direct call-through guardrail) | +| `panel_input` | input helpers + `regionAt` (1965–2010), click routing `handleBanksChromeClick`…`handleKey`/accelerator (2384–2710), plus new-content detection (1647–1874, ~230) | ~800 | **over — needs the drag cut below** | +| *(unplanned)* **`panel_drag`** | `updateDropTarget`/`dropTargetBankId`/`classifyCardDrag`/`applyDragCursor`/`resolveHover`/`updateHover`/`maybeShowTooltip`/`onMouseMove`/`doReorderDrop`/`doReplaceDrop`/`resetDragState`/`onLBtnUp`/`handleRightClick` (2711–3185) | ~475 | **new TU required** — the card-drag/hover state machine is a cohesive cluster of its own (it already has a pure mirror, `card_drag`); extracting it puts `panel_input` at ~550 | +| `panel_bank_ops` | `promptText`/`mintBankId`/`doCreateBank`…`removeSamples`/`focusedSelectionIds`/`resolveDragPathsForOs` (2006–2231) + popup menus (2231–2384) | ~375 | fine (menus ride with bank_ops or input — either works; they invoke the ops) | +| `panel_window` | `handleDropFiles`/`dlgProc`/`openPanel`/`closePanel` (3185–3336) + public API (3336–3459) | ~275 | fine | + +**Proposal:** eight TUs, not six — add `panel_layout` and `panel_drag`. +**Severity:** high (it is the biggest file in the repo). **Disposition: reshapes wave Q-W2** — +the wave brief must name eight seams, or two of its six TUs ship >600 on day one. + +**T4-02 — `src/main.cpp` (1897; plan assumed 1762).** +The plan's three hoists are the right seams, but `capture_orchestrator` as specced lands **~885 +lines** — over by half again. Tally: `FxBypassGuard` (627–731, ~105), `renderOffline` + +`captureAndIndexOne` + `RunCapture` + `RunCaptureItemAssign` (731–909, ~180), batch family +(`ItemSelectionGuard`/`selectOnlyItem`/`RunBatchCaptureItems`/`collectRazorAreas`/ +`TrackSelectionGuard`/`RunBatchCaptureRazor`, 909–1181, ~270), `RunRecaptureFromSource` (1200–1398, +~200), realtime + insert actions (1398–1512, ~115). `scope_resolve` (361–590) ≈ 230 ✓; +`realtime_lifecycle` (186–360) ≈ 175 ✓; registration/entry residue (1512–1897) ≈ 385 ✓ (shrinks +further under Q-W6's table). +**Proposal:** split the orchestrator seam once more: `capture_orchestrator` (FxBypassGuard + +single-capture path + realtime/insert action bodies, ~450) and **`capture_batch`** (batch family + +`RunRecaptureFromSource` + the two selection guards, ~470). Recapture is planner-driven like batch +and shares the selection-guard machinery — it belongs with batch, not the single-shot path. +**Severity:** high. **Disposition: reshapes wave Q-W3** — add the fourth TU to the brief. + +**T4-03 — `src/actions.cpp` (1016; plan assumed 981).** +Plan's seams still land: `design_view_actions` (59–421, ~360), `bank_actions` (422–1016 minus prune, +~490), `prune_action` (`doBankPruneFolder` 821–916 + registration share, ~130). All sub-600. +**Disposition: no change to Q-W4.** + +**T4-04 — `src/persist.cpp` (852; plan assumed 766).** +Plan's seams still land: `ext_state_io` (helpers + `saveToActiveProject` + `writeAssignmentRequest`, +112–288, ~180), `prune_fs` (`scanPruneOrphans`/`pruneDryRun`/`pruneOrphanSet`/`deleteOrphanFile`/ +`pruneReclaim`, 288–539, ~250 — pS-usage growth landed here, exactly where the plan isolates it), +`session` (load/guid/poll/reload, 539–852, ~315). All sub-600. +**Disposition: no change to Q-W5.** + +### 1.2 Known offenders beyond the planned four — extension side + +**T4-05 — `src/bank_book.cpp` (1109) + `bank_book.h` (457).** +Three genuine seams: **`SlotMap`** (27–143, ~115 — a self-contained ordered-slot container with its +own serialize at 614), **`BankBook`** registry/CRUD/transfer/slot-reconcile (145–545, ~400), and +**JSON serialize + `Parser`** (547–1109, ~560). Q-W1 deletes the Parser + `ObjWriter`/`writeEscaped` +copies; what remains of serialization rewired onto `core/json` is ~150. +**Proposal:** after Q-W1, split `slot_map` into its own TU/header pair (it is a distinct type with +its own tests-worthy invariants); `bank_book.cpp` lands ~550. **Severity:** medium. +**Disposition:** fold into Q-W1 (the JSON rewire already opens this file; the `slot_map` file split +is one `git mv`-shaped extraction on top). + +**T4-06 — `src/view_mode_model.cpp` (1049) + `view_mode_model.h` (748).** +Four seams: **indexes** (`ModeRegistry`/`MembershipIndex`/`LaneOwnershipIndex`, 29–140), **pure +planners** (`autoTagNewContent`/`planItemRetag`/`planLaneMinting`/`makeParkPlan`/`makeRestorePlan`/ +`nextModeId`, 143–326, ~185), **`ViewModeModel`** state + visibility/toggle planning (332–490), and +**JSON serialize + `Parser`** (494–1049, ~555). Q-W1 deletes the Parser (~390); remainder ~660. +**Proposal:** split planners (`view_plan.cpp`) from model+indexes (`view_mode_model.cpp`, ~450 after +JSON extraction). The header's 26 structs split the same way: mode/membership/lane types + model +class vs. the plan-record structs (`TrackPlan`/`TogglePlan`/`AutoTag`/`ItemRetagOp`/`LaneMint*`). +**Severity:** medium. **Disposition:** fold into Q-W1 (JSON rewire opens the file; planner split +rides it). If the wave wants to stay minimal, the planner split can defer — post-extraction ~660 is +marginal, not pathological. + +**T4-07 — `src/bank_model.cpp` (767).** +Two seams only: the model (`Sample` equality + `BankIndex` verbs, 25–145, ~120) and JSON +(`ObjWriter`/`writeSample`/`Parser::parseSample`/`parseIndex`, 148–767, ~620). This file is the +poster child for Q-W1: after the extraction it is ~250 total (model + thin serialize using +`core/json`). **Disposition: already owned by Q-W1; no new seam needed.** + +**T4-08 — `src/capture_realtime.cpp` (867).** +Two seams: the **async record lifecycle** (`RealtimeCaptureState` snapshot/restore, `begin`/`tick`/ +`abort`, 177–324 + 569–867, ~450) and the **file-side finalize** (`readAllBytes`/`writeU32LE`/ +`trimAutoTailInPlace`/`finalizeRecording`, 324–566, ~240). The lifecycle is genuinely one +responsibility (the header itself documents why the restore lives on the state object). The finalize +half — WAV byte-patching, decay-scan trim, move-into-bank — is a distinct concern that talks to +`wav_trim`, not to the transport. +**Proposal:** split `capture_realtime_finalize.cpp` (~240); lifecycle TU lands ~600 with the file +banner. Both stay in `shell/capture/`. **Severity:** low-medium. **Disposition:** ride Q-W3 (the +wave already renames this family per the Q-9 naming rider — same-wave file surgery is free). + +**T4-09 — `src/view.cpp` (677).** +Two halves: **park/restore + flag application** (`snapshotTrack`/`applyFlags`/`parkFxOffline`/ +`restoreFxOffline`/`applyMode`, ~350) and **lane management** (`laneName`/`managedLaneOrdinals`/ +`applyLanePlays`/`applyLaneOps`/`readLaneTracks`/`assignItemToLane`/`applyMintPlan`/ +`mintManagedLanes`/`reconcileManagedLanes`, ~330). The D2 lane machinery arrived after the file's +original charter and is a separable concern. +**Proposal:** split `view_lanes.cpp`. **Severity:** low (677 is barely over). **Disposition:** +document-and-defer unless Q-W1's relocation is already touching it — then take the free split. + +**T4-10 — `src/ingest.cpp` (636).** +Seams: **pure-ish WAV/PCM helpers** (`buildFloat32Wav`/`decodePcmSource` + byte I/O, 86–217, ~130 — +see T4-22: `buildFloat32Wav` is a pure function trapped in a shell TU), **`importFileIntoActiveBank`** +(249–420, ~170), and the three ingest surfaces + registration (420–636). Extracting the pure WAV +build into a testable core module (`wav_write` beside `wav_trim`, or one `wav_codec`) drops the shell +to ~500 and gains a test target. +**Severity:** low-medium. **Disposition:** fix in whichever wave lands the WAV consolidation +(T4-22); the file split itself is a rider. + +### 1.3 Known offenders — VST side (entirely absent from the current plan) + +**T4-11 — `src/vst/reasampler_editor.cpp` (3065) + `reasampler_editor.h` (539).** +The single biggest unplanned file — it grew past `main.cpp` in the r11 recomposition, *after* the +plan was written. It is now the `bank_panel.cpp` of the VST artifact, with the same god-module +profile. Real seams, from the skeleton: + +| Proposed TU | Functions (line spans) | Est. LOC | +|---|---|---| +| `editor_session` | ctor/dtor, `refreshFromBank`/`rebuildVisible`/`onSyncTimer`/`commitAndReload`/`loadSelection`/`upsertPickedOverride`/effective-zone helpers (146–400), PCM + thumbnail caches `monoPcmFor`/`thumbnailFor` (719–800) | ~420 | +| `editor_controls` | control-value map `controlValue`/`applyControl` (399–475), knob-deck plumbing `zoneDeckGroupDescs`/`deckGroupDescs`/`deckControlNorm`/`applyDeckKnob`/`deckValueLabel` (475–649), envelope pack/unpack + `commitPickedMarkers` (649–719), `applyZoneControl` (2576–2587) | ~470 | +| `editor_layout` (pure candidate — see T4-23) | anon-ns geometry: `computeSampleBands`/`clusterRects`/zone-panel areas/`channelToggleRects` (904–1076), `computeBrowseModal` (1756–1785), `zoneContentArea` (1917–1924) | ~250 | +| `editor_paint_sample` | `paint` dispatch (1169), `drawTitleBand`, `paintSample`/`paintEnvelopeOverlay`/`paintVelocityCurve`/`paintKnobDeck`/`paintCurveButton`/`paintCurvePopup`/`paintEmptyState` + `drawKnobFace`/`drawSpectralStrip`/`drawRootMarker` (1076–1756) | ~590 | +| `editor_paint_browse_zone` | `paintBrowse` (1785–1917), `paintZone` (1924–2041) | ~260 | +| `editor_input` | `resolveHover` (2041–2145), `onMouseDown` (2147–2576 — a 430-line per-face dispatch), popup/curve mouse (1637–1735), `onMouseMove`/`onMouseUp`/`onMouseRDown`/`onMouseWheel`/`onSearchChar`/`onFilesDropped` (2587–2929) | ~880 → **split by face**: `editor_input_sample` (~500: sample-face hit branches + drag state + curve popup) and `editor_input_browse_zone` (~380: browser cards/scroll/search + zone strip/note entry) | +| `editor_platform` | IPlugView overrides `isPlatformTypeSupported`/`canResize`/`checkSizeConstraint`/`attachedToParent`/`removedFromParent`/`onSize`/`invalidate` (800–904), `wndProc` + non-Windows stubs (2929–3065) | ~280 | + +Seven-to-eight TUs, all sub-600, each a real cohesive cluster (session/bridge state · param +plumbing · layout · paint × 2 · input × 2 · platform). The face structure (Sample / Browse / Zone) +is the natural input/paint split axis — it mirrors how the code already dispatches. +**Severity: highest of the audit** — this is the largest unowned file in the repo. +**Disposition: reshapes the wave plan — needs a new owning wave** (see §1.5 / summary table). + +**T4-12 — `src/vst/reasampler_processor.cpp` (1164) + `reasampler_processor.h` (502).** +Four seams: **VST3 lifecycle + bus boilerplate** (`queryInterface`…`setupProcessing`, +`setBusArrangements`, 118–209 + 492–506, ~120), **component-state I/O** (`setState`/`getState` + +`legacyLiftShouldRun`, 209–349 + 774–904, ~270), **accessors/param setters** (349–492, ~140), and +**reload + drain + usage-publish + render** (`reloadInstrument`/`publishUsage`/ +`publishBuiltLocked`/`rebuildVoiceEngine`/`retireIdleDrain` 506–774 ~270, `process` 904–1164 ~260). +**Proposal:** three TUs — `processor_state` (state I/O + accessors, ~410), `processor_reload` +(reload/drain/usage, ~290), `reasampler_processor.cpp` (lifecycle + `process()`, ~400). Keep +`process()` and its block-render helpers in one TU (heuristic c — see T4-27). All are member +functions of one class; partial-class-across-TUs is the same pattern the Q-W2 panel split uses. +**Severity:** medium-high. **Disposition:** new VST wave (see §1.5). + +**T4-13 — `src/vst/sample_map.cpp` (970) + `sample_map.h` (708).** +Two clean halves plus a small third: **resolution core** (bank-JSON distill/select/list, SampleRefs +management, PCM downmix/extract/decode, `resolvePlay`, keymap builders, performance-map resolution + +`reconcileSingleCaptureZones`, 14–406, ~390) and **binary component-state codec** (`putU32le`/ +`putU64le`/`ByteReader`, zones payload put/read, `serializePerformance`/`deserializePerformance`, +`serializeComponentState`/`deserializeComponentState` v3→v11 lift ladder, selection codec, +406–970, ~565). +**Proposal:** split `component_state_io.cpp` (the codec, ~565) from `sample_map.cpp` (resolution, +~400). The header splits the same way: wire/state structs (`ComponentState`/`SampleRefEntry`/codec +decls) vs. resolution API (`SelectedSample`/`PerformanceMap`/`ZonePlaySeconds`/resolvers). This is +the same shape as the extension's model-vs-JSON split and directly reduces rebuild fan-out — the +editor and processor both include `sample_map.h` today and recompile on every codec tweak. +**Severity:** medium-high (the codec grows every ComponentState version bump — v6→v11 in one +quarter; it will cross 600 on its own soon). **Disposition:** new VST wave. + +**T4-14 — `src/vst/sampler_core.cpp` (968) + `sampler_core.h` (762).** +Contents: pitch math (15–30), `Keymap` (34–58), `AdsrEnvelope`/`TriggerEnvelope`/`PitchEnvelope` +(62–251, ~190), `Voice` (255–680 — `advanceFrame` alone is ~200), `VoiceEngine` (680–968, ~290). +**This TU is a genuine single responsibility — the realtime voice engine — and it is the hottest +code in the repo.** Every function in it sits on the per-sample render path; the envelope `tick()`s +and `Voice::advanceFrame` benefit from same-TU inlining (no LTO assumption in the build). Splitting +the .cpp along class lines would put per-sample calls across TU boundaries — precisely the +heuristic-(c) violation the phase forbids. +**Proposal:** **leave the TU whole at 968** (documented exception to the 600 bar, justified by the +hot path), but **split the header**, which is where the pain actually is: `zone_params.h` (the enums ++ `AdsrParams`/`TriggerParams`/`PitchEnvParams`/`ZonePlayParams`/`SampleLoop`/`SampleData` — what +`sample_map`, the editor, and the codec actually need, ~250) vs. `sampler_core.h` (Keymap + the +engine classes, ~500). Today every UI TU that reads a param struct recompiles when a `Voice` member +changes. **Severity:** medium. **Disposition:** header split in the new VST wave; TU stays — +recommend recording the exception in the wave brief so nobody "fixes" it later. + +**T4-15 — `src/view_mode_model.h` (748)** — covered under T4-06 (splits with its TU). +**T4-16 — `src/vst/sample_map.h` (708)** — covered under T4-13. +**T4-17 — `src/vst/sampler_core.h` (762)** — covered under T4-14. + +### 1.4 Borderline (no action, for the record) + +`capture.cpp` (549), `reasampler_editor.h` (539 — shrinks when the editor splits move private +helpers into their TUs), `reasampler_processor.h` (502 — same), `bank_book.h` (457), `pitch_shift.cpp` +(371 — single responsibility, hot, leave), `persist.h`/`capture.h` (341/234 — Q-W6 fat-header pass +already owns them). None need action beyond what their TU splits imply. + +### 1.5 The structural conclusion for the wave plan + +The existing plan splits 4 files, all extension-side. The census says **9 files need splitting and 2 +need header-only splits — 5 of them VST-side, which currently have no owning wave.** The VST work is +the same kind and size as Q-W2 (the editor alone ≈ the old bank_panel). Recommendation: add one VST +god-module wave (call it **Q-W2v**, runnable in parallel with Q-W2 — different artifact, zero file +overlap; or sequence after W5 as Q-W7 if Daniel wants serial waves). Q-W1's relocation scope also +grows: the ~20 clean VST pure libs relocate + namespace in W1 alongside the extension's 30. + +--- + +## 2. `src/vst/` placement in the Q-3 directory map + +The settled Q-3 map (`core/{model,view,capture,audio,ui,reclaim,version,json}`, +`shell/{capture,panel,view,persist,actions}`, `app/`) covers only the extension. Two viable shapes +for the VST artifact; **this is Daniel's fork to call at triage.** + +**T4-18 — Leading recommendation: integrate into the same `core/`/`shell/` top split, with +`instrument/` subsystem dirs beneath.** + +``` +core/instrument/engine/ sampler_core, pitch_shift, velocity_curve, master_gain +core/instrument/map/ sample_map (+component_state_io), bank_sync, bridge_marshal, note_entry, trigger_seam +core/instrument/ui/ editor_geometry, keyboard_strip, waveform_view, capture_browser, + browser_scroll, param_slider, knob_deck, curve_popup, + envelope_overlay, envelope_edit, embed_strip +shell/instrument/ reaper_bridge, processor TUs, editor TUs, reasampler_embed, vst_entry, + reasampler_vst.h / reasampler_uid.h +``` + +Rationale: +1. **One rule, no special case.** Q-3's settled reasoning is "top-level by the load-bearing + discipline, because the pure/shell split is the invariant worth making structural." That reasoning + is artifact-agnostic — a file's directory should tell you whether it may touch a *host* type + (REAPER or VST3 SDK), and `shell/instrument/` says exactly that. +2. **The artifact boundary is a link-graph fact, not a source-layout fact — and the sources already + straddle it.** Verified cross-artifact consumers: `sample_map` links `bank_book` + `wav_trim` + + `sampler_core`; the editor includes `draw_kit`/`theme`/`component_geometry`/`capture_paths`/ + `peaks`/`wav_trim`/`app_version`/`ext_keys` (extension-side modules); the extension's pure + `instrument_drop` includes `vst/reasampler_uid.h`. An artifact-first subtree would either + duplicate these or still reach across — the boundary it draws is already false. +3. **Namespace map falls out:** `reasampler::instrument::{engine,map,ui}` beside + `reasampler::model` etc. — the Q-4 rule applied uniformly. +4. **CMake impact: path edits only.** Targets, links, and test executables are unchanged; the + VST3-gate (`EXISTS pluginfactory.cpp`) already guards targets, not directories. + +Cost to name: the VST3-gated targets stay interleaved through the top-level `CMakeLists.txt` rather +than being isolatable behind one `add_subdirectory`. Mitigable by grouping the instrument targets +into one guarded block (or one `include()`d .cmake file) without moving sources. + +**T4-19 — Alternative: parallel artifact-first subtree** — `src/vst/core/{engine,map,ui}` + +`src/vst/shell/`, extension keeps `src/core|shell|app`. Pros: the artifact boundary is visible at +top level; the whole VST tree (sources *and* a dedicated `src/vst/CMakeLists.txt`) can sit behind +one SDK-gated `add_subdirectory`, which is the cleanest possible expression of "this half only +exists on Windows with the submodule slice". Cons: two parallel `core/` trees dilute the "directory += may it touch a host type" invariant into "check which subtree first"; the shared-module reality +(point 2 above) means the subtree is not actually self-contained — its purity is cosmetic; and the +gated-`add_subdirectory` win is achievable under T4-18 with an `include()` anyway. **Recommend +T4-18; T4-19 is defensible if Daniel weighs artifact legibility above discipline uniformity.** +**Disposition: reshapes Q-W1** (the relocation wave executes whichever shape is chosen). + +--- + +## 3. Template-collapse opportunities + +Judged per heuristic (b) — proposed only where duplication is real and the template earns it; two +anti-recommendations included, because a forced template is the worse smell. + +**T4-20 — Little-endian byte codec: real template win.** +Five hand-rolled copies, verified: `putU32le`/`putU64le` + `ByteReader` (`vst/sample_map.cpp` +406–500), `writeU32LE` (`capture_realtime.cpp` 342), `readU32LE` lambda (`capture_paths.cpp` 49), +`putU32` lambda (`ingest.cpp` 134), `appendU32LE` (`instrument_drop.cpp` 18). One header — +`core/wire/bytes.h` (or beside `core/json`): `template void putLE(std::vector&, +T)` / `template bool readLE(ByteReader&, T&)` with the double↔bits helpers — replaces all +five, compile-time dispatched, zero runtime cost, and gives the ComponentState codec (T4-13) a +tested primitive. Entirely off hot paths (serialization/file I/O only). +**Severity:** medium (each new ComponentState version re-duplicates today). **Disposition:** fix in +the wave that lands `component_state_io` (the biggest consumer); consumers rewire opportunistically. + +**T4-21 — Rect family: unify, but with a concrete type, NOT a template.** +Verified 12+ byte-identical `{int x,y,width,height}` structs (`ActionBarRect`/`CellRect`/ +`PanelClientRect`/`FooterBarRect`/`HeaderRect`/`SegmentRect`/`MenuBarRect`/`MenuButtonRect`/ +`FooterRect`/`ButtonRect`/`TabStripRect`/`KitBox`…) plus a *second grammar* on the VST side +(`editor_geometry`'s `Rect` is LTRB with `left/top/right/bottom` + `height()`). The right tool is +one concrete `ui::Rect` + `contains()` with per-role type aliases (`using ButtonRect = ui::Rect;`) +so call sites keep their semantic names — Q-W1's "one `ui::` owner" note already points here; this +finding extends it: (a) retire the XYWH-vs-LTRB fork by picking one grammar (LTRB has the live +`contains`/`height` users; either works — pick once), and (b) the per-type `contains`/`hitTest*` +one-liners collapse for free. A template rect would model nothing — the types differ in name only. +**Watch:** cross-lib name collisions (extension `Rect` vs vst `Rect`) surface only when both headers +meet in one TU — `sample_map` and the editor are exactly such TUs; the Q-4 sub-namespaces are the fix. +**Severity:** medium. **Disposition:** ride Q-W1 (it is the settled `ui::` unification, widened to +include `editor_geometry::Rect`). + +**T4-22 — Linear rect-scan hit-tests: small template, real but modest.** +`hitTestCell` (`bank_grid`), `hitTestSlot` (`card_drag`), and the tab/segment scans are the same +first-rect-containing-point loop over records that carry a rect plus extra fields (`SlotCellRect` +adds `slot`). After T4-21, a single `template int hitIndex(int px, int py, +span)` (requiring `r.rect.contains(px,py)` or a rect accessor) collapses them. Earns its +keep only if T4-21 lands first; alone it would be a forced template. +**Severity:** low. **Disposition:** document-and-defer; opportunistic rider on Q-W1. + +**T4-23 — WAV build/parse consolidation (dedup, not template).** +`buildFloat32Wav` (ingest, 116–179) hand-writes the float32 header that `wav_trim` hand-parses and +`capture_paths`/`capture_realtime` chunk-scan/byte-patch. One pure `wav_codec` (or fold build into +`wav_trim`, renamed) gives one tested owner of the RIFF layout. Concrete functions; nothing to +template. **Severity:** low-medium. **Disposition:** fix-now-sized, but assign to the wave that +opens `ingest.cpp` (T4-10) to avoid a standalone churn commit. + +**T4-24 — `clamp01`: dedup with one inline, anti-template.** +Six verified copies (`envelope_overlay`, `master_gain`, `param_slider`, `reasampler_editor`, +`velocity_curve` + `envelope_edit`'s `clamp`). One `constexpr inline double clamp01(double)` in a +shared core header (or just `std::clamp` at call sites). Not a template candidate — `std::clamp` +already is one. **Severity:** trivial. **Disposition:** rider on Q-W1 relocation. + +**T4-25 — JSON `Parser`/`ObjWriter`/`writeEscaped`/`intToStr` ×4 — already owned by Q-W1; +confirmed still accurate** (verified in `bank_model`/`bank_book`/`view_mode_model`/ +`owned_manifest`; `sample_usage` uses its own `rsusage` k/v wire, *not* a fifth JSON parser — no +scope growth). Concrete class, not a template. **Disposition:** no change. + +--- + +## 4. Indirection audit (heuristic c) + +**T4-26 — `ICaptureBackend` is now a dead abstraction: one implementation, zero polymorphic call +sites. The brief's "two implementations — earning its keep" assumption is FALSE at current state.** +Verified: `capture.h` itself documents (lines 139–148, the "SEAM CHOICE" comment) that +`RealtimeRecordBackend` **deliberately does not implement** `ICaptureBackend` — it has a bespoke +async `begin/tick/abort` seam. `OfflineRenderBackend` is the sole deriver, and the only +construction site (`main.cpp:738`) instantiates the concrete type; nobody anywhere holds an +`ICaptureBackend*`/`&`. The interface costs a vtable and models nothing. +**Proposal:** delete `ICaptureBackend`; `OfflineRenderBackend` becomes a plain concrete class. Note +CLAUDE.md/CONTEXT still describe the module as "`ICaptureBackend` interface; two backends" — the doc +should be corrected in the same commit. **Severity:** low runtime, medium hygiene (it misleads — +this audit's own brief was misled). **Disposition:** fix in Q-W3 (the wave that rehomes the capture +orchestration and touches every call site). + +**T4-27 — Warning to the Q-W2v brief (T4-14): do not split `sampler_core.cpp` along class lines.** +`AdsrEnvelope::tick`/`TriggerEnvelope::amplitudeAt`/`PitchEnvelope::tick` are called per-voice +per-sample from `Voice::advanceFrame`, which is called per-sample from `VoiceEngine::render`. +Same-TU definition is what lets the compiler inline this stack today (no LTO configured). A +by-class TU split converts the hottest inner loop into cross-TU calls — the exact dispatch-stack +blowout heuristic (c) forbids. If a split is ever wanted, the envelopes must move as +header-defined (inline) classes, not to a TU. The engine TU staying whole at 968 is the correct +trade. + +**T4-28 — Warning to the Q-W2 brief (reaffirming the plan's own guardrail).** `panel_audition` and +the preview idle path must stay direct call-throughs after the 8-TU split — the plan already says +this; the two *added* TUs (T4-01: `panel_layout`, `panel_drag`) introduce no new risk (layout is +paint-time, drag is input-time), but the split of `onMouseMove` (which calls hover + drag + tooltip) +should keep per-mouse-move work as plain free-function calls, no interface. + +**T4-29 — Warning to the processor split (T4-12).** Keep `process()` and any per-block helpers it +calls in one TU. The state/reload/accessor TUs are UI-thread or setup-time — safe to move freely. +The atomic-pointer-swap pattern (`publishBuiltLocked`) must not gain a virtual seam. + +**T4-30 — No other gratuitous indirection found (verified, not assumed).** The only extension-side +`virtual` is T4-26. VST-side virtuals are all VST3-SDK-mandated overrides (`SingleComponentEffect`, +`CPluginView`, `IReaperUIEmbedInterface`) — not ours to remove. The layered pure→shell pairs +(`card_drag`→`bank_panel`, `realtime_record`→`capture_realtime`, `drag_out`→`drag_out_win`, +`prune_reconcile`→`persist`) are the load-bearing discipline, not forwarding waste — each layer +adds the decision/side-effect split, and all are direct calls. `bank_panel`'s ~20-function free-API +is a module boundary, not a dispatch chain; Q-W2's header segmentation thins it. + +--- + +## Summary table — every oversize file → proposed seams → owning wave + +| File (LOC) | Proposed TUs/headers | Owning wave | +|---|---|---| +| `bank_panel.cpp` (3459) | 8 TUs: render / **layout (new)** / thumbnails / audition / input / **drag (new)** / bank_ops / window | **Q-W2 (reshaped: 6→8 seams)** | +| `vst/reasampler_editor.cpp` (3065) | 8 TUs: session / controls / layout (pure candidate) / paint_sample / paint_browse_zone / input_sample / input_browse_zone / platform | **NEW wave Q-W2v** | +| `main.cpp` (1897) | orchestrator / **batch+recapture (new)** / scope_resolve / realtime_lifecycle / app-entry residue | **Q-W3 (reshaped: 3→4 hoists)** + Q-W6 | +| `vst/reasampler_processor.cpp` (1164) | processor_state / processor_reload / lifecycle+process (whole) | **NEW wave Q-W2v** | +| `bank_book.cpp` (1109) | slot_map / bank_book / JSON→`core/json` | Q-W1 (+slot_map rider) | +| `view_mode_model.cpp` (1049) + `.h` (748) | indexes+model / planners / JSON→`core/json`; header splits likewise | Q-W1 (+planner rider) | +| `actions.cpp` (1016) | design_view_actions / bank_actions / prune_action (unchanged) | Q-W4 (no change) | +| `vst/sample_map.cpp` (970) + `.h` (708) | sample_map (resolution) / component_state_io (codec); header splits likewise | **NEW wave Q-W2v** | +| `vst/sampler_core.cpp` (968) + `.h` (762) | **TU stays whole (hot-path exception, T4-27)**; header → zone_params.h + sampler_core.h | **NEW wave Q-W2v** (header only) | +| `capture_realtime.cpp` (867) | lifecycle / finalize | Q-W3 (rides the Q-9 rename) | +| `persist.cpp` (852) | session / ext_state_io / prune_fs (unchanged) | Q-W5 (no change) | +| `bank_model.cpp` (767) | model / JSON→`core/json` (unchanged) | Q-W1 (no change) | +| `view.cpp` (677) | apply / lanes | defer, or Q-W1 rider | +| `ingest.cpp` (636) | wav helpers→core / import / surfaces | wave owning T4-23 | + +**Wave-plan deltas requested of triage:** (1) Q-W2 grows to 8 seams; (2) Q-W3 grows to 4 hoists + +the `ICaptureBackend` deletion; (3) a **new VST god-module wave (Q-W2v)** owns the editor / +processor / sample_map splits + the sampler_core header split — parallel-safe with Q-W2 (zero file +overlap); (4) Q-W1's relocation scope includes the VST pure libs under the placement shape chosen +at the T4-18/T4-19 fork; (5) the `core/wire/bytes.h` LE-codec template (T4-20) lands with +`component_state_io`. From 3eaa0b886ebac78a4ec1ae68f318fc44bfc20753 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 17:00:06 -0400 Subject: [PATCH 5/5] =?UTF-8?q?docs(Q-W0):=20committed=20code-quality=20au?= =?UTF-8?q?dit=20=E2=80=94=2059=20findings=20triaged,=20Q-W2v=20VST=20wave?= =?UTF-8?q?=20+=20Q-W2/Q-W3=20reshapes=20proposed,=20Daniel=20decision=20l?= =?UTF-8?q?ist=20for=20sign-off?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/product/code-quality-audit.md | 370 +++++++++++++++++++++++++++++ 1 file changed, 370 insertions(+) create mode 100644 docs/product/code-quality-audit.md diff --git a/docs/product/code-quality-audit.md b/docs/product/code-quality-audit.md new file mode 100644 index 0000000..df59c85 --- /dev/null +++ b/docs/product/code-quality-audit.md @@ -0,0 +1,370 @@ +# ReaSampler code-quality audit — Q-W0 findings report and triage + +Date: 2026-07-28 · Branch: `pq-w0-audit` · Static analysis only; no code changed by the audit. + +This is the committed Q-W0 findings report (Q-10 SETTLED: a committed doc beside the SOLID/naming +audit — `docs/product/code-organization.md` §2c.3; deliverable contract in PLAN.md §Q-W0). It +synthesizes four parallel audit tracks; the full track notes remain in the tree as appendices and +are the evidence base for every claim here — this report cites finding IDs and does not restate +mechanisms in full: + +- **Track 1 — DSP / audio algorithm quality:** [`audit-notes/q-w0-t1-dsp.md`](audit-notes/q-w0-t1-dsp.md) (T1-01…T1-11) +- **Track 2 — architecture smells (functional lens):** [`audit-notes/q-w0-t2-architecture.md`](audit-notes/q-w0-t2-architecture.md) (T2-01…T2-11) +- **Track 3 — env-coupled-constant domain modeling:** [`audit-notes/q-w0-t3-env-constants.md`](audit-notes/q-w0-t3-env-constants.md) (T3-01…T3-07) +- **Track 4 — structural sizing + placement:** [`audit-notes/q-w0-t4-sizing.md`](audit-notes/q-w0-t4-sizing.md) (T4-01…T4-30) + +Dispositions below are **proposals**. Per the Q-W0 sign-off gate, Q-W1 does not begin until Daniel +has signed off on every disposition; the open calls are collected in §4. + +--- + +## 1. Verdicts up front + +**DSP / pitch engine (the Q-11 question).** The correlation-aligned SOLA in `pitch_shift` is +**sound — no technique replacement (phase-vocoder / WSOLA) is warranted on this evidence** (T1 +overall verdict). Track 1 finds the implementation "unusually well-defended" (normalized +correlation, ratio-scaled fades with drain-headroom derivation, prime-with-real-content onset, +frozen-writer tail, filled-span clamping) with RT discipline intact throughout. Every T1 finding +sits on the Q-11 escalation ladder's first rungs — bounded fixes within the existing technique, or +documented operating limits — exactly the settled default. The one High finding (T1-01, stereo +splice decorrelation) is a bounded fix inside the current design (link the per-channel lag +search), not a technique change. Track 1 states plainly that it cannot listen: every artifact is +mechanism + predicted audible consequence, and perceptual materiality is Daniel's call. + +**Architecture (functional smells).** The load-bearing boundaries hold: no pure module includes a +host type, the VST bridge is genuinely read-only, WAV *decoding* has exactly one owner, the prune +and exception boundaries audit clean (T2 clean list). What Track 2 found instead is the classic +cost of duplicated *algorithms*: the length-prefixed wire `Cursor` copy-pasted 3× **with +security-hardening drift** — the oldest copy (`provenance`) missing the overflow guards its +siblings gained (T2-01, High, with a cheap Q-W0 backport); a fifth hand-rolled JSON decoder the +§2 audit did not count (T2-02); `readFileBytes` ×5 (T2-03); the ext-state grow-loop ×3 with its +pure decode half-adopted (T2-04); a 19-struct rect zoo (T2-05); and drifting copy-paste in the +capture-stamp epilogue (T2-09). All are dedup/relocation-shaped and route onto the reorg waves; +none is a live user-facing bug except the T2-01 robustness gap. + +**Env-coupled constants (the prior-incident category).** The persistence surfaces — the +highest-stakes case — are **clean**: every wall-clock quantity written to disk since the S12 +remediation is in seconds or ms, and every persisted frame-domain value is a source-file fact +whose rate travels with it (T3 clean list). Seven findings, only two fix-now: the master-gain +ramp step hard-codes 20 ms × 48 kHz (T3-01) and the Trigger-fade UI ceiling hard-codes +2 s × 44.1 kHz (T3-03) — both live hardcoded-rate residues in `src/`, both trivial, both in files +no then-planned downstream wave opens. The rest are deliberate-and-documented couplings or +recorded legacy residue, plus one systemic usability note (no DPI/content-scale support, T3-05) +that is a future phase of its own. + +**Sizing + placement.** The wave plan's four planned splits are necessary but no longer +sufficient: the census measures **9 files needing TU splits and 2 needing header-only splits — 5 +of them VST-side, which currently have no owning wave** (T4 §1.5). `reasampler_editor.cpp` +(3,065 LOC) is now the largest unowned file in the repo. Track 4's structural answer is a new +VST god-module wave (**Q-W2v**), reshaped seam lists for Q-W2 (6→8) and Q-W3 (3→4 hoists), one +documented exception (`sampler_core.cpp` stays whole — hot path, T4-14/T4-27), and one dead +abstraction to delete (`ICaptureBackend`, T4-26 — the brief's "two implementations" assumption is +false at current state). The `src/vst/` placement question is a genuine fork for Daniel +(T4-18/T4-19, §4a). + +--- + +## 2. Unified findings register + +Every finding from all four tracks, exactly once, with proposed disposition. Severities are the +tracks' own. "Q-W0" as a destination means remediated in this wave before it closes (post +sign-off). Cross-track overlaps are reconciled in §2.5. + +### 2.1 Track 1 — DSP (appendix: `q-w0-t1-dsp.md`) + +| ID | Finding (one line) | Sev | Proposed disposition | Rationale | +|----|--------------------|-----|----------------------|-----------| +| T1-01 | Stereo Preserve: per-channel independent splice alignment decorrelates L/R (image wander + mono-sum combing on stereo captures) | High | **Fix-now in Q-W0** (bounded SOLA fix: linked lag/schedule across channels) — **Daniel's call, §4b** | Hits the flagship path (Preserve default + permanently stereo bus); standard stereo-SOLA practice; no technique change | +| T1-02 | Ratio slew mid-fade can drain the outgoing tap past the writer (pitch-env attack case) | Med | Document-and-defer; bounded re-cap noted for when the file is next opened | Needs pitch-env + Preserve + steep attack to trigger; constant-ratio case already covered | +| T1-03 | Preserve prime ignores Trigger `playEnd_` bound; zero-pads sub-window samples as declared-valid ring content | Med | **Fix-now in Q-W0** (prime to feedBound + immediate `freezeTail()`) **if Daniel agrees short one-shots matter, else defer with note — §4b** | Small, contained in `Voice::start`; re-uses designed GA3 machinery | +| T1-04 | No sustain-loop crossfade — hard loop seam clicks unless loop points amplitude-matched | Med | Document-and-defer (record beside the zone-loop spec) | A crossfade is a feature (parameter + UI), wrong scope for a reorg phase | +| T1-05 | Linear interpolation + no band-limiting on repitch (both engines) | Low | Document-and-defer (recorded trade-off; cubic Hermite noted as drop-in if ever wanted) | Classic sampler behavior, deterministic and consistent across engines | +| T1-06 | Correlation search: coarse step-4 can mis-lock above ~5 kHz; maxLag bounds alignment to ≥ ~80 Hz | Low | Document-and-defer (record as the engine's stated operating range) | Inherent SOLA range/cost trades; widening costs splice-burst CPU linearly | +| T1-07 | `splice()` up-jump clamp comment contradicts the code (code is exactly tight; comment's margin direction is backwards) | Low | **Fix-now in Q-W0** (comment rewrite, zero behavior change) | Misleads the next maintainer of a safety-critical clamp; one line | +| T1-08 | Linear-in-amplitude ADSR decay/release (constant-dB nowhere; abrupt-late releases) | Low | Document-and-defer | Character-vs-correctness product decision; changing it alters every existing instrument's feel | +| T1-09 | `declickR_` is dead state (blend correctly shares one weight; R is seeded/decayed, never read) | Low | Fix-now-trivial **as a rider on any Q-W0 `sampler_core` edit** (T1-01/T1-03); else defer | Hygiene only, no audio effect; not worth a standalone change | +| T1-10 | `planWavTruncate` silently drops RIFF chunks after `data` (metadata loss on trim) | Low | Document-and-defer (note in the header's FORMAT ASSUMPTION block when next touched) | Metadata-only; preserving trailing chunks complicates the single-truncating-write design for no audio benefit | +| T1-11 | `makeUniqueTag` 1 s resolution → same-second batch captures collide (silent overwrite) | Low-Med | **Fix-now, assigned to Q-W3** (per-session monotonic counter, both call sites); Q-W0 fallback if triage prefers | T1 explicitly routes at triage; Q-W3 is the nearest wave opening the extension capture flow | + +### 2.2 Track 2 — architecture (appendix: `q-w0-t2-architecture.md`) + +| ID | Finding (one line) | Sev | Proposed disposition | Rationale | +|----|--------------------|-----|----------------------|-----------| +| T2-01 | Wire `Cursor` ×3 with hardening drift — `provenance` lacks the overflow/length guards its siblings have; unbounded `reserve` reachable from persisted input | High | **Fix-now, split:** (a) backport hardened `field()` + count sanity bound to `provenance.cpp` **in Q-W0** (§4c); (b) structural collapse to one shared wire codec **in Q-W1** | Hazard is cheap to close now with existing `provenance_tests`; the dedup should ride the wave already creating `core/` | +| T2-02 | Fifth hand-rolled JSON decoder in `tail_control` (the §2 "4× Parser" undercount) | Med | **Fix-now, folded into Q-W1** — add `tail_control` to the `core/json` consumer list explicitly | Zero extra cost when `core/json` lands; a stray fifth decoder afterward would be a defect of the wave | +| T2-03 | `readFileBytes` hand-rolled ×5 across both artifacts | Med | **Fix-now, folded into Q-W1** — one pure helper in the `core/` utility home; both targets link it | Five copies of a ten-line function; creating its home is exactly Q-W1's job | +| T2-04 | `GetProjExtState` grow-loop ×3; pure `bridge_marshal` decode only half-adopted (`usage_scan`'s copy is prune-safety-adjacent) | Med | **Fix-now, assigned to Q-W5** (the wave that splits `persist.cpp` — T2's own rule; its "Q-W4" label predates the plan's persist=W5 numbering) | Touching persist's session machinery outside its own wave risks the highest-traffic shell for a dedup with no live bug | +| T2-05 | 19 rect structs + ~15 inline point-in-rect predicates across the pure UI family | Med | **Fix-now, folded into Q-W1** — reconciled with T4-21 into one disposition, see §2.5(1) | Same-moment-as-relocation principle; PLAN Q-W1 already owns the `ui::` rect unification | +| T2-06 | Pure-computable layout math stranded in the VST editor shell (~49 inline geometry computations; §2's VST scope gap) | Med | Document-and-defer **with named reshape — satisfied by Q-W2v's `editor_layout` pure-candidate TU** (see §2.5(4), §3) | Behavior-preserving hoist best done under the reorg's test discipline; must be a recorded point or the layer keeps growing | +| T2-07 | Extension links the entire voice engine to serialize one preset blob (codec not separable from engine) | Low | Document-and-defer → **executed by Q-W2v's `component_state_io` split** (same split as T4-13, see §2.5(3)) | Right abstraction, wrong granularity; a module-homing decision the reorg waves exist to make once | +| T2-08 | WAV/RIFF container knowledge in 4 modules / 2 chunk walkers (dedup-by-hash + null-test invariants sit on their agreement) | Low | Document-and-defer → **consolidation moment is a triage question, §4e** (T2 prefers the `core/wav` homing moment; T4-23 prefers the wave that opens `ingest.cpp`) | All four currently correct against each other; pre-reorg consolidation churns the capture hot path for no functional gain | +| T2-09 | Capture backends' Sample-stamping epilogue copy-paste with silent divergences (active-project vs pinned-project time-sig read) | Med | **Fix-now, folded into Q-W3** — extract shared `stampCaptureSample` helper; divergent bits stay in the realtime caller | Q-W3 opens both backend TUs anyway; keeps one review of precision-invariant-adjacent code | +| T2-10 | Thumbnail cache-invalidation drifted across the split (extension: pure generation-baked key; editor: ad-hoc string key + call-site `clear()`s) | Low | Document-and-defer — adopt the pure `ThumbnailKey` on the VST side **as a rider on Q-W2v's editor work** | No live bug; pointless as standalone churn, natural rider on the editor wave | +| T2-11 | ComponentState v1→v11 deserialize chain sound, but v3/v4/v5 legacy branches triplicate the shared read | Low | Document-and-defer, explicitly — record that the next envelope bump (v12) extends the shared path rather than minting another branch | Legacy branches are frozen back-compat contract; rewriting them risks the one thing they must never break | + +### 2.3 Track 3 — env-coupled constants (appendix: `q-w0-t3-env-constants.md`) + +| ID | Finding (one line) | Sev | Proposed disposition | Rationale | +|----|--------------------|-----|----------------------|-----------| +| T3-01 | Master-gain ramp step is a per-sample constant baking in 20 ms × 48 kHz (`kGainRampRate = 1/960`); FB1 no-zipper contract degrades silently at higher rates | Med | **Fix-now in Q-W0** — store `kGainRampSeconds`, derive step from `sampleRate_` (the file's own `kPreserveWindowMs` pattern) — **§4d** | Trivial, isolated, behavior-identical at 48 kHz; a live violation of the no-hardcoded-rate ruling in a file no then-planned wave opens | +| T3-02 | Takeover-declick decay is a per-frame coefficient (~2× faster at 96 kHz) — documented deliberate in-code | Low | Document-and-defer — triage ratifies the in-code note as the record | Already an explicit, written, bounded design decision; converting buys no audible improvement | +| T3-03 | Trigger-fade UI throw ceiling hardcodes 2 s × 44 100 as `88200.0` frames (knob full-scale varies per source rate) | Low | **Fix-now in Q-W0** — `kFadeMaxSeconds = 2.0` resolved against the loaded source's rate; T3's stated fallback (defer, amend comment) if zero UI-feel change is preferred — **§4d** | Small and contained; storage domain unchanged; the editor already threads `frameCount + rate` through pack/unpack | +| T3-04 | Drop-hint banner duration stored in sync-timer ticks (6 × 500 ms) | Low | Document-and-defer; fold in opportunistically if the file is opened | Cosmetic, self-documenting, cadence and decay live three lines apart | +| T3-05 | Systemic: no DPI/content-scale support in either UI surface (all layout constants are physical px at ~96 DPI) | Med | Document-and-defer — record as a named future phase; Q-W1's geometry relocation keeps constants centralized so the eventual scale factor lands in one place | A proper UI-scaling pass is a feature wave of its own, far outside Q-W0's remediation budget | +| T3-06 | Legacy v3 zone-payload lift divides by the *current* project rate (skewed times if the rate changed since write; write-era rate never recorded) | Low | Document-and-defer — this report is the record; the skew is a known, not a future mystery bug | Unrecoverable in principle; the documented residue of the incident that motivated the seconds invariant | +| T3-07 | SOLA correlation-segment cap of 512 frames — frame-domain by design (CPU bound); wall-clock span halves at 96 kHz | Low | Document-and-defer — **resolved against Track 1's verdict, see §2.5(2)** | T3 judged the frame domain arguably correct for a compute bound and handed the quality call to T1 | + +### 2.4 Track 4 — sizing + placement (appendix: `q-w0-t4-sizing.md`) + +| ID | Finding (one line) | Sev | Proposed disposition | Rationale | +|----|--------------------|-----|----------------------|-----------| +| T4-01 | `bank_panel.cpp` (3459): the plan's six seams no longer land sub-600 — `panel_render` ~700, `panel_input` ~800 | High | **Fix-now → reshapes Q-W2:** eight TUs, adding `panel_layout` and `panel_drag` | Without the two new seams, two of six TUs ship >600 on day one | +| T4-02 | `main.cpp` (1897): `capture_orchestrator` as specced lands ~885 | High | **Fix-now → reshapes Q-W3:** fourth hoist `capture_batch` (batch family + recapture + selection guards) | Recapture is planner-driven like batch and shares the guard machinery — it belongs with batch | +| T4-03 | `actions.cpp` (1016): plan's seams still land sub-600 | — | No change to Q-W4 (confirmation) | Measured against the current tree | +| T4-04 | `persist.cpp` (852): plan's seams still land sub-600; pS-usage growth landed exactly where the plan isolates it | — | No change to Q-W5 (confirmation) | Measured against the current tree | +| T4-05 | `bank_book.cpp` (1109): `SlotMap` is a self-contained type; post-JSON-extraction remainder splits cleanly | Med | Fix-now, fold into Q-W1 (`slot_map` extraction rides the JSON rewire already opening this file) | One `git mv`-shaped extraction on top of owned work | +| T4-06 | `view_mode_model.cpp` (1049) + `.h` (748): planners separable from model+indexes after JSON extraction | Med | Fix-now, fold into Q-W1 (planner split rides the JSON rewire); T4 allows deferring the planner split if the wave wants to stay minimal (~660 post-extraction is marginal) | JSON rewire opens the file; header splits the same way | +| T4-07 | `bank_model.cpp` (767): model vs JSON — the Q-W1 poster child, ~250 after extraction | — | Already owned by Q-W1; no new seam (confirmation) | — | +| T4-08 | `capture_realtime.cpp` (867): async lifecycle vs file-side finalize are distinct concerns | Low-Med | Fix-now, ride Q-W3 (`capture_realtime_finalize.cpp` split rides the Q-9 naming rider) | Same-wave file surgery is free | +| T4-09 | `view.cpp` (677): park/restore vs D2 lane machinery are separable halves | Low | Document-and-defer unless Q-W1's relocation touches it — then take the free `view_lanes` split | 677 is barely over the bar | +| T4-10 | `ingest.cpp` (636): pure WAV/PCM build helpers trapped in a shell TU | Low-Med | Fix in whichever wave lands the WAV consolidation — **moment is the §4e triage question** (circular with T4-23; no current wave opens `ingest.cpp`) | Extracting the pure WAV build gains a test target and drops the shell to ~500 | +| T4-11 | `vst/reasampler_editor.cpp` (3065): the largest unowned file — the `bank_panel` of the VST artifact | Highest | **Fix-now → new wave Q-W2v:** eight TUs (session / controls / layout / paint ×2 / input ×2 / platform), split axis = the face structure | Grew past `main.cpp` after the plan was written; same god-module profile | +| T4-12 | `vst/reasampler_processor.cpp` (1164): four seams | Med-High | **Fix-now → Q-W2v:** three TUs (`processor_state` / `processor_reload` / lifecycle+`process()` whole) | Partial-class-across-TUs is the same pattern as the Q-W2 panel split | +| T4-13 | `vst/sample_map.cpp` (970) + `.h` (708): resolution core vs binary ComponentState codec | Med-High | **Fix-now → Q-W2v:** split `component_state_io.cpp` + matching header split — one disposition with T2-07, see §2.5(3) | The codec grows every envelope bump (v6→v11 in one quarter); reduces editor/processor rebuild fan-out | +| T4-14 | `vst/sampler_core.cpp` (968) + `.h` (762): TU is a genuine single responsibility on the hottest path | Med | **Fix-now → Q-W2v, header only:** split `zone_params.h` from `sampler_core.h`; **TU stays whole — documented exception to the 600 bar** (record in the wave brief so nobody "fixes" it later) | Same-TU inlining on the per-sample path; no LTO in the build (see T4-27) | +| T4-15 | `view_mode_model.h` (748) | — | Covered under T4-06 (splits with its TU) | — | +| T4-16 | `vst/sample_map.h` (708) | — | Covered under T4-13 | — | +| T4-17 | `vst/sampler_core.h` (762) | — | Covered under T4-14 | — | +| T4-18 | VST placement, leading recommendation: integrate into the one `core/`/`shell/` split with `instrument/` subsystem dirs | — | **Daniel's fork — §4a** (T4 recommends T4-18) | One rule, no special case; the artifact boundary is a link-graph fact the sources already straddle | +| T4-19 | VST placement, alternative: parallel artifact-first subtree (`src/vst/core|shell` behind one SDK-gated `add_subdirectory`) | — | **Daniel's fork — §4a** | Defensible if artifact legibility outweighs discipline uniformity; T4 notes its self-containment is cosmetic | +| T4-20 | Little-endian byte codec hand-rolled ×5 — real template win (`putLE`/`readLE`) | Med | Fix-now, lands with `component_state_io` in Q-W2v (its biggest consumer); other consumers rewire opportunistically — relationship to T2-03/T2-04 noted in §2.5(5) | Compile-time dispatch, zero runtime cost, entirely off hot paths; gives the codec a tested primitive | +| T4-21 | Rect family: unify with one **concrete** `ui::Rect` + `contains()` + per-role aliases — NOT a template; retire the XYWH-vs-LTRB fork | Med | **Fix-now, ride Q-W1** — reconciled with T2-05 into one disposition, see §2.5(1) | The types differ in name only; a template would model nothing; cross-lib `Rect` collision is fixed by the Q-4 sub-namespaces | +| T4-22 | Linear rect-scan hit-tests: one small `hitIndex` template collapses them — only after T4-21 | Low | Document-and-defer; opportunistic rider on Q-W1 once the rect unification lands | Alone it would be a forced template | +| T4-23 | WAV build/parse consolidation into one pure `wav_codec` owner (dedup, not template) | Low-Med | Fix-now-sized, but the owning moment is the **§4e triage question** (T4 assigns it to the wave opening `ingest.cpp`; T2-08 prefers the `core/wav` homing moment; no current wave opens ingest) | Avoid a standalone churn commit; one tested owner of the RIFF layout | +| T4-24 | `clamp01` ×6 — one `constexpr inline` (or `std::clamp` at sites); anti-template | Trivial | Fix-now, rider on Q-W1 relocation | — | +| T4-25 | JSON `Parser` ×4 confirmed still accurate; `sample_usage` is **not** a fifth JSON parser (no scope growth from this track) | — | No change to Q-W1 (confirmation; T2-02's fifth decoder is a different file and does grow the consumer list) | — | +| T4-26 | `ICaptureBackend` is a dead abstraction: one deriver, zero polymorphic call sites; the brief's "two implementations" assumption is FALSE (realtime deliberately has a bespoke seam) | Low (runtime) / Med (hygiene) | **Fix-now, in Q-W3:** delete the interface, `OfflineRenderBackend` becomes concrete; **correct CLAUDE.md/CONTEXT ("ICaptureBackend interface; two backends") in the same commit** | It misleads — this audit's own brief was misled; Q-W3 touches every call site | +| T4-27 | Warning: do **not** split `sampler_core.cpp` along class lines — envelope `tick()`s are per-voice-per-sample; a by-class split is the exact heuristic-(c) dispatch blowout | — | Record as a guardrail in the Q-W2v brief (pairs with T4-14's whole-TU exception) | Same-TU definition is what lets the compiler inline the stack today | +| T4-28 | Warning to Q-W2: audition/preview stays a direct call-through across the 8-TU split; per-mouse-move work stays plain free-function calls | — | Record as a guardrail in the (reshaped) Q-W2 brief — reaffirms the plan's own guardrail; the two added TUs introduce no new risk | — | +| T4-29 | Warning to the processor split: `process()` + per-block helpers stay one TU; the atomic-pointer-swap pattern must not gain a virtual seam | — | Record as a guardrail in the Q-W2v brief | — | +| T4-30 | No other gratuitous indirection found (verified: only extension-side `virtual` is T4-26; VST virtuals are SDK-mandated; layered pure→shell pairs are the discipline, all direct calls) | — | Clean verification — see §5 | — | + +### 2.5 Cross-track dedupes and reconciliations + +1. **T2-05 ≡ T4-21 (+ T4-22 rider) — the rect zoo. Reconciled into ONE disposition: fix-now, + ride Q-W1.** Both tracks found the same duplication (19 XYWH structs + inline predicates; + T4-21 adds the VST side's second LTRB grammar) and both prescribe the same mechanism — one + concrete `ui::Rect` + `contains()` with per-role aliases, explicitly **not** a template + (T4-21's ruling). The only divergence was the wave label: T2-05 said "Q-W2 (the ui/ + relocation wave)", but in the plan the relocation wave — and the settled `ui::` rect + unification (PLAN §Q-W1, "shared pure-UI rect types … get one `ui::` owner") — is **Q-W1**. + T2-05's own rationale ("same-moment-as-relocation") therefore points at Q-W1; reconciled + there. T4-22's hit-test template stays a deferred opportunistic rider behind it. +2. **T3-07 → T1 — the 512-frame correlation cap. Resolved: document-and-defer.** T3 handed the + "is 512 the right number" quality call to Track 1. Track 1's verdict on the correlation + search's bounds (T1-06) is document-and-defer — record the alignment limits as the engine's + stated operating range; "widening either costs splice-burst CPU linearly." Note for accuracy: + T1-06 adjudicates the coarse-step and `maxLag` bounds specifically and does not name the + 512-frame `corrFrames_` cap; the resolution here rests on T1's general verdict (bounded + limits recorded, no change recommended) applied to the same search-cost family. If triage + wants the 512 value separately adjudicated, that is a residual question — flagged rather than + silently absorbed. +3. **T2-07 ≡ T4-13 — the ComponentState codec split. One disposition: Q-W2v's + `component_state_io`.** T2-07's complaint (extension links the whole voice engine to share + the preset serializer) is *solved by* T4-13's proposed split; T2-07's link-weight rationale + rides the T4-13 seam. T4-14's `zone_params.h` header split completes the extension-side + decoupling. T2 had provisionally pointed at "Q-W1/Q-W2 module-homing"; with Q-W2v now + proposed as the wave that opens `sample_map`, that is the owning wave. +4. **T2-06 ≡ T4-11's `editor_layout` — the stranded editor layout math.** T2-06's + document-and-defer explicitly demanded a *named* downstream reshape; Q-W2v's `editor_layout` + TU (T4-11, marked "pure candidate") is that reshape. One disposition: assigned to Q-W2v, with + the hoist targeting the existing pure homes (`editor_geometry` is T2's named natural owner). + T2-06's scope note (the §2 god-module catalogue missed the VST side) is also the evidence + base for Q-W2v existing at all. +5. **T4-20 / T2-03 / T2-04 — related but distinct dedup family, three separate dispositions.** + All three converge on shared `core/` utility homes but touch disjoint code: T4-20 (LE byte + codec ×5) lands with `component_state_io` in Q-W2v; T2-03 (`readFileBytes` ×5) lands in + Q-W1's utility home; T2-04 (ext-state grow-loop ×3, generalizing `bridge_marshal`) lands in + Q-W5 with the persist split. Marked here so triage sees them as one family and no wave + assumes another already covered its slice. +6. **T2-08 / T4-23 / T4-10 — WAV/RIFF consolidation. Genuine track disagreement on the moment; + surfaced as §4e** rather than silently picked. T2-08 defers to "the reorg wave that + relocates `wav_trim`" (Q-W1's relocation); T4-23 assigns to "the wave that opens + `ingest.cpp`" — and T4-10 assigns the ingest split to "whichever wave lands the WAV + consolidation," which is circular: **no current wave opens `ingest.cpp`.** +7. **T1-11 — capture-tag collision.** T1 offered it to Track 2 ("Track 2 may claim it"); + Track 2 did not. It remains a single T1 finding, routed per T1's own suggestion to Q-W3. +8. **Cross-track interaction on the DSP/VST fix-nows.** T1 and T3 both justified + fix-now-in-Q-W0 partly by "no downstream wave opens these files" — written before T4 + proposed Q-W2v, which *does* open `reasampler_processor.cpp` / `reasampler_editor.cpp` / + `sampler_core.h`. The recommendation stands unchanged: Q-W2v is a behavior-preserving + mechanical-split wave, and folding behavior-changing DSP/domain fixes into it would break the + wave discipline (CTest-green mechanical moves, no logic change). Fix-now items stay in Q-W0; + noted so the rationale reads correctly against the reshaped plan. + +--- + +## 3. Plan reshape — what Q-W0 proposes for Q-W1..Q-W6 + +The concrete deltas to the PLAN §Phase Q wave graph. Every wave is named; "no change" +confirmations included deliberately. + +**Q-W0 (this wave, post sign-off) — remediations before close:** T2-01(a) provenance cursor +hardening backport; T3-01 gain-ramp seconds; T3-03 fade-ceiling seconds (or its stated fallback); +T1-07 comment fix; pending §4b — T1-01 linked-lag stereo fix and T1-03 prime bound, with T1-09 +riding any `sampler_core` edit. Each behavior-touching remediation lands with its module's CTest +target green per the Q-W0 verify contract. + +**Q-W1 (safe opener — scope grows):** +- Add `tail_control` to the `core/json` consumer list (T2-02) — the wave's "one JSON path" goal + is not met without it. (T4-25 confirms the original 4× scope is otherwise accurate.) +- Add the shared `readFileBytes` pure helper to the `core/` utility home (T2-03). +- Rect unification: one concrete `ui::Rect` + `contains()` + per-role aliases, including + retiring the XYWH-vs-LTRB fork and folding in `editor_geometry`'s `Rect` (T2-05 ≡ T4-21); + `clamp01` dedup rider (T4-24); `hitIndex` template only as an opportunistic follow-on (T4-22). +- Structural collapse of the wire `Cursor` family into one shared `wire` codec module beside + `core/json`, consumed by `provenance` / `assignment_request` / `sample_usage` / + `parseBankGeneration` (T2-01(b)). +- `slot_map` extraction rider on the `bank_book` JSON rewire (T4-05); `view_mode_model` planner + split rider (T4-06, optional per T4); `bank_model` unchanged-as-planned (T4-07); `view_lanes` + split only if relocation touches `view.cpp` anyway (T4-09). +- Relocation scope grows to include the ~20 clean VST pure libs, under whichever placement shape + §4a settles (T4 §1.5, T4-18/T4-19). + +**Q-W2 (bank_panel split — 6→8 seams):** the wave brief must name **eight** TUs — the planned +six plus `panel_layout` and `panel_drag` (T4-01) — or two of its TUs ship >600 on day one. +Guardrails reaffirmed: audition direct call-through; per-mouse-move work stays plain free-function +calls (T4-28). + +**Q-W2v (NEW — VST god-module wave; the T2-06/T4 §1.5 scope gap made structural):** +- `reasampler_editor.cpp` → eight TUs: session / controls / **layout (pure-candidate hoist into + the existing pure homes — this discharges T2-06)** / paint_sample / paint_browse_zone / + input_sample / input_browse_zone / platform (T4-11). +- `reasampler_processor.cpp` → three TUs: `processor_state` / `processor_reload` / + lifecycle+`process()` kept whole (T4-12), with the T4-29 guardrail (no virtual seam on the + atomic-swap pattern). +- `sample_map` → `component_state_io` codec split + matching header split (T4-13 ≡ T2-07). +- `sampler_core`: **TU stays whole at 968 — documented hot-path exception** recorded in the wave + brief (T4-14, T4-27); header splits into `zone_params.h` + `sampler_core.h`. +- The `core/wire/bytes.h` LE-codec template lands here with its biggest consumer (T4-20). +- Rider: adopt the pure `ThumbnailKey` on the VST side while the editor is open (T2-10). +- **Scheduling:** parallel-safe with Q-W2 (different artifact, zero file overlap) — T4 also + offers serial-after-W5 as "Q-W7" if Daniel prefers serial waves (§4f). + +**Q-W3 (main.cpp split — 3→4 hoists):** add **`capture_batch`** (batch family + +`RunRecaptureFromSource` + the two selection guards) as a fourth TU so `capture_orchestrator` +lands ~450 (T4-02). Also owned here: **delete `ICaptureBackend`** and correct the +CLAUDE.md/CONTEXT description in the same commit (T4-26); the shared `stampCaptureSample` +epilogue dedup (T2-09); the `capture_realtime_finalize` split riding the Q-9 naming rider +(T4-08); the `makeUniqueTag` monotonic-counter fix (T1-11). + +**Q-W4 (actions split): no change** — the planned seams still land sub-600 (T4-03). + +**Q-W5 (persist split): seams unchanged** (T4-04); **add** the ext-state grow-loop dedup — +generalize the retry policy into `bridge_marshal` (or its `core/` successor) and rewire all three +loops, `usage_scan`'s prune-safety-adjacent copy included (T2-04). + +**Q-W6 (registration table): no change**; T4-02 notes the `main.cpp` registration residue (~385) +shrinks further under its table. + +**Unassigned pending §4e:** the WAV/RIFF consolidation family (T2-08 / T4-23 / T4-10) — no +current wave opens `ingest.cpp`; the owning moment is Daniel's call. + +Updated dependency sketch: + +``` +Q-W0 (this report + remediations) ── SUB-GATE: Daniel signs off every disposition (§4) ── + ▼ +Q-W1 (core/json + wire codec + relocation incl. VST pure libs + rect unification + riders) + ├─► Q-W2 (bank_panel split, 8 seams) ──► Q-W4 (actions; unchanged) + ├─► Q-W2v (NEW: VST god-modules — editor/processor/sample_map splits, sampler_core header, + │ LE codec) [parallel-safe with Q-W2; serial "Q-W7" alternative — §4f] + ├─► Q-W3 (main split, 4 hoists; ICaptureBackend deletion + doc fix; stamp dedup; T1-11) + │ └──► Q-W6 (registration table; unchanged) + └─► Q-W5 (persist; seams unchanged; + ext-state-loop dedup) [best after Q-W4] +``` + +--- + +## 4. Daniel's decision list (sign-off gate) + +Each item: leading recommendation first, alternative second. Settled matters (Q-10, Q-11 framing, +the clean bills) are deliberately absent. + +- **(a) VST placement fork — T4-18 vs T4-19.** *Recommend T4-18:* integrate `src/vst/` into the + one `core/`/`shell/` top split with `instrument/{engine,map,ui}` subsystem dirs — one rule + ("directory = may it touch a host type"), the artifact boundary is a link-graph fact the + sources already straddle, CMake impact is path-edits only. *Alternative T4-19:* artifact-first + subtree (`src/vst/core|shell` behind one SDK-gated `add_subdirectory`) — cleanest expression + of the Windows-only gate, at the cost of two parallel `core/` trees and a subtree whose + self-containment is cosmetic. Q-W1 executes whichever shape is chosen. +- **(b) DSP bounded fixes in Q-W0 — T1-01 and T1-03.** *Recommend fix-now for T1-01* (linked + L/R lag + splice schedule): High severity on the flagship stereo-Preserve path, standard + stereo-SOLA practice, no technique change. *Alternative:* defer and record as the known + stereo-Preserve limitation. *For T1-03* (prime bound + immediate `freezeTail()` on + sub-window spans): T1's own framing — fix-now **if the short-one-shot case matters** to you + (short drum one-shots are realistic content); else document-and-defer with the T1 note as the + record. T1-09 (`declickR_` dead state) rides whichever `sampler_core` edit happens. +- **(c) T2-01(a) provenance wire-cursor hardening backport in Q-W0.** *Recommend yes:* small, + pure, closes a concrete robustness gap on persisted user-editable input, covered by existing + `provenance_tests`. *Alternative:* wait for the Q-W1 wire-codec collapse to fix it + structurally — leaves the gap open through the gate for no saving. +- **(d) Env-constant fix-nows in Q-W0 — T3-01 and T3-03.** *Recommend fix-now for both:* each + is trivial, isolated, and a live hardcoded-rate residue Daniel's standing ruling forbids; + behavior-identical at the baked-in rates. *Alternative for T3-03 only* (T3's stated + fallback): document-and-defer with the comment amended to name the 44.1 k assumption, if zero + UI-feel change is preferred. (Folding either into Q-W2v instead is *not* recommended — it + would put behavior changes inside a mechanical-split wave; §2.5(8).) +- **(e) WAV/RIFF consolidation moment — the one true track disagreement (T2-08 vs T4-23/T4-10).** + T2 prefers the `core/wav` homing moment (the relocation wave); T4 prefers "the wave that opens + `ingest.cpp`" — which does not exist, and T4-10 points back circularly. *Recommend:* record + the consolidation (one pure `wav_codec` owner: walker + layout + build + patch, absorbing + T4-10's ingest extraction) as a named rider on **Q-W3** — the wave already opening the capture + family (`capture_realtime` finalize, T4-08) — with Q-W1-relocation as the alternative moment + if Daniel prefers T2's framing. Either way it must land somewhere named, or the + dedup-by-hash/null-test maintenance surface stays quadruplicated. +- **(f) Q-W2v scheduling.** *Recommend parallel with Q-W2* (different artifact, zero file + overlap — T4 §1.5). *Alternative:* sequence it serially after W5 as "Q-W7" if you want serial + waves throughout. + +--- + +## 5. Clean bills — surfaces audited and found clean + +Consolidated coverage evidence; details in the appendices' clean-surface sections. + +**DSP (T1):** `peaks` (bin partition exact, overflow-guarded, per-channel no-fold), `master_gain` +(taper math correct end to end), `velocity_curve` (Fritsch–Carlson monotonicity/no-overshoot +claims hold) — fully clean. Clean with only the noted findings: `wav_trim` (T1-10 metadata note), +offline capture path (T1-11; precision-invariant plumbing "disciplined"), realtime capture path +(T1-11 + already-in-code DAW-verify flags), `sampler_core`'s voice/steal/mono/panic machinery +(rate-free-seconds invariant honored; takeover blend mathematically sound), `pitch_shift`'s core +machinery (safe-band geometry, clamps, correlation, fades, `freezeTail` continuity all audit +sound; down-shift writer-lap unreachable above ≈ −109 st). + +**Architecture (T2):** pure-module include hygiene across both trees (zero host-type includes in +any claimed-pure module); `bank_sync`; `bridge_marshal`; the realtime record lifecycle (explicit +enum state machine, not implicit); project-identity transitions; `usage_scan` (decisions +delegated pure, depth-bounded recursion, protect-on-truncation); the three `catch (...)` sites +(documented, narrow, non-swallowing); `FxBypassGuard` vs `view.cpp` park/restore (duplication of +shape, not concept — correctly separate); path resolution (`resolveBankFile` is the single +resolver both sides); WAV decode (one decoder); the draw layer (no parallel vocabulary on the VST +side); interface cost (no hot-path virtual/`std::function` chains); boolean parameters (no +smell). Also T2-11's headline: the ComponentState v1→v11 lift chain is **functionally sound** — +the finding is shape, not correctness. + +**Env-coupled constants (T3):** `sample_map` v5+ persistence (the reference implementation); +ComponentState v6–v11 fields; Trigger fade/loop frames as source-file facts with the rate stored +alongside; `trigger_seam`; `kPreserveWindowMs` (the model pattern); `pitch_shift` internal +geometry; the tail system; `wav_trim`; `bank_model` persisted metadata; all ext-state wires; the +envelope schematic's param-domain scaling; every timer surveyed; `peaks` / `waveform_view` / +`master_gain` / `velocity_curve` / `keyboard_strip`. The persistence layer — the category's +highest-stakes surface — is clean end to end. + +**Sizing/indirection (T4):** borderline files needing no action: `capture.cpp` (549), +`reasampler_editor.h`/`reasampler_processor.h` (shrink with their TU splits), `bank_book.h`, +`pitch_shift.cpp` (371 — single responsibility, hot, leave), `persist.h`/`capture.h` (owned by +Q-W6). T4-30: no gratuitous indirection anywhere beyond the dead `ICaptureBackend` — VST-side +virtuals are SDK-mandated, and the layered pure→shell pairs are the load-bearing discipline, all +direct calls. Plan-confirmations: Q-W4 and Q-W5 seams land as planned (T4-03/T4-04); Q-W1's JSON +scope confirmed accurate modulo T2-02 (T4-25).