reasampler/docs/product/audit-notes/q-w0-t2-architecture.md

# 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 `<len>':'<bytes>` 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) |