test(analysis): add Tier 1 real-audio accuracy acceptance - #890
test(analysis): add Tier 1 real-audio accuracy acceptance#890seonghobae wants to merge 1 commit into
Conversation
Prove a decoded C major WAV recovers C and a 120 BPM click WAV passes tempo Acc1, with checksum-fail-closed case reports. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 26 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — Tier 1 real-audio accuracy (ccbdae2d)
Tempo Acc1 is real buyer evidence: evaluate_click_tempo_file checksums, TemporalAnalyzer.analyze decodes the WAV, and a wrong true tempo or 240 BPM octave error fails.
The C-major headline case is not. test_c_major_wav_recovers_c_after_file_decode writes a file, then scores the pre-write in-memory triad. Doctoring that says “written to WAV, decoded, and scored” overstates the code. Checksum-fail-closed is also only half-true: the mismatch test never enters evaluate_*, and C-major scoring never verifies bytes.
The 0.70 recall floor is a BandScope tolerance. Odekerken et al. (2021) define WCSR/CSR; they do not prescribe 0.70 for a 3-second triad.
Buyer next action: Do not merge this head as “decoded WAV” evidence. Land a narrow honesty successor that (1) checksums + decodes the C-major file before scoring, (2) fails closed through evaluate_* on digest mismatch, and (3) cites 0.70 as an in-repo floor. Do not mix #884 Stem Lab or #888 lock-in onto this slice.
Merge posture: COMMENT only. Author is seonghobae. This automation does not approve or merge.
Sent by Cursor Automation: Fix Issues
| C_MAJOR_RECALL_FLOOR = 0.70 | ||
|
|
||
|
|
||
| def evaluate_c_major_pcm( |
There was a problem hiding this comment.
This helper scores the in-memory array it is given. The default digest is 64 zeros and is never checked against bytes on disk. The headline acceptance case writes a WAV, then still calls this with the pre-write buffer, so a corrupt or empty file would still pass.
Add a file-path entry that checksums, decodes (sf.read or librosa.load), then scores. Keep this PCM helper for the silence unit case only.
| assert len(first) == 64 | ||
|
|
||
|
|
||
| def test_c_major_wav_recovers_c_after_file_decode(tmp_path: Path) -> None: |
There was a problem hiding this comment.
The test name and docstring claim a decoded WAV. After write_pcm_wav, scoring still uses the pre-write audio array. assert_fixture_checksum here only proves the write helper hashed its own output.
Point this case at a file-path evaluator so the engine has to hear the bytes on disk. Drive test_checksum_mismatch_fails_closed through that evaluator and evaluate_click_tempo_file so a tampered file cannot emit passed: true.
There was a problem hiding this comment.
Review — Tier 1 real-audio accuracy (ccbdae2d)
Tempo Acc1 is real buyer evidence. The C-major headline case on this head is not: it writes a WAV, then scores the pre-write in-memory triad. Checksum-fail-closed also never enters evaluate_*.
Prefer #892 11e156d7 #892. That successor checksums the file, decodes those bytes, scores the decoded PCM, fails silence-on-disk even when a C major array exists in memory, and fails closed through both file evaluators. The 0.70 floor is documented as a BandScope tolerance.
Buyer next action: Do not merge #890 at ccbdae2d. Land #892 after its required checks. Do not mix #884 or #888 onto either branch.
Merge posture: COMMENT only. Author of #890 is seonghobae. This automation authored #892 and does not approve either PR.
Sent by Cursor Automation: Fix Issues
2 participants
Summary
Closes the first buyer-visible slice of #770. A decoded C major WAV must recover
Cfor most of its duration, and a decoded 120 BPM click WAV must pass tempo Acc1. Reports are checksum-fail-closed and versioned.Exact current head:
ccbdae2d71810adbaabe9197142e59b5445d1ef1.Protected base:
develop@acdbea6344fe1231c39535b575f4de35e4c607c9.This is not a substitute for #881 (NIM contract), #740 (handoff import), or #884 (Stem Lab). Do not mix those slices here.
Held behavior:
ChordRecognizer/TemporalAnalyzeroutput;0.70forC;Con an active lead-vocal role.Keys-left, keys-right, and acoustic-guitar still carry arrangement defaults. That honesty gap stays out of this PR.
Verification
uv run --project services/analysis-engine pytest services/analysis-engine/tests/test_accuracy_acceptance.py --cov=src/bandscope_analysis/accuracy --cov-report=term-missing --cov-fail-under=100(13 passed, 100% on the new package)ruff check/ruff format --check/mypy src/bandscope_analysis/accuracy/verify_security_notes.py./scripts/harness/quickcheck.sh(exact-head CI still required)Security Notes
Attack surface
Generated WAV bytes, SHA-256 digests, decoded PCM, and JSON-shaped accuracy reports.
Trust boundary
Untrusted: on-disk fixture bytes and parsed report mappings. Trusted: in-repo generators, metric definitions, and registered floors. No network and no shell.
Mitigations
Checksum fail-closed before tempo scoring. Manifest parsing rejects missing or mistyped fields. Fixtures are short, synthetic, and license-clean.
Test points
Deterministic C major digest, C recall after file write, 120 BPM Acc1 after file decode, checksum mismatch, malformed report, silence must not pass as C major, pipeline lead-vocal
Cwhen the vocal stem is active.Dependency and Supply Chain
Supersession evidence
Closed as superseded by #892, not as duplicate-by-title. Exact comparison proves preservation before enhancement:
ccbdae2d71810adbaabe9197142e59b5445d1ef1has treee1e2d0f928bc3e5b5d11ed6b8aeb2d709c808ca9.4e5a25271933124d2c65f7daa918ba458c327b90, whose tree is exactly the samee1e2d0f928bc3e5b5d11ed6b8aeb2d709c808ca9; therefore no test(analysis): add Tier 1 real-audio accuracy acceptance #890 product/test/doc tree content is lost.developbefore its follow-up repair; predecessor checks/reviews do not transfer.#892 is therefore the single surviving owner for this Tier-1 acceptance slice. Keep #892 Draft and require its own exact-head CI/security/coverage/review/approval gates before integration.