From e16782c208320138d77b88f2ff359e3e9e059e76 Mon Sep 17 00:00:00 2001 From: tanaka takashi Date: Sat, 11 Jul 2026 19:57:02 +0900 Subject: fix: レビュー指摘事項の修正（エラー戻り値統一、タイ延長、ネストリピート、専用エラーコード） MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit High severity: - compose action エラー戻り値の validation キー統一（server.py） - ppmck/pyxel タイ & の duration 延長修正（_tie_pending フラグ導入） - pyxel ネストリピート対応（RepeatFrame + イベントスナップショット方式） Medium severity: - 専用エラーコード追加: VALIDATION_MISSING_PARAMETER, VALIDATION_INVALID_MODE, VALIDATION_INVALID_ACTION - SYSTEM_INTERNAL_ERROR キャッチ範囲拡大（compose/validate全体をtry/except） Low severity: - 設計書 line/column を1始まりに明記、APIレベルエラーコード追記 テスト: 129 passed, ruff: All checks passed --- doc/Design_err.md | 19 +++- src/mml_composemusic_mcp/ir.py | 5 + src/mml_composemusic_mcp/parser_ppmck.py | 21 +++- src/mml_composemusic_mcp/parser_pyxel.py | 59 ++++++++--- src/mml_composemusic_mcp/server.py | 170 ++++++++++++++++++------------- tests/test_compose_mml_tool.py | 8 +- tests/test_error_codes.py | 41 +++++++- tests/test_parser_detailed.py | 37 +++++++ 8 files changed, 261 insertions(+), 99 deletions(-) diff --git a/doc/Design_err.md b/doc/Design_err.md index 3718023..d74f3a8 100644 --- a/doc/Design_err.md +++ b/doc/Design_err.md @@ -7,8 +7,8 @@ ErrorDetail に `code`（機械処理用のエラーコード）を追加して ``` { code: string, # エラーコード（機械処理用、LLMがパターン認識可能） - line: int, # 行番号 - column: int, # 列番号 + line: int, # 行番号（1始まり） + column: int, # 列番号（1始まり） message: string, # エラーメッセージ（プレースホルダ展開済み） severity: "error" | "warning", hint: string, # LLM向けの修正ヒント @@ -17,6 +17,7 @@ ErrorDetail に `code`（機械処理用のエラーコード）を追加して ``` > `code` を追加しました。LLMは `code` を見てエラーパターンを分類し、`hint` に従って修正できます。`context` は該当行のテキスト前後を切り出したもので、LLMが位置を特定しやすくなります。 +> **注意**: `line` と `column` は **1始まり** です（最初の行・最初の列が1）。システムエラーやAPIレベルエラーなどで位置情報がない場合は `0` を使用します。 --- @@ -78,6 +79,18 @@ LLMの修正対象外。内部処理の失敗。`hint` は空または「再試 --- +### APIレベルエラー（VALIDATION_*） + +MML構文ではなく、APIパラメータのバリデーションエラー。`line` / `column` は `0` となります。 + +| code | trigger | message テンプレート | hint | severity | +|---|---|---|---|---| +| `VALIDATION_MISSING_PARAMETER` | compose/validateで必須パラメータ(mml, mode)が欠落 | `mml と mode は compose/validate の必須パラメータです。` | `mml に MML 文字列、mode に 'ppmck' または 'pyxel' を指定してください。` | error | +| `VALIDATION_INVALID_MODE` | 未知のmodeが指定された | `未知のモード '{mode}' です。` | `mode は 'ppmck' または 'pyxel' を指定してください。` | error | +| `VALIDATION_INVALID_ACTION` | 未知のactionが指定された | `未知の action '{action}' です。` | `action は 'compose', 'validate', 'template' のいずれかを指定してください。` | error | + +--- + ### エラーレスポンスの実例 #### 構文エラーの例（LLMが修正可能） @@ -147,4 +160,4 @@ compose action で ERROR がある場合: --- -これで、エラーまわりの仕様を規定しました。構文エラーは14種類、システムエラーは3種類。LLMが `code` を見てパターン認識し、`hint` に従って修正できる構造になっています。 +これで、エラーまわりの仕様を規定しました。構文エラーは14種類、システムエラーは3種類、APIレベルエラーは3種類。LLMが `code` を見てパターン認識し、`hint` に従って修正できる構造になっています。 diff --git a/src/mml_composemusic_mcp/ir.py b/src/mml_composemusic_mcp/ir.py index 87385df..42adc13 100644 --- a/src/mml_composemusic_mcp/ir.py +++ b/src/mml_composemusic_mcp/ir.py @@ -27,6 +27,11 @@ class ErrorCode(Enum): SYSTEM_WAV_WRITE_FAILED = "SYSTEM_WAV_WRITE_FAILED" SYSTEM_INTERNAL_ERROR = "SYSTEM_INTERNAL_ERROR" + # API-level errors (parameter validation, not MML syntax) + VALIDATION_MISSING_PARAMETER = "VALIDATION_MISSING_PARAMETER" + VALIDATION_INVALID_MODE = "VALIDATION_INVALID_MODE" + VALIDATION_INVALID_ACTION = "VALIDATION_INVALID_ACTION" + @dataclass class ErrorDetail: diff --git a/src/mml_composemusic_mcp/parser_ppmck.py b/src/mml_composemusic_mcp/parser_ppmck.py index 34a7dd2..db91950 100644 --- a/src/mml_composemusic_mcp/parser_ppmck.py +++ b/src/mml_composemusic_mcp/parser_ppmck.py @@ -41,6 +41,7 @@ class PpmckParser: self.headers: list[tuple[str, str]] = [] self.loop_start: int | None = None self.tracks_seen: set[str] = set() + self._tie_pending: bool = False def _context_line(self, token: Token) -> str: lines = self.source.splitlines() @@ -202,6 +203,13 @@ class PpmckParser: else: break + if self._tie_pending: + # Tie: extend the last note/rest's duration instead of creating a new event + self._extend_last_note(ticks) + self.tick_position += ticks + self._tie_pending = False + return + if self.current_channel == "Noise": self.ctx.add_error( code=ErrorCode.SYNTAX_CHANNEL_MISMATCH, @@ -242,6 +250,11 @@ class PpmckParser: self.ctx.advance() length, dots = self._read_length() ticks = length_value_to_ticks(length, dots) + if self._tie_pending: + self._extend_last_note(ticks) + self.tick_position += ticks + self._tie_pending = False + return event = RestEvent(tick_position=self.tick_position, duration=ticks) self._add_event(event) self.tick_position += ticks @@ -372,11 +385,11 @@ class PpmckParser: self._extend_last_note(ticks) self.tick_position += ticks return - # For note/rest, parser will create next event; we just note that previous - # event should be merged. Simple approach: reduce tick_position and mark. - # Here we just allow the next note to overwrite position and merge later. - # Implementation: back up tick position so next note duration adds to previous. + # For note/rest: back up tick_position so the next event starts at the + # end of the last note/rest. The next note's duration will be added + # to the previous note's duration via _tie_pending flag. self.tick_position = self._last_note_end() + self._tie_pending = True def _last_note_end(self) -> int: ch = self.note_sequence.channels[self.current_channel] # type: ignore[index] diff --git a/src/mml_composemusic_mcp/parser_pyxel.py b/src/mml_composemusic_mcp/parser_pyxel.py index 3bacb5b..336a55e 100644 --- a/src/mml_composemusic_mcp/parser_pyxel.py +++ b/src/mml_composemusic_mcp/parser_pyxel.py @@ -36,6 +36,15 @@ PYXEL_DUTY = { } +class RepeatFrame: + """Stack frame for nested repeat expansion.""" + + def __init__(self, start_tick: int) -> None: + self.start_tick = start_tick + # Snapshot of event indices that existed before the repeat section + self.event_count: int = 0 + + class PyxelParser: def __init__(self, source: str, tokens: list[Token]) -> None: self.source = source @@ -51,7 +60,8 @@ class PyxelParser: self.detune_cents = 0 self.tick_position = 0 self.tracks_seen: set[str] = set() - self.repeat_stack: list[int] = [] + self.repeat_stack: list[RepeatFrame] = [] + self._tie_pending: bool = False def _context_line(self, token: Token) -> str: lines = self.source.splitlines() @@ -71,7 +81,7 @@ class PyxelParser: self._parse_statement() if self.repeat_stack: - for _start_tick in self.repeat_stack: + for _frame in self.repeat_stack: self.ctx.add_error( code=ErrorCode.SYNTAX_UNTERMINATED_REPEAT, line=1, @@ -189,6 +199,13 @@ class PyxelParser: else: break + if self._tie_pending: + # Tie: extend the last note/rest's duration instead of creating a new event + self._extend_last_note(ticks) + self.tick_position += ticks + self._tie_pending = False + return + if self.current_channel == "Noise": midi = note_to_midi(note, self.octave, accidental) else: @@ -221,6 +238,11 @@ class PyxelParser: self.ctx.advance() length, dots = self._read_length() ticks = length_value_to_ticks(length, dots) + if self._tie_pending: + self._extend_last_note(ticks) + self.tick_position += ticks + self._tie_pending = False + return event = RestEvent(tick_position=self.tick_position, duration=ticks) self._add_event(event) self.tick_position += ticks @@ -404,7 +426,11 @@ class PyxelParser: self._extend_last_note(ticks) self.tick_position += ticks return + # For note/rest: back up tick_position so the next event starts at the + # end of the last note/rest. The next note's duration will be added + # to the previous note's duration via _tie_pending flag. self.tick_position = self._last_note_end() + self._tie_pending = True def _last_note_end(self) -> int: ch = self.note_sequence.channels[self.current_channel] # type: ignore[index] @@ -422,7 +448,10 @@ class PyxelParser: def _parse_repeat_start(self) -> None: self.ctx.advance() - self.repeat_stack.append(self.tick_position) + ch = self.note_sequence.channels[self.current_channel] # type: ignore[index] + frame = RepeatFrame(start_tick=self.tick_position) + frame.event_count = len(ch.events) + self.repeat_stack.append(frame) def _parse_repeat_end(self) -> None: token = self.ctx.advance() @@ -437,7 +466,10 @@ class PyxelParser: context=self._context_line(token), ) return - start_tick = self.repeat_stack.pop() + frame = self.repeat_stack.pop() + start_tick = frame.start_tick + ch = self.note_sequence.channels[self.current_channel] # type: ignore[index] + count_str = token.value if count_str == "": count = 0 # infinite @@ -452,24 +484,23 @@ class PyxelParser: ) else: count = int(count_str) - segment = [ - e - for e in self.note_sequence.channels[self.current_channel].events # type: ignore[index] - if start_tick <= e.tick_position < self.tick_position - ] + + # Collect events that belong to this repeat section (created after the frame snapshot) + segment_events = ch.events[frame.event_count :] + segment_length = self.tick_position - start_tick + repeat_count = 2 if count == 0 else count for i in range(1, repeat_count): - offset = i * (self.tick_position - start_tick) - for ev in segment: + offset = i * segment_length + for ev in segment_events: new_event = self._clone_event(ev, ev.tick_position + offset) self._add_event(new_event) if isinstance(new_event, (NoteEvent, RestEvent)): end = new_event.tick_position + new_event.duration if end > self.tick_position: self.tick_position = end - self.note_sequence.channels[self.current_channel].events.sort( # type: ignore[index] - key=lambda e: e.tick_position - ) + + ch.events.sort(key=lambda e: e.tick_position) self._add_event( RepeatEvent( start_tick=start_tick, diff --git a/src/mml_composemusic_mcp/server.py b/src/mml_composemusic_mcp/server.py index 90d4dab..3061e6d 100644 --- a/src/mml_composemusic_mcp/server.py +++ b/src/mml_composemusic_mcp/server.py @@ -35,7 +35,7 @@ def _parse_mml(mml: str, mode: str) -> tuple[dict, list[ErrorDetail]]: else: errors = [ ErrorDetail( - code=ErrorCode.SYNTAX_INVALID_TOKEN, + code=ErrorCode.VALIDATION_INVALID_MODE, line=0, column=0, message=f"未知のモード '{mode}' です。", @@ -70,100 +70,126 @@ def compose_mml( return { "success": False, "valid": False, - "errors": [ - { - "code": ErrorCode.SYNTAX_INVALID_TOKEN.value, - "line": 0, - "column": 0, - "message": "mml と mode は compose/validate の必須パラメータです。", - "severity": "error", - "hint": "mml に MML 文字列、mode に 'ppmck' または 'pyxel' を指定してください。", - } - ], - "warnings": [], + "note_sequence": None, + "validation": { + "errors": [ + { + "code": ErrorCode.VALIDATION_MISSING_PARAMETER.value, + "line": 0, + "column": 0, + "message": "mml と mode は compose/validate の必須パラメータです。", + "severity": "error", + "hint": "mml に MML 文字列、mode に 'ppmck' または 'pyxel' を指定してください。", + } + ], + "warnings": [], + }, } - note_sequence_dict, errors = _parse_mml(mml, mode) - errors_list, warnings_list = _split_errors(errors) - if action == "validate": + try: + note_sequence_dict, errors = _parse_mml(mml, mode) + errors_list, warnings_list = _split_errors(errors) + if action == "validate": + return { + "valid": not _is_error(errors), + "errors": errors_list, + "warnings": warnings_list, + "note_sequence": note_sequence_dict, + "channel_summary": build_channel_summary(note_sequence_dict) + if note_sequence_dict + else [], + } + # compose + if _is_error(errors): + return { + "success": False, + "wav_path": None, + "duration_sec": 0, + "note_sequence": note_sequence_dict, + "validation": {"errors": errors_list, "warnings": warnings_list}, + } + + ns = _dict_to_note_sequence(note_sequence_dict) + try: + wave_data, duration, synth_errors = synthesize( + ns, mode, sample_rate, normalize + ) + except Exception as exc: + return { + "success": False, + "wav_path": None, + "duration_sec": 0, + "note_sequence": note_sequence_dict, + "validation": { + "errors": [ + { + "code": ErrorCode.SYSTEM_SYNTHESIS_FAILED.value, + "line": 0, + "column": 0, + "message": f"音声合成中にエラーが発生しました: {exc}", + "severity": "error", + "hint": "MMLの内容を確認の上、再度お試しください。問題が続く場合は、短いMMLから試してください。", + } + ], + "warnings": warnings_list, + }, + } + errors.extend(synth_errors) + errors_list, warnings_list = _split_errors(errors) + if _is_error(errors): + return { + "success": False, + "wav_path": None, + "duration_sec": 0, + "note_sequence": note_sequence_dict, + "validation": {"errors": errors_list, "warnings": warnings_list}, + } + wav_path = OUTPUT_DIR / "output.wav" + wav_errors = write_wav(wav_path, wave_data, sample_rate) + errors.extend(wav_errors) + errors_list, warnings_list = _split_errors(errors) return { - "valid": not _is_error(errors), - "errors": errors_list, - "warnings": warnings_list, - "note_sequence": note_sequence_dict, - "channel_summary": build_channel_summary(note_sequence_dict) - if note_sequence_dict - else [], - } - # compose - if _is_error(errors): - return { - "success": False, - "wav_path": None, - "duration_sec": 0, + "success": not _is_error(errors), + "wav_path": str(wav_path) if not _is_error(errors) else None, + "duration_sec": duration, "note_sequence": note_sequence_dict, "validation": {"errors": errors_list, "warnings": warnings_list}, } - - ns = _dict_to_note_sequence(note_sequence_dict) - try: - wave_data, duration, synth_errors = synthesize( - ns, mode, sample_rate, normalize - ) except Exception as exc: return { "success": False, "wav_path": None, "duration_sec": 0, - "note_sequence": note_sequence_dict, + "note_sequence": None, "validation": { "errors": [ { - "code": ErrorCode.SYSTEM_SYNTHESIS_FAILED.value, + "code": ErrorCode.SYSTEM_INTERNAL_ERROR.value, "line": 0, "column": 0, - "message": f"音声合成中にエラーが発生しました: {exc}", + "message": f"内部エラーが発生しました: {exc}", "severity": "error", - "hint": "MMLの内容を確認の上、再度お試しください。問題が続く場合は、短いMMLから試してください。", + "hint": "MMLの内容を確認の上、再度お試しください。", } ], - "warnings": warnings_list, + "warnings": [], }, } - errors.extend(synth_errors) - errors_list, warnings_list = _split_errors(errors) - if _is_error(errors): - return { - "success": False, - "wav_path": None, - "duration_sec": 0, - "note_sequence": note_sequence_dict, - "validation": {"errors": errors_list, "warnings": warnings_list}, - } - wav_path = OUTPUT_DIR / "output.wav" - wav_errors = write_wav(wav_path, wave_data, sample_rate) - errors.extend(wav_errors) - errors_list, warnings_list = _split_errors(errors) - return { - "success": not _is_error(errors), - "wav_path": str(wav_path) if not _is_error(errors) else None, - "duration_sec": duration, - "note_sequence": note_sequence_dict, - "validation": {"errors": errors_list, "warnings": warnings_list}, - } return { "success": False, - "errors": [ - { - "code": ErrorCode.SYNTAX_INVALID_TOKEN.value, - "line": 0, - "column": 0, - "message": f"未知の action '{action}' です。", - "severity": "error", - "hint": "action は 'compose', 'validate', 'template' のいずれかを指定してください。", - } - ], - "warnings": [], + "validation": { + "errors": [ + { + "code": ErrorCode.VALIDATION_INVALID_ACTION.value, + "line": 0, + "column": 0, + "message": f"未知の action '{action}' です。", + "severity": "error", + "hint": "action は 'compose', 'validate', 'template' のいずれかを指定してください。", + } + ], + "warnings": [], + }, } diff --git a/tests/test_compose_mml_tool.py b/tests/test_compose_mml_tool.py index af1e3e2..eb1b093 100644 --- a/tests/test_compose_mml_tool.py +++ b/tests/test_compose_mml_tool.py @@ -85,13 +85,13 @@ def test_template_invalid_fallback(compose_mml): def test_compose_missing_mml(compose_mml): result = compose_mml(action="compose", mml="", mode="ppmck") assert result["success"] is False - assert any(e["severity"] == "error" for e in result["errors"]) + assert any(e["severity"] == "error" for e in result["validation"]["errors"]) def test_compose_missing_mode(compose_mml): result = compose_mml(action="compose", mml="A c", mode="") assert result["success"] is False - assert any(e["severity"] == "error" for e in result["errors"]) + assert any(e["severity"] == "error" for e in result["validation"]["errors"]) def test_validate_missing_mode(compose_mml): @@ -103,7 +103,7 @@ def test_compose_unknown_mode(compose_mml): result = compose_mml(action="compose", mml="A c", mode="xyz") assert result["success"] is False assert any( - e["code"] == "SYNTAX_INVALID_TOKEN" for e in result["validation"]["errors"] + e["code"] == "VALIDATION_INVALID_MODE" for e in result["validation"]["errors"] ) @@ -127,7 +127,7 @@ def test_compose_with_warning(compose_mml, tmp_output_dir): def test_action_invalid(compose_mml): result = compose_mml(action="unknown") assert result["success"] is False - assert any("action" in e["message"] for e in result["errors"]) + assert any("action" in e["message"] for e in result["validation"]["errors"]) def test_compose_sample_rates(compose_mml, tmp_output_dir): diff --git a/tests/test_error_codes.py b/tests/test_error_codes.py index 19b4575..496ce84 100644 --- a/tests/test_error_codes.py +++ b/tests/test_error_codes.py @@ -17,14 +17,16 @@ def _find_code(result, code, key="errors"): def _has_error(result): - return any(e["severity"] == "error" for e in result["validation"]["errors"]) + if "validation" in result: + return any(e["severity"] == "error" for e in result["validation"]["errors"]) + return any(e["severity"] == "error" for e in result["errors"]) # --- SYNTAX_INVALID_TOKEN --- def test_syntax_invalid_token_mode(compose_mml): result = compose_mml(action="compose", mml="A c", mode="xyz") assert result["success"] is False - assert _find_code(result, ErrorCode.SYNTAX_INVALID_TOKEN.value) + assert _find_code(result, ErrorCode.VALIDATION_INVALID_MODE.value) # --- SYNTAX_INVALID_NUMBER --- @@ -201,6 +203,27 @@ def test_syntax_unterminated_header(compose_mml): assert _find_code(result, ErrorCode.SYNTAX_UNTERMINATED_HEADER.value) +# --- VALIDATION_MISSING_PARAMETER --- +def test_validation_missing_parameter(compose_mml): + result = compose_mml(action="compose", mml="", mode="ppmck") + assert result["success"] is False + assert _find_code(result, ErrorCode.VALIDATION_MISSING_PARAMETER.value) + + +# --- VALIDATION_INVALID_MODE --- +def test_validation_invalid_mode(compose_mml): + result = compose_mml(action="compose", mml="A c", mode="xyz") + assert result["success"] is False + assert _find_code(result, ErrorCode.VALIDATION_INVALID_MODE.value) + + +# --- VALIDATION_INVALID_ACTION --- +def test_validation_invalid_action(compose_mml): + result = compose_mml(action="unknown") + assert result["success"] is False + assert _find_code(result, ErrorCode.VALIDATION_INVALID_ACTION.value) + + # --- SYSTEM_SYNTHESIS_FAILED --- def test_system_synthesis_failed(compose_mml): with patch( @@ -234,6 +257,20 @@ def test_system_wav_write_failed(tmp_path): # --- SYSTEM_INTERNAL_ERROR --- +def test_system_internal_error(compose_mml): + with patch( + "mml_composemusic_mcp.server._parse_mml", + side_effect=RuntimeError("unexpected internal error"), + ): + result = compose_mml( + action="compose", + mml="A t120 l4 o4\n c", + mode="ppmck", + ) + assert result["success"] is False + assert _find_code(result, ErrorCode.SYSTEM_INTERNAL_ERROR.value) + + def test_system_internal_error_not_exposed(compose_mml): result = compose_mml(action="compose", mml="A t120 l4 o4\n c", mode="ppmck") assert isinstance(result, dict) diff --git a/tests/test_parser_detailed.py b/tests/test_parser_detailed.py index 88d7095..3752562 100644 --- a/tests/test_parser_detailed.py +++ b/tests/test_parser_detailed.py @@ -143,6 +143,28 @@ def test_pyxel_repeat_count3(): assert len(notes) == 6 +def test_pyxel_nested_repeat(): + source = "0: T120 L4 O4\n[ [ C D ]2 E ]2" + tokens = tokenize(source, "pyxel") + ns, errors = parse_pyxel(source, tokens) + assert not any(e.severity == "error" for e in errors) + notes = [e for e in ns.channels["Pulse1"].events if isinstance(e, NoteEvent)] + # Inner: [C D]2 -> C D C D (4 notes) + # Outer: [ ... E ]2 -> (C D C D E) * 2 = 10 notes + assert len(notes) == 10 + + +def test_pyxel_nested_repeat_count3(): + source = "0: T120 L4 O4\n[ [ C D ]2 E ]3" + tokens = tokenize(source, "pyxel") + ns, errors = parse_pyxel(source, tokens) + assert not any(e.severity == "error" for e in errors) + notes = [e for e in ns.channels["Pulse1"].events if isinstance(e, NoteEvent)] + # Inner: [C D]2 -> C D C D (4 notes) + # Outer: [ ... E ]3 -> (C D C D E) * 3 = 15 notes + assert len(notes) == 15 + + # --- Tie --- @@ -152,6 +174,21 @@ def test_ppmck_tie_note(): ns, errors = parse_ppmck(source, tokens) # Tie should merge notes; check no error assert not any(e.severity == "error" for e in errors) + notes = [e for e in ns.channels["Pulse1"].events if isinstance(e, NoteEvent)] + # c4 & c4 should produce a single note with doubled duration (192*2=384) + assert len(notes) == 1 + assert notes[0].duration == 384 + + +def test_pyxel_tie_note(): + source = "0: T120 L4 O4\n C4 & C4" + tokens = tokenize(source, "pyxel") + ns, errors = parse_pyxel(source, tokens) + assert not any(e.severity == "error" for e in errors) + notes = [e for e in ns.channels["Pulse1"].events if isinstance(e, NoteEvent)] + # C4 & C4 should produce a single note with doubled duration (192*2=384) + assert len(notes) == 1 + assert notes[0].duration == 384 def test_pyxel_tie_length(): -- cgit v1.2.3