diff --git a/prompts/pr_review.md b/prompts/pr_review.md index 798f5537..40dec18c 100644 --- a/prompts/pr_review.md +++ b/prompts/pr_review.md @@ -18,6 +18,12 @@ You are reviewing a pull request for a **production quantitative trading and dat - Minor refactoring opportunities - Test coverage suggestions +## Review completeness + +- Review the entire diff holistically and report all independent actionable findings in one response. Do not stop after the first blocking issue. +- Do not invent backward-compatibility requirements that are absent from the repository and PR contract. If both explicitly define a clean-slate namespace, check for accidental legacy fallback instead of requesting dual-read or migration. This never overrides security or data-integrity findings. +- For public JSON/wire contracts, systematically check optional-key presence versus explicit null, recursive JSON-safe types, every identity-bearing integer range, one canonical timestamp representation, deterministic encode/decode round-trips and digests, deep immutability, and identifier/path safety. + ## Severity definitions | Severity | Definition | Example | diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py index 4b04f62f..fdf1ae1a 100644 --- a/scripts/run_codex_pr_review.py +++ b/scripts/run_codex_pr_review.py @@ -325,7 +325,10 @@ def build_review_prompt(diff: str, pr_title: str, pr_body: str, repo: str) -> st 1. Focus on **security vulnerabilities, logic errors, data corruption, crash bugs, race conditions, and API compatibility breaks**. 2. Do NOT flag: code style, formatting, naming suggestions, minor refactoring preferences, or documentation issues. 3. Do not emit a finding that concludes no code change is needed. For OIDC, `job_workflow_ref` is absent for explicit direct callers; flag a bypass only when a non-direct repository can reach the direct-caller path despite the allowlists. -4. For each finding, classify its severity: +4. Review the entire diff holistically and report all independent actionable findings in one response. Do not stop after the first blocking issue. +5. Do not invent backward-compatibility requirements that are absent from the repository and PR contract. When the repository and PR explicitly define a clean-slate namespace with legacy compatibility out of scope, review that boundary for accidental fallback instead of requesting dual-read or migration. This never overrides security or data-integrity findings. +6. For public JSON/wire contracts, systematically check optional-key presence versus explicit null, recursive JSON-safe types, every identity-bearing integer range, one canonical timestamp representation, deterministic encode/decode round-trips and digests, deep immutability, and identifier/path safety. +7. For each finding, classify its severity: - **critical**: security vulnerability, data loss, production crash - **high**: logic error that produces wrong results, API break, memory/connection leak - **medium**: missing error handling, performance degradation, race condition diff --git a/tests/test_run_codex_pr_review.py b/tests/test_run_codex_pr_review.py index cec69a0b..09ff08e5 100644 --- a/tests/test_run_codex_pr_review.py +++ b/tests/test_run_codex_pr_review.py @@ -36,6 +36,20 @@ def test_review_prompt_states_direct_oidc_contract(self) -> None: self.assertIn("`job_workflow_ref` is absent for explicit direct callers", prompt) self.assertIn("Do not emit a finding that concludes no code change is needed", prompt) + def test_review_prompt_requires_holistic_contract_review(self) -> None: + prompt = run_codex_pr_review.build_review_prompt( + "diff", + "clean-slate contract", + "Legacy compatibility is explicitly out of scope.", + "org/repo", + ) + self.assertIn("report all independent actionable findings in one response", prompt) + self.assertIn("Do not stop after the first blocking issue", prompt) + self.assertIn("clean-slate", prompt) + self.assertIn("optional-key presence versus explicit null", prompt) + self.assertIn("every identity-bearing integer", prompt) + self.assertIn("one canonical timestamp representation", prompt) + def test_review_script_never_imports_from_the_pr_checkout(self) -> None: source = Path(run_codex_pr_review.__file__).read_text(encoding="utf-8") self.assertNotIn("SOURCE_ROOT = BRIDGE_ROOT.parent / \"source\"", source)