-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add PR body generator from agent bundle #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,130 @@ | ||||||||||||||||||||||||||||||
| #!/usr/bin/env python3 | ||||||||||||||||||||||||||||||
| """Render a deterministic pull-request body from an agent artifact bundle.""" | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| from __future__ import annotations | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import argparse | ||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| REPO_ROOT = Path(__file__).resolve().parents[1] | ||||||||||||||||||||||||||||||
| if str(REPO_ROOT) not in sys.path: | ||||||||||||||||||||||||||||||
| sys.path.insert(0, str(REPO_ROOT)) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| from scripts.validate_agent_artifact_bundle import DEFAULT_BUNDLE_PATH, _bundle_from_payload, _load_json_object, validate_bundle_payload | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _bullet_list(values: list[str], empty: str) -> list[str]: | ||||||||||||||||||||||||||||||
| if not values: | ||||||||||||||||||||||||||||||
| return [f"- {empty}"] | ||||||||||||||||||||||||||||||
| return [f"- `{value}`" for value in values] | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _validation_lines(validation_evidence: object) -> list[str]: | ||||||||||||||||||||||||||||||
| if not isinstance(validation_evidence, list) or not validation_evidence: | ||||||||||||||||||||||||||||||
| return ["- No validation evidence provided in bundle."] | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| lines: list[str] = [] | ||||||||||||||||||||||||||||||
| for entry in validation_evidence: | ||||||||||||||||||||||||||||||
| if not isinstance(entry, dict): | ||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||
| command = entry.get("command") | ||||||||||||||||||||||||||||||
| result = entry.get("result") | ||||||||||||||||||||||||||||||
| if isinstance(command, str) and isinstance(result, str): | ||||||||||||||||||||||||||||||
| lines.append(f"- `{command}`: `{result}`") | ||||||||||||||||||||||||||||||
| return lines or ["- No validation evidence provided in bundle."] | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _safe_gate_lines(safe_pr_gate: object) -> list[str]: | ||||||||||||||||||||||||||||||
| if not isinstance(safe_pr_gate, dict): | ||||||||||||||||||||||||||||||
| return ["- safe_pr_gate: `unavailable`"] | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| lines = [ | ||||||||||||||||||||||||||||||
| f"- result: `{safe_pr_gate.get('result')}`", | ||||||||||||||||||||||||||||||
| f"- ok: `{str(safe_pr_gate.get('ok')).lower()}`", | ||||||||||||||||||||||||||||||
| f"- allow_dirty: `{str(safe_pr_gate.get('allow_dirty')).lower()}`", | ||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||
| problems = safe_pr_gate.get("problems") | ||||||||||||||||||||||||||||||
| if isinstance(problems, list) and problems: | ||||||||||||||||||||||||||||||
| lines.append("- problems:") | ||||||||||||||||||||||||||||||
| lines.extend(f" - `{problem}`" for problem in problems if isinstance(problem, str)) | ||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||
| lines.append("- problems: `none`") | ||||||||||||||||||||||||||||||
|
Comment on lines
+49
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problems field should follow the strictness rule for expected list fields. If the field is present but is not a list, it should raise a RuntimeError instead of silently falling back to the default message. Additionally, use type guards for items within the list to prevent crashes on malformed data.
Suggested change
References
|
||||||||||||||||||||||||||||||
| return lines | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def render_pr_body_from_bundle(bundle: dict[str, Any]) -> str: | ||||||||||||||||||||||||||||||
| changed_files = bundle.get("changed_files") | ||||||||||||||||||||||||||||||
| changed_file_lines = _bullet_list(changed_files if isinstance(changed_files, list) else [], "No changed files provided in bundle.") | ||||||||||||||||||||||||||||||
|
Comment on lines
+59
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changed_files field should also be handled strictly. If it is present but not a list, a RuntimeError should be raised per the repository's general rules.
Suggested change
References
|
||||||||||||||||||||||||||||||
| validation_lines = _validation_lines(bundle.get("validation_evidence")) | ||||||||||||||||||||||||||||||
| safe_gate_lines = _safe_gate_lines(bundle.get("safe_pr_gate")) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| evidence_lines = [ | ||||||||||||||||||||||||||||||
| f"- branch: `{bundle.get('branch')}`", | ||||||||||||||||||||||||||||||
| f"- bundle_result: `{bundle.get('result')}`", | ||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||
| mcp_ref = bundle.get("mcp_context_output_ref") | ||||||||||||||||||||||||||||||
| if isinstance(mcp_ref, str): | ||||||||||||||||||||||||||||||
| evidence_lines.append(f"- mcp_context_output_ref: `{mcp_ref}`") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| lines = [ | ||||||||||||||||||||||||||||||
| "## Summary", | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| "Deterministic agent artifact bundle evidence for this change.", | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| "## Scope", | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| *changed_file_lines, | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| "## Validation", | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| *validation_lines, | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| "## Safety Gate", | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| *safe_gate_lines, | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| "## Evidence", | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| *evidence_lines, | ||||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||
| return "\n".join(lines) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def render_pr_body_from_payload(payload: dict[str, Any]) -> str: | ||||||||||||||||||||||||||||||
| validation = validate_bundle_payload(payload) | ||||||||||||||||||||||||||||||
| if not validation["ok"]: | ||||||||||||||||||||||||||||||
| issues = "\n".join(f"- {issue}" for issue in validation["issues"]) | ||||||||||||||||||||||||||||||
| raise RuntimeError(f"agent artifact bundle failed validation:\n{issues}") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| bundle, bundle_issues = _bundle_from_payload(payload) | ||||||||||||||||||||||||||||||
| if bundle is None: | ||||||||||||||||||||||||||||||
| raise RuntimeError("; ".join(bundle_issues)) | ||||||||||||||||||||||||||||||
| return render_pr_body_from_bundle(bundle) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def render_pr_body_from_file(path: Path) -> str: | ||||||||||||||||||||||||||||||
| return render_pr_body_from_payload(_load_json_object(path)) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _parse_args(argv: list[str]) -> argparse.Namespace: | ||||||||||||||||||||||||||||||
| parser = argparse.ArgumentParser(description="Render deterministic PR body Markdown from an agent artifact bundle.") | ||||||||||||||||||||||||||||||
| parser.add_argument("--bundle", type=Path, default=DEFAULT_BUNDLE_PATH, help="Bundle JSON path.") | ||||||||||||||||||||||||||||||
| return parser.parse_args(argv) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def main(argv: list[str] | None = None) -> int: | ||||||||||||||||||||||||||||||
| args = _parse_args(sys.argv[1:] if argv is None else argv) | ||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||
| sys.stdout.write(render_pr_body_from_file(args.bundle)) | ||||||||||||||||||||||||||||||
| return 0 | ||||||||||||||||||||||||||||||
| except RuntimeError as exc: | ||||||||||||||||||||||||||||||
| sys.stderr.write(f"{exc}\n") | ||||||||||||||||||||||||||||||
| return 1 | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if __name__ == "__main__": | ||||||||||||||||||||||||||||||
| raise SystemExit(main()) | ||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| from pathlib import Path | ||
|
|
||
| import scripts.pr_body_from_agent_bundle as pr_body | ||
|
|
||
| ARTIFACT_PATH = Path("artifacts/agent_artifact_bundle_example.json") | ||
|
|
||
|
|
||
| def test_render_pr_body_from_committed_bundle_is_deterministic() -> None: | ||
| first = pr_body.render_pr_body_from_file(ARTIFACT_PATH) | ||
| second = pr_body.render_pr_body_from_file(ARTIFACT_PATH) | ||
|
|
||
| assert first == second | ||
| assert first == "\n".join( | ||
| [ | ||
| "## Summary", | ||
| "", | ||
| "Deterministic agent artifact bundle evidence for this change.", | ||
| "", | ||
| "## Scope", | ||
| "", | ||
| "- `artifacts/agent_artifact_bundle_example.json`", | ||
| "- `scripts/generate_agent_artifact_bundle_example.py`", | ||
| "- `tests/test_agent_artifact_bundle.py`", | ||
| "", | ||
| "## Validation", | ||
| "", | ||
| "- `python -m compileall -q scripts/agent_artifact_bundle.py scripts/generate_agent_artifact_bundle_example.py`: `pass`", | ||
| "- `pytest tests/test_agent_artifact_bundle.py -q`: `pass`", | ||
| "", | ||
| "## Safety Gate", | ||
| "", | ||
| "- result: `PASS`", | ||
| "- ok: `true`", | ||
| "- allow_dirty: `false`", | ||
| "- problems: `none`", | ||
| "", | ||
| "## Evidence", | ||
| "", | ||
| "- branch: `feat/agent-artifact-bundle-example`", | ||
| "- bundle_result: `PASS`", | ||
| "- mcp_context_output_ref: `artifacts/mcp_context_layer_example.json`", | ||
| "", | ||
| ] | ||
| ) | ||
|
|
||
|
|
||
| def test_render_pr_body_uses_only_bundle_validation_evidence() -> None: | ||
| bundle = { | ||
| "branch": "feat/no-validation", | ||
| "changed_files": [], | ||
| "ok": True, | ||
| "result": "PASS", | ||
| "safe_pr_gate": { | ||
| "allow_dirty": False, | ||
| "allowed_prefixes": [], | ||
| "branch": "feat/no-validation", | ||
| "changed_paths": [], | ||
| "ok": True, | ||
| "problems": [], | ||
| "result": "PASS", | ||
| "status_short": [], | ||
| }, | ||
| "validation_evidence": [], | ||
| } | ||
|
|
||
| rendered = pr_body.render_pr_body_from_payload(bundle) | ||
|
|
||
| assert "- No validation evidence provided in bundle." in rendered | ||
| assert "pytest" not in rendered | ||
|
|
||
|
|
||
| def test_render_pr_body_rejects_invalid_bundle_without_markdown() -> None: | ||
| invalid = { | ||
| "branch": "feat/bad", | ||
| "changed_files": [], | ||
| "ok": True, | ||
| "result": "FAIL", | ||
| "safe_pr_gate": { | ||
| "allow_dirty": False, | ||
| "allowed_prefixes": [], | ||
| "branch": "feat/bad", | ||
| "changed_paths": [], | ||
| "ok": True, | ||
| "problems": [], | ||
| "result": "PASS", | ||
| "status_short": [], | ||
| }, | ||
| "validation_evidence": [], | ||
| } | ||
|
|
||
| try: | ||
| pr_body.render_pr_body_from_payload(invalid) | ||
| except RuntimeError as exc: | ||
| assert "bundle.result must match bundle.ok" in str(exc) | ||
| else: | ||
| raise AssertionError("expected invalid bundle to raise RuntimeError") | ||
|
|
||
|
|
||
| def test_cli_outputs_markdown_only_for_valid_bundle(tmp_path: Path, capsys) -> None: | ||
| payload = json.loads(ARTIFACT_PATH.read_text(encoding="utf-8")) | ||
| bundle_path = tmp_path / "bundle.json" | ||
| bundle_path.write_text(json.dumps(payload, indent=2, sort_keys=True) + "\n", encoding="utf-8") | ||
|
|
||
| exit_code = pr_body.main(["--bundle", str(bundle_path)]) | ||
| captured = capsys.readouterr() | ||
|
|
||
| assert exit_code == 0 | ||
| assert captured.err == "" | ||
| assert captured.out.startswith("## Summary\n") | ||
| assert "## Safety Gate\n" in captured.out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation performs a silent fallback if validation_evidence is not a list. According to the general rules, non-list types for expected list fields should raise a RuntimeError to maintain strictness, while null or missing values should be treated as empty lists.
References