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
@seonghobae seonghobae added enhancement New feature or request priority: high High-priority or P1 work labels Sep 7, 2026 — with ChatGPT Codex Connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority: high High-priority or P1 work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant