From e3e6755bf822c9a582aec0f1731c4da52b920b0e Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 18:50:26 +0100 Subject: [PATCH] fix: validate a custom PR template before any branch is created --- pr_split/cli.py | 72 ++++++++++++++++++++++++------- tests/test_cli_coverage.py | 87 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 15 deletions(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index c6c7738..0223bf1 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -125,6 +125,14 @@ def _add_children(parent_id: str, prefix: str) -> None: return f"## Dependency graph\n\nMerge in this order:\n\n```\n{tree_block}\n```" +def _check_pr_template() -> None: + try: + validate_pr_template() + except PRSplitError as exc: + console.print(f"[red]{exc}[/red]") + raise typer.Exit(1) from exc + + def _validate_inputs(dev_branch: str, base: str, *, dry_run: bool = False) -> None: if not branch_exists(dev_branch): console.print(f"[red]{ErrorMsg.BRANCH_NOT_FOUND(branch=dev_branch)}[/red]") @@ -138,6 +146,8 @@ def _validate_inputs(dev_branch: str, base: str, *, dry_run: bool = False) -> No if not dry_run and not check_gh_auth(): console.print(f"[red]{ErrorMsg.GH_AUTH_FAILED()}[/red]") raise typer.Exit(1) + if not dry_run: + _check_pr_template() def _handle_loc_bound_warnings(warnings: list[str], *, strict_loc_bounds: bool) -> None: @@ -357,10 +367,54 @@ def _format_file_list(files: list[str]) -> str: return "\n".join(lines) +_PR_TEMPLATE_PLACEHOLDERS = ( + "description", + "files", + "added", + "removed", + "loc", + "dependencies", + "dag", + "id", + "title", +) + + +def _render_pr_template(template_vars: dict[str, object]) -> str: + try: + template = _PR_TEMPLATE_PATH.read_text(encoding="utf-8") + return template.format(**template_vars) + except (KeyError, ValueError, IndexError, AttributeError, TypeError) as exc: + # AttributeError/TypeError cover "{title.nope}" and "{added[0]}", + # which str.format raises as plain attribute/subscript errors. + available = ", ".join(f"{{{k}}}" for k in sorted(template_vars)) + raise PRSplitError( + f"Invalid PR template at {_PR_TEMPLATE_PATH}: {exc}. " + f"Available placeholders: {available}. " + "Escape literal braces with {{ and }}." + ) from exc + except OSError as exc: + raise PRSplitError(f"Could not read PR template at {_PR_TEMPLATE_PATH}: {exc}") from exc + + +def validate_pr_template() -> None: + """Render the custom template against placeholder values. + + The template is otherwise first used when the PRs are opened, i.e. after + every branch has been created and pushed; a typo in it must fail before + that. Raises PRSplitError with the same message the real render would. + """ + if not _PR_TEMPLATE_PATH.exists(): + return + sample: dict[str, object] = {k: k for k in _PR_TEMPLATE_PLACEHOLDERS} + sample.update({"added": 0, "removed": 0, "loc": 0}) + _render_pr_template(sample) + + def _build_pr_body(group: Group, all_groups: list[Group]) -> str: if _PR_TEMPLATE_PATH.exists(): files = [a.file_path for a in group.assignments] - template_vars = { + template_vars: dict[str, object] = { "description": group.description, "files": _format_file_list(files), "added": group.estimated_added, @@ -371,20 +425,7 @@ def _build_pr_body(group: Group, all_groups: list[Group]) -> str: "id": group.id, "title": group.title, } - try: - template = _PR_TEMPLATE_PATH.read_text(encoding="utf-8") - return template.format(**template_vars) - except (KeyError, ValueError, IndexError) as exc: - available = ", ".join(f"{{{k}}}" for k in sorted(template_vars)) - raise PRSplitError( - f"Invalid PR template at {_PR_TEMPLATE_PATH}: {exc}. " - f"Available placeholders: {available}. " - "Escape literal braces with {{ and }}." - ) from exc - except OSError as exc: - raise PRSplitError( - f"Could not read PR template at {_PR_TEMPLATE_PATH}: {exc}" - ) from exc + return _render_pr_template(template_vars) files = [a.file_path for a in group.assignments] sections = [group.description] @@ -1030,6 +1071,7 @@ def execute( if not check_gh_auth(): console.print(f"[red]{ErrorMsg.GH_AUTH_FAILED()}[/red]") raise typer.Exit(1) + _check_pr_template() parsed_diff = parse_diff(plan.raw_diff) diff --git a/tests/test_cli_coverage.py b/tests/test_cli_coverage.py index 6ff01b3..b3afee2 100644 --- a/tests/test_cli_coverage.py +++ b/tests/test_cli_coverage.py @@ -210,6 +210,93 @@ def test_template_os_error(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch _build_pr_body(group, [group]) +class TestPrTemplateEarlyValidation: + def _use_template(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, text: str) -> None: + template_file = tmp_path / "template.md" + template_file.write_text(text, encoding="utf-8") + monkeypatch.setattr("pr_split.cli._PR_TEMPLATE_PATH", template_file) + + @pytest.mark.parametrize( + "text", + ["{title.nope}", "{added[0]}", "{unknown}", "{title", "{0}"], + ) + def test_bad_placeholders_are_friendly_errors( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, text: str + ) -> None: + from pr_split.cli import validate_pr_template + + self._use_template(tmp_path, monkeypatch, text) + with pytest.raises(PRSplitError, match="Invalid PR template"): + validate_pr_template() + group = _group("pr-1", "t", files=["a.py"]) + with pytest.raises(PRSplitError, match="Invalid PR template"): + _build_pr_body(group, [group]) + + def test_valid_template_passes_and_renders( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.cli import validate_pr_template + + self._use_template(tmp_path, monkeypatch, "# {title}\n\n{files}\n\n{{literal}} {loc}") + validate_pr_template() + group = _group("pr-1", "t", files=["a.py"]) + body = _build_pr_body(group, [group]) + assert body.startswith("# t\n\n- `a.py`") + assert "{literal} 15" in body + + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + def test_split_inputs_reject_a_bad_template_before_planning( + self, + mock_be: MagicMock, + mock_clean: MagicMock, + mock_auth: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + from pr_split.cli import console + + self._use_template(tmp_path, monkeypatch, "{title.nope}") + with console.capture() as capture, pytest.raises(typer.Exit): + _validate_inputs("feature", "main") + assert "Invalid PR template" in " ".join(capture.get().split()) + # a dry run never opens PRs, so the template is not consulted + _validate_inputs("feature", "main", dry_run=True) + + @patch("pr_split.cli._create_branches_and_commits") + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_execute_rejects_a_bad_template_before_creating_branches( + self, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_be: MagicMock, + mock_clean: MagicMock, + mock_auth: MagicMock, + mock_confirm: MagicMock, + mock_create: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + self._use_template(tmp_path, monkeypatch, "{added[0]}") + mock_plan_file = MagicMock() + mock_plan_file.git_state.branches = [] + mock_plan_file.git_state.prs = [] + mock_plan_file.plan.raw_diff = "some diff" + mock_plan_file.plan.merge_base_sha = "abc123" + mock_load.return_value = mock_plan_file + result = runner.invoke(app, ["execute"]) + assert result.exit_code == 1 + assert "Invalid PR template" in " ".join(result.output.split()) + mock_confirm.assert_not_called() + mock_create.assert_not_called() + + # --------------------------------------------------------------------------- # _send_webhook # ---------------------------------------------------------------------------