From 960b9ec6add07ff49e0b3859b0358eceaead547b Mon Sep 17 00:00:00 2001 From: Vitalii Mishchenko Date: Wed, 20 May 2026 11:11:50 -0700 Subject: [PATCH] Separate verify failure templates for static and suggested Split verify failure handling into dedicated paths with separate templates for each. Render templated feedback for spec context while appending full command output to logs for debugging. --- src/draft/steps/implement_spec/__init__.py | 59 ++++- .../implement_spec/static_verify_errors.md | 5 + .../implement_spec/suggested_verify_errors.md | 5 + .../steps/implement_spec/verify_errors.md | 5 - tests/draft/test_code_spec.py | 203 +++++++++++++++++- 5 files changed, 256 insertions(+), 21 deletions(-) create mode 100644 src/draft/steps/implement_spec/static_verify_errors.md create mode 100644 src/draft/steps/implement_spec/suggested_verify_errors.md delete mode 100644 src/draft/steps/implement_spec/verify_errors.md diff --git a/src/draft/steps/implement_spec/__init__.py b/src/draft/steps/implement_spec/__init__.py index c88a178..19fe181 100644 --- a/src/draft/steps/implement_spec/__init__.py +++ b/src/draft/steps/implement_spec/__init__.py @@ -40,13 +40,7 @@ def _render_prompt(ctx, template: str, verify_commands: str) -> str: spec = ctx.get("spec", "") spec_section = f"## Current Spec\n\n{spec}" verify_errors = ctx.step_get("implement-spec", "verify_errors", "") - if verify_errors: - verify_template = ( - files("draft.steps.implement_spec").joinpath("verify_errors.md").read_text() - ) - verify_section = verify_template.replace("{{ERRORS}}", verify_errors) - else: - verify_section = "" + verify_section = verify_errors if verify_errors else "" original_spec_section = original_spec.render_original_spec(ctx) return ( template.replace("{{VERIFY_COMMANDS}}", verify_commands) @@ -219,11 +213,52 @@ def _filter_dupes(suggested: list[dict], static_cmds: list[str]) -> list[dict]: return [e for e in suggested if _normalize_cmd(e["cmd"]) not in normalized_static] +def _format_commands_bullets(failures: list[HookResult]) -> str: + return "\n".join(f"- {r.cmd}" for r in failures) + + +def _render_static_verify_failures(failures: list[HookResult]) -> str: + template = ( + files("draft.steps.implement_spec") + .joinpath("static_verify_errors.md") + .read_text() + ) + return template.replace("{{COMMANDS}}", _format_commands_bullets(failures)) + + +def _render_suggested_verify_failures(failures: list[HookResult]) -> str: + template = ( + files("draft.steps.implement_spec") + .joinpath("suggested_verify_errors.md") + .read_text() + ) + return template.replace("{{COMMANDS}}", _format_commands_bullets(failures)) + + +def _format_static_verify_failures(failures: list[HookResult]) -> str: + parts = "\n\n".join(f"$ {r.cmd}\n{r.output}" for r in failures) + return f"## Verify failures (output)\n\n{parts}" + + def _format_suggested_failures(failures: list[HookResult]) -> str: parts = "\n\n".join(f"$ {r.cmd}\n{r.output}" for r in failures) return f"## Suggested check failures\n\n{parts}" +def _append_failure_block(log_path, block: str) -> None: + try: + with open(log_path, "a", encoding="utf-8") as f: + f.write("---\n") + f.write(block) + if not block.endswith("\n"): + f.write("\n") + except OSError as exc: + print( + f"warning: could not write failure block to {log_path}: {exc}", + file=sys.stderr, + ) + + def _suggest_checks( ctx, engine, @@ -417,7 +452,11 @@ def run(self, ctx, engine, lifecycle, step_metrics): ctx.step_set( self.name, "verify_errors", - "\n\n".join(f"$ {r.cmd}\n{r.output}" for r in failures), + _render_static_verify_failures(failures), + ) + _append_failure_block( + ctx.log_path(self.name), + _format_static_verify_failures(failures), ) ctx.save() continue @@ -445,6 +484,10 @@ def run(self, ctx, engine, lifecycle, step_metrics): ctx.step_set( self.name, "verify_errors", + _render_suggested_verify_failures(suggest_failures), + ) + _append_failure_block( + ctx.log_path(self.name), _format_suggested_failures(suggest_failures), ) ctx.save() diff --git a/src/draft/steps/implement_spec/static_verify_errors.md b/src/draft/steps/implement_spec/static_verify_errors.md new file mode 100644 index 0000000..992d91c --- /dev/null +++ b/src/draft/steps/implement_spec/static_verify_errors.md @@ -0,0 +1,5 @@ +## Verify failures + +The following commands failed. You can re-run any of them yourself to see actual output. + +{{COMMANDS}} diff --git a/src/draft/steps/implement_spec/suggested_verify_errors.md b/src/draft/steps/implement_spec/suggested_verify_errors.md new file mode 100644 index 0000000..66ec0be --- /dev/null +++ b/src/draft/steps/implement_spec/suggested_verify_errors.md @@ -0,0 +1,5 @@ +## Suggested check failures + +The following commands failed. You can re-run any of them yourself to see actual output. + +{{COMMANDS}} diff --git a/src/draft/steps/implement_spec/verify_errors.md b/src/draft/steps/implement_spec/verify_errors.md deleted file mode 100644 index 15886bc..0000000 --- a/src/draft/steps/implement_spec/verify_errors.md +++ /dev/null @@ -1,5 +0,0 @@ -## Verified errors - -{{ERRORS}} - -Fix the above failures. diff --git a/tests/draft/test_code_spec.py b/tests/draft/test_code_spec.py index 67d1a5d..70e9681 100644 --- a/tests/draft/test_code_spec.py +++ b/tests/draft/test_code_spec.py @@ -9,12 +9,16 @@ from draft.steps.implement_spec import ( ImplementSpecStep, _filter_dupes, + _format_commands_bullets, + _format_static_verify_failures, _format_suggested_failures, _generate_commit_message, _load_template, _log_prompt, _parse_suggestions, _render_prompt, + _render_static_verify_failures, + _render_suggested_verify_failures, _render_verify_commands, _run_suggested_checks, ) @@ -450,7 +454,9 @@ def hooks_side_effect(step_name, event): calls = ctx.step_set.call_args_list verify_calls = [c for c in calls if c.args[1] == "verify_errors"] - assert any("boom" in str(c.args[2]) for c in verify_calls) + assert any("## Verify failures" in str(c.args[2]) for c in verify_calls) + assert any("- make test" in str(c.args[2]) for c in verify_calls) + assert not any("boom" in str(c.args[2]) for c in verify_calls) sha_calls = [c for c in calls if c.args[1] == "commit_sha"] assert len(sha_calls) == 1 @@ -778,6 +784,177 @@ def test_format_suggested_failures_structure(): assert "E" in text +# --- new render/format helpers tests --- + + +def test_format_commands_bullets_single(): + failures = [HookResult(cmd="make test", rc=1, output="fail\n", duration=1.0)] + assert _format_commands_bullets(failures) == "- make test" + + +def test_format_commands_bullets_multiple(): + failures = [ + HookResult(cmd="make lint", rc=1, output="e1\n", duration=1.0), + HookResult(cmd="make test", rc=1, output="e2\n", duration=1.0), + ] + assert _format_commands_bullets(failures) == "- make lint\n- make test" + + +def test_render_static_verify_failures_heading_intro_bullets(): + failures = [HookResult(cmd="make lint", rc=1, output="err\n", duration=0.5)] + result = _render_static_verify_failures(failures) + assert result.startswith("## Verify failures") + assert "re-run" in result + assert "- make lint" in result + assert "err" not in result + + +def test_render_static_verify_failures_two_commands_no_output(): + failures = [ + HookResult(cmd="make lint", rc=1, output="e1\n", duration=0.5), + HookResult(cmd="make test", rc=1, output="e2\n", duration=0.5), + ] + result = _render_static_verify_failures(failures) + assert "- make lint" in result + assert "- make test" in result + assert "e1" not in result + assert "e2" not in result + + +def test_render_static_verify_failures_timeout_no_output(): + failures = [ + HookResult( + cmd="make test", rc=124, output="timed out after 120s", duration=120.0 + ) + ] + result = _render_static_verify_failures(failures) + assert "- make test" in result + assert "timed out" not in result + + +def test_render_suggested_verify_failures_heading_intro_bullets(): + failures = [ + HookResult(cmd="pytest -x", rc=1, output="UNIQUE_OUTPUT_XYZ\n", duration=0.5) + ] + result = _render_suggested_verify_failures(failures) + assert result.startswith("## Suggested check failures") + assert "re-run" in result + assert "- pytest -x" in result + assert "UNIQUE_OUTPUT_XYZ" not in result + + +def test_render_suggested_verify_failures_timeout_no_output(): + failures = [ + HookResult(cmd="slow check", rc=124, output="timed out", duration=120.0) + ] + result = _render_suggested_verify_failures(failures) + assert "- slow check" in result + assert "timed out" not in result + + +def test_format_static_verify_failures_structure(): + failures = [HookResult(cmd="make lint", rc=1, output="E\n", duration=0.5)] + text = _format_static_verify_failures(failures) + assert "## Verify failures" in text + assert "$ make lint" in text + assert "E" in text + + +def test_static_verify_failure_appends_full_output_to_log(tmp_path): + cfg = {"max_retries": 3, "timeout": 60} + log_path = tmp_path / "implement-spec.log" + ctx = _make_ctx(cfg, spec="my spec", tmp_path=tmp_path) + ctx.log_path.return_value = log_path + + fail_result = MagicMock() + fail_result.rc = 1 + fail_result.cmd = "make test" + fail_result.output = "boom output" + + call_count = 0 + + def hooks_side_effect(step_name, event): + nonlocal call_count + call_count += 1 + if call_count == 1: + return [fail_result] + return [] + + lifecycle = MagicMock() + lifecycle.run_hooks.side_effect = hooks_side_effect + engine = _make_engine() + + step = ImplementSpecStep() + with ( + patch("draft.steps.implement_spec._has_changes", return_value=True), + patch( + "draft.steps.implement_spec._generate_commit_message", + return_value=("Add foo", False), + ), + patch("draft.steps.implement_spec._run_git_capture", return_value="sha\n"), + patch( + "draft.steps.implement_spec._run_git_capture_allow_fail", + return_value=subprocess.CompletedProcess([], 0, b"", b""), + ), + ): + step.run(ctx, engine, lifecycle, MagicMock()) + + log_content = log_path.read_text() + assert "## Verify failures" in log_content + assert "$ make test" in log_content + assert "boom output" in log_content + + +def test_suggested_check_failure_appends_full_output_to_log(tmp_path): + cfg = _make_suggest_cfg(max_retries=2) + log_path = tmp_path / "implement-spec.log" + ctx = _make_ctx(cfg, tmp_path=tmp_path) + ctx.log_path.return_value = log_path + + fail_result = HookResult(cmd="false", rc=1, output="fail output\n", duration=0.1) + sugg_call_count = 0 + + def run_suggested_side(suggested, wt_dir, run_dir, eng, c, stage): + nonlocal sugg_call_count + sugg_call_count += 1 + if sugg_call_count == 1: + return [fail_result] + return [] + + lifecycle = MagicMock() + lifecycle.run_hooks.return_value = [] + engine = _make_engine() + + step = ImplementSpecStep() + with ( + patch("draft.steps.implement_spec._has_changes", return_value=True), + patch("draft.steps.implement_spec._load_suggest_template", return_value="tpl"), + patch( + "draft.steps.implement_spec._suggest_checks", + return_value=[{"cmd": "false"}], + ), + patch( + "draft.steps.implement_spec._run_suggested_checks", + side_effect=run_suggested_side, + ), + patch( + "draft.steps.implement_spec._generate_commit_message", + return_value=("msg", False), + ), + patch("draft.steps.implement_spec._run_git_capture", return_value="sha\n"), + patch( + "draft.steps.implement_spec._run_git_capture_allow_fail", + return_value=subprocess.CompletedProcess([], 0, b"", b""), + ), + ): + step.run(ctx, engine, lifecycle, MagicMock()) + + log_content = log_path.read_text() + assert "## Suggested check failures" in log_content + assert "$ false" in log_content + assert "fail output" in log_content + + # --- _run_suggested_checks tests --- @@ -1524,7 +1701,7 @@ def test_render_verify_commands_heading_is_verified_commands(): assert result.startswith("## Verified commands") -def test_render_prompt_verify_errors_section_uses_verified_errors_heading(tmp_path): +def test_render_prompt_verify_errors_section_inserted_verbatim(tmp_path): tpl = tmp_path / "tpl.md" tpl.write_text("{{SPEC}}\n{{VERIFY_ERRORS}}") template = tpl.read_text() @@ -1534,8 +1711,8 @@ def test_render_prompt_verify_errors_section_uses_verified_errors_heading(tmp_pa ctx.step_get.return_value = "some error" prompt = _render_prompt(ctx, template, "") - assert "## Verified errors" in prompt - assert "## Test failures" not in prompt + assert "some error" in prompt + assert "## Verified errors" not in prompt # --- _render_prompt four-placeholder tests --- @@ -1741,7 +1918,7 @@ def test_render_prompt_verify_errors_no_literal_placeholder(tmp_path): prompt = _render_prompt(ctx, template, "") assert "{{ERRORS}}" not in prompt - assert "Fix the above failures." in prompt + assert "boom" in prompt def test_render_prompt_empty_verify_errors_no_fix_sentence(tmp_path): @@ -1779,15 +1956,25 @@ def test_render_prompt_verify_errors_byte_equivalence(tmp_path): ctx.step_get.return_value = "boom" prompt = _render_prompt(ctx, template, "") - expected_errors_section = "## Verified errors\n\nboom\n\nFix the above failures." - assert expected_errors_section in prompt + assert "boom" in prompt + assert "## Verified errors" not in prompt + assert "Fix the above failures." not in prompt def test_verify_template_files_exist(): from importlib.resources import files assert files("draft.steps.implement_spec").joinpath("verify_commands.md").is_file() - assert files("draft.steps.implement_spec").joinpath("verify_errors.md").is_file() + assert ( + files("draft.steps.implement_spec") + .joinpath("static_verify_errors.md") + .is_file() + ) + assert ( + files("draft.steps.implement_spec") + .joinpath("suggested_verify_errors.md") + .is_file() + ) def test_render_verify_commands_cmd_containing_placeholder():