Skip to content

fix: patchSmith spectralBalance calibration + Phase 1 v1→v2 replay guard - #135

Merged
slittycode merged 1 commit into
mainfrom
claude/kind-bardeen-5AeHy
Jun 2, 2026
Merged

fix: patchSmith spectralBalance calibration + Phase 1 v1→v2 replay guard#135
slittycode merged 1 commit into
mainfrom
claude/kind-bardeen-5AeHy

Conversation

@slittycode

Copy link
Copy Markdown
Owner

Why

Two confirmed defects surfaced by an independent cold review of the loudness-v2 program (#128#133). Both are local, low-risk fixes; the v2 migration itself was sound.

Fix 1 — patchSmith reads spectralBalance on the wrong scale (always-on, confirmed broken on real input)

patchSmith.ts treated spectralBalance.{band} as "dB above/below the mix average," but the analyzer emits absolute band energy (10·log10(mean_energy), ~ −100..0 dB — see analyze_core.py). On real tracks (golden: subBass −41.6, highs −62.2):

  • the sub-oscillator guard subBass <= 1.0 never fired ⇒ the sub layer never engaged;
  • the acid cutoff 1200·2^(brightness/6) with brightness ≈ −63 pinned to the 200 Hz floor every time;
  • the cited rationale ("sits X dB above the mix average") misstated the measurement (invariant Add phase1 visual story v2 deck assets and build scripts #2);
  • 14 green tests masked it because the fixtures used fabricated small/positive values.

Fix (local — the analyzer contract is unchanged; mixDoctor/dashboard rely on the absolute scale): add meanBandDb() and read each band as a prominence relative to the 7-band mix average. The sub osc now engages on genuinely sub-forward mixes, the acid cutoff tracks relative brightness, and the rationales are now true. Tests use realistic large-negative dB, with a regression case that fails on the old logic. The SUB_PROMINENCE_DB threshold is a documented first cut to revisit against the recommendation corpus (GOAL.md).

Fix 2 — phase1Version replay guard (persisted pre-v2 runs mis-render the core surface)

phase1Version was emitted but no consumer branched on it, and analysis_runtime stores raw payloads verbatim with no migration. Replaying a pre-#128 run fed v1 units into v2 consumers: a linear truePeak of 0.98 read as a +0.98 dBTP over (false TRUE_PEAK_OVER → false MISSING_LOUDNESS_ACTION), and a raw bpmConfidence of 3.2 rendered as 320% — a measurement-honesty regression (invariants #1/#4).

Fix: add _migrate_v1_loudness_units() at the top of _build_phase1 — the single chokepoint every persisted snapshot passes through (_normalize_run_snapshot). A payload lacking "phase1.v2" gets truePeak linear→dBTP, bpmConfidence raw→0–1, the incoherent plr dropped and recomputed, then stamped v2. No-op for v2 payloads.

Verification

Gate Result
npm run lint (tsc) ✅ clean
npm run test:unit ✅ 750/750 (incl. +3 patchSmith regression tests)
npm run build ✅ 2211 modules
backend unittest discover ✅ 1102 passed, skipped=3 (incl. +7 v1→v2 migration tests)
npm run test:smoke ⛔ not run — Playwright chromium download blocked by the sandbox network allowlist (403 'Host not in allowlist'). Both changes are pure logic covered by the suites that ran.

Not included

Corroborated minor doc/cosmetic items (e.g. phase2Validator.ts:486 stale "linear" comment, loudness_rec_evaluation.py inverted docstring, MeasurementDashboard.tsx:1674 decimals, phase1Picker.ts:104 per-stem truePeak unit) and verifying the .vital actually loads in Vital — left for a focused follow-up.

https://claude.ai/code/session_01C2VkJk4JAUGtz3b9BWqVGE


Generated by Claude Code

Two defects surfaced by an independent review of the loudness-v2 program
(#128#133):

patchSmith read spectralBalance bands as "dB above/below the mix average", but
the analyzer emits absolute band energy (10·log10(mean_energy), ~ -100..0). On
real tracks the sub oscillator never engaged (guard subBass<=1.0 against a real
~ -41 dB reading) and the acid cutoff pinned to the 200 Hz floor, while the
cited rationale misstated the measurement (invariant #2). Fix locally: add
meanBandDb() and read each band as a prominence relative to the 7-band mix
average — the analyzer contract is unchanged (mixDoctor/dashboard rely on the
absolute scale). Rationales are now true; tests use realistic absolute dB with
a regression case that fails on the old logic.

phase1Version was emitted but never consumed, so persisted pre-#128 runs
(stored verbatim in analysis_runtime, never migrated) replayed v1-unit values
into v2 consumers: a linear truePeak of 0.98 read as a +0.98 dBTP over (false
TRUE_PEAK_OVER / MISSING_LOUDNESS_ACTION) and a raw bpmConfidence of 3.2
rendered as 320%. Add _migrate_v1_loudness_units() at the top of _build_phase1
(the single chokepoint every persisted snapshot passes through): a payload
lacking "phase1.v2" gets truePeak linear→dBTP, bpmConfidence raw→0-1, plr
dropped and recomputed, then stamped v2. No-op for v2 payloads.

Verified: tsc clean; 750 frontend unit tests; vite build; 1102 backend
unittests (skipped=3). Playwright smoke could not run (chromium download
blocked by the sandbox network allowlist); both changes are pure logic covered
by the suites that ran.

https://claude.ai/code/session_01C2VkJk4JAUGtz3b9BWqVGE

@slittycode slittycode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE (posted as COMMENT — can't self-review)

Summary

Two real, confirmed bugs fixed: patchSmith.ts was comparing absolute dB values (~−100..0) against small absolute thresholds that never fired on real input, and _build_phase1 had no migration path for persisted v1 payloads whose truePeak was linear amplitude and bpmConfidence was raw Essentia scale. Both fixes are correct, the tests are meaningful (realistic dB fixtures, explicit regression case that fails on old logic), and the phase boundary invariants are clean. This is mergeable.

Findings

Worth considering (not blocking):

mapAcidFilter's brightness metric is (highs + brilliance) / 2 − mean(all 7 bands), where highs and brilliance are included in the 7-band mean. That means you're measuring "how much brighter are the top 2 bands than the whole-spectrum average including themselves." If the intent is "brighter than the low/mid body," excluding those two bands from the denominator would be more precise. In practice the difference is small and the current metric is a valid "is this a bright track?" signal — just noting the circularity so it's on record before the SUB_PROMINENCE_DB corpus revisit.

Worth knowing (no action needed):

_migrate_v1_loudness_units does a shallow dict copy (migrated = dict(payload)). Correct for the scalar fields mutated here. If a future version migrates nested dicts (e.g. spectralBalance), a shallow copy won't be sufficient — worth a note then, not now.

Test results

Tests could not be executed in this review environment (node_modules / Python venv not installed). The PR reports 750/750 frontend unit and 1102/1102 backend tests passing. New test cases verified by code trace:

  • test_v1_linear_true_peak_is_converted_to_dbtp: 20·log10(0.98) = −0.175 → −0.2 dBTP
  • test_v1_plr_is_recomputed_as_dbtp_minus_lufs: linear 1.0 → 0.0 dBTP, PLR = 0.0 − (−8.2) = 8.2
  • test_v2_payload_loudness_fields_are_not_migrated: guard phase1Version == "phase1.v2" skips migration, values unchanged ✅
  • The _minimal_payload helper getting "phase1Version": "phase1.v2" is load-bearing, not cosmetic — without it every pre-existing coercion test silently enters the migration path and gets bpmConfidence 0.92 → 0.184.

Phase boundary check

Clean.

  • patchSmith.ts (Phase 3): reads spectralBalance correctly, no Phase 1 mutation.
  • server_phase1.py: migration operates on a local dict() copy before building the HTTP response. The SQLite-persisted v1 record is untouched. Migration sits in _build_phase1, the single chokepoint every replayed snapshot passes through via _normalize_run_snapshot. Idempotent on v2 payloads.

Generated by Claude Code

@slittycode
slittycode merged commit 1dc9b80 into main Jun 2, 2026
4 checks passed
@slittycode
slittycode deleted the claude/kind-bardeen-5AeHy branch June 2, 2026 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants