feat: implement role extraction target and part graph (closes #34)#86
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough섹션별 역할 추출 파이프라인이 추가되었습니다. RoleExtractor가 입력 섹션에서 Changes
Sequence Diagram(s)sequenceDiagram
participant API as API 레이어
participant Extractor as RoleExtractor
participant Model as Role 모델/타입
participant Result as RoleExtractionResult
API->>Extractor: extract([sections])
activate Extractor
Extractor->>Model: 생성 요청: RehearsalRole, PartGraphNode
Model-->>Extractor: 타입/구조 반환
Extractor->>Result: 조립: SectionRoleTopology 목록 생성
deactivate Extractor
Result-->>API: RoleExtractionResult (topologies, notes)
API->>API: topologies[0].active_roles 및 part_graph를 섹션 페이로드에 주입
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/analysis-engine/src/bandscope_analysis/api.py (1)
215-234:⚠️ Potential issue | 🟡 Minor타입 주석 오류:
RehearsalRole.manualOverrides의 타입 선언이 실제 데이터 구조와 불일치합니다.
extractor.py에서 생성하는manualOverrides는{"field": str, "value": HarmonyPayload, "source": str}구조입니다. 하지만model.py의RehearsalRoleTypedDict에서는manualOverrides: list[ManualOverride]로 선언하고 있으며,ManualOverride는field,previousValue,reason필드를 가집니다.
model.py의RehearsalRole.manualOverrides타입을list[ManualOverride]에서 올바른 구조로 수정하거나,extractor.py의 구조를ManualOverride정의에 맞게 변경해야 합니다. 현재는api.py에 정의된ManualOverridePayload가 실제 데이터 구조와 일치합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/analysis-engine/src/bandscope_analysis/api.py` around lines 215 - 234, The TypedDict definitions in model.py are mismatched with the payload produced by extractor.py: update RehearsalRole.manualOverrides to match the actual payload shape (replace list[ManualOverride] with list[ManualOverridePayload] or redefine ManualOverride to have keys "field": str, "value": HarmonyPayload, "source": str) so that the RehearsalRole TypedDict, the ManualOverride (or ManualOverridePayload) type, and any references to manualOverrides across api.py and extractor.py are consistent; adjust the ManualOverride type name or contents in model.py to exactly mirror the structure returned by RoleExtractor (the ManualOverridePayload used in api.py) and update any imports/usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/analysis-engine/src/bandscope_analysis/roles/extractor.py`:
- Around line 28-41: The extract method declares an audio_features parameter but
never uses it; either remove the unused audio_features parameter from the
extract(signature in class BandscopeRoleExtractor or whichever class implements
extract) and update its docstring and callers to stop passing it, or keep it for
future use but mark it as intentionally unused (rename to _audio_features or add
a brief docstring note) and update RoleExtractionResult-related logic if audio
features should influence outputs; ensure any changes are reflected in the
extract method signature, its docstring, and any call sites that construct
RoleExtractionResult so there are no unused-parameter warnings.
- Around line 114-124: The manualOverrides object produced in extractor.py does
not match the ManualOverride TypedDict in model.py: extractor.py is emitting
keys "value" and "source" but the model expects "previousValue" and "reason";
update the code that constructs manualOverrides (look for the dictionary literal
under "manualOverrides" or the function that builds overrides in extractor.py)
to use the model's field names ("previousValue" and "reason") and ensure their
types/structures match ManualOverride in model.py, or alternatively update the
ManualOverride TypedDict in model.py if you intend to keep the current
shape—make the change consistently so keys and types align between extractor.py
and ManualOverride.
In `@services/analysis-engine/src/bandscope_analysis/roles/model.py`:
- Around line 63-68: The ManualOverride TypedDict in roles.model does not match
how overrides are constructed in extractor.py (which creates entries with keys
value: dict and source: str); update the ManualOverride definition (class
ManualOverride) to match extractor.py by replacing previousValue: str and
reason: str with value: dict (or the appropriate mapping type) and source: str,
and adjust the docstring to reflect the new fields so the model and extractor
agree (leave field: str as-is).
- Around line 71-84: The RehearsalRole TypedDict currently uses plain str for
enum-like fields (e.g., roleType, rehearsalPriority, simplification) which loses
type safety; change those annotations to Literal[...] with the allowed string
values (or reference existing enum types) and import Literal from typing (or
typing_extensions) so the type checker can validate values; update RehearsalRole
to use Literal for roleType, rehearsalPriority and any other comment-marked enum
fields and adjust any places that construct RehearsalRole to use the literal
values or the enum members.
In `@services/analysis-engine/tests/test_roles.py`:
- Around line 3-10: Remove the unused pytest import and reorder the import block
so it is alphabetically/sorted and grouped correctly: delete the top-level
"import pytest" since it causes F401 and ensure the from-imports for RoleType,
RehearsalPriority, CueAnchorKind (from bandscope_analysis.roles.model) and
RoleExtractor (from bandscope_analysis.roles.extractor) are sorted and grouped
consistently with project import ordering rules to fix the I001 lint failure.
---
Outside diff comments:
In `@services/analysis-engine/src/bandscope_analysis/api.py`:
- Around line 215-234: The TypedDict definitions in model.py are mismatched with
the payload produced by extractor.py: update RehearsalRole.manualOverrides to
match the actual payload shape (replace list[ManualOverride] with
list[ManualOverridePayload] or redefine ManualOverride to have keys "field":
str, "value": HarmonyPayload, "source": str) so that the RehearsalRole
TypedDict, the ManualOverride (or ManualOverridePayload) type, and any
references to manualOverrides across api.py and extractor.py are consistent;
adjust the ManualOverride type name or contents in model.py to exactly mirror
the structure returned by RoleExtractor (the ManualOverridePayload used in
api.py) and update any imports/usages accordingly.
🪄 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: 9c7eef44-b5b9-46cd-9995-d7dc9fb1a822
📒 Files selected for processing (5)
services/analysis-engine/src/bandscope_analysis/api.pyservices/analysis-engine/src/bandscope_analysis/roles/__init__.pyservices/analysis-engine/src/bandscope_analysis/roles/extractor.pyservices/analysis-engine/src/bandscope_analysis/roles/model.pyservices/analysis-engine/tests/test_roles.py
- Renamed audio_features to _audio_features to indicate intentional unuse. - Used Enum references directly instead of string literals (.value) in RehearsalRole dict assignments to satisfy structural typing since Python 3.12 Enums serialize nicely to JSON. - Updated TypedDict definitions in model.py to use the actual Enum types instead of string, for better type safety.
Owner
Author
|
@coderabbitai resolve |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/analysis-engine/src/bandscope_analysis/api.py (1)
216-235:⚠️ Potential issue | 🟠 Major
build_demo_rehearsal_song에서 role graph를 버려 topology 재사용성이 끊깁니다.Line 219에서 topology를 가져오지만, 반환 payload에
part_graph를 싣지 않아 handoff/활성상태 정보가 소실됩니다. PR 목표(그래프 재사용 가능 출력)와 맞지 않습니다.수정 제안 diff
+class PartGraphNodePayload(TypedDict): + """Typed part-graph node payload nested inside sections.""" + + role_id: str + is_active: bool + handoff_to: list[str] + handoff_from: list[str] + + class RehearsalSectionPayload(TypedDict): @@ confidence: ConfidencePayload roles: list[RehearsalRolePayload] + partGraph: list[PartGraphNodePayload] @@ - verse_roles = role_result["topologies"][0]["active_roles"] + verse_topology = role_result["topologies"][0] + verse_roles = verse_topology["active_roles"] @@ - "roles": cast(list[RehearsalRolePayload], verse_roles), + "roles": cast(list[RehearsalRolePayload], verse_roles), + "partGraph": verse_topology["part_graph"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/analysis-engine/src/bandscope_analysis/api.py` around lines 216 - 235, The return payload in build_demo_rehearsal_song currently drops the role graph/topology (you extract role_result and verse_roles) which loses handoff/active-state info; modify the returned section to include the original topology/part_graph from role_result (e.g., add a "part_graph" or "topology" field populated from role_result["topologies"][0] or role_result["part_graph"] alongside "roles") so the graph is preserved for reuse by downstream consumers; ensure the field name matches the RehearsalRolePayload consumers expect and keep the cast for verse_roles unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/analysis-engine/src/bandscope_analysis/roles/extractor.py`:
- Around line 45-47: The loop using "for i, section in enumerate(sections)"
assumes each section is a dict and calls section.get(...), which will raise
AttributeError for non-dict inputs; update the input validation by first
validating that "sections" is an iterable/list and then inside the loop ensure
each "section" is a dict (use isinstance(section, dict)); if a section is
invalid, either skip it with a logged warning or raise a clear TypeError with
context including the index i; finally, construct "section_id" only after
validation (i.e., keep the existing "section_id = section.get('id',
f'section-{i}')" but only after confirming section is a dict) so untrusted input
cannot cause a runtime AttributeError.
- Around line 127-153: The topology omits inactive role nodes for non-first
sections, so modify the part_graph construction in roles/extractor.py: always
append nodes for "keys-right" and "lead-vocal" (using the same structure as the
active case) but set "is_active" to True only when i == 0 and False otherwise;
keep their "handoff_to" and "handoff_from" lists and only add the handoff link
(part_graph[0]["handoff_to"].append("lead-vocal") and
part_graph[X]["handoff_from"].append("bass-guitar")) when appropriate for the
active case. Also ensure active_roles is only extended with keys_role and
vocal_role when i == 0 so inactive nodes remain present in part_graph but are
not added to active_roles.
---
Outside diff comments:
In `@services/analysis-engine/src/bandscope_analysis/api.py`:
- Around line 216-235: The return payload in build_demo_rehearsal_song currently
drops the role graph/topology (you extract role_result and verse_roles) which
loses handoff/active-state info; modify the returned section to include the
original topology/part_graph from role_result (e.g., add a "part_graph" or
"topology" field populated from role_result["topologies"][0] or
role_result["part_graph"] alongside "roles") so the graph is preserved for reuse
by downstream consumers; ensure the field name matches the RehearsalRolePayload
consumers expect and keep the cast for verse_roles unchanged.
🪄 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: 41f306de-9232-4dbd-9720-24cfe3a78e54
📒 Files selected for processing (4)
services/analysis-engine/src/bandscope_analysis/api.pyservices/analysis-engine/src/bandscope_analysis/roles/extractor.pyservices/analysis-engine/src/bandscope_analysis/roles/model.pyservices/analysis-engine/tests/test_roles.py
- Add PartGraphNodePayload to RehearsalSectionPayload in api.py - Add section dict type validation to RoleExtractor - Include inactive roles in part_graph for all sections - Add tests for new behaviors
Owner
Author
|
@coderabbitai resolve |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ Actions performedComments resolved and changes approved. |
✅ Actions performedReview triggered.
|
✅ Actions performedComments resolved and changes approved. |
This was referenced Mar 26, 2026
This was referenced Mar 28, 2026
This was referenced Apr 25, 2026
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.
Resolves #34 by implementing the Python domain models for Role extraction (bass, guitar, vocals, etc.), active/inactive tracking, and a part graph with
handoff_to/handoff_from. Includes 100% test coverage.📝 Walkthrough
Walkthrough
섹션별 역할 추출 파이프라인을 추가했습니다. 하드코딩된 데모 역할을 제거하고 새로 도입한 RoleExtractor가 섹션을 받아 RehearsalRole과 PartGraph를 생성하여 API에서 재사용하도록 변경했습니다.
Changes
services/analysis-engine/src/bandscope_analysis/roles/model.pyRehearsalRole,PartGraphNode,SectionRoleTopology,RoleExtractionResult등 TypedDict 기반 도메인 스키마를 추가했습니다.services/analysis-engine/src/bandscope_analysis/roles/extractor.pyRoleExtractor클래스와extract(sections, audio_features=None)구현을 추가했습니다. 섹션별로 고정된 목업 역할을 생성하고 첫 섹션에만 일부 역할을 활성화하며 파트 그래프의 handoff 관계를 구성합니다.services/analysis-engine/src/bandscope_analysis/roles/__init__.pyRoleExtractor를 패키지 레벨에서 임포트·노출하도록__all__을 정의했습니다.services/analysis-engine/src/bandscope_analysis/api.pybuild_demo_rehearsal_song()에서 기존 하드코딩된roles목록을 제거하고RoleExtractor().extract([verse_section])로 대체하여 반환되는topologies[0].active_roles를 섹션에 포함하도록 변경했습니다.services/analysis-engine/tests/test_roles.pyRoleExtractor동작(빈 입력, 다중 섹션, 활성 역할 및 handoff 관계 검증)을 추가했습니다.Sequence Diagram
sequenceDiagram participant API as API Layer participant Extractor as RoleExtractor participant Model as Role Models participant Result as RoleExtractionResult API->>Extractor: extract([sections]) activate Extractor Extractor->>Model: create RehearsalRole objects (bass, keys, vocal) Model-->>Extractor: RehearsalRole objects Extractor->>Model: build PartGraphNode(s) and handoff links Model-->>Extractor: PartGraphNode objects Extractor->>Result: assemble RoleExtractionResult(topologies[], notes) deactivate Extractor Result-->>API: return RoleExtractionResult API->>API: inject topologies[0].active_roles into demo section payloadEstimated Code Review Effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
build_demo_rehearsal_song()엔트리포인트를 수정하여 데모 섹션/역할 처리 방식을 변경하는 PR로 코드 수준의 연관성이 큽니다.Poem