Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions prompts/pr_review.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
5 changes: 4 additions & 1 deletion scripts/run_codex_pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions tests/test_run_codex_pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading