fix(ci): add repo flag to gh release upload#161
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough개요이 PR은 버전 0.1.1로 업그레이드하면서 CI 워크플로우 권한 구성을 재구성하고, 데스크톱 앱에 분석 결과 표시 기능을 추가하며, 음성 분석 엔진(화음, 음역대, 음성 분리, 템포 분석) 모듈을 구현하고, JSDoc 문서화를 확대하며, YouTube URL 검증을 강화하고, 광범위한 테스트 커버리지를 추가합니다. 변경사항
시퀀스 다이어그램sequenceDiagram
participant Desktop as 데스크톱 App<br/>(App.tsx)
participant Analysis as Analysis Service<br/>(Python CLI)
participant Temporal as TemporalAnalyzer<br/>(BPM/비트)
participant Chord as ChordRecognizer<br/>(화음 감지)
participant Pitch as PitchTracker<br/>(음역대)
participant Roles as RoleExtractor<br/>(역할 합성)
participant Ranges as RangeAnalyzer<br/>(음역대 겹침)
participant SharedTypes as SharedTypes<br/>(RehearsalSong)
Desktop->>Analysis: 로컬 오디오 분석 요청
Analysis->>Temporal: 오디오 파일 분석
Temporal-->>Analysis: TemporalFeatures {BPM, beats}
par 화음 & 음역대 분석
Analysis->>Chord: 오디오 신호 분석
Chord-->>Analysis: ChordAnalysisResult {sections, chords}
Analysis->>Pitch: 오디오 신호 분석
Pitch-->>Analysis: TrackedPitchRange {lowest, highest}
end
Analysis->>Roles: 추출된 화음/음역대 + 섹션
Roles-->>Analysis: RoleExtractionResult {roles, ranges, chords}
Analysis->>Ranges: 역할 음역대 데이터
Ranges-->>Analysis: RangeAnalysisResult {overlaps, warnings}
Analysis-->>Desktop: 완전 분석 결과
Desktop->>SharedTypes: RehearsalSong {overlapWarnings, partGraph}
Desktop->>Desktop: Feature 컴포넌트 렌더링<br/>(Ranges, Chords, Home)
예상 코드 리뷰 노력🎯 4 (Complex) | ⏱️ ~45분 관련 가능성 있는 PR
제안 라벨
시
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
scripts/release/package_desktop_artifact.py (1)
123-128:⚠️ Potential issue | 🟠 MajorMajor: dist 내부 심볼릭 링크를 그대로 따라 파일을 zip에 포함할 수 있습니다.
frontend_dist.rglob("*")후path.is_file()체크만으로 zip에 넣고 있어, 만약frontend_dist아래에 심볼릭 링크가 존재할 경우(예: dist 내에 링크로 dist 밖 파일을 가리키는 케이스) 해당 링크 타겟이 “파일”로 판정되어 dist 외부 파일이 release artifact에 포함될 수 있습니다. CI 산출물 오염/공급망 공격 시 위험합니다.아래처럼 심링크를 명시적으로 건너뛰는 방어를 권장합니다.
✅ 제안 수정(diff)
for path in frontend_dist.rglob("*"): + if path.is_symlink(): + continue if path.is_file(): archive.write( path, arcname=str(Path("frontend") / path.relative_to(frontend_dist)), )가능하다면 메타데이터 경로들(
metadata_paths)에도 동일한 심링크 배제(또는resolve()기반 경로 검증)를 적용해 일관성을 맞추는 것도 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/package_desktop_artifact.py` around lines 123 - 128, The loop using frontend_dist.rglob("*") writes any file-like target (path.is_file()) into the archive and may follow symlinks; update the logic to explicitly skip symlinks (check path.is_symlink()) and/or validate that path.resolve() is inside frontend_dist.resolve() before calling archive.write (same treatment for metadata_paths), so external targets are never included; reference the loop variables frontend_dist, path, path.is_file(), archive.write and metadata_paths when making the change.docs/security/dependency-policy.md (1)
93-106:⚠️ Potential issue | 🟠 Major예외 관리 정책 미준수: audit.toml의 15개 RUSTSEC ID가 policy.md에 문서화되지 않음.
라인 95-100에 명시된 정책("every exception must reference the exact advisory ID")을 적용하면,
apps/desktop/src-tauri/.cargo/audit.toml에서 무시하도록 설정된 18개의 RUSTSEC ID 중 15개(RUSTSEC-2024-0370,RUSTSEC-2024-0411,RUSTSEC-2024-0412,RUSTSEC-2024-0414,RUSTSEC-2024-0415,RUSTSEC-2024-0416,RUSTSEC-2024-0417,RUSTSEC-2024-0418,RUSTSEC-2024-0419,RUSTSEC-2024-0420,RUSTSEC-2025-0075,RUSTSEC-2025-0080,RUSTSEC-2025-0081,RUSTSEC-2025-0098,RUSTSEC-2025-0100)이 policy.md의 "Current controlled exception" 섹션에서 누락되어 있습니다.라인 105의 bullet이
(e.g. ...)로 일부 예시만 나열함으로써, 실제 무시되는 모든 예외를 투명하게 문서화하지 않고 있습니다. 이는 다음 요건들을 위반합니다:
- exact advisory ID 전수 명시
- 각 예외별 reason, scope, exposure, compensating controls 구체화
line 105의 예외를 audit.toml의 모든 무시 ID와 각각의 정당성(reason)을 포함하여 완전히 문서화해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/security/dependency-policy.md` around lines 93 - 106, Update the "Vulnerability exception handling" section to enumerate and justify every RUSTSEC advisory that the repo ignores: compare the ignore list in audit.toml and add the 15 missing IDs (RUSTSEC-2024-0370, RUSTSEC-2024-0411, RUSTSEC-2024-0412, RUSTSEC-2024-0414, RUSTSEC-2024-0415, RUSTSEC-2024-0416, RUSTSEC-2024-0417, RUSTSEC-2024-0418, RUSTSEC-2024-0419, RUSTSEC-2024-0420, RUSTSEC-2025-0075, RUSTSEC-2025-0080, RUSTSEC-2025-0081, RUSTSEC-2025-0098, RUSTSEC-2025-0100) into the "Current controlled exception" bullets and for each provide the exact advisory ID, a concise reason, documented scope, exposure assessment, and any compensating controls so the policy satisfies the "every exception must reference the exact advisory ID and reason" requirement and matches the audit.toml ignore entries.services/analysis-engine/tests/test_separation.py (1)
17-126: 🧹 Nitpick | 🔵 Trivial분류 경계/정규화 회귀를 막는 테스트를 추가해 주세요.
현재 케이스로는 부분 문자열 오탐과
roleType대소문자 변형 회귀를 잡기 어렵습니다. 해당 두 케이스를 명시적으로 추가하는 것을 권장합니다.추가 테스트 예시
+def test_categorize_role_avoids_substring_false_positive() -> None: + """부분 문자열 오탐을 방지한다.""" + assert _categorize_role("ambassador-1", "Ambassador", "instrument") == StemCategory.OTHER + + +def test_stem_separator_role_type_case_insensitive() -> None: + """roleType 대소문자 변형에서도 일관된 confidence를 유지한다.""" + separator = StemSeparator() + result = separator.separate( + [{"id": "lead-vocal", "name": "Lead Vocal", "roleType": "Vocal"}] + ) + assert result["stems"][0]["category"] == "vocals" + assert result["stems"][0]["confidence"] == "high"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/analysis-engine/tests/test_separation.py` around lines 17 - 126, Add two regression tests targeting _categorize_role and StemSeparator.separate: (1) a substring false-positive test that ensures _categorize_role("bassoon-1", "Bassoon", "instrument") (and the separator with role id "bassoon-1") does not classify as BASS (verifies exact/word-boundary matching), and (2) a roleType case-variation test that passes roleType values like "Instrument", "INSTRUMENT", or "vOcAl" to _categorize_role and to StemSeparator.separate to ensure classification and confidence remain correct regardless of case; place these new asserts alongside existing tests to prevent regressions in _categorize_role and StemSeparator.separate..github/workflows/sbom.yml (1)
82-86:⚠️ Potential issue | 🟠 Major
gh release upload명령에--repo플래그를 추가하세요.
.github/workflows/sbom.yml:86의gh release upload호출이--repo플래그를 누락했습니다. 다른 워크플로(build-baseline.yml)의 동일 명령들은 올바르게--repo "${{ github.repository }}"를 사용하고 있습니다. 이를 추가해 저장소 해석을 명시적으로 고정하고 CI 오류를 방지하세요.제안 수정안
run: gh release upload "$RELEASE_TAG" bandscope-sbom.cdx.json supply-chain/supplemental-component-inventory.json --clobber --repo "${{ github.repository }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/sbom.yml around lines 82 - 86, Update the `gh release upload` step so it explicitly sets the target repository using the --repo flag; modify the command that currently runs `gh release upload "$RELEASE_TAG" bandscope-sbom.cdx.json supply-chain/supplemental-component-inventory.json --clobber` to include --repo "${{ github.repository }}" (keeping GH_TOKEN and RELEASE_TAG env usage intact) so repository resolution is fixed like the other workflow's `gh release upload` invocations.apps/desktop/src-tauri/src/main.rs (2)
790-826:⚠️ Potential issue | 🟠 Major외부 프로세스가 돌려준
filepath를 그대로 프로젝트 소스로 저장하면 안 됩니다.이 값은 Python 분석 엔진이 반환한 비신뢰 메타데이터인데, 지금은 캐시 루트 밖 경로·심볼릭 링크·허용되지 않은 확장자·디렉터리 경로도 그대로
source_path/file_name으로 저장됩니다. 여기서는 수동 파싱 대신normalize_local_audio_source()로 정규화하고, canonical path 가cache_root하위인지까지 확인한 뒤 저장하세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/main.rs` around lines 790 - 826, The code currently uses raw metadata "filepath" and builds LocalAudioSourcePayload (source_path, file_name, extension, file_size_bytes) directly; instead call and use normalize_local_audio_source(filepath) to produce a sanitized payload (or sanitized path/filename) before constructing LocalAudioSourcePayload, ensure you resolve the canonical path (std::fs::canonicalize) and verify it is under cache_root (compare prefixes) to reject paths outside the cache, reject or normalize symlinks and directory paths, and validate the extension against allowed set before assigning file_name/extension/source_path.
768-783:⚠️ Potential issue | 🟠 Major타임아웃이 YouTube import 프로세스를 실제로 중단하지 못합니다.
timeout(... spawn_blocking(|| Command::new(...).output()))패턴은 타임아웃 발생 시 await를 취소하지만, 블로킹 태스크는 스레드 풀에서 계속 실행되므로 자식 프로세스 핸들을 확보할 수 없습니다. 따라서 호출자는 실패 메시지를 받지만 실제 다운로드/변환 프로세스는 백그라운드에서 계속 실행될 수 있습니다.
spawn()으로Child핸들을 획득한 후 타임아웃 시kill()/wait()를 호출하는 방식으로 변경하십시오. (분석 작업의 구현 패턴 참고)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/main.rs` around lines 768 - 783, The current pattern uses tokio::time::timeout with tauri::async_runtime::spawn_blocking and Command::new(...).output() which cancels the await but leaves the blocking task and its child process running; change to spawn the child process directly (use tauri::async_runtime::spawn or tokio::spawn) so you can obtain a Child handle from Command::new(...).spawn(), store/await that Child with YOUTUBE_IMPORT_TIMEOUT, and on timeout call Child::kill() and then Child::wait() (or wait_with_output after kill) to ensure the subprocess is terminated; replace the spawn_blocking + .output() usage with logic that spawns the Child, monitors the timeout, and performs kill()/wait() on timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/.cargo/audit.toml`:
- Around line 1-21: Update the dependency policy docs to fully document every
advisory ignored in apps/desktop/src-tauri/.cargo/audit.toml: remove the "e.g."
shorthand in docs/security/dependency-policy.md and replace it with an explicit
list of all 18 RUSTSEC IDs found in audit.toml (e.g., RUSTSEC-2024-0413,
RUSTSEC-2024-0416, RUSTSEC-2025-0057, RUSTSEC-2024-0412, RUSTSEC-2024-0418,
RUSTSEC-2024-0411, RUSTSEC-2024-0417, RUSTSEC-2024-0414, RUSTSEC-2024-0415,
RUSTSEC-2024-0420, RUSTSEC-2024-0419, RUSTSEC-2024-0370, RUSTSEC-2025-0081,
RUSTSEC-2025-0075, RUSTSEC-2025-0080, RUSTSEC-2025-0100, RUSTSEC-2025-0098,
RUSTSEC-2024-0429) and for each advisory add the required scope, exposure, and
compensating-controls entries (e.g., scope: which component or dependency chain
like Tauri v2/wry/webkit2gtk, exposure: why app is/not vulnerable, compensating
controls: mitigations or monitoring). Finally, add a short comment at the top of
apps/desktop/src-tauri/.cargo/audit.toml referencing
docs/security/dependency-policy.md so future reviewers can find the
justification.
In `@apps/desktop/src/App.tsx`:
- Line 24: Remove the meaningless placeholder JSDoc "/** Documented. */" used
across the App component and other top-level symbols in this file and either
delete those empty comments or replace each with a concise, useful description
that documents the component/function purpose, inputs/props,
outputs/side-effects, and any important invariants; ensure you update all
occurrences of the exact comment string so every exported component/function in
this file has either a real JSDoc block or no JSDoc at all.
In `@apps/desktop/src/features/ranges/index.tsx`:
- Around line 9-12: The muted paragraph uses color "#999" which has low
contrast; update the <p style={{ color: "#999" }}> in this component to a
higher-contrast value (for example replace "#999" with "#666" or
"rgba(0,0,0,0.6)"), or better yet move the style into a class/variable (e.g.,
theme.colors.textMuted) so the muted text is consistently accessible across the
app; change the inline style on the <p> element accordingly.
In `@apps/desktop/src/features/workspace/RoleSwitcher.tsx`:
- Line 9: 상단의 간단한 JSDoc `/** Documented. */`을 컴포넌트 목적과 입력(props) 및 동작을 설명하는 실사용
설명으로 바꿔주세요: RoleSwitcher 컴포넌트가 어떤 역할(예: 사용자 역할 전환 UI)을 수행하는지, 수신하는 주요 props(예:
currentRole, onChange 등)와 그 타입/기대값, 그리고 버튼 클릭이나 드롭다운 선택 시 어떤 부수효과(예: 로컬 상태 변경,
콜백 호출, API 요청 등)가 발생하는지 한두 문장으로 명확히 적어 유지보수성과 검색성을 높이십시오.
In `@apps/desktop/src/features/workspace/SectionRoadmap.tsx`:
- Around line 135-143: The code accesses role.overlapWarnings.length and maps
over role.overlapWarnings without guarding for undefined; update the render to
first verify that role.overlapWarnings is an array (e.g.,
Array.isArray(role.overlapWarnings) && role.overlapWarnings.length > 0) before
rendering the container and calling role.overlapWarnings.map so you avoid
"Cannot read properties of undefined" errors; apply this check around the block
that reads role.overlapWarnings and the map invocation in SectionRoadmap.tsx.
In `@apps/desktop/src/i18n/index.ts`:
- Around line 4-7: Replace the placeholder JSDoc comments for the exported types
with meaningful descriptions (or remove them) so they convey the contract and
intent: update the JSDoc for the Locale type to explain allowed locale values
and intended usage and for TranslationKey to explain it maps to keys of the
enCommon translations (referencing the TranslationKey type and the enCommon
object), and apply the same change to the other placeholder comments in the same
file (lines covering the related types/exports between 14-22) to provide useful
documentation rather than the generic "Documented." note.
In `@apps/desktop/src/lib/export.ts`:
- Around line 17-18: The CSV formula-injection check only looks for a formula
character at the very start and misses values with leading whitespace/tabs;
update the condition that currently uses /^[=+\-@]/.test(value) to detect
formulas even after leading whitespace (e.g., use a regex that allows leading
whitespace like /^\s*[=+\-@]/ or call value.trimStart() and then check
startsWith for '=', '+', '-', '@'), and keep the escaping behavior that sets
escapedValue = `'${value}` so the original spacing is preserved while
neutralizing the formula; adjust the check around the variables value and
escapedValue in export.ts accordingly.
In `@CHANGELOG.md`:
- Around line 1-2: Change the top-level header to an H1 (replace "##
[Unreleased]" with a H1 like "# [Unreleased]" or "# Changelog") and ensure there
is a blank line before and after each subsection header such as "### Added" and
"### Fixed" so linting rules are satisfied; search for the headers "##
[Unreleased]" and any "### Added"/"### Fixed" blocks and insert or adjust
newlines accordingly.
In `@docs/plans/2026-03-28-ml-engine-integration.md`:
- Around line 3-55: The markdown has MD022 warnings because several headings
(e.g., "## Overview", "### Track 1: Temporal Foundation (`#105`)", "### Track 2:
Spectral & Stem Separation (`#106`)", "### Track 3: Harmonic & Pitch Pipelines
(`#107`) (COMPLETED)", "### Track 4: Structural Graph Assembly (`#108`)", "### Track
5: Orchestration & UX (`#109`)", and "### Attack Surface"/"### Trust
Boundary"/"### Mitigations") are missing consistent blank lines before and/or
after them; fix by ensuring there is a single blank line immediately before and
after every level-2 and level-3 heading throughout the document so the MD022
linter no longer flags the file.
In `@docs/plans/2026-04-28-pr-159-rollout.md`:
- Line 1: Remove the local restore-point comment that exposes a user-specific
path from the document by deleting the HTML comment line containing
"~/.gstack/projects/bandscope/feature-issue-107-harmonic-pitch-autoplan-restore-20260428074924.md"
in docs/plans/2026-04-28-pr-159-rollout.md; ensure no other user-specific
filesystem paths remain and replace with a generic note if you need to reference
a restore point (e.g., "local restore point removed for privacy").
- Around line 154-155: 문서의 "GitHub Releases serves as the trusted source of
artifacts" 문구는 과도하게 낙관적이므로 배포 채널(예: GitHub Releases)을 잠정적 공급자( deployment
channel )로 명시하고 "검증 후 신뢰(trust after verification)"로 분리하도록 수정하세요; 구체적으로 문서에서
GitHub Releases를 직접 신뢰한다고 표현한 문장(또는 항목)을 "배포 채널"로 재분류하고, 별도의 "Security Notes"
섹션을 추가하여 untrusted inputs(파일, URL, 서브프로세스, IPC, WebView, 업데이트, 모델 다운로드 등), 명확한
trust boundaries, 허용목록(allowlists) 및 검증/정책(validation) 절차, 안전한 실패(safe failure)
동작, 로깅/개인정보 영향(logging/privacy impact), 그리고 테스트 포인트(test points)를 항목별로 기술하도록
변경하세요.
In `@packages/shared-types/src/index.ts`:
- Around line 840-847: The parser now rejects older JSON because overlapWarnings
and partGraph were made required; update the parsing logic in the functions that
use isDenseArray/invalidField (references: overlapWarnings, partGraph,
isDenseArray, invalidField, and the caller parseRehearsalSong) to treat missing
fields as empty defaults rather than erroring: if value.overlapWarnings or
value.partGraph is undefined/null, set them to [] (or an appropriate empty
structure) before the existing type checks, or run a small migration step in
parseRehearsalSong that ensures these fields exist with default values prior to
validation; keep the existing element-type checks for non-empty arrays.
- Around line 853-884: validatePartGraphNode currently only checks types but not
that role_id, handoff_to, and handoff_from reference actual roles; update
validation so partGraph references are checked against the section's roles set:
in validateRehearsalSection build a Set of roles[].id and either pass that Set
into validatePartGraphNode or add a new helper (e.g.,
validatePartGraphNodeWithRoles) that accepts the Set and ensures value.role_id
is in the Set and every string in value.handoff_to and value.handoff_from is in
the Set (return invalidField(...) for any missing reference); apply the same
change pattern to the similar checks around the 921-929 area.
In `@SECURITY.md`:
- Line 11: 문구 "generally providing a 90 days expectation to fix before public
disclosure"를 정책이 아닌 권장사항임을 명확히 하도록 변경하세요: 예컨대 SECURITY.md의 해당 문장을 찾아 "대부분의 경우 약
90일(권장)을 목표로 협의하며, 긴급 상황이나 재현 난이도 등 예외가 있을 수 있습니다"처럼 권장임과 예외 가능성을 한 문장으로 덧붙이거나
교체해 제보자가 혼동하지 않도록 만드세요.
- Around line 9-10: Replace the hard-coded personal email string
`seonghobae@example.com` with a dedicated security alias (e.g.,
`security@your-domain.com`) and update the adjacent guidance to prioritize
GitHub private vulnerability reporting/ Security Advisory drafts when available;
specifically, edit the lines mentioning the personal email and the Private
Vulnerability Report link so the contact option is the centralized alias and the
GitHub secure reporting path is emphasized as the primary channel.
In `@services/analysis-engine/src/bandscope_analysis/chords/analyzer.py`:
- Around line 76-78: Normalize and validate the "source" field before storing it
in the ChordLabel dict: ensure the value comes only as "model" or "user"
(default to "model" for any other/unknown input) when building the dict from the
harmony object (the place constructing {"functionLabel": ..., "source": ...});
also update the user-source detection logic later (the code around lines
handling user source detection) to use this normalized value instead of the raw
harmony.get("source"), so downstream code and the ChordLabel contract only ever
see "model" or "user".
- Around line 67-69: The loop over section_roles assumes each role is a dict and
calls role.get which throws on non-dict entries; before accessing
role.get("harmony") add a type guard to skip non-dict roles (e.g., check
isinstance(role, dict) or similar) so only dicts are processed, then proceed
with the existing isinstance(harmony, dict) and "chord" check; update the loop
handling around the variables section_roles and role in analyzer.py to safely
skip invalid entries.
In `@services/analysis-engine/src/bandscope_analysis/cli.py`:
- Around line 79-85: The current code calls
request["localSource"].get("sourcePath") without ensuring localSource is a dict,
causing AttributeError for malformed payloads; update the check around request
to verify localSource is a dict (e.g., change the condition to also assert
isinstance(request.get("localSource"), dict)) before accessing .get, or move
this temporal analyzer logic to run after the overall request validation step;
adjust the symbols request, "localSource", and audio_path accordingly so
audio_path is only set when localSource is confirmed to be a dict.
In `@services/analysis-engine/src/bandscope_analysis/ranges/analyzer.py`:
- Around line 201-203: The loop over section_roles in analyzer.py uses
role.get("range") without confirming role is a dict, which can raise
AttributeError for malformed input; update the loop in the function/method
containing "for role in section_roles" to guard each item (e.g., if not
isinstance(role, dict): continue) before calling role.get("range"), then only
process role_range when it is a dict (retain the existing isinstance(role_range,
dict) check) so non-dict entries are safely skipped.
- Around line 66-67: The _parse_note function currently attempts to parse
octave_str with int(octave_str) when octave_str is non-empty and starts with a
digit or "-", which will raise ValueError for invalid strings like "-" and abort
analysis; update _parse_note to validate octave_str more robustly (e.g., ensure
octave_str is a full optional-sign integer using str.lstrip('+ -').isdigit() or
a try/except around int(octave_str)) and on failure return a safe default (e.g.,
(name, default_octave) or (name, None)) so malformed octave_str does not
propagate exceptions; ensure you reference the existing variables/return shape
(name, int(octave_str)) and keep behavior consistent for valid inputs.
In `@services/analysis-engine/src/bandscope_analysis/separation/model.py`:
- Around line 24-26: StemDescriptor.category is currently typed as a plain str
which allows invalid values; tighten the contract by replacing its annotation
with the stricter union type (e.g. StemCategory or a Literal[...] listing
allowed values) and update any imports/definitions so StemCategory is defined
and used throughout the module (ensure any factory/constructor, type checks and
consumers of StemDescriptor.category accept the new type). Locate the
StemDescriptor definition and change the category: str to category: StemCategory
(or category: Literal["...", "..."]) and fix any downstream type errors or
missing imports accordingly.
In `@services/analysis-engine/src/bandscope_analysis/separation/separator.py`:
- Around line 35-36: The code compares role_type directly (e.g., the if
role_type == "vocal" branch returning StemCategory.VOCALS) without normalizing
inputs; normalize role_type once at the start of the function (e.g.,
role_type_norm = role_type.strip().lower()) and use that normalized variable in
all classification and confidence calculations (replace direct uses of role_type
in the separator logic and the later block around the other classification at
93-99) so comparisons are case- and whitespace-insensitive and consistent across
StemCategory assignment and confidence computation.
- Around line 38-41: The current substring check in separator.py (building
search_text from role_id and role_name and iterating _ROLE_TO_STEM) allows
partial matches; change the condition around the loop that uses "keyword in
search_text" to a token/boundary-aware match (e.g., use regex word-boundary
checks with re.escape(keyword) or explicit tokenization of search_text) so only
whole-keyword matches map to categories; update the matching logic where
search_text, role_id, role_name, and _ROLE_TO_STEM are used to return the
correct category.
In `@services/analysis-engine/tests/test_chord_recognizer.py`:
- Around line 17-26: The test test_chord_recognizer_unvoiced_audio contains a
debugging print that should be removed; delete the print("RESULT:", result)
statement in the test (inside function test_chord_recognizer_unvoiced_audio
where recognizer = ChordRecognizer() and result = recognizer.recognize(...)) so
the test relies on the existing assertion only and avoids noisy CI logs.
In `@services/analysis-engine/tests/test_cli.py`:
- Around line 308-343: The test test_cli_main_temporal_analyzer_mock doesn't
actually verify the TemporalAnalyzer path executed; update it to assert the
analyzer was invoked or that a warning was emitted after run_analysis_job is
called: replace the hand-rolled FakeAnalyzer with a mock (or add a flag on
FakeAnalyzer) that records calls to analyze and then assert that
cli.TemporalAnalyzer().analyze was called (or assert stdout/stderr contains the
expected warning message emitted when analyze fails); ensure you reference
cli.TemporalAnalyzer, its analyze method, and cli.main/run_analysis_job so the
test fails if the temporal-analyzer branch is not executed.
In `@services/analysis-engine/tests/test_priority.py`:
- Around line 3-61: Replace the repeated cast(Any, role) in tests with a small
typed test helper or a test-only TypedDict to enforce required fields and catch
typos: define a RoleTyped (or make_role) used by
test_calculate_rehearsal_priority, test_calculate_priority_with_overlap,
test_calculate_priority_medium_confidence,
test_calculate_priority_with_setup_note and test_calculate_priority_low so each
role passed into calculate_rehearsal_priority has explicit typed keys
(confidence, overlapWarnings, manualOverrides, setupNote); update assertions to
pass the typed object (no cast) to calculate_rehearsal_priority and keep using
RehearsalPriority for expected values.
---
Outside diff comments:
In @.github/workflows/sbom.yml:
- Around line 82-86: Update the `gh release upload` step so it explicitly sets
the target repository using the --repo flag; modify the command that currently
runs `gh release upload "$RELEASE_TAG" bandscope-sbom.cdx.json
supply-chain/supplemental-component-inventory.json --clobber` to include --repo
"${{ github.repository }}" (keeping GH_TOKEN and RELEASE_TAG env usage intact)
so repository resolution is fixed like the other workflow's `gh release upload`
invocations.
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 790-826: The code currently uses raw metadata "filepath" and
builds LocalAudioSourcePayload (source_path, file_name, extension,
file_size_bytes) directly; instead call and use
normalize_local_audio_source(filepath) to produce a sanitized payload (or
sanitized path/filename) before constructing LocalAudioSourcePayload, ensure you
resolve the canonical path (std::fs::canonicalize) and verify it is under
cache_root (compare prefixes) to reject paths outside the cache, reject or
normalize symlinks and directory paths, and validate the extension against
allowed set before assigning file_name/extension/source_path.
- Around line 768-783: The current pattern uses tokio::time::timeout with
tauri::async_runtime::spawn_blocking and Command::new(...).output() which
cancels the await but leaves the blocking task and its child process running;
change to spawn the child process directly (use tauri::async_runtime::spawn or
tokio::spawn) so you can obtain a Child handle from Command::new(...).spawn(),
store/await that Child with YOUTUBE_IMPORT_TIMEOUT, and on timeout call
Child::kill() and then Child::wait() (or wait_with_output after kill) to ensure
the subprocess is terminated; replace the spawn_blocking + .output() usage with
logic that spawns the Child, monitors the timeout, and performs kill()/wait() on
timeout.
In `@docs/security/dependency-policy.md`:
- Around line 93-106: Update the "Vulnerability exception handling" section to
enumerate and justify every RUSTSEC advisory that the repo ignores: compare the
ignore list in audit.toml and add the 15 missing IDs (RUSTSEC-2024-0370,
RUSTSEC-2024-0411, RUSTSEC-2024-0412, RUSTSEC-2024-0414, RUSTSEC-2024-0415,
RUSTSEC-2024-0416, RUSTSEC-2024-0417, RUSTSEC-2024-0418, RUSTSEC-2024-0419,
RUSTSEC-2024-0420, RUSTSEC-2025-0075, RUSTSEC-2025-0080, RUSTSEC-2025-0081,
RUSTSEC-2025-0098, RUSTSEC-2025-0100) into the "Current controlled exception"
bullets and for each provide the exact advisory ID, a concise reason, documented
scope, exposure assessment, and any compensating controls so the policy
satisfies the "every exception must reference the exact advisory ID and reason"
requirement and matches the audit.toml ignore entries.
In `@scripts/release/package_desktop_artifact.py`:
- Around line 123-128: The loop using frontend_dist.rglob("*") writes any
file-like target (path.is_file()) into the archive and may follow symlinks;
update the logic to explicitly skip symlinks (check path.is_symlink()) and/or
validate that path.resolve() is inside frontend_dist.resolve() before calling
archive.write (same treatment for metadata_paths), so external targets are never
included; reference the loop variables frontend_dist, path, path.is_file(),
archive.write and metadata_paths when making the change.
In `@services/analysis-engine/tests/test_separation.py`:
- Around line 17-126: Add two regression tests targeting _categorize_role and
StemSeparator.separate: (1) a substring false-positive test that ensures
_categorize_role("bassoon-1", "Bassoon", "instrument") (and the separator with
role id "bassoon-1") does not classify as BASS (verifies exact/word-boundary
matching), and (2) a roleType case-variation test that passes roleType values
like "Instrument", "INSTRUMENT", or "vOcAl" to _categorize_role and to
StemSeparator.separate to ensure classification and confidence remain correct
regardless of case; place these new asserts alongside existing tests to prevent
regressions in _categorize_role and StemSeparator.separate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23d86fc1-75cf-49b2-a38c-773a4a5c9810
⛔ Files ignored due to path filters (3)
apps/desktop/src-tauri/Cargo.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.jsonservices/analysis-engine/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (78)
.github/workflows/bandit.yml.github/workflows/build-baseline.yml.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/dependency-review.yml.github/workflows/ossf-scorecard.yml.github/workflows/sbom.yml.github/workflows/security-audit.yml.github/workflows/trivy.yml.gitignoreCHANGELOG.mdREADME.mdSECURITY.mdVERSIONapps/desktop/src-tauri/.cargo/audit.tomlapps/desktop/src-tauri/src/main.rsapps/desktop/src/App.test.tsxapps/desktop/src/App.tsxapps/desktop/src/features/chords/index.tsxapps/desktop/src/features/home/index.tsxapps/desktop/src/features/player/index.tsxapps/desktop/src/features/ranges/index.tsxapps/desktop/src/features/settings/index.tsxapps/desktop/src/features/workspace/ConfidenceBadge.tsxapps/desktop/src/features/workspace/RoleSwitcher.tsxapps/desktop/src/features/workspace/SectionRoadmap.tsxapps/desktop/src/features/workspace/Workspace.tsxapps/desktop/src/features/workspace/WorkspaceStates.tsxapps/desktop/src/i18n/index.tsapps/desktop/src/lib/analysis.tsapps/desktop/src/lib/export.test.tsapps/desktop/src/lib/export.tsdocs/plans/2026-03-28-ml-engine-integration.mddocs/plans/2026-04-28-pr-159-rollout.mddocs/security/dependency-policy.mdeslint.config.jspackage.jsonpackages/shared-types/src/index.tspackages/shared-types/test/index.test.tsscripts/checks/security_gates.pyscripts/checks/verify_docs.pyscripts/checks/verify_github_bootstrap_policy.pyscripts/checks/verify_security_notes.pyscripts/checks/verify_supply_chain.pyscripts/fix-version-format.shscripts/release/package_desktop_artifact.pyservices/analysis-engine/pyproject.tomlservices/analysis-engine/src/bandscope_analysis/chords/__init__.pyservices/analysis-engine/src/bandscope_analysis/chords/analyzer.pyservices/analysis-engine/src/bandscope_analysis/chords/chord_recognizer.pyservices/analysis-engine/src/bandscope_analysis/chords/model.pyservices/analysis-engine/src/bandscope_analysis/cli.pyservices/analysis-engine/src/bandscope_analysis/ranges/__init__.pyservices/analysis-engine/src/bandscope_analysis/ranges/analyzer.pyservices/analysis-engine/src/bandscope_analysis/ranges/model.pyservices/analysis-engine/src/bandscope_analysis/ranges/pitch_tracker.pyservices/analysis-engine/src/bandscope_analysis/roles/extractor.pyservices/analysis-engine/src/bandscope_analysis/separation/__init__.pyservices/analysis-engine/src/bandscope_analysis/separation/model.pyservices/analysis-engine/src/bandscope_analysis/separation/separator.pyservices/analysis-engine/src/bandscope_analysis/temporal/__init__.pyservices/analysis-engine/src/bandscope_analysis/temporal/analyzer.pyservices/analysis-engine/src/bandscope_analysis/temporal/model.pyservices/analysis-engine/src/bandscope_analysis/youtube.pyservices/analysis-engine/tests/test_chord_recognizer.pyservices/analysis-engine/tests/test_chords.pyservices/analysis-engine/tests/test_cli.pyservices/analysis-engine/tests/test_pitch_tracker.pyservices/analysis-engine/tests/test_priority.pyservices/analysis-engine/tests/test_ranges.pyservices/analysis-engine/tests/test_release_packaging.pyservices/analysis-engine/tests/test_roles_ml.pyservices/analysis-engine/tests/test_sections.pyservices/analysis-engine/tests/test_separation.pyservices/analysis-engine/tests/test_supply_chain_policy.pyservices/analysis-engine/tests/test_temporal.pyservices/analysis-engine/tests/test_tuning.pyservices/analysis-engine/tests/test_youtube.py
💤 Files with no reviewable changes (3)
- scripts/checks/verify_security_notes.py
- scripts/checks/verify_docs.py
- scripts/checks/verify_github_bootstrap_policy.py
Owner
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
20 tasks
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.
Fixes the 'not a git repository' error in the attach artifacts jobs during release.