feat: add safe PR gate#202
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a deterministic local safety gate script (scripts/safe_pr_gate.py) designed to validate pull request readiness by checking branch names, working tree status, and file path prefixes. It also updates the Git workflow documentation and provides a comprehensive test suite. Review feedback highlighted a bug in the Git porcelain status parser regarding renames and copies, suggested more robust error handling for Git subprocess calls to prevent unhandled exceptions, and recommended refactoring integration tests to remove environment dependencies and ensure determinism.
| continue | ||
| path = entry[3:] | ||
| status = entry[:2] | ||
| if status and status[0] in {"R", "C"}: |
There was a problem hiding this comment.
The current check status[0] in {"R", "C"} only identifies renames or copies if the status character is in the first position (index status). In git status --porcelain, a rename can also be indicated in the second position (working tree status, e.g., R or C). If this occurs, the parser will treat the entry as a normal change, fail to consume the second path entry (the destination), and then incorrectly parse the destination path as a new status entry in the next iteration.
| if status and status[0] in {"R", "C"}: | |
| if any(s in {"R", "C"} for s in status): |
| def _run_git(args: list[str]) -> str: | ||
| completed = subprocess.run( | ||
| ["git", *args], | ||
| cwd=REPO_ROOT, | ||
| check=True, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| return completed.stdout |
There was a problem hiding this comment.
The _run_git function lacks robust error handling. If git is missing from the environment or if the command is run outside of a git repository, it will raise a FileNotFoundError or subprocess.CalledProcessError with a traceback. Following the repository's pattern of strict error reporting, it is better to catch these and raise a RuntimeError with descriptive context.
| def _run_git(args: list[str]) -> str: | |
| completed = subprocess.run( | |
| ["git", *args], | |
| cwd=REPO_ROOT, | |
| check=True, | |
| capture_output=True, | |
| text=True, | |
| ) | |
| return completed.stdout | |
| def _run_git(args: list[str]) -> str: | |
| try: | |
| completed = subprocess.run( | |
| ["git", *args], | |
| cwd=REPO_ROOT, | |
| check=True, | |
| capture_output=True, | |
| text=True, | |
| ) | |
| return completed.stdout | |
| except (subprocess.CalledProcessError, FileNotFoundError) as e: | |
| raise RuntimeError(f"Failed to run git {' '.join(args)}: {e}") from e |
References
- When encountering errors during external operations or file loading, raise a RuntimeError with descriptive context to ensure failures are visible and handled strictly.
| def test_cli_pass_and_fail_outputs_are_deterministic() -> None: | ||
| passing = _run_gate( | ||
| "--allow-dirty", | ||
| "--allowed-prefix", | ||
| "docs/", | ||
| "--allowed-prefix", | ||
| "scripts/", | ||
| "--allowed-prefix", | ||
| "tests/", | ||
| ) | ||
| dirty_path = REPO_ROOT / "_safe_pr_gate_dirty_test.tmp" | ||
| dirty_path.write_text("dirty\n", encoding="utf-8") | ||
| try: | ||
| failing = _run_gate( | ||
| "--allow-dirty", | ||
| "--allowed-prefix", | ||
| "docs/", | ||
| "--allowed-prefix", | ||
| "scripts/", | ||
| "--allowed-prefix", | ||
| "tests/", | ||
| ) | ||
| finally: | ||
| dirty_path.unlink(missing_ok=True) | ||
|
|
||
| assert passing.returncode == 0 | ||
| assert failing.returncode == 1 | ||
|
|
||
| passing_payload = json.loads(passing.stdout) | ||
| failing_payload = json.loads(failing.stdout) | ||
|
|
||
| assert passing_payload["result"] == "PASS" | ||
| assert passing_payload["allow_dirty"] is True | ||
| assert failing_payload["result"] == "FAIL" | ||
| assert failing_payload["problems"] == [ | ||
| "changed_files_outside_allowed_prefixes", | ||
| "outside_prefix:_safe_pr_gate_dirty_test.tmp", | ||
| ] |
There was a problem hiding this comment.
This integration test is environment-dependent and will fail if the local repository where tests are run has any untracked or modified files outside the hardcoded allowed prefixes (docs/, scripts/, tests/). For instance, a .gitignore change or a temporary file in the root would cause the passing check to fail.
To make this test deterministic, consider refactoring it to call main() directly and use pytest's monkeypatch to mock collect_gate_state with a controlled GateState object.
Adds a minimal deterministic local safe PR gate for AI-assisted work.
Includes:
scripts/safe_pr_gate.py--allow-dirty--allowed-prefixscope checksScope
Local one-shot safety gate only. No network, no external APIs, no destructive git commands.
Validation
python -m compileall -q scripts/safe_pr_gate.pypytest tests/test_safe_pr_gate.py -q