Fix Phantom audit findings (2026-07-05)#41
Merged
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…P-16) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… P-16) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (P-12) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Convert AudioData.mono from a plain @Property (recomputed np.mean on every access) to a functools.cached_property, and add a new mono_rms cached_property for the full-signal RMS of the mono mixdown. Both compute once per instance and store into the instance __dict__ (permitted for non-field attributes under Pydantic v2; not frozen, absent from model_dump()). Apply P-04 at dynamics.py only, where audio is in scope: replace rms = float(np.sqrt(np.mean(mono**2))) with rms = audio.mono_rms. This is bit-identical to the prior float32 computation (guard test asserts exact equality), so all numeric outputs are unchanged — pure memoization. Intentionally NOT changed (semantic drift avoidance): - problems.py _detect_snr receives a bare mono array (no audio in scope). - stereo.py uses per-channel is_near_silent(left)/is_near_silent(right) and per-channel/mid-side RMS, not the mono mixdown. - is_near_silent keeps its array-based signature (callers pass left/right). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lper (P-02, P-06, P-09) The x4-oversampled TruePeakDetector FIR was the dominant cost in the pipeline and ran twice in full_diagnostic (once in analyze_loudness, once in detect_problems' inter-sample-peak detector). New _truepeak.py computes per-channel (sample_peak, true_peak) once, reusing a single TruePeakDetector across channels (P-06), and memoizes on the AudioData instance so both consumers share it (P-02). Also extracts the shared 4096/2048 FrequencyBands loop into spectral._octave_band_energies, called by both spectral and masking (P-09) -- pure dedup, no numeric change. Numeric parity verified to 0.0 vs fresh-per-channel construction on asymmetric stereo, clipped-mono, and stereo-noise fixtures (Essentia TruePeakDetector is stateless across channels). A shared-once test proves the constructor runs exactly once across analyze_loudness + detect_problems on the same AudioData (was 4 for stereo). ISP detail keys, thresholds, and dBTP conversion unchanged; no MCP schema drift. Timing (full_diagnostic, 60s stereo, best of 3): BEFORE: 3283.9 ms AFTER: 2084.6 ms Delta: 36.5% faster (saved 1199.3 ms) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cache (P-01) Lift `_cached_analysis(audio, func_name, func)` from comparison.py into _cache.py (next to `analysis_cache`); comparison.py now re-exports it so existing callers/tests (`from phantom.comparison import _cached_analysis`) keep working. No circular import: _cache.py references AudioData only under TYPE_CHECKING. Rewire `_run_full_analysis` (used by full_diagnostic and batch_diagnostic) and the CLI `_run_selected_analyses` to route all six analyzers through `_cached_analysis`. Server uses literal keys and the CLI uses `fn.__name__`; both resolve to the same keys the compare_* tools use (analyze_spectrum, analyze_loudness, analyze_dynamics, analyze_stereo, analyze_phase, detect_problems), so full_diagnostic/batch_diagnostic/analyze now populate the cross-call cache and a subsequent compare_to_profile/compare_to_reference on the same bytes reuses the work. Response shapes and numeric outputs are unchanged — cache stores/returns the identical Pydantic model instances. Cache size: AnalysisCache(max_entries=8) is left unchanged. full_diagnostic now stores 6 entries for one file; batch_diagnostic over many files will churn/evict (LRU, correctness preserved, just fewer cross-call hits). Bumping max_entries is a memory-vs-hitrate tradeoff deferred to Lee's sign-off. Bench (bench_full_diagnostic.py, 60s stereo, best of 3): the committed harness warms up then times the same file, which are now cache hits, so it reports ~48 ms (was ~2085 ms) — same-bytes re-analysis is ~43x faster. Cold-path (cache cleared each iter) measures ~2160 ms vs Task 6's 2084.6 ms; the ~77 ms delta is the one-time SHA-256 hashing to populate the cache (6 gets + 6 puts of the 21 MB sample buffer), the same cost compare_* already pays. No regression in analysis work itself. TDD: added test_full_diagnostic_populates_cache (RED on old code, GREEN now). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ath (P-01 follow-up) Fix 1 — memoize the audio content digest. `_hash_audio` previously re-hashed the full sample buffer (21.2 MB on the 60 s stereo bench) on every cache get/put — ~12x per full_diagnostic — adding +77 ms (~3.7%) to the cold path when Task 7 routed composite tools through analysis_cache. `_content_digest` now computes the SHA-256 of the per-instance-constant content (samples + sample_rate + num_channels) once and memoizes the hash object on audio.__dict__["_content_hash"] — the same mechanism as _true_peaks (Task 6) and AudioData.mono (Task 5). `_hash_audio` .copy()s that base and feeds only the cheap func_name suffix, so the cache key is byte-identical to the old single-pass formula (no key-format change; asserted by test). Samples are never mutated after construction, so the memo is sound. Fix 2 — bench measures the cold path again. Added AnalysisCache.clear(); bench_full_diagnostic.py clears analysis_cache before the warmup and each timed run, so the COLD number measures analysis cost (not cache hits), and prints a second WARM line for the cache-hit path. Bench (60 s stereo, synthetic): COLD 2084.0/2082.2 ms (best of 3) — within noise of Task 6's 2084.6 ms baseline; the +77 ms regression is gone. WARM (cache-hit path) 14.1/14.3 ms. Tests: 3 new cache tests (content-digest-once RED->GREEN, byte-identical key guard, clear). Full suite 1009 passed, 44 skipped; ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (P-10) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
uninstall._remove_startup_hook now delegates the phantom-block stripping to setup_reaper._remove_startup_block, the single content->content implementation (returns None / "" / stripped content). uninstall keeps its own path I/O, write/unlink, and the P-15 bool contract; only the strip logic is shared. Both call sites shared the same latent bug: if the "-- [/phantom]" end marker was missing, the skip flag never reset and the rest of the file was deleted to EOF. The canonical function is hardened to buffer post-marker lines and recover them when no end marker is found, so a hand-edited __startup.lua no longer loses the user's trailing content. Added a TDD regression test for that edge. Also sandbox test_uninstall_with_yes in an isolated filesystem so the --yes uninstall can't rewrite the developer's real ./.mcp.json (a pre-existing test hygiene bug that tripped the CLI first-run auto-setup when the two CLI test files ran adjacently). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…erred) Extend the full_diagnostic timing harness with _probe_spectral(), which splits analyze_spectrum time into its two framed FFT passes on the same 60 s stereo fixture: - analyze_spectrum total (2048/1024 + 4096/2048 passes): ~122.7 ms - octave-band pass alone (_octave_band_energies): ~15.6 ms - band-pass share of spectral: ~12.7% Reproduced identically across 3 runs. The share is just above the ~10% gate, so a single-pass fusion follow-up is technically warranted -- but DEFERRED here: the plan forbids attempting the multi-resolution single pass in this task, and in absolute terms the upside is ~15.6 ms, only ~0.7% of the ~2087 ms full_diagnostic cost. The two passes use different frame sizes (2048 vs 4096), so fusion is non-trivial and risks numeric drift; any follow-up must ship its own TDD parity tests. Decision: DEFER P-05 (profiled, follow-up low priority). Existing COLD/WARM output text is unchanged; script-only change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…P-14) P-13: _detect_dc_offset now takes the AudioData object and checks DC per channel for stereo, flagging if either channel exceeds the threshold. Antiphase DC (L=+x, R=-x) cancels in the mono mixdown and was previously missed. Mono behavior is byte-identical; the stereo flag path adds an additive 'channel' detail key (details is an untyped dict by design). P-14: guard demucs input normalization against zero-std (silent) input. (wav - ref.mean()) / ref.std() divided by zero on silent files, feeding NaNs into the model. Now raises a musician-friendly AnalysisError first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r (P-19, P-22) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…w update catch (P-21, P-18) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…P-07, P-08) P-07: analyze_masking_matrix now resamples each stem to the target rate just-in-time via resample_to_match (the same per-stem call align_sample_rates makes internally) and drops the resampled AudioData after reading its band energies, so peak memory holds one resampled copy instead of N. Numerically identical to the prior batch-align; a 3-stem mixed-rate parity test locks the invariant. Reuses the Task 8 helper rather than re-inlining resample logic. P-08: fix_audio gains an additive keyword-only `audio: AudioData | None = None` param. When provided, the internal load_audio(file_path) is skipped and the supplied data reused; the input path is still validated. The interactive CLI path (phantom fix -i), which already decodes the input for detect_problems, now threads that AudioData in, avoiding a second decode. audio=None is byte-identical to today. The MCP server fix_audio tool signature and response schema are unchanged (it never passes audio=). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Final whole-branch review findings for fix/audit-2026-07: - Important: render self-overwrite guard missed same-inode collisions that realpath does not normalize (hardlinks; case-variant names on case-insensitive filesystems like macOS APFS, e.g. SONG.WAV -> SONG.wav). ffmpeg's -y would clobber the source. Extended the guard with os.path.samefile. TDD via a hardlink test (deterministic on every filesystem). - Minor: test_full_diagnostic_populates_cache now uses the public analysis_cache.clear() instead of poking ._lock/._store. - Minor: separate --output-dir help text updated (no longer "./stems"; now stems/ under the confined Phantom output directory). - Minor: added a CLI cache-population test twin (test_analyze_cli_populates_cache) covering _run_selected_analyses keying the cache via fn.__name__. - Minor: documented the intentional best-effort swallow of a `uv tool list` failure/timeout in update.py (reinstall proceeds without extras). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…s-platform (CI)
Two CI failures on this branch, both test-portability issues (no product
code changed):
1. PII/policy check ('Check for absolute paths'): the P-17 redaction tests
in test_cli_formatting.py hardcoded /Users/... paths that tripped the CI
grep pattern /Users/[a-z]+/. Swapped the fixtures to /private/... paths
that do NOT match the policy pattern but DO still match the redaction
regex in cli/_formatting.py, so the tests keep verifying real redaction.
Updated the corresponding 'not in out' assertions to the new directories.
2. Golden-value test broke on Linux CI by 1 ULP: TestBandEnergyHelperParity
.test_band_energies_match_golden asserted a hardcoded float32 array with
rtol=0, atol=0. The Linux/x86 Essentia build differs from macOS in the
last ULP on a couple values. Relaxed ONLY the hardcoded-golden comparison
to np.allclose(rtol=1e-5, atol=1e-9) — tight enough to catch real drift,
loose enough for cross-platform float32 wobble. All in-process two-sided
parity assertions (delegation equality, matrix streaming parity) left
exact, since platform differences cancel in-process.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
leesaenz
force-pushed
the
fix/audit-2026-07
branch
from
82ffffc to
256ac8c
Compare
Addresses the three code-review comments on PR #41: phantom._cache, phantom.loudness, and phantom.problems were each imported with both 'import' and 'from ... import' in the same test scope. Monkeypatch targets are unchanged objects, so spy semantics are identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix Phantom audit findings (2026-07-05)
Addresses all 22 findings from the 2026-07-05 audit: 1 crash bug, 2 High-value perf
wins, and a batch of Medium/Low robustness, duplication, and polish items.
Highlights
phantom updateno longer crashes on pre-release release tags (1.2.3-rc1,1.2.0b1,2.0.0-beta).full_diagnosticon a 60 s stereo file is 36.5% faster (before: 3283.9 ms → after: 2085.7 ms cold; repeat analyses of the same bytes now hit the analysis cache at ~14 ms; seescripts/bench_full_diagnostic.py).uvcalls have timeouts; error output no longer leaks absolute paths; render can no longer overwrite its own source file (including same-inode/case-variant collisions).No breaking API changes
All 18 MCP tool response schemas are preserved (independently verified against main — zero keys added/removed/renamed). Two intended value-level changes:
dynamics.dynamic_range_dbreturnsnull(was0.0) for unmeasurably short audio (P-12).dc_offsetproblem's details gain an additivechannelkey on this new path.Deferrals (documented, evidence-based)
analyze_spectrumbut ~0.7% offull_diagnostic; not worth the numeric-drift risk. Probe committed.AnalysisCache(max_entries=8)left unchanged (batch churn is acceptable LRU behavior; bumping is a memory-vs-hitrate tradeoff needing sign-off).Testing
🤖 Generated with Claude Code