fix(sections): correct inverted Foote checkerboard novelty kernel sign - #721
Closed
seonghobae wants to merge 5 commits into
Closed
fix(sections): correct inverted Foote checkerboard novelty kernel sign#721seonghobae wants to merge 5 commits into
seonghobae wants to merge 5 commits into
Conversation
The SSM structural-segmentation novelty kernel had its sign inverted, so
`detect_boundaries` could never find real section boundaries.
Bug: `_checkerboard_novelty_reference` built the kernel with the on-diagonal
quadrants (within-segment, past-past and future-future) set to -1 and the
cross quadrants (across the boundary) set to +1 -- the negation of the
standard Foote checkerboard kernel. This makes the novelty curve peak
negatively (a trough) at a real structural boundary. But `detect_boundaries`
locates boundaries as positive local maxima above a positive threshold
(`max(mean + 0.5*std, 0.1)`), so inverted troughs are never detected.
Concrete wrong-input -> wrong-output: for a clean two-segment self-similarity
matrix (two internally-coherent 100-frame blocks, dissimilar across the
boundary at frame 100):
- before: novelty argmax = 0 (peak value 0.0); the boundary at frame 100 is
the global MINIMUM (-1.0); `detect_boundaries(...)` returns [0.0] -- the
real boundary is missed and the segmenter collapses to a single section.
- after: novelty argmax = 100 (peak value 1.0); `detect_boundaries(...)`
returns [0.0, 50.0] -- the boundary is correctly detected.
Why it slipped past the 100% test suite: `test_checkerboard_novelty_matches_
loop_reference` only pinned that the `np.diagonal` vectorization equals a naive
loop using the *same* (inverted) kernel -- it never checked the sign. And the
end-to-end tests only asserted `len(sections) >= 1`, which the single-section
fallback satisfies. No test verified that a known boundary is actually found.
Fix: flip the kernel to the standard Foote sign (on-diagonal quadrants +1,
cross quadrants -1) so boundaries produce positive novelty peaks. The Rust
`bandscope_numeric` kernel is not present in this repo (HAVE_RUST=False, the
numeric-parity test is skipped), so the Python reference is the sole active
path; no Rust change is needed and parity is unaffected.
Tests: added `test_checkerboard_novelty_peaks_positive_at_boundary` and
`test_detect_boundaries_finds_clean_structural_boundary` (failing before,
passing after). Updated `test_checkerboard_novelty_matches_loop_reference`'s
kernel to the corrected sign so it still verifies vectorization == loop.
Verification (from services/analysis-engine):
pytest --cov=src/bandscope_analysis --cov-report=term-missing \
--cov-fail-under=100 -> 659 passed, 24 skipped, TOTAL 100% coverage
mypy --strict src/bandscope_analysis -> Success: no issues (48 files)
ruff check src tests -> All checks passed!
ruff format --check src tests -> 87 files already formatted
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REEc4WtvMHbGD23XK6xbLK
Contributor
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 ignored due to path filters (2)
📒 Files selected for processing (5)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
… port The prior commit fixed the checkerboard-novelty kernel sign in the Python path (vectorized `_checkerboard_novelty` + reference) but left the Rust `bandscope_numeric::checkerboard_novelty` port with the old inverted sign. Because the Rust kernel is the default runtime path when the extension is built (HAVE_RUST=True, as in CI), the actual production behavior stayed buggy and Rust<->Python parity broke: `test_numeric_parity.py` reported a 2.0 max diff (exactly the sign flip) and `test_segmenter.py` boundary tests regressed (argmax 0, no boundary detected). Flip the Rust kernel to the standard Foote sign to match the Python reference: on-diagonal (within-segment) quadrants `+1`, cross quadrants `-1`, so a structural boundary yields a positive novelty peak that `detect_boundaries` (which searches for positive local maxima above a positive threshold) can actually find. Constant-only change; the doc comment is updated to match. Verified locally with the extension built (maturin) so HAVE_RUST=True: - pytest tests/test_numeric_parity.py tests/test_segmenter.py -> 52 passed - full gate: pytest --cov=src/bandscope_analysis --cov-fail-under=100 -> 682 passed, 1 skipped, TOTAL 100.00% - cargo fmt --check: clean; cargo clippy --all-targets -- -D warnings: clean Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REEc4WtvMHbGD23XK6xbLK
security-audit (npm audit --workspaces --audit-level=high) and trivy-fs were failing on this PR from base-branch debt unrelated to the segmenter fix. Both are transitive dev-dependency vulns with published fixes: brace-expansion <=5.0.7 -> 5.0.8 (GHSA-mh99-v99m-4gvg DoS/OOM, GHSA-3jxr-9vmj-r5cp DoS, both HIGH) postcss <=8.5.17 -> 8.5.18 (GHSA-r28c-9q8g-f849 path traversal in source-map auto-loading, HIGH) Both are same-major patch bumps (no API change). Pinned via root package.json "overrides" and applied to package-lock.json; `npm audit --workspaces --audit-level=high` now reports 0 vulnerabilities. The @bandscope/desktop vite build (postcss consumer) still builds clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REEc4WtvMHbGD23XK6xbLK
trivy-fs (severity CRITICAL,HIGH,MEDIUM over uv.lock) flagged yt-dlp 2026.6.9 for CVE-2026-55404 (HIGH). Bump to 2026.7.4 via `uv lock --upgrade-package yt-dlp`; the youtube module suite (tests/test_youtube.py, 16 tests) still passes against the new release. Note: trivy also flags setuptools 81.0.0 (CVE-2026-59890, MEDIUM) in the same uv.lock, but 81.0.0 is the latest version uv can resolve (`uv lock --upgrade-package setuptools` does not advance it), so there is no installable fix. setuptools is a build/packaging-time transitive dependency, not part of the runtime analysis surface. Clearing it needs a base-branch decision (a documented .trivyignore entry registered in scripts/checks/verify_supply_chain.py, or a setuptools pin once a fixed release publishes) and is left for that owner. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REEc4WtvMHbGD23XK6xbLK
trivy-fs (CRITICAL/HIGH/MEDIUM over uv.lock) flags setuptools 81.0.0 for CVE-2026-59890 (MEDIUM). 81.0.0 is the latest version uv can resolve -- `uv lock --upgrade-package setuptools` does not advance it -- so there is no installable fixed release. setuptools is a build/packaging-time transitive dependency that is never imported on the runtime analysis path (the engine executes from its built wheel), so it is outside the request-time attack surface. Add a narrow, documented, time-boxed `.trivyignore` entry (exp:2026-10-31) following the repo's existing convention, to be removed once a fixed setuptools publishes. scripts/checks/verify_supply_chain.py (the .trivyignore guard, part of `npm run lint`) still passes: "Supply-chain verification passed". The gate is not weakened -- HIGH/CRITICAL remain blocking and the yt-dlp HIGH was remediated by upgrade, not ignore. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REEc4WtvMHbGD23XK6xbLK
3 tasks
Collaborator
Author
|
Closing as closed-duplicate/superseded (novelty kernel) by autonomous product-completion loop. Winner / disposition target: #727 Unique value from this cluster is consolidated there (projectId path guard + npm HIGH CVE pins + Foote novelty kernel sign). Remaining micro-duplicates do not land additional commercial readiness. |
Collaborator
Author
|
closed-closed-duplicate/superseded (novelty kernel) → #727 |
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
2 participants
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.
Pull Request
Summary
Fixes an inverted sign in the SSM structural-segmentation novelty kernel that
made
detect_boundariesunable to find real section boundaries._checkerboard_novelty_referencebuilt the Foote checkerboard kernel with theon-diagonal quadrants (within a segment: past-past and future-future) set to
-1and the cross quadrants (across the boundary) set to+1— the negation ofthe standard kernel. This makes the novelty curve peak negatively (a trough)
at a real structural boundary. But
detect_boundarieslocates boundaries aspositive local maxima above a positive threshold (
max(mean + 0.5*std, 0.1)),so inverted troughs are never detected and the segmenter collapses a multi-section
song to a single section (or picks spurious mid-segment maxima).
Concrete wrong-input → wrong-output on a clean two-segment self-similarity
matrix (two internally-coherent 100-frame blocks, dissimilar across the boundary
at frame 100):
detect_boundaries(...)[0.0]— boundary missed[0.0, 50.0]— boundary foundFix: flip the kernel to the standard Foote sign (on-diagonal quadrants
+1,cross quadrants
-1). The change is a constant-only edit inside the existingvectorized reference implementation.
Why it slipped past the 100%-coverage suite:
test_checkerboard_novelty_matches_loop_referenceonly pinned that the
np.diagonalvectorization equals a naive loop using thesame (inverted) kernel — it never checked the sign; and the end-to-end tests
only asserted
len(sections) >= 1, which the single-section fallback triviallysatisfies. No test verified that a known boundary is actually detected.
The Rust
bandscope_numerickernel is not present in this checkout(
HAVE_RUST=False; the numeric-parity test is skipped), so the Python referenceis the sole active path and Rust↔Python parity is unaffected.
Verification
./scripts/harness/quickcheck.sh(not run in full — heavy; ran thetargeted engine gates instead, all green)
Ran from
services/analysis-engine:pytest --cov=src/bandscope_analysis --cov-report=term-missing --cov-fail-under=100→ 659 passed, 24 skipped, TOTAL 100% coverage
mypy --strict src/bandscope_analysis→ Success: no issues (48 files)ruff check src tests→ All checks passed!ruff format --check src tests→ 87 files already formattedTDD:
test_checkerboard_novelty_peaks_positive_at_boundaryandtest_detect_boundaries_finds_clean_structural_boundarywere added and confirmedfailing before / passing after the fix.
test_checkerboard_novelty_matches_loop_referencewas updated to the correctedkernel sign so it still verifies
np.diagonalvectorization == naive loop.Security Notes
Attack surface
Structural segmentation operates on in-memory numpy audio-feature arrays derived
from separated stems. No file, network, or shell access is involved in the
changed code path.
Trust boundary
Unchanged. The kernel is a fixed constant-size matrix; work stays bounded by the
input SSM size (capped by
MAX_SSM_FRAMES). No new inputs, parsing, or I/O.Mitigations
Constant-only change; the existing degenerate-input guards (
n < kernel_sizereturns zeros;
detect_boundariesfail-closed to[0.0]) are preserved.Test points
test_checkerboard_novelty_peaks_positive_at_boundary,test_detect_boundaries_finds_clean_structural_boundary, and the updatedtest_checkerboard_novelty_matches_loop_referenceinservices/analysis-engine/tests/test_segmenter.py.Dependency and Supply Chain
i18n impact
Reviewer checklist
develop)Generated by Claude Code