Skip to content

refactor(engine): promote temporal probe from cli hack to api integration - #1117

Open
seonghobae wants to merge 8 commits into
developfrom
refactor/temporal-features-api
Open

refactor(engine): promote temporal probe from cli hack to api integration#1117
seonghobae wants to merge 8 commits into
developfrom
refactor/temporal-features-api

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

Promotes temporal analysis from the old cli.py probe into the production api.py orchestration path so tempo metadata survives stem-separation degradation and a local recording keeps its operator-visible source label.

  • api._build_local_temporal_features(request) runs TemporalAnalyzer for local_audio requests and always preserves the already-validated sourceLabel; if temporal DSP raises FileNotFoundError or ValueError, tempo/beat fields are omitted rather than invented.
  • run_analysis_job_updates composes temporal metadata with stem features; validated stem values retain precedence on collisions.
  • _build_from_arrangement uses the local recording title when available instead of falling back to Late Night Set.
  • Full completed-analysis cache schema is v2 under analysis-cache-v2, so pre-temporal v1 results cannot short-circuit the new metadata path.
  • Intermediate stem-feature cache remains independently versioned at FEATURE_CACHE_SCHEMA_VERSION = 1 and retains its existing analysis-cache-v1 digest/path so compatible expensive separation results remain reusable.
  • Stem separation timeout is 300 seconds and the Rust analysis-process timeout is 360 seconds, preserving an outer bound around the inner Python work.

Regression coverage

  • test_temporal_metadata_regression.py proves temporal failure retains the operator recording label, pre-temporal v1 full results are rejected, full results move to v2, and compatible v1 stem-feature paths remain unchanged.
  • test_branch_coverage_contract.py keeps the analyzer-failure branch aligned with the source-label-preserving fallback.
  • test_run_analysis_job_updates_report_progress_and_cache now checks completed results in analysis-cache-v2 separately from reusable .features.json metadata in analysis-cache-v1, while preserving the second-run cache-hit/separator-not-recalled contract.
  • Existing API/CLI orchestration tests continue to cover tempo preservation and progress streaming.

Security Notes

  • Attack surface: unchanged. TemporalAnalyzer already operated on the same operator-selected local recording path; this change moves that existing computation into the production orchestration path. No network, dependency, privilege, or new executable authority is introduced.
  • Trust boundary: local sourcePath remains supplied through the existing desktop selection/validation boundary. Cache files remain untrusted app-owned persisted data and are accepted only through their schema/shape validation paths.
  • Safe failure: temporal FileNotFoundError/ValueError drops unavailable DSP metadata but retains the known validated sourceLabel. Stem separation failure continues through the existing bounded fallback behavior. No tempo or beat value is synthesized after a failed temporal analysis.
  • Cache migration: completed result cache v1 is deliberately invalidated because its result semantics predate temporal title/tempo metadata. Intermediate stem features remain schema v1 because their persisted contract is unchanged; their legacy key and directory are intentionally preserved so a result-schema migration does not force another source-separation pass.
  • Logging/privacy: the existing safe temporal warning remains; no source contents or cache payloads are newly logged. Cache metadata continues to avoid persisting the original absolute source path in completed-result payloads.

Dependency / supply chain

No dependency, lockfile, model, or workflow change is introduced by this PR.

i18n impact

None. The title comes from the recording label already supplied by the desktop boundary; no new customer-facing locale key is introduced.

Exact-head gate

  • Protected target: develop@749511c3ad4000090048718f685c6bee6b3d2c25.
  • Exact current head: b98f266d2356d56be624fb617580b5252e85baaa.
  • All currently visible review threads are resolved against the current implementation, but there is still no qualifying independent non-author approval on this exact head.
  • Exact-head coverage-evidence is terminal-success. A fresh exact-head opencode-review dispatch is queued; queued evidence is not passing evidence. At the latest refetch no exact-head check had a terminal failure, but remaining required gates must still become terminal-success on this unchanged head.

Predecessor, queued, skipped, cancelled, failed, stale-head, model-only, self/author, status-only, or administrative-bypass evidence does not transfer. Merge only after every applicable required exact-head gate and a qualifying independent non-author approval are terminal-success with addressed threads resolved.


Devin Review

…tion

Replace the "Temporary: Inject temporal analyzer ... just to prove it
works" block in cli.py with api._build_local_temporal_features(), merged
into audio_features under stem features so tempo/beat cues and the real
recording's title survive into the job result even when stem separation
is unavailable. _build_from_arrangement now takes the song title from
audio_features when present instead of the hard-coded "Late Night Set".

Raise STEM_SEPARATION_TIMEOUT_SECONDS 20 -> 300 and the Rust
ANALYSIS_PROCESS_TIMEOUT 30s -> 360s: the old values guaranteed a
timeout on any real-length song and forced the fallback path
unconditionally.

Move the empty/unreadable-source branch coverage off the deleted cli
probe onto api._build_local_temporal_features; add a tempo-preserved
assertion to the stem-degrade test; repoint the progress-jsonl test at
bandscope_analysis.api.TemporalAnalyzer.

ruff + ruff format + mypy src + pytest --cov-fail-under=100 all pass
(678 passed, 100% coverage).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SoJBAAXwv58S8P4hQBQQAw
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

로컬 오디오 분석에 TemporalAnalyzer를 통합했습니다. Stem 분석 실패 시 temporal feature를 사용합니다. 분석 및 stem separation 제한 시간을 늘렸습니다. Feature 캐시 구조와 metadata 보존 동작을 갱신했습니다.

Changes

분석 파이프라인 변경

Layer / File(s) Summary
Temporal feature 추출과 제한 시간
apps/desktop/core/src/lib.rs, services/analysis-engine/src/bandscope_analysis/api.py
로컬 오디오에서 title과 tempo를 추출합니다. 분석 제한 시간은 360초로, stem separation 제한 시간은 300초로 변경했습니다.
Stem 및 temporal feature 병합
services/analysis-engine/src/bandscope_analysis/api.py
Stem feature와 temporal feature를 별도로 관리합니다. Stem 분석 실패 시 fallback을 수행하고 두 feature를 최종 audio_features로 병합합니다. 데모 결과는 입력 title을 사용합니다.
Fallback 및 캐시 회귀 검증
services/analysis-engine/tests/test_api.py, services/analysis-engine/tests/test_branch_coverage_contract.py, services/analysis-engine/tests/test_cli.py, services/analysis-engine/tests/test_temporal_metadata_regression.py
Temporal feature 추출 실패, degraded 결과의 tempo, source label 보존, 새 분석 캐시 경로와 기존 캐시 거부 동작을 검증합니다. CLI 테스트는 API 모듈의 TemporalAnalyzer를 패치합니다.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🟡 Moderate · up to 9f20f

The PR moves tempo and recording-title generation into the main analysis flow, but stale v1 cache entries can bypass those changes, and an unreadable recording may still fall back to the hard-coded rehearsal title. These bounded correctness issues should be resolved before merging.

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant AnalysisAPI
  participant TemporalAnalyzer
  participant StemSeparation
  CLI->>AnalysisAPI: 분석 작업 요청 전달
  AnalysisAPI->>TemporalAnalyzer: 로컬 오디오 temporal feature 추출
  TemporalAnalyzer-->>AnalysisAPI: title과 tempo 반환 또는 실패
  AnalysisAPI->>StemSeparation: stem feature 추출
  StemSeparation-->>AnalysisAPI: stem feature 또는 오류 반환
  AnalysisAPI->>AnalysisAPI: temporal feature와 stem feature 병합
  AnalysisAPI-->>CLI: 최종 분석 결과와 상태 반환
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 94.12% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 6 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 CLI의 임시 temporal probe를 API 통합으로 전환한 주요 변경 사항을 정확하고 간결하게 설명합니다.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/temporal-features-api

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

seonghobae added a commit that referenced this pull request Aug 31, 2026
… progress

Iteration-2 status: gates still fail closed. Central .github landed 8
Noema-reliability fixes (#1477-#1504) plus an active "remove fixed LLM
response timeout" branch. Local response: staged merge-ready work behind
the closed gates — PR #1116 (this baseline) and PR #1117 (temporal probe
promoted from cli hack to api integration, 100% coverage locally).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SoJBAAXwv58S8P4hQBQQAw
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Take the sole writer lease for the current exact CI repair on canonical refactor/temporal-features-api, only while it still resolves to 7cfddc53294b07e6d35c4340a5cfc2e38c7dd575 over protected develop@749511c3ad4000090048718f685c6bee6b3d2c25. Apply superpowers:using-superpowers, systematic-debugging, test-driven-development where behavior changes, receiving-code-review, and verification-before-completion. Refetch exact head/base and owning blobs before writing; abort/adapt if they move.

Exact repository CI run 33399139786, job 99517008529, checked out merge result fb1a4a239ec226a57b36cdd4c9dd9676b15ec664 (head 7cfddc5… + base 749511c…). Quickcheck gets through lint, docstrings, Bandit, typecheck, desktop 216/216 at exact 100% statement/branch/function/line coverage, and shared-types 20/20 at exact 100%. Python then fails only tests/test_api.py::test_run_analysis_job_updates_report_progress_and_cache: it still searches full result JSON under analysis-cache-v1, gets zero, and aborts the suite; resulting 99.78% coverage is secondary fallout. Current production deliberately moved completed result cache to schema/directory v2 while preserving feature-cache schema/directory v1, and the dedicated temporal/cache regression already asserts that split.

Treat this as a stale test contract caused by this branch, not a reason to roll production cache semantics back. Make the smallest test-only repair in services/analysis-engine/tests/test_api.py: assert one completed result under analysis-cache-v2; separately assert the reusable .features.json stem metadata remains under analysis-cache-v1; preserve the existing second-run cache-hit/separator-not-recalled contract. No production change unless a RED regression proves a different root cause. Run the focused failing test first, then full Python 100% coverage and repository quickcheck. Commit to this existing contributor branch and report successor exact SHA plus focused/full GREEN. Do not force-push/rebase, alter workflows/gates/dependencies, touch foreign repos, create another PR, or self-review.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Review the unchanged exact head b98f266d2356d56be624fb617580b5252e85baaa against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25 as an independent non-author reviewer. Do not write source, docs, refs, or workflow state. Refetch this exact head/base, current diff, current resolved threads, and current required checks before judging it. Verify the production temporal integration, full-result cache v2 vs reusable feature-cache v1 split, fallback title/tempo semantics, 300s inner separation vs 360s outer process timeout, payload-safe cache handling, and the current tests. Submit a formal APPROVED only if this exact head is review-clean; otherwise submit formal CHANGES_REQUESTED with concrete current-head findings. Predecessor reviews, model summaries, queued/neutral gates, and author/self evidence do not qualify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant