diff --git a/.jules/bolt.md b/.jules/bolt.md index d54cf10fc..e81b93c6d 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -61,3 +61,6 @@ ## 2026-07-13 - Array.from mapping optimization **Learning:** Using `Array.from({ length: N }).map(...)` creates an intermediate array of `undefined` values which requires memory allocation and garbage collection, adding O(N) unnecessary overhead in frequently re-rendered UI components. **Action:** Use `Array.from({ length: N }, (_, index) => ...)` to map elements directly during array creation, avoiding intermediate allocations. +## 2026-03-06 - [파이썬 O(N^2) 리스트 룩업을 O(1) 딕셔너리로 최적화] +**Learning:** `chart.py`의 텍스트 변환 로직에서 `not in list`로 중복을 방지하며 삽입하는 방식은 리스트 크기가 커질 때 O(N^2) 병목을 유발합니다. 파이썬 3.7+부터 딕셔너리가 삽입 순서를 유지하므로, `ordered_role_ids[role_id] = None`처럼 의미가 드러나는 키 저장소를 사용하면 순서를 보존하면서 평균 O(1) 조회가 가능합니다. +**Action:** 순서 보존 중복 제거가 필요한 경로에서는 도메인 이름을 가진 딕셔너리 키를 사용하고, 외부 문자열은 해시·truthiness 연산 전에 안전한 built-in 문자열로 정규화합니다. diff --git a/CHANGELOG.md b/CHANGELOG.md index 34331fb86..224b824fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ ### Changed +- Changed chart-export role, cue, and priority de-duplication to semantically named insertion-ordered dictionaries, preserving first-occurrence output while replacing repeated linear membership scans with average constant-time key lookups. - Consolidated Bandit, dependency audits, supplemental secret checks, and Trivy into one trusted-branch security backstop, delegated CodeQL to GitHub default setup, and removed duplicate local PR security and release-preflight runs. - Pinned npm `10.9.9` as the approved lockfile generator, activated it through Node-bundled Corepack before dependency consumption, and fail closed unless its bundled `tar` is at least `7.5.19`; primary CI still consumes the committed lock only through frozen `npm ci` validation, rejects mutable npm resolution in the lock gate, requires integrity evidence for public-registry lock entries, and preserves generator-sensitive root `@esbuild/*` peer metadata. diff --git a/services/analysis-engine/src/bandscope_analysis/exports/chart.py b/services/analysis-engine/src/bandscope_analysis/exports/chart.py index 3a84b59c8..c6a10e8cb 100644 --- a/services/analysis-engine/src/bandscope_analysis/exports/chart.py +++ b/services/analysis-engine/src/bandscope_analysis/exports/chart.py @@ -7,7 +7,7 @@ Security Notes: - Pure dict-to-string transformation: no file, network, or process I/O. - Never reads source-path fields and never emits filesystem paths. - - Safe failure: ``None``, empty, or malformed input yields ``""`` / ``[]``; + - Safe failure: ``None``, empty, or malformed input yields ``\"\"`` / ``[]``; missing or malformed keys are skipped and no exceptions escape. """ @@ -73,22 +73,34 @@ def _section_roles(section: Mapping[str, object]) -> list[Mapping[str, object]]: return [role for role in roles if isinstance(role, Mapping)] -def _active_role_ids(section: Mapping[str, object]) -> list[str] | None: +def _hashable_text(raw_text_value: object) -> str | None: + """Return compatible string-like text as a safe built-in mapping key.""" + if not isinstance(raw_text_value, str): + return None + try: + hash(raw_text_value) + normalized_text = str.__str__(raw_text_value) + except Exception: + return None + return normalized_text if normalized_text else None + + +def _active_role_ids(section_payload: Mapping[str, object]) -> list[str] | None: """Return active role ids from the part graph, or ``None`` when absent.""" - part_graph = section.get("partGraph") + part_graph = section_payload.get("partGraph") if not isinstance(part_graph, list): return None - active: list[str] = [] - for node in part_graph: - if not isinstance(node, Mapping) or node.get("is_active") is not True: + active_role_ids_by_id: dict[str, None] = {} + for part_graph_node in part_graph: + if not isinstance(part_graph_node, Mapping) or part_graph_node.get("is_active") is not True: continue - role_id = node.get("role_id") - if isinstance(role_id, str) and role_id and role_id not in active: - active.append(role_id) - return active + role_id = _hashable_text(part_graph_node.get("role_id")) + if role_id is not None: + active_role_ids_by_id[role_id] = None + return list(active_role_ids_by_id) -def _active_roles(section: Mapping[str, object]) -> list[Mapping[str, object]]: +def _active_roles(section_payload: Mapping[str, object]) -> list[Mapping[str, object]]: """Return the section's active role payloads. Activity is derived from the part graph's ``is_active`` flags; when the @@ -96,50 +108,50 @@ def _active_roles(section: Mapping[str, object]) -> list[Mapping[str, object]]: graph nodes without a matching role payload keep their ``role_id`` as a display name. """ - roles = _section_roles(section) - active_ids = _active_role_ids(section) - if active_ids is None: - return roles - by_id: dict[str, Mapping[str, object]] = {} - for role in roles: - role_id = role.get("id") - if isinstance(role_id, str) and role_id not in by_id: - by_id[role_id] = role - return [by_id.get(role_id, {"id": role_id, "name": role_id}) for role_id in active_ids] - - -def _role_display_name(role: Mapping[str, object]) -> str | None: - """Return the role's display name, falling back to its id.""" - name = role.get("name") - if isinstance(name, str) and name: - return name - role_id = role.get("id") - if isinstance(role_id, str) and role_id: - return role_id - return None + section_role_payloads = _section_roles(section_payload) + active_role_ids = _active_role_ids(section_payload) + if active_role_ids is None: + return section_role_payloads + role_payload_by_id: dict[str, Mapping[str, object]] = {} + for role_payload in section_role_payloads: + role_id = _hashable_text(role_payload.get("id")) + if role_id is not None and role_id not in role_payload_by_id: + role_payload_by_id[role_id] = role_payload + return [ + role_payload_by_id.get(role_id, {"id": role_id, "name": role_id}) + for role_id in active_role_ids + ] -def _active_role_names(section: Mapping[str, object]) -> list[str]: +def _role_display_name(role_payload: Mapping[str, object]) -> str | None: + """Return a hashable display name, falling back to a hashable role id.""" + display_name = _hashable_text(role_payload.get("name")) + if display_name is not None: + return display_name + return _hashable_text(role_payload.get("id")) + + +def _active_role_names(section_payload: Mapping[str, object]) -> list[str]: """Return de-duplicated display names for the section's active roles.""" - names: list[str] = [] - for role in _active_roles(section): - name = _role_display_name(role) - if name is not None and name not in names: - names.append(name) - return names + active_role_names_by_name: dict[str, None] = {} + for role_payload in _active_roles(section_payload): + display_name = _role_display_name(role_payload) + if display_name is not None: + active_role_names_by_name[display_name] = None + return list(active_role_names_by_name) -def _section_cue(section: Mapping[str, object]) -> str: +def _section_cue(section_payload: Mapping[str, object]) -> str: """Join the active roles' cue values into a single cue string.""" - cues: list[str] = [] - for role in _active_roles(section): - cue = role.get("cue") - if not isinstance(cue, Mapping): + active_cue_values: dict[str, None] = {} + for role_payload in _active_roles(section_payload): + cue_payload = role_payload.get("cue") + if not isinstance(cue_payload, Mapping): continue - value = cue.get("value") - if isinstance(value, str) and value and value not in cues: - cues.append(value) - return "; ".join(cues) + cue_value = _hashable_text(cue_payload.get("value")) + if cue_value is not None: + active_cue_values[cue_value] = None + return "; ".join(active_cue_values) def _confidence_level(section: Mapping[str, object]) -> str | None: @@ -185,28 +197,30 @@ def _section_lines(sections: list[Mapping[str, object]]) -> list[str]: return lines -def _footer_lines(song: Mapping[str, object], sections: list[Mapping[str, object]]) -> list[str]: +def _footer_lines( + song_payload: Mapping[str, object], + section_payloads: list[Mapping[str, object]], +) -> list[str]: """Build the footer: per-role rehearsal priorities and the export focus.""" - lines: list[str] = [] - priorities: list[str] = [] - for section in sections: - for role in _section_roles(section): - name = _role_display_name(role) - priority = role.get("rehearsalPriority") - if name is None or not isinstance(priority, str) or not priority: + footer_lines: list[str] = [] + rehearsal_priority_lines: dict[str, None] = {} + for section_payload in section_payloads: + for role_payload in _section_roles(section_payload): + display_name = _role_display_name(role_payload) + rehearsal_priority = _hashable_text(role_payload.get("rehearsalPriority")) + if display_name is None or rehearsal_priority is None: continue - entry = f" - {name}: {priority}" - if entry not in priorities: - priorities.append(entry) - if priorities: - lines.append("Priorities:") - lines.extend(priorities) - summary = song.get("exportSummary") - if isinstance(summary, Mapping): - headline = summary.get("headline") - if isinstance(headline, str) and headline: - lines.append(f"Focus: {headline}") - return lines + priority_line = f" - {display_name}: {rehearsal_priority}" + rehearsal_priority_lines[priority_line] = None + if rehearsal_priority_lines: + footer_lines.append("Priorities:") + footer_lines.extend(rehearsal_priority_lines) + export_summary = song_payload.get("exportSummary") + if isinstance(export_summary, Mapping): + focus_headline = export_summary.get("headline") + if isinstance(focus_headline, str) and focus_headline: + footer_lines.append(f"Focus: {focus_headline}") + return footer_lines def build_chart_text(song: Mapping[str, object] | None) -> str: @@ -216,7 +230,7 @@ def build_chart_text(song: Mapping[str, object] | None) -> str: section (``[mm:ss-mm:ss] LABEL (confidence) roles: ...``), and a footer with rehearsal priorities and the export focus headline. Output is deterministic and never contains filesystem paths. Malformed input - yields ``""``. + yields ``\"\"``. """ if not isinstance(song, Mapping): return "" diff --git a/services/analysis-engine/tests/test_chart_export_dedup.py b/services/analysis-engine/tests/test_chart_export_dedup.py new file mode 100644 index 000000000..0e35b0ca0 --- /dev/null +++ b/services/analysis-engine/tests/test_chart_export_dedup.py @@ -0,0 +1,208 @@ +"""Regression tests for order-preserving chart export de-duplication.""" + +from typing import Any + +from bandscope_analysis.exports import build_chart_text, build_cue_sheet_rows + + +class _UnhashableText(str): + """String-like malformed payload value that cannot be a mapping key.""" + + __hash__: Any = None + + +class _HashableText(str): + """Compatible string subclass that remains safe as a mapping key.""" + + +class _ExplodingTruthText(str): + """Hashable string-like payload whose custom truth check must never run.""" + + def __bool__(self) -> bool: + """Raise if production accidentally delegates truthiness to the subclass.""" + raise TypeError("subclass truthiness must not execute") + + +def _role(role_id: str, name: str, cue: str, priority: str = "") -> dict[str, Any]: + """Build the minimal role evidence consumed by the chart export boundary.""" + return { + "id": role_id, + "name": name, + "cue": {"kind": "entrance", "value": cue}, + "rehearsalPriority": priority, + } + + +def _section( + section_id: str, + label: str, + start: int, + end: int, + roles: list[dict[str, Any]], +) -> dict[str, Any]: + """Build a valid section whose part graph activates roles in list order.""" + part_graph = [{"role_id": role["id"], "is_active": True} for role in roles] + return { + "id": section_id, + "label": label, + "timeRange": {"start": start, "end": end}, + "roles": roles, + "partGraph": part_graph, + } + + +def test_duplicate_display_names_and_cues_keep_first_occurrence_order() -> None: + """Distinct role ids may share display/cue text without duplicating export output.""" + section = _section( + "verse", + "verse", + 0, + 16, + [ + _role("guitar-left", "Guitar", "Count in"), + _role("guitar-right", "Guitar", "Count in"), + _role("bass", "Bass", "Hold root"), + _role("guitar-double", "Guitar", "Count in"), + ], + ) + + rows = build_cue_sheet_rows({"sections": [section]}) + + assert rows == [ + { + "section": "verse", + "start": "00:00", + "end": "00:16", + "cue": "Count in; Hold root", + "roles": ["Guitar", "Bass"], + } + ] + + +def test_duplicate_priorities_across_sections_keep_first_occurrence_order() -> None: + """Repeated name/priority entries collapse once without reordering later entries.""" + song: dict[str, Any] = { + "title": "Order regression", + "sections": [ + _section( + "verse", + "verse", + 0, + 16, + [ + _role("guitar", "Guitar", "Count in", "Lock chorus"), + _role("bass", "Bass", "Hold root", "Watch cutoff"), + ], + ), + _section( + "chorus", + "chorus", + 16, + 32, + [ + _role("guitar-2", "Guitar", "Count in", "Lock chorus"), + _role("bass-2", "Bass", "Hold root", "Watch cutoff"), + ], + ), + ], + } + + text = build_chart_text(song) + priority_lines = text.split("Priorities:\n", maxsplit=1)[1].splitlines() + + assert priority_lines == [ + " - Guitar: Lock chorus", + " - Bass: Watch cutoff", + ] + + +def test_unhashable_string_subclasses_fail_closed_in_public_exports() -> None: + """Malformed unhashable text is skipped while a valid role id remains usable.""" + section = _section( + "verse", + "verse", + 0, + 16, + [ + _role(_UnhashableText("bad-id"), "Bad id", "Bad id cue"), + _role("guitar", _UnhashableText("Guitar"), _UnhashableText("Count in")), + _role("bass", "Bass", "Hold root"), + ], + ) + song = {"sections": [section]} + + assert build_cue_sheet_rows(song) == [ + { + "section": "verse", + "start": "00:00", + "end": "00:16", + "cue": "Hold root", + "roles": ["guitar", "Bass"], + } + ] + assert "roles: guitar, Bass" in build_chart_text(song) + + +def test_hashable_string_subclasses_remain_compatible_export_values() -> None: + """Hashable string subclasses retain pre-optimization role and cue semantics.""" + section = _section( + "verse", + "verse", + 0, + 16, + [ + _role(_HashableText("guitar"), _HashableText("Guitar"), _HashableText("Count in")), + _role("bass", "Bass", "Hold root"), + ], + ) + + assert build_cue_sheet_rows({"sections": [section]}) == [ + { + "section": "verse", + "start": "00:00", + "end": "00:16", + "cue": "Count in; Hold root", + "roles": ["Guitar", "Bass"], + } + ] + + +def test_string_subclass_truthiness_cannot_abort_public_exports() -> None: + """Hashable text is normalized without invoking subclass-defined truthiness.""" + section = _section( + "verse", + "verse", + 0, + 16, + [ + _role("guitar", _ExplodingTruthText("Guitar"), _ExplodingTruthText("Count in")), + _role("bass", "Bass", "Hold root"), + ], + ) + song = {"sections": [section]} + + assert build_cue_sheet_rows(song) == [ + { + "section": "verse", + "start": "00:00", + "end": "00:16", + "cue": "Count in; Hold root", + "roles": ["Guitar", "Bass"], + } + ] + assert "roles: Guitar, Bass" in build_chart_text(song) + + +def test_priority_truthiness_cannot_abort_chart_export() -> None: + """Rehearsal priority text is normalized before footer truthiness checks.""" + section = _section( + "verse", + "verse", + 0, + 16, + [_role("guitar", "Guitar", "Count in", _ExplodingTruthText("Lock chorus"))], + ) + + text = build_chart_text({"sections": [section]}) + + assert " - Guitar: Lock chorus" in text diff --git a/services/analysis-engine/tests/test_chart_export_dedup_contract.py b/services/analysis-engine/tests/test_chart_export_dedup_contract.py new file mode 100644 index 000000000..19c21dc37 --- /dev/null +++ b/services/analysis-engine/tests/test_chart_export_dedup_contract.py @@ -0,0 +1,148 @@ +"""Regression contract for ordered chart-export de-duplication.""" + +import ast +import inspect +from typing import Any + +from bandscope_analysis.exports import build_chart_text, build_cue_sheet_rows +from bandscope_analysis.exports import chart as chart_export + + +def test_deduplication_helpers_use_chart_domain_identifiers() -> None: + """Private de-duplication code must name the rehearsal concept it carries.""" + chart_syntax = ast.parse(inspect.getsource(chart_export)) + deduplication_helpers = { + "_hashable_text", + "_active_role_ids", + "_active_roles", + "_role_display_name", + "_active_role_names", + "_section_cue", + "_footer_lines", + } + ambiguous_identifiers = { + "active", + "cue", + "cues", + "entry", + "headline", + "lines", + "name", + "names", + "node", + "priorities", + "priority", + "role", + "roles", + "section", + "sections", + "song", + "summary", + "text", + "value", + } + violations: set[tuple[str, str]] = set() + + for syntax_node in chart_syntax.body: + if ( + not isinstance(syntax_node, ast.FunctionDef) + or syntax_node.name not in deduplication_helpers + ): + continue + helper_identifiers = { + child_node.id + for child_node in ast.walk(syntax_node) + if isinstance(child_node, ast.Name) + } + helper_identifiers.update(argument.arg for argument in syntax_node.args.args) + violations.update( + (syntax_node.name, identifier) + for identifier in helper_identifiers & ambiguous_identifiers + ) + + assert not violations, f"ambiguous chart-export identifiers: {sorted(violations)}" + + +def _role(role_id: str, name: str, cue: str, priority: str) -> dict[str, Any]: + """Build the minimum role shape consumed by the chart exporter.""" + return { + "id": role_id, + "name": name, + "cue": {"kind": "entrance", "value": cue}, + "rehearsalPriority": priority, + } + + +def _song() -> dict[str, Any]: + """Build ordered duplicate values that must keep first-occurrence order.""" + return { + "title": "Ordered Dedup Contract", + "sections": [ + { + "id": "section-1", + "label": "verse", + "timeRange": {"start": 0, "end": 16}, + "roles": [ + _role("bass-main", "Bass", "Walk up", "high"), + _role("drums", "Drums", "Hit on 1", "medium"), + _role("bass-copy", "Bass", "Walk up", "high"), + ], + "partGraph": [ + {"role_id": "bass-main", "is_active": True}, + {"role_id": "drums", "is_active": True}, + {"role_id": "bass-main", "is_active": True}, + {"role_id": "bass-copy", "is_active": True}, + ], + } + ], + } + + +def test_ordered_deduplication_preserves_first_occurrence_semantics() -> None: + """Duplicate ids and display values collapse without reordering the chart.""" + rows = build_cue_sheet_rows(_song()) + assert rows == [ + { + "section": "verse", + "start": "00:00", + "end": "00:16", + "cue": "Walk up; Hit on 1", + "roles": ["Bass", "Drums"], + } + ] + + text = build_chart_text(_song()) + priority_lines = [line for line in text.splitlines() if line.startswith(" - ")] + assert priority_lines == [" - Bass: high", " - Drums: medium"] + + +def test_duplicate_role_ids_preserve_first_payload_and_graph_position() -> None: + """Repeated role identities keep the first role payload and one active position.""" + song: dict[str, Any] = { + "sections": [ + { + "id": "section-1", + "label": "verse", + "timeRange": {"start": 0, "end": 16}, + "roles": [ + _role("bass", "Bass", "Walk up", "high"), + _role("bass", "Bass Copy", "Late replacement", "low"), + ], + "partGraph": [ + {"role_id": "bass", "is_active": True}, + {"role_id": "bass", "is_active": True}, + ], + } + ] + } + + rows = build_cue_sheet_rows(song) + assert rows == [ + { + "section": "verse", + "start": "00:00", + "end": "00:16", + "cue": "Walk up", + "roles": ["Bass"], + } + ]