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 Parsers 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)