feat: add local analysis orchestration#51
Closed
seonghobae wants to merge 6 commits into
Closed
Conversation
📝 WalkthroughSummary by CodeRabbit릴리스 노트
Walkthrough로컬 분석 작업 오케스트레이션 시스템을 구현합니다. React 프론트엔드가 Tauri IPC를 통해 Rust 백엔드와 통신하고, Rust는 stdin/stdout으로 Python 분석 엔진을 호출하며, 메모리 내 작업 저장소에서 상태를 추적하고 타입화된 요청/응답 검증을 수행합니다. Changes
Sequence DiagramsequenceDiagram
actor User
participant React as React UI
participant TauriRT as Tauri Desktop
participant JobStore as Job Store
participant PythonCLI as Python CLI
User->>React: 분석 시작 클릭
React->>TauriRT: start_analysis_job (IPC)
TauriRT->>JobStore: 새 작업 생성, 상태=Queued
TauriRT->>PythonCLI: stdin으로 JSON 페이로드 전송
activate PythonCLI
PythonCLI->>PythonCLI: 분석 실행
PythonCLI-->>TauriRT: stdout으로 AnalysisJobStatus 반환
deactivate PythonCLI
TauriRT->>JobStore: 상태=Succeeded, 결과 저장
React->>TauriRT: get_analysis_job_status (폴링)
TauriRT->>JobStore: 작업 조회
TauriRT-->>React: AnalysisJobStatus 반환
React->>User: 결과 렌더링
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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/package.json`:
- Line 14: The dependency entry for "@tauri-apps/api" is pinned to "2.8.0" but
newer compatible releases (2.9.x, 2.10.x) exist; verify compatibility with Tauri
v2.3.1 and update the package.json dependency accordingly: run compatibility
tests or check changelogs for breaking changes, choose the highest compatible
version (e.g., "2.10.1") and update the "@tauri-apps/api" version string in
package.json, then run npm install and the app test suite to confirm no
regressions.
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 356-444: The child process's stdout/stderr are piped but never
read while looping on process.try_wait(), which can block the child when its OS
pipe buffers fill; fix by immediately draining those pipes in background threads
or tasks after spawning the process (use process.stdout.take() and
process.stderr.take() to spawn threads that read to a Vec or /dev/null) so the
main loop that uses process.try_wait() and the timeout constants
(ANALYSIS_PROCESS_TIMEOUT, ANALYSIS_WAIT_POLL) can safely poll; ensure those
reader threads are joined or their buffers are consumed before calling
process.wait_with_output() or replace the try_wait loop with a wait_with_output
that consumes output atomically if you prefer.
- Around line 22-26: AppStateInner의 jobs: Mutex<HashMap<String,
AnalysisJobStatus>>에 완료된 작업들이 그대로 남아 메모리 누수가 발생하므로, jobs에 TTL/상한 또는 완료 후 정리 로직을
추가하세요: AnalysisJobStatus에 완료 시각(timestamp)을 포함하거나 삽입 시 만료 시간을 저장하고, 작업을 insert하는
코드 경로(현재 jobs.insert 호출 위치)를 수정해 타임스탬프를 기록하며, 터미널 상태를 반환하는 코드 경로(현재 terminal 상태만
반환하는 부분)에서 해당 엔트리를 삭제하거나 백그라운드 청소 루프를 만들어 만료된/완료된 항목을 주기적으로 제거하도록 구현합니다; Mutex
잠금/동시성 안전을 유지하고 필요하면 상한(예: 최대 항목 수)도 적용하세요.
In `@apps/desktop/src/App.test.tsx`:
- Around line 7-15: The mocks for startAnalysisJob and getAnalysisJobStatus
bypass the real schema validation in apps/desktop/src/lib/analysis.ts (which
throws on invalid status envelopes), so update the test mock to replicate that
behavior: instead of returning tauriInvoke raw results directly, have the mocked
startAnalysisJob and getAnalysisJobStatus call the same validation logic (or
invoke the real tauriInvoke wrapper used in analysis.ts) and propagate/reject
when the envelope is invalid; keep createDefaultAnalysisRequest as-is but ensure
startAnalysisJob/getAnalysisJobStatus mimic the production validation/throwing
behavior so rejected paths (as exercised in the tests around lines ~172-180 and
~246-251) are exercised correctly.
In `@apps/desktop/src/App.tsx`:
- Around line 21-35: The progressMessage function currently maps every "failed"
state to the same message, which mixes up pre-start failures and runtime
failures; update progressMessage (and its signature used by callers like
startAnalysisJob and the polling logic) to accept an additional context/phase
flag (e.g., isStartPhase: boolean or failurePhase: "start" | "run") or extend
AnalysisJobStatus state to distinguish "failedToStart" vs "failedDuringRun",
then return distinct translator keys (e.g., "analysisCouldNotStart" for
start-phase failures and "analysisCouldNotRun"/"analysisFailedDuringExecution"
for runtime failures) so callers can pass the correct context and the UI shows
the correct failure message.
- Around line 155-156: The job status and error messages are currently rendered
without ARIA live regions so screen readers won't announce asynchronous changes;
wrap the jobStatus output (the expression using jobStatus and progressMessage)
in an element with role="status" and appropriate aria-live (e.g.,
"polite")/aria-atomic="true" so queued/running updates are announced, and wrap
jobError in an element with role="alert" (or aria-live="assertive") so failures
are immediately announced; update the JSX that references jobStatus,
progressMessage(t, jobStatus.state) and jobError to render these accessible
containers only when the respective values exist.
In `@apps/desktop/src/lib/analysis.ts`:
- Around line 22-28: getInvoke currently always returns a truthy function in
environments with window because it uses the nullish coalescing operator,
preventing browserFallback from ever running; update getInvoke to explicitly
check for a valid Tauri invoke function and return null when not present so
browserFallback() can run. Specifically, in getInvoke() verify typeof
window.__TAURI_INVOKE__ === 'function' (or that invoke is defined) and return
that function only when valid, otherwise return null; ensure callers like
browserFallback() (referenced around browserFallback and getInvoke) rely on a
null result to trigger the non-Tauri fallback.
In `@docs/plans/2026-03-12-issue-32-analysis-orchestration.md`:
- Line 11: 문서에서 "### Task 1: Add shared analysis job contracts" 등 Task 제목이 h3로
시작해 제목 레벨을 건너뛰고 있으니 해당 Task 제목들을 h2로 변경하세요; 구체적으로 파일 내 "### Task 1: Add shared
analysis job contracts"와 이후의 Task 2–5 제목들을 모두 "## Task X: ..." 형식으로 바꿔 문서 구조를
일관되게 유지하도록 수정하세요.
- Line 7: Update the tech stack line in the document where the string "Python
3.14" appears (the line starting with "**Tech Stack:** React 19, Tauri/Rust,
TypeScript shared contracts, Python 3.14, `uv`, Vitest, pytest") to "Python
3.12" so it matches the project's pyproject.toml `requires-python = ">=3.12"`
constraint; ensure only the version token is changed and keep the rest of the
tech stack text intact.
In `@packages/shared-types/src/index.ts`:
- Around line 95-113: AnalysisJobStatus currently allows invalid combinations
(e.g., state "succeeded" without result or "queued" with error); change
AnalysisJobStatus into a discriminated union keyed by state (e.g., QueuedStatus
| InProgressStatus | SucceededStatus | FailedStatus) so each variant requires or
forbids result/error appropriately, update createAnalysisJobStatus (and any
factory like createAnalysisJobSnapshot if present) to switch on input.state and
validate/assign only the fields allowed for that variant, and update
AnalysisJobSnapshot.status usage to use the new discriminated union type to
enforce the invariant at compile time and runtime.
- Around line 282-288: The helper createDemoAnalysisJobRequest currently sets
roleFocus from only the first section, which will break when
demoRehearsalSongSeed has multiple sections; update roleFocus to collect role
ids from every section (iterate demoRehearsalSongSeed.sections, flatMap each
section.roles to role.id), deduplicate the ids (e.g., via a Set) and return that
array so the helper represents all roles across the entire demo seed.
- Around line 346-352: 현재 roleFocus 배열 항목이 빈 문자열 또는 공백만 있는 값도 통과하므로 downstream
오류를 초래할 수 있습니다; roleFocus를 검증하는 루프(참조: isDenseArray, value.roleFocus, role
variable)에서 각 항목이 문자열인지 확인하는 기존 체크에 추가로 role.trim()이 빈 문자열인지 검사하여 공백만인 값도 거부하고,
거부 시 기존 메시지 형식(`Invalid analysis job request: invalid field
'roleFocus[${index}]'`)으로 즉시 반환하도록 수정하세요.
In `@services/analysis-engine/src/bandscope_analysis/cli.py`:
- Around line 48-49: After writing JSON to stdout using json.dump in the CLI
(e.g., the json.dump(response, sys.stdout) call in main and the other json.dump
calls in the same module), immediately call sys.stdout.flush() so the subprocess
reader (Rust) receives the output without buffering delay; add
sys.stdout.flush() after each json.dump or do a single flush at the end of
main() to ensure all written JSON is flushed to stdout before returning from the
CLI.
In `@services/analysis-engine/tests/test_api.py`:
- Around line 35-57: Replace the manual try/except loop in
test_validate_analysis_job_request_rejects_bad_payloads with a parametric pytest
style: convert the cases into a pytest.mark.parametrize on payload and expected
message, and inside the test use pytest.raises(ValueError) as excinfo and assert
expected message in str(excinfo.value); keep the same test name and still call
validate_analysis_job_request for each payload so behavior is identical but
clearer and idiomatic.
🪄 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: 3a55c7eb-6f21-4795-9af5-502b1f1f725a
⛔ Files ignored due to path filters (6)
apps/desktop/src-tauri/Cargo.lockis excluded by!**/*.lockapps/desktop/src-tauri/gen/schemas/acl-manifests.jsonis excluded by!**/gen/**apps/desktop/src-tauri/gen/schemas/capabilities.jsonis excluded by!**/gen/**apps/desktop/src-tauri/gen/schemas/desktop-schema.jsonis excluded by!**/gen/**apps/desktop/src-tauri/gen/schemas/macOS-schema.jsonis excluded by!**/gen/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.github/workflows/sbom.ymlARCHITECTURE.mdapps/desktop/package.jsonapps/desktop/src-tauri/Cargo.tomlapps/desktop/src-tauri/build.rsapps/desktop/src-tauri/capabilities/main.jsonapps/desktop/src-tauri/permissions/autogenerated/get_analysis_job_status.tomlapps/desktop/src-tauri/permissions/autogenerated/start_analysis_job.tomlapps/desktop/src-tauri/src/main.rsapps/desktop/src-tauri/tauri.conf.jsonapps/desktop/src/App.test.tsxapps/desktop/src/App.tsxapps/desktop/src/i18n/index.tsapps/desktop/src/lib/analysis.tsapps/desktop/src/locales/en/common.jsonapps/desktop/src/locales/ko/common.jsondocs/architecture/overview.mddocs/plans/2026-03-12-issue-32-analysis-orchestration-design.mddocs/plans/2026-03-12-issue-32-analysis-orchestration.mddocs/security/app-security.mdpackages/shared-types/src/index.tspackages/shared-types/test/index.test.tsservices/analysis-engine/src/bandscope_analysis/api.pyservices/analysis-engine/src/bandscope_analysis/cli.pyservices/analysis-engine/tests/test_api.pyservices/analysis-engine/tests/test_cli.py
| "test": "node -e \"require('node:fs').mkdirSync('coverage/.tmp', { recursive: true })\" && vitest run --coverage" | ||
| }, | ||
| "dependencies": { | ||
| "@tauri-apps/api": "^2.8.0", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify `@tauri-apps/api` version 2.8.0 exists on npm
echo "Checking `@tauri-apps/api` available versions..."
npm view `@tauri-apps/api` versions --json | jq -r '.[]' | grep -E "^2\." | tail -10
echo ""
echo "Checking if 2.8.0 specifically exists..."
npm view `@tauri-apps/api`@2.8.0 version 2>/dev/null || echo "Version 2.8.0 not found"Repository: seonghobae/bandscope
Length of output: 220
@tauri-apps/api 버전 존재 확인 완료
@tauri-apps/api 버전 2.8.0은 npm 레지스트리에 존재합니다. 다만 더 최신의 호환 버전들(2.9.0, 2.9.1, 2.10.0, 2.10.1)도 available하므로, Tauri v2.3.1과의 호환성을 재검토하여 더 최신 버전 사용을 고려하십시오.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/package.json` at line 14, The dependency entry for
"@tauri-apps/api" is pinned to "2.8.0" but newer compatible releases (2.9.x,
2.10.x) exist; verify compatibility with Tauri v2.3.1 and update the
package.json dependency accordingly: run compatibility tests or check changelogs
for breaking changes, choose the highest compatible version (e.g., "2.10.1") and
update the "@tauri-apps/api" version string in package.json, then run npm
install and the app test suite to confirm no regressions.
Comment on lines
+22
to
+26
| struct AppStateInner { | ||
| next_job: AtomicU64, | ||
| in_flight_jobs: AtomicUsize, | ||
| jobs: Mutex<HashMap<String, AnalysisJobStatus>>, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
작업 저장소에 정리 경로가 없습니다.
jobs는 Line 345-346에서 insert만 하고, Line 529-544에서도 terminal status를 반환만 합니다. 결과 payload 전체가 맵에 남기 때문에 장시간 세션이나 반복 분석에서 메모리가 계속 증가합니다. TTL/상한 또는 완료 후 정리 경로를 두는 편이 안전합니다.
Also applies to: 344-348, 529-544
🤖 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 22 - 26, AppStateInner의
jobs: Mutex<HashMap<String, AnalysisJobStatus>>에 완료된 작업들이 그대로 남아 메모리 누수가 발생하므로,
jobs에 TTL/상한 또는 완료 후 정리 로직을 추가하세요: AnalysisJobStatus에 완료 시각(timestamp)을 포함하거나 삽입
시 만료 시간을 저장하고, 작업을 insert하는 코드 경로(현재 jobs.insert 호출 위치)를 수정해 타임스탬프를 기록하며, 터미널
상태를 반환하는 코드 경로(현재 terminal 상태만 반환하는 부분)에서 해당 엔트리를 삭제하거나 백그라운드 청소 루프를 만들어 만료된/완료된
항목을 주기적으로 제거하도록 구현합니다; Mutex 잠금/동시성 안전을 유지하고 필요하면 상한(예: 최대 항목 수)도 적용하세요.
Comment on lines
+356
to
+444
| let mut process = match Command::new(program) | ||
| .args(args) | ||
| .current_dir(working_dir) | ||
| .stdin(Stdio::piped()) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .spawn() | ||
| { | ||
| Ok(process) => process, | ||
| Err(_) => { | ||
| return failed_status( | ||
| job_id, | ||
| requested_at, | ||
| AnalysisJobErrorCode::EngineUnavailable, | ||
| "Analysis engine is unavailable.", | ||
| ) | ||
| } | ||
| }; | ||
|
|
||
| let payload = json!({ | ||
| "jobId": job_id, | ||
| "request": request, | ||
| }); | ||
|
|
||
| if let Some(mut stdin) = process.stdin.take() { | ||
| if stdin.write_all(payload.to_string().as_bytes()).is_err() { | ||
| let _ = process.kill(); | ||
| return failed_status( | ||
| payload["jobId"] | ||
| .as_str() | ||
| .unwrap_or("unknown-job") | ||
| .to_string(), | ||
| requested_at, | ||
| AnalysisJobErrorCode::EngineUnavailable, | ||
| "Analysis engine is unavailable.", | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| let deadline = Instant::now() + ANALYSIS_PROCESS_TIMEOUT; | ||
| loop { | ||
| match process.try_wait() { | ||
| Ok(Some(_)) => break, | ||
| Ok(None) => { | ||
| if Instant::now() >= deadline { | ||
| let _ = process.kill(); | ||
| let _ = process.wait(); | ||
| return failed_status( | ||
| payload["jobId"] | ||
| .as_str() | ||
| .unwrap_or("unknown-job") | ||
| .to_string(), | ||
| requested_at, | ||
| AnalysisJobErrorCode::EngineUnavailable, | ||
| "Analysis engine timed out.", | ||
| ); | ||
| } | ||
| thread::sleep(ANALYSIS_WAIT_POLL); | ||
| } | ||
| Err(_) => { | ||
| let _ = process.kill(); | ||
| let _ = process.wait(); | ||
| return failed_status( | ||
| payload["jobId"] | ||
| .as_str() | ||
| .unwrap_or("unknown-job") | ||
| .to_string(), | ||
| requested_at, | ||
| AnalysisJobErrorCode::EngineUnavailable, | ||
| "Analysis engine is unavailable.", | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let output = match process.wait_with_output() { | ||
| Ok(output) => output, | ||
| Err(_) => { | ||
| return failed_status( | ||
| payload["jobId"] | ||
| .as_str() | ||
| .unwrap_or("unknown-job") | ||
| .to_string(), | ||
| requested_at, | ||
| AnalysisJobErrorCode::EngineUnavailable, | ||
| "Analysis engine is unavailable.", | ||
| ) | ||
| } | ||
| }; |
There was a problem hiding this comment.
파이프를 비우지 않은 채 종료만 기다리면 엔진이 막힐 수 있습니다.
Line 359-361에서 stdout/stderr를 pipe로 열고, Line 395-444에서는 그 출력을 읽지 않은 채 try_wait()만 반복합니다. 자식 프로세스가 버퍼를 채우면 write에서 block되고, 부모는 이를 timeout으로 오진합니다.
🤖 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 356 - 444, The child
process's stdout/stderr are piped but never read while looping on
process.try_wait(), which can block the child when its OS pipe buffers fill; fix
by immediately draining those pipes in background threads or tasks after
spawning the process (use process.stdout.take() and process.stderr.take() to
spawn threads that read to a Vec or /dev/null) so the main loop that uses
process.try_wait() and the timeout constants (ANALYSIS_PROCESS_TIMEOUT,
ANALYSIS_WAIT_POLL) can safely poll; ensure those reader threads are joined or
their buffers are consumed before calling process.wait_with_output() or replace
the try_wait loop with a wait_with_output that consumes output atomically if you
prefer.
Comment on lines
+7
to
+15
| vi.mock("./lib/analysis", () => ({ | ||
| createDefaultAnalysisRequest: () => ({ | ||
| sourceKind: "demo", | ||
| sourceLabel: "Late Night Set", | ||
| roleFocus: ["bass-guitar", "keys-right", "lead-vocal"] | ||
| }), | ||
| startAnalysisJob: (request: unknown) => tauriInvoke("start_analysis_job", { request }), | ||
| getAnalysisJobStatus: (jobId: string) => tauriInvoke("get_analysis_job_status", { jobId }) | ||
| })); |
There was a problem hiding this comment.
모킹이 실제 스키마 검증을 건너뜁니다.
apps/desktop/src/lib/analysis.ts:102-116는 잘못된 status envelope를 예외로 바꾸는데, Line 13과 Line 14의 mock은 tauriInvoke 결과를 그대로 반환합니다. 그래서 Line 172-180이나 Line 246-251처럼 실제 프로덕션에서는 rejected path로 가야 할 payload를 정상 흐름으로 테스트하고 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.test.tsx` around lines 7 - 15, The mocks for
startAnalysisJob and getAnalysisJobStatus bypass the real schema validation in
apps/desktop/src/lib/analysis.ts (which throws on invalid status envelopes), so
update the test mock to replicate that behavior: instead of returning
tauriInvoke raw results directly, have the mocked startAnalysisJob and
getAnalysisJobStatus call the same validation logic (or invoke the real
tauriInvoke wrapper used in analysis.ts) and propagate/reject when the envelope
is invalid; keep createDefaultAnalysisRequest as-is but ensure
startAnalysisJob/getAnalysisJobStatus mimic the production validation/throwing
behavior so rejected paths (as exercised in the tests around lines ~172-180 and
~246-251) are exercised correctly.
Comment on lines
+21
to
+35
| function progressMessage( | ||
| t: ReturnType<typeof createTranslator>, | ||
| state: AnalysisJobStatus["state"] | ||
| ): string { | ||
| switch (state) { | ||
| case "queued": | ||
| return t("analysisStateQueued"); | ||
| case "running": | ||
| return t("analysisStateRunning"); | ||
| case "succeeded": | ||
| return t("analysisStateSucceeded"); | ||
| case "failed": | ||
| return t("analysisStateFailed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
실패 단계별 문구가 서로 뒤바뀝니다.
Line 33은 모든 failed 상태를 실행 중 실패로 표시하지만, 이 PR의 startAnalysisJob 경로에는 시작 전 실패도 포함됩니다. 반대로 Line 114와 Line 118은 이미 시작된 작업의 폴링 실패에 analysisCouldNotStart를 재사용합니다. 실패 원인을 상태값 하나로만 보지 말고, 시작 단계 실패와 실행 단계 실패를 구분해서 문구를 고르는 편이 맞습니다.
Also applies to: 113-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.tsx` around lines 21 - 35, The progressMessage function
currently maps every "failed" state to the same message, which mixes up
pre-start failures and runtime failures; update progressMessage (and its
signature used by callers like startAnalysisJob and the polling logic) to accept
an additional context/phase flag (e.g., isStartPhase: boolean or failurePhase:
"start" | "run") or extend AnalysisJobStatus state to distinguish
"failedToStart" vs "failedDuringRun", then return distinct translator keys
(e.g., "analysisCouldNotStart" for start-phase failures and
"analysisCouldNotRun"/"analysisFailedDuringExecution" for runtime failures) so
callers can pass the correct context and the UI shows the correct failure
message.
Comment on lines
+95
to
+113
| export type AnalysisJobStatus = { | ||
| jobId: string; | ||
| state: AnalysisJobState; | ||
| requestedAt: string; | ||
| updatedAt: string; | ||
| progressLabel?: string; | ||
| result?: RehearsalSong; | ||
| error?: AnalysisJobError; | ||
| }; | ||
|
|
||
| export type AnalysisJobSnapshot = { | ||
| jobId: string; | ||
| request: AnalysisJobRequest; | ||
| status: AnalysisJobStatus; | ||
| startedAt?: string; | ||
| finishedAt?: string; | ||
| error?: AnalysisJobError; | ||
| metadata?: Record<string, unknown>; | ||
| }; |
There was a problem hiding this comment.
상태 불변식을 타입과 생성자에서 함께 고정해 주세요.
현재 AnalysisJobStatus는 state와 result/error의 관계를 타입으로 표현하지 못하고, createAnalysisJobStatus()도 키 존재 여부만 보고 필드를 복사합니다. 그래서 succeeded인데 result가 없거나, queued인데 error가 달린 객체를 정적으로도 런타임에서도 만들 수 있습니다. 이 타입이 AnalysisJobSnapshot.status로도 재사용되는 만큼 discriminated union + input.state 분기로 invariant를 한 번에 고정하는 편이 안전합니다.
예시 수정안
-export type AnalysisJobStatus = {
- jobId: string;
- state: AnalysisJobState;
- requestedAt: string;
- updatedAt: string;
- progressLabel?: string;
- result?: RehearsalSong;
- error?: AnalysisJobError;
-};
+type AnalysisJobStatusBase = {
+ jobId: string;
+ requestedAt: string;
+ updatedAt: string;
+ progressLabel?: string;
+};
+
+export type AnalysisJobStatus =
+ | (AnalysisJobStatusBase & { state: "queued" | "running" })
+ | (AnalysisJobStatusBase & { state: "succeeded"; result: RehearsalSong })
+ | (AnalysisJobStatusBase & { state: "failed"; error: AnalysisJobError });
export function createAnalysisJobStatus(input:
| {
jobId: string;
state: "queued" | "running";
@@
): AnalysisJobStatus {
const now = new Date().toISOString();
- const status: AnalysisJobStatus = {
+ const base = {
jobId: input.jobId,
- state: input.state,
requestedAt: input.requestedAt ?? now,
updatedAt: input.updatedAt ?? now,
+ ...(input.progressLabel !== undefined ? { progressLabel: input.progressLabel } : {})
};
- if (input.progressLabel !== undefined) {
- status.progressLabel = input.progressLabel;
- }
- if ("result" in input) {
- status.result = input.result;
- }
- if ("error" in input) {
- status.error = input.error;
+ if (input.state === "succeeded") {
+ return { ...base, state: "succeeded", result: input.result };
}
-
- return status;
+ if (input.state === "failed") {
+ return { ...base, state: "failed", error: input.error };
+ }
+ return { ...base, state: input.state };
}Also applies to: 290-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-types/src/index.ts` around lines 95 - 113, AnalysisJobStatus
currently allows invalid combinations (e.g., state "succeeded" without result or
"queued" with error); change AnalysisJobStatus into a discriminated union keyed
by state (e.g., QueuedStatus | InProgressStatus | SucceededStatus |
FailedStatus) so each variant requires or forbids result/error appropriately,
update createAnalysisJobStatus (and any factory like createAnalysisJobSnapshot
if present) to switch on input.state and validate/assign only the fields allowed
for that variant, and update AnalysisJobSnapshot.status usage to use the new
discriminated union type to enforce the invariant at compile time and runtime.
Comment on lines
+282
to
+288
| export function createDemoAnalysisJobRequest(): AnalysisJobRequest { | ||
| return { | ||
| sourceKind: "demo", | ||
| sourceLabel: demoRehearsalSongSeed.title, | ||
| roleFocus: demoRehearsalSongSeed.sections[0].roles.map((role) => role.id) | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
roleFocus는 첫 섹션에만 묶지 않는 편이 안전합니다.
지금 seed는 섹션이 하나라 결과가 같지만, 섹션이 추가되는 순간 이 helper가 조용히 일부 role만 담은 요청을 만들게 됩니다. demo source 전체를 대표하는 helper라면 모든 섹션의 role ID를 수집하고 dedupe하는 편이 덜 취약합니다.
예시 수정안
export function createDemoAnalysisJobRequest(): AnalysisJobRequest {
+ const roleFocus = Array.from(
+ new Set(
+ demoRehearsalSongSeed.sections.flatMap((section) =>
+ section.roles.map((role) => role.id)
+ )
+ )
+ );
+
return {
sourceKind: "demo",
sourceLabel: demoRehearsalSongSeed.title,
- roleFocus: demoRehearsalSongSeed.sections[0].roles.map((role) => role.id)
+ roleFocus
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function createDemoAnalysisJobRequest(): AnalysisJobRequest { | |
| return { | |
| sourceKind: "demo", | |
| sourceLabel: demoRehearsalSongSeed.title, | |
| roleFocus: demoRehearsalSongSeed.sections[0].roles.map((role) => role.id) | |
| }; | |
| } | |
| export function createDemoAnalysisJobRequest(): AnalysisJobRequest { | |
| const roleFocus = Array.from( | |
| new Set( | |
| demoRehearsalSongSeed.sections.flatMap((section) => | |
| section.roles.map((role) => role.id) | |
| ) | |
| ) | |
| ); | |
| return { | |
| sourceKind: "demo", | |
| sourceLabel: demoRehearsalSongSeed.title, | |
| roleFocus | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-types/src/index.ts` around lines 282 - 288, The helper
createDemoAnalysisJobRequest currently sets roleFocus from only the first
section, which will break when demoRehearsalSongSeed has multiple sections;
update roleFocus to collect role ids from every section (iterate
demoRehearsalSongSeed.sections, flatMap each section.roles to role.id),
deduplicate the ids (e.g., via a Set) and return that array so the helper
represents all roles across the entire demo seed.
Comment on lines
+346
to
+352
| if (!isDenseArray(value.roleFocus)) { | ||
| return "Invalid analysis job request: invalid field 'roleFocus'"; | ||
| } | ||
| for (const [index, role] of value.roleFocus.entries()) { | ||
| if (typeof role !== "string") { | ||
| return `Invalid analysis job request: invalid field 'roleFocus[${index}]'`; | ||
| } |
There was a problem hiding this comment.
빈 roleFocus 항목도 지금은 통과합니다.
sourceLabel은 trim 검사를 하는데 role ID는 ""나 공백 문자열도 허용하고 있습니다. 이런 값은 IPC 경계에서 걸러지지 않고 downstream not_found/실패 상태로 늦게 드러나기 쉬워서 여기서 바로 막는 편이 낫습니다.
예시 수정안
for (const [index, role] of value.roleFocus.entries()) {
- if (typeof role !== "string") {
+ if (typeof role !== "string" || role.trim().length === 0) {
return `Invalid analysis job request: invalid field 'roleFocus[${index}]'`;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-types/src/index.ts` around lines 346 - 352, 현재 roleFocus 배열
항목이 빈 문자열 또는 공백만 있는 값도 통과하므로 downstream 오류를 초래할 수 있습니다; roleFocus를 검증하는 루프(참조:
isDenseArray, value.roleFocus, role variable)에서 각 항목이 문자열인지 확인하는 기존 체크에 추가로
role.trim()이 빈 문자열인지 검사하여 공백만인 값도 거부하고, 거부 시 기존 메시지 형식(`Invalid analysis job
request: invalid field 'roleFocus[${index}]'`)으로 즉시 반환하도록 수정하세요.
Comment on lines
+48
to
+49
| json.dump(response, sys.stdout) | ||
| return 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
subprocess 통신을 위한 stdout 플러시 권장.
json.dump 후 sys.stdout.flush()가 없으면 버퍼링된 I/O로 인해 Rust 측에서 응답을 즉시 읽지 못할 수 있습니다. 특히 파이프를 통한 subprocess 통신에서 중요합니다.
🔧 권장 수정
response = run_analysis_job(job_id, request, requested_at)
json.dump(response, sys.stdout)
+ sys.stdout.flush()
return 0다른 json.dump 호출(lines 32, 36, 42)에도 동일하게 적용하거나, main() 함수 끝에서 한 번 플러시하는 것을 권장합니다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| json.dump(response, sys.stdout) | |
| return 0 | |
| json.dump(response, sys.stdout) | |
| sys.stdout.flush() | |
| return 0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/analysis-engine/src/bandscope_analysis/cli.py` around lines 48 - 49,
After writing JSON to stdout using json.dump in the CLI (e.g., the
json.dump(response, sys.stdout) call in main and the other json.dump calls in
the same module), immediately call sys.stdout.flush() so the subprocess reader
(Rust) receives the output without buffering delay; add sys.stdout.flush() after
each json.dump or do a single flush at the end of main() to ensure all written
JSON is flushed to stdout before returning from the CLI.
Comment on lines
+35
to
+57
| def test_validate_analysis_job_request_rejects_bad_payloads() -> None: | ||
| """Ensure the request validator reports every expected safe-failure path.""" | ||
| cases = [ | ||
| ([], "root"), | ||
| ({}, "sourceKind"), | ||
| ({"sourceKind": "file", "sourceLabel": "Late Night Set", "roleFocus": []}, "sourceKind"), | ||
| ({"sourceKind": "demo", "roleFocus": []}, "sourceLabel"), | ||
| ({"sourceKind": "demo", "sourceLabel": " ", "roleFocus": []}, "sourceLabel"), | ||
| ({"sourceKind": "demo", "sourceLabel": "Late Night Set", "roleFocus": {}}, "roleFocus"), | ||
| ({"sourceKind": "demo", "sourceLabel": "Late Night Set", "roleFocus": [7]}, "roleFocus[0]"), | ||
| ( | ||
| {"sourceKind": "demo", "sourceLabel": "Late Night Set", "roleFocus": [], "extra": True}, | ||
| "extra", | ||
| ), | ||
| ] | ||
|
|
||
| for payload, message in cases: | ||
| try: | ||
| validate_analysis_job_request(payload) | ||
| except ValueError as error: | ||
| assert message in str(error) | ||
| else: | ||
| raise AssertionError(f"Expected ValueError for {payload!r}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
pytest.mark.parametrize 사용 권장.
유효하지 않은 페이로드 테스트가 수동 try/except 루프로 구현되어 있습니다. pytest.mark.parametrize와 pytest.raises를 사용하면 더 명확하고 pytest 스타일에 맞는 테스트가 됩니다.
♻️ 권장 리팩토링
+import pytest
+
+@pytest.mark.parametrize(
+ "payload,expected_field",
+ [
+ ([], "root"),
+ ({}, "sourceKind"),
+ ({"sourceKind": "file", "sourceLabel": "Late Night Set", "roleFocus": []}, "sourceKind"),
+ ({"sourceKind": "demo", "roleFocus": []}, "sourceLabel"),
+ ({"sourceKind": "demo", "sourceLabel": " ", "roleFocus": []}, "sourceLabel"),
+ ({"sourceKind": "demo", "sourceLabel": "Late Night Set", "roleFocus": {}}, "roleFocus"),
+ ({"sourceKind": "demo", "sourceLabel": "Late Night Set", "roleFocus": [7]}, "roleFocus[0]"),
+ ({"sourceKind": "demo", "sourceLabel": "Late Night Set", "roleFocus": [], "extra": True}, "extra"),
+ ],
+)
+def test_validate_analysis_job_request_rejects_bad_payloads(payload, expected_field) -> None:
+ """Ensure the request validator reports every expected safe-failure path."""
+ with pytest.raises(ValueError, match=expected_field):
+ validate_analysis_job_request(payload)
-def test_validate_analysis_job_request_rejects_bad_payloads() -> None:
- """Ensure the request validator reports every expected safe-failure path."""
- cases = [
- ([], "root"),
- # ... rest of cases
- ]
-
- for payload, message in cases:
- try:
- validate_analysis_job_request(payload)
- except ValueError as error:
- assert message in str(error)
- else:
- raise AssertionError(f"Expected ValueError for {payload!r}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/analysis-engine/tests/test_api.py` around lines 35 - 57, Replace the
manual try/except loop in
test_validate_analysis_job_request_rejects_bad_payloads with a parametric pytest
style: convert the cases into a pytest.mark.parametrize on payload and expected
message, and inside the test use pytest.raises(ValueError) as excinfo and assert
expected message in str(excinfo.value); keep the same test name and still call
validate_analysis_job_request for each payload so behavior is identical but
clearer and idiomatic.
Owner
Author
|
Superseded by #52 to clear the remaining stale check and CodeRabbit failure states after all current required checks passed and no new code changes were needed. |
auto-merge was automatically disabled
March 12, 2026 05:04
Pull request was closed
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.
Summary
Verification
./scripts/harness/quickcheck.shcargo check --manifest-path apps/desktop/src-tauri/Cargo.toml