fix: make PR review contract checks holistic (#89)

Co-authored-by: Codex <noreply@openai.com>

Author: Pigbibi

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 |

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

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)