Skip to content

feat(workspace): name tonight's first ritardando plan on the map - #1047

Open
seonghobae wants to merge 25 commits into
developfrom
feat/workspace-first-ritardando-plan
Open

feat(workspace): name tonight's first ritardando plan on the map#1047
seonghobae wants to merge 25 commits into
developfrom
feat/workspace-first-ritardando-plan

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Name tonight's first ritardando plan on the mounted rehearsal map so the vocal or bass that eases into a slower tempo can open that landing.

  • Engine emits ritardandoPlan only when existing tempo-stability reports a sustained slowing (to_bpm < from_bpm) that is not a half-time (~0.45–0.55) or double-time (~2.0) feel flip.
  • The owned copy lands on the highest-priority active named vocal or bass in the section that contains the change. Vocal is preferred on priority ties. Accompaniment never owns. Heuristic/demo topology stays unnamed.
  • Workspace callout opens the matching rendered map section. Fail-closed resolvers ignore inherited, accessor, or Proxy metadata. Customer copy names the next action in EN/KO.
  • This is not a new MIR product. test(analysis): govern real YouTube known-stem benchmark #828 owns [Product Gap] Add real-audio MIR accuracy acceptance benchmarks #770. Beat times reuse existing librosa.beat.beat_track / analyze_tempo_stability.

Protected base: develop@749511c3ad4000090048718f685c6bee6b3d2c25.
Exact current head: 877ee60af8a0e8d19c522c5437b7aa16934c8bf6.

Do not mix with #811, #828, or #897.

Test plan

  • Python ritardando tests (18) at 100% module coverage
  • Rust ritardando-plan contract tests
  • Vitest resolver, i18n, provenance, callout, particle, and workspace armed-state tests

Do not self-approve. Do not wait on checks.


Devin Review

Summary by CodeRabbit

  • 새로운 기능

    • 실시간 분석에서 지속적인 감속을 감지하면 첫 리타르단도 연습 계획을 제공합니다.
    • 담당 파트, BPM 변화, 위치와 시간을 확인하고 해당 곡 구조의 지도 섹션을 바로 열 수 있습니다.
    • 영어와 한국어 안내 문구 및 섹션 라벨 현지화를 지원합니다.
  • 개선 사항

    • 연습 진행률이나 파트 변경 후에도 안내 상태가 안정적으로 유지됩니다.
    • 잘못되거나 불완전한 계획 정보는 표시하지 않습니다.
    • 로컬 오디오 분석에서 템포와 비트 정보 재사용 효율이 향상되었습니다.

Security Notes

  • No new file, URL, subprocess, IPC, model, or runtime network boundary is introduced.
  • Persisted ritardandoPlan remains fail-closed: Unicode line separators and whitespace-only values are rejected, while user-authored single-line copy is preserved without normalization.
  • The shared-types validator and native Rust project loader mirror this boundary; malformed-input cases are covered without logging sensitive values.

Latest exact-head verification

  • Protected base: develop@749511c3ad4000090048718f685c6bee6b3d2c25; current head: 877ee60af8a0e8d19c522c5437b7aa16934c8bf6.
  • Shared-types: 34 tests, 100% statements/branches/functions/lines.
  • Desktop resolver: 17 tests; Rust core: 19 tests; Rust ritardando contract: 6 tests.
  • Shared/desktop lint and typecheck, Rust format, documentation check, and diff check pass.

Latest current-head evidence

  • Current HEAD: 8c846ad0b9a95dbf451ad78a27d67a0e0e903984
  • Root cause fixed: ritardando change timing and section selection now preserve precise Python boundaries through the shared contract, Rust validator, and desktop projection.
  • Local verification: Python 703 passed, 24 skipped; Python statement/branch/function/line coverage 100%; Ruff check/format and mypy passed; shared-types and desktop ESLint/typecheck plus targeted Vitest passed; Rust contract/core tests and cargo fmt --check passed.
  • Hosted release-preflight must rerun against this exact HEAD.

Latest review repair

  • Current HEAD: 76caa7752dbf9ac71d71a447d27013f0f773c291
  • Model-sourced ritardando copy is now validated for the owned template, positive BPM values, slowing direction, and half-time exclusion at the native project boundary. User-sourced single-line copy remains arbitrary and is preserved.
  • Verification: cargo fmt --check; 19 Rust core tests and 8 ritardando contract tests passed.
  • Hosted checks must rerun against this exact HEAD.

Latest review repair

  • Current HEAD: 8fc1a19
  • Native and shared contracts now accept and round-trip the optional analysis tempo, with positive finite validation. Model ritardando plans use the same owned template/tempo semantics in shared save validation and native load validation; user-authored single-line copy remains arbitrary.
  • Verification: shared-types 24 targeted tests, desktop 250 tests with 100% statements/branches/functions/lines, Rust core 21 tests, and ritardando contract 8 tests passed; Python remains 703 passed / 24 skipped with 100% coverage.
  • Hosted checks must rerun against this exact HEAD.

Latest review repair

  • Current HEAD: f2879ed
  • Explicit JSON null for tempo is now rejected while an omitted optional tempo remains valid; positive finite tempo round-trips through analysis status and project load/save.
  • Verification: Rust core 22 tests and ritardando contract 8 tests passed; shared-types 24 targeted tests and desktop 250 tests with 100% coverage remain green.
  • Hosted checks must rerun against this exact HEAD.

Stamp one owned ritardandoPlan from existing tempo-stability slowing
onto the highest-priority active named vocal or bass, and open that
landing on the rehearsal map. Half-time and double-time feel flips
stay unnamed. Heuristic demo topology stays unnamed. This is not a
new MIR product.
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

첫 ritardando 계획을 위한 역할 계약, 템포 분석, 캐시 전달, 데스크톱 resolver, 워크스페이스 안내와 지도 탐색이 추가되었습니다. 지속 감속만 처리하며, 반감·배속 전환과 휴리스틱 데모 토폴로지는 제외합니다.

Changes

첫 ritardando 계획

Layer / File(s) Summary
역할 계약과 출처 검증
packages/shared-types/..., apps/desktop/core/...
ritardandoPlanritardandoPlanSource 필드를 추가하고 함께 존재하는지 검증합니다. Unicode 공백과 줄 구분자도 검증합니다.
템포 분석과 엔진 스탬프
services/analysis-engine/src/bandscope_analysis/..., services/analysis-engine/tests/...
BPM과 비트 시간을 전달하고 캐시에 보존합니다. 첫 지속 감속을 적합한 활성 보컬 또는 베이스 역할에 기록합니다.
첫 계획 resolver
apps/desktop/src/features/workspace/firstRitardando.ts, apps/desktop/src/features/workspace/firstRitardando.test.ts
계획, 섹션, 시간, 역할을 검증하고 가장 이른 유효 계획을 선택합니다. 잘못된 런타임 구조에서는 null을 반환합니다.
워크스페이스 안내와 탐색
apps/desktop/src/features/workspace/..., apps/desktop/src/i18n/..., apps/desktop/src/locales/...
안내 문구를 현지화하고 지도 섹션으로 이동합니다. 로컬 진행률 변경 뒤에도 열린 상태를 유지합니다.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟡 Moderate · up to 877ee

This change persists and displays ritardando plan text, but model-sourced plans are not fully checked for ritardando semantics and oversized persisted values can be scanned repeatedly during workspace loading, potentially slowing the desktop experience. The PR is not merge-ready until these bounded validation risks are fixed or explicitly accepted.

Sequence Diagram(s)

sequenceDiagram
  participant 오디오분리
  participant 분석엔진
  participant 계획Resolver
  participant 워크스페이스
  participant 지도섹션
  오디오분리->>분석엔진: BPM과 beat_times 제공
  분석엔진->>계획Resolver: 지속 감속 계획 데이터 제공
  계획Resolver->>워크스페이스: 첫 계획과 landing role 반환
  워크스페이스->>지도섹션: 일치하는 section index로 이동
  지도섹션-->>워크스페이스: 열린 계획 상태 표시
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 90.23% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 133 functions across 25 files. (3 skipped: …
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 제목은 PR의 핵심 변경인 첫 번째 ritardando 계획을 rehearsal map에 표시하고 이름을 부여하는 작업을 정확히 설명합니다. 간결하고 변경 범위를 충분히 식별할 수 있습니다.
Full details: Docstring Coverage

Explanation

Docstring coverage is 90.23% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 133 functions across 25 files. (3 skipped: 3 unsupported.)

✨ 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 feat/workspace-first-ritardando-plan

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.

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 Work only on the existing canonical branch feat/workspace-first-ritardando-plan for PR #1047, starting from exact current head bac0d90d1f1ae88141c93dd50483eebb63ed1e85 against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25, and only if that head is still current immediately before mutation. Refetch the branch/base/target blobs/review threads/checks before every write; adapt if compatible movement occurred. Apply receiving-code-review + systematic-debugging + test-driven-development + verification-before-completion. Never force-push, self-approve, weaken gates, or touch foreign repositories.

Two current-head findings have been independently revalidated against production source and are BandScope/#1047-owned:

  1. Untrusted persisted model copy can masquerade as a ritardando. Current boundedGeneratedRitardandoPlan in apps/desktop/src/features/workspace/firstRitardando.ts validates only the template and numeric tokens. It therefore accepts a speed-up such as Ease this part from 80 BPM into 120 BPM; let the next downbeat land later. and a half-time feel flip such as 120→60 even though the engine's authoritative is_ritardando_change requires finite positive to_bpm < from_bpm and rejects ratios 0.45..0.55. Treat thread PRRT_kwDORjvEXs6dGvff as valid. RED first: add realistic resolver regressions for at least speed-up and half-time model copy while preserving valid 120→80 and arbitrary bounded user copy. Then make the narrow resolver fix by parsing the already-regex-validated numeric tokens and enforcing the same slowing/half-time invariant before returning model guidance. Do not change user provenance behavior.

  2. The real local-audio pipeline never supplies the beat_times feature consumed by _apply_ritardando. Current _coerce_beat_times looks for audio_features["beat_times"], but _build_local_audio_features does not populate it, so the fallback always re-runs beat tracking on the normalized reconstructed stem sum rather than reusing the decoded/temporal-analysis signal. Treat thread PRRT_kwDORjvEXs6dGvgv as valid only after reproducing it on the fresh head. Compare the existing working TemporalAnalyzer/feature construction path and choose the smallest owning fix that reuses one authoritative beat-time result without introducing a second MIR product or changing source-separation authority. RED first with a pipeline-level regression proving the real local-audio features path supplies/reuses authoritative beat times; then make the narrow causal fix and preserve fail-closed malformed-feature fallback.

Do not duplicate unresolved thread PRRT_kwDORjvEXs6dGNt7's optional Rust role fields here: that persisted-contract defect already has the earlier canonical owner #1040 and must be repaired/merged dependency-root first rather than copied into this ritardando branch.

Verification: focused Vitest resolver tests; focused Python ritardando/pipeline tests; Rust contract tests applicable to this branch; then the canonical full quickcheck and all exposed exact owned statement/branch/function/line coverage, formatting/lint, security and build gates. Resolve only the two #1047 threads actually repaired. Refetch exact new head/live base after push. Do not mark merge-ready or merge without unchanged-head required checks plus qualifying independent approval.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Please perform the required independent formal review on exact current head be9f10f465145bc2094c1b9ad69fee4f27fba6a5 against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25. Review only; do not mutate the branch. This head includes the test-first model-copy tempo-semantics repair, section-local role-stamp isolation, and coverage regressions for runtime role-list drift. Verify the exact head and submit APPROVED or CHANGES_REQUESTED anchored to this SHA. Do not use predecessor-head evidence.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Repair PR #1047 on its existing canonical branch only. Refetch immediately before every mutation; current evidence is exact head be9f10f465145bc2094c1b9ad69fee4f27fba6a5 against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25. Apply receiving-code-review, systematic-debugging, test-driven-development, and verification-before-completion. Do not race or overwrite a moved head.

Two current-head BandScope-owned gaps remain. First, ownedRitardandoPlan() still accepts a nonblank ritardandoPlan when ritardandoPlanSource is absent, even though shared-types and native persistence now enforce plan/source presence parity. Add the smallest RED resolver/callout regression for source-less valid-looking copy, then fail closed unless source is exactly model or user; preserve valid user copy and the model slowing/half-time/template checks already repaired. Resolve PRRT_kwDORjvEXs6dKgTf only after successor-head proof.

Second, review thread PRRT_kwDORjvEXs6dGvgv remains valid at the production boundary: _apply_ritardando consumes audio_features['beat_times'], while the real _build_local_audio_features path does not populate authoritative beat times, so the normal path silently re-derives a beat grid from the reconstructed normalized stem sum instead of reusing the decoded-audio/TemporalAnalyzer timing source. Write the smallest realistic integration regression through the real local-audio feature construction/apply boundary proving the authoritative beat grid is carried and consumed; then plumb that existing timing evidence through the feature payload rather than adding a second tracker or success-shaped mock. Preserve bounded/fail-closed handling for malformed beat grids and the current explicit fallback only where no authoritative beat evidence legitimately exists.

Run focused ritardando unit/integration tests first, then repository-pinned lint/format and canonical relevant/full analysis/desktop/shared-types verification. Do not touch the native optional-field parity owned by #1040, central workflows, unrelated fade/accelerando lanes, or security/review gates. No force-push, destructive rebase, self-approval, or gate weakening.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

@OpenCode Repair only the existing canonical branch feat/workspace-first-ritardando-plan. Refetch immediately and require exact head 02b58dd5144a1168279b3ade31a51756d9dbf090 against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25; if either moved, inspect/remap the intervening delta rather than overwriting it. Apply receiving-code-review + systematic-debugging + TDD + verification-before-completion.

Current unresolved thread PRRT_kwDORjvEXs6dNjJ3 is valid. Exact source proves the fresh local path decodes the original audio in AudioStemSeparator._load_audio, then _temporal_features_for_request opens/decodes the same source again via TemporalAnalyzer.analyze. The reusable-feature cache also stores/loads only stems/sample rate/separation/role metadata, so every cache hit calls TemporalAnalyzer.analyze and re-decodes the source again. This is a BandScope-owned fast-path/performance/reliability defect, not a correctness excuse to revert source-authoritative beat evidence.

I added the smallest RED at exact head 02b58dd5144a1168279b3ade31a51756d9dbf090: services/analysis-engine/tests/test_temporal_feature_cache.py::test_feature_cache_round_trips_authoritative_tempo_grid. Keep it. It requires source-derived bpm and beat_times to survive feature-cache store/load; current code drops them.

Repair the owning boundary rather than reintroducing reconstructed-stem timing. Acceptance has two parts:

  1. Cache hit: extend the versioned feature-cache metadata contract to validate/store/load finite positive BPM and a finite non-negative beat grid when present, so a valid cached original-source temporal grid is reused without invoking TemporalAnalyzer.analyze again. Malformed temporal metadata must fail closed as a cache miss or omit unusable temporal evidence; never accept NaN/Inf/negative beats.
  2. Fresh analysis: eliminate the second full source decode. Reuse the audio that the separation boundary has already decoded, ideally by extracting a shared in-memory temporal-analysis helper from TemporalAnalyzer and computing compact BPM/beat evidence from that same decoded source before/alongside separation. Return only compact temporal metadata across the worker boundary; do not send raw audio through IPC, do not use reconstructed stem sum as the authoritative signal, and do not add network/model authority. If the cleanest root fix requires AudioStemSeparator to expose the already-decoded signal to the worker-local temporal helper, keep that change narrow and private.

RED first for both paths: retain the cache round-trip regression and add a realistic orchestration regression proving a reusable-feature cache hit does not call the file-decoding TemporalAnalyzer.analyze; add a fresh-path regression that proves one source decode owns both separation and temporal evidence (or an equivalent falsifiable call-count boundary). Preserve bounded file-size/duration/path validation, process timeout, feature-cache schema/version integrity and fallback behavior.

Run focused Python tests, pinned Ruff lint/format, exact 100% owned analysis-engine coverage and full ./scripts/harness/quickcheck.sh; report exact commands/results and successor head. Do not touch central workflows, #828/#811/#897, do not force-push/rebase, weaken gates, or self-approve. Resolve only PRRT_kwDORjvEXs6dNjJ3 after successor exact-head GREEN proves both no repeated cache-hit decode and no duplicate fresh-path decode.

devin-ai-integration[bot]

This comment was marked as resolved.

github-code-quality[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.

coderabbitai[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

Fresh ownership handoff from #1049 review, verified against current #1047 head 11b88488305a28ca2a1e5f3a61ae119f45c23b2a and protected develop@749511c3ad4000090048718f685c6bee6b3d2c25.

The shared feature-cache path is still coupled to the analysis-result schema: _feature_cache_paths() derives its base from _analysis_cache_path(). That means an ANALYSIS_CACHE_SCHEMA_VERSION bump changes the digest/directory for stem arrays even when FEATURE_CACHE_SCHEMA_VERSION and the separator payload contract are unchanged. #1049 hit this as a current-head review finding after its result-schema bump; #1047 is the earlier active owner because it already owns the shared temporal/feature-cache path and is currently carrying FEATURE_CACHE_SCHEMA_VERSION plus beat-grid persistence.

Please keep this repair in #1047 rather than duplicating it in #1049. Smallest regression: prove a feature-cache path is governed only by FEATURE_CACHE_SCHEMA_VERSION/source identity and does not change when only ANALYSIS_CACHE_SCHEMA_VERSION changes, while the analysis-result path does change. Narrow fix: give feature-cache metadata/arrays their own versioned digest/path instead of deriving them from _analysis_cache_path; preserve current app-owned cacheRoot, source/project identity, fail-closed metadata validation, and existing temporal feature-cache behavior. Focused GREEN should include feature-cache store/load and the temporal no-redecode contracts. After this dependency-root repair is integrated, #1049 should be deliberately restacked/revalidated; predecessor checks/reviews must not transfer.

Copy link
Copy Markdown
Collaborator Author

Ownership correction after a fresh whole-stack pass: do not implement the shared feature-cache schema decoupling in #1047. #1045 is the earlier canonical BandScope owner and its current exact head 284d0a39bd0fe2483eaf2d0d43774eff5a44081e already contains the RED regression (test(cache): preserve stems across result schema bumps) while its current production _feature_cache_paths() remains coupled to _analysis_cache_path(). I have routed the causal fix there against its exact api.py blob.

Keep #1047 scoped to ritardando/temporal behavior and, once #1045 lands the cache repair, deliberately restack/revalidate this branch on that dependency-root result. Do not copy the cache fix here and do not transfer #1045 predecessor checks/reviews.

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.

@seonghobae

Copy link
Copy Markdown
Collaborator Author

Current HEAD \ is pushed. The ritardando fix now preserves precise detected change timing and fractional section boundaries across Python, shared-types, Rust, and desktop; local Python verification is 703 passed / 24 skipped with 100% statement, branch, function, and line coverage. Please rerun hosted checks on this exact HEAD.

@seonghobae

Copy link
Copy Markdown
Collaborator Author

Current HEAD 8c846ad0b9a95dbf451ad78a27d67a0e0e903984 is pushed. The ritardando fix preserves precise detected change timing and fractional section boundaries across Python, shared-types, Rust, and desktop. Local Python verification: 703 passed, 24 skipped; 100% statement, branch, function, and line coverage. Please rerun hosted checks on this exact HEAD.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Collaborator Author

Fixed the valid CodeRabbit finding in current HEAD 76caa7752dbf9ac71d71a447d27013f0f773c291: native project validation now enforces the model ritardando template and tempo semantics while retaining arbitrary user copy. cargo fmt --check, 19 Rust core tests, and 8 ritardando contract tests pass. Please rerun hosted checks on this exact HEAD.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Collaborator Author

Fixed the two valid Devin findings in current HEAD 8fc1a19: optional tempo now round-trips through the native analysis-status/project contract, and shared/native model ritardando validation is aligned so save/load cannot disagree. Shared targeted tests, desktop 250-test 100% coverage, Rust 21+8 tests, and Python 703 passed / 24 skipped remain green. Please rerun hosted checks on this exact HEAD.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Collaborator Author

Fixed the remaining valid Devin finding in current HEAD f2879ed: explicit tempo null is rejected while omitted tempo remains valid. Added missing/null/zero/negative/string tempo coverage; Rust core now has 22 passing tests and the ritardando contract 8. Please rerun hosted checks on this exact HEAD.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Devin Review

Comment thread apps/desktop/core/src/lib.rs
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