diff --git a/.github/workflows/codex_review_gate.yml b/.github/workflows/codex_review_gate.yml index 673a5dec..b2c9465c 100644 --- a/.github/workflows/codex_review_gate.yml +++ b/.github/workflows/codex_review_gate.yml @@ -1,14 +1,15 @@ name: Codex Review Gate -# Two-mode gate for the Codex GitHub App (chatgpt-codex-connector): +# Static gate plus advisory observer for the Codex GitHub App +# (chatgpt-codex-connector): # -# 1. WAIT mode (PR opened/synchronized): -# Creates a pending check → polls for Codex review up to N minutes. -# If Codex responds → check reflects review state. -# If Codex times out → check passes (don't block when App is broken). +# 1. PR mode (PR opened/synchronized): +# Runs the blocking static security guard, then observes any current-head +# connector review without polling. # # 2. REACT mode (Codex submits review): -# Updates check instantly — CHANGES_REQUESTED → fail, APPROVED → pass. +# Reports the advisory review immediately. Connector verdicts never block; +# the repository-owned Codex PR Review workflow is the AI merge gate. # # No API keys needed — reads the App's review via GitHub API. @@ -39,8 +40,6 @@ jobs: timeout-minutes: 15 env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" - CODEX_GATE_POLL_SECONDS: ${{ vars.CODEX_GATE_POLL_SECONDS || '30' }} - CODEX_GATE_MAX_WAIT_MINUTES: ${{ vars.CODEX_GATE_MAX_WAIT_MINUTES || '5' }} steps: - name: Checkout diff --git a/docs/ai_autonomy_architecture.md b/docs/ai_autonomy_architecture.md index d04509d6..06d8b851 100644 --- a/docs/ai_autonomy_architecture.md +++ b/docs/ai_autonomy_architecture.md @@ -205,7 +205,11 @@ AIAuditBridge 是 QuantStrategyLab 的 AI 审计控制面，负责： Contract Oscillation Guard 是 `AIAuditBridge` 的中央 PR review gate 语义，不是要求每个消费者仓库新增一套 branch rule。消费者仍使用原有 required check、branch protection 和 merge queue；guard 不提供 label、admin 或人工确认绕过。 -trusted review comment 只保存最近固定轮数、固定字节上限且脱敏后的 blocking finding 摘要，包括 head SHA、file、category、severity、description 和 suggestion。历史只能由已验证的 review bot comment 恢复；legacy comment 没有 history marker 时保持兼容，但既有 blocker 会被迁移为 `invalid_history` 并继续 fail closed，不能因一次 clean review 自动清除。畸形或超限 history 同样 fail closed。 +仓库自有的 `Codex PR Review` 是唯一具有 finding fingerprint、contract arbitration、remediation/freeze 语义的 AI 阻断源。GitHub 官方 `chatgpt-codex-connector` review 仅作为 advisory：`Codex Review Gate` 仍执行可阻断的静态 secret/path/metadata 检查，但 connector 的 `CHANGES_REQUESTED` 不得进入自有 finding history，也不得触发 closure、freeze、merge 或 reslice。connector review 通过 `pull_request_review: submitted` 事件即时报告，不再轮询等待。 + +blocking finding 必须给出当前 diff 到真实 repository caller 或已声明 public untrusted boundary 的可达证据。私有 typed value 不因假设存在 raw JSON caller、`object.__new__`/`object.__setattr__`、自定义 stateful mapping、损坏私有文件或未来 consumer 而升级成阻断项；JSON/wire 审计只覆盖当前变更实际暴露的边界与契约。缺少具体 `evidence` 的 finding 可保留为 advisory，但不能阻断合并。 + +trusted review comment 只保存最近固定轮数、固定字节上限且脱敏后的 blocking finding 摘要，包括 head SHA、file、category、severity、evidence、description 和 suggestion。历史只能由已验证的 review bot comment 恢复；legacy comment 没有 history marker 时保持兼容，但既有 blocker 会被迁移为 `invalid_history` 并继续 fail closed，不能因一次 clean review 自动清除。畸形或超限 history 同样 fail closed。 若 `overflow` / `invalid_history` 状态中没有可供仲裁的 trusted prior finding，系统不得用空上下文自动 `clear`。此时需要人工确认 source-of-truth 后修复或删除损坏的 trusted bot state，再重新运行普通 required review check；这只恢复可审计状态，不直接放行 merge，也不绕过 branch protection。 diff --git a/prompts/pr_review.md b/prompts/pr_review.md index 40dec18c..9ed8a8f4 100644 --- a/prompts/pr_review.md +++ b/prompts/pr_review.md @@ -17,19 +17,23 @@ You are reviewing a pull request for a **production quantitative trading and dat - Documentation quality - Minor refactoring opportunities - Test coverage suggestions +- Hypothetical hardening for unsupported inputs or callers that do not exist in the repository +- Object-forging or mutation escape hatches unless a repository-backed caller actually uses them ## Review completeness -- Review the entire diff holistically and report all independent actionable findings in one response. Do not stop after the first blocking issue. +- Review the entire diff holistically and report all independent reachable 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. +- Emit a finding only when the current diff causes or exposes a defect on a supported input through a repository-backed caller or a declared public untrusted boundary. Encode evidence as `kind|path|line|symbol`, where `kind` is `repository_call` or `public_boundary`, `path` is repository-relative, and `symbol` appears on that exact line in the current checkout. Free-form evidence is advisory only. +- Do not invent a raw JSON/parser boundary for private typed values. Do not treat `object.__new__`, `object.__setattr__`, `dataclasses.replace`, custom stateful mappings, corrupted private files, or similar escape hatches as reachable unless the repository or PR contract explicitly exposes them. +- For JSON/wire code, check only requirements evidenced by the changed public boundary and its real callers. Do not expand the task into a generic canonicalization or adversarial-parser checklist. ## Severity definitions | Severity | Definition | Example | |----------|-----------|---------| | critical | Causes data loss, security breach, or production crash | SQL injection, credential in plaintext, deletion without backup | -| high | Produces wrong results or breaks downstream systems | Wrong formula, API signature change, resource leak | +| high | A concrete supported call path produces wrong results or breaks downstream systems | Wrong formula, reachable API signature change, resource leak | | medium | Degrades reliability or performance under load | Missing error handling, N+1 query, unbounded growth | | low | Misleading or confusing but not dangerous | Stale comment, redundant code, unclear intent | @@ -46,6 +50,7 @@ Return exactly one JSON object (do not wrap in markdown fences): "category": "security", "file": "path/to/file.py", "line": 42, + "evidence": "repository_call|service/handler.py|42|review(request.body)", "description": "What's wrong", "suggestion": "How to fix it" } diff --git a/scripts/gate_codex_app_review.py b/scripts/gate_codex_app_review.py index 5a813374..49d434d3 100644 --- a/scripts/gate_codex_app_review.py +++ b/scripts/gate_codex_app_review.py @@ -1,14 +1,14 @@ #!/usr/bin/env python3 -"""PR merge gate: static scan + Codex App review → job exit code = check status. +"""PR check: blocking static scan plus advisory Codex App review. Two phases, zero API keys needed: 1. STATIC — scan diff for secrets, blocked files, metadata issues (<30s). Fail job immediately on hard violations. - 2. WAIT — poll for Codex GitHub App review up to N min. - Fail job on CHANGES_REQUESTED, pass on APPROVED/timeout. - 3. REACT — on Codex bot review submitted: update instantly. + 2. OBSERVE — read a current-head Codex GitHub App review without polling. + 3. REACT — on Codex bot review submitted: report it immediately. -The workflow job IS the check — exit 0 = pass, exit 1 = fail. +Only the static guard can fail this check. The repository-owned Codex PR Review +workflow remains the authoritative AI review gate. """ from __future__ import annotations @@ -16,7 +16,6 @@ import json import os import sys -import time import urllib.error import urllib.request from pathlib import Path @@ -66,13 +65,6 @@ def env(name: str, default: str = "") -> str: return os.environ.get(name, default).strip() -def env_int(name: str, default: int) -> int: - try: - return int(env(name, str(default))) - except ValueError: - return default - - def github_request(token: str, method: str, path: str, payload: dict[str, Any] | None = None) -> Any: url = f"{API_BASE}{path}" if not path.startswith("https://") else path @@ -150,12 +142,25 @@ def run_static_guard(token: str, repo: str, pr_number: int) -> int: # ─── app review ────────────────────────────────────────────────────────────── -def get_codex_review(token: str, repo: str, pr_number: int) -> dict[str, Any] | None: +def review_matches_head(review: dict[str, Any], head_sha: str) -> bool: + return type(review.get("commit_id")) is str and review["commit_id"] == head_sha + + +def get_codex_review( + token: str, + repo: str, + pr_number: int, + head_sha: str, +) -> dict[str, Any] | None: reviews = github_request(token, "GET", f"/repos/{repo}/pulls/{pr_number}/reviews?per_page=100") if not isinstance(reviews, list): return None for r in reversed(reviews): - if isinstance(r, dict) and (r.get("user") or {}).get("login") == BOT_LOGIN: + if ( + isinstance(r, dict) + and (r.get("user") or {}).get("login") == BOT_LOGIN + and review_matches_head(r, head_sha) + ): return r return None @@ -163,8 +168,11 @@ def get_codex_review(token: str, repo: str, pr_number: int) -> dict[str, Any] | def app_decision(review: dict[str, Any] | None) -> tuple[int, str, str]: """(exit_code, title, summary)""" if review is None: - return (0, "Codex: no review — passed through", - "Codex did not respond in time. Merge allowed to avoid blocking development.") + return ( + 0, + "Codex advisory: no current-head review", + "No current-head connector review is available; this advisory does not block merge.", + ) state = (review.get("state") or "").strip().upper() url = review.get("html_url", "") body = (review.get("body") or "").strip() @@ -172,11 +180,15 @@ def app_decision(review: dict[str, Any] | None) -> tuple[int, str, str]: if state == "CHANGES_REQUESTED": snippet = (body[:500] + "...") if len(body) > 500 else body - return (1, "Codex: changes requested — MERGE BLOCKED", - f"Codex **requested changes** at {at}.\n\n{snippet}\n\n[View review]({url})") + return ( + 0, + "Codex advisory: changes requested", + f"Codex **requested changes** at {at}, but this advisory does not block merge.\n\n" + f"{snippet}\n\n[View review]({url})", + ) if state == "APPROVED": - return (0, "Codex: approved", f"Codex approved at {at}. [View review]({url})") - return (0, f"Codex: reviewed ({state.lower()})", + return (0, "Codex advisory: approved", f"Codex approved at {at}. [View review]({url})") + return (0, f"Codex advisory: reviewed ({state.lower()})", f"Codex state `{state}` at {at}. Not blocking. [View review]({url})") @@ -216,10 +228,20 @@ def main() -> int: return 1 print("STATIC → clean") - # ── Phase 2: App review ─────────────────────────────────────────── + # ── Phase 2: advisory App review ────────────────────────────────── # REACT: Codex just submitted a review review_event = event.get("review") or {} - if event_name == "pull_request_review" and (review_event.get("user") or {}).get("login") == BOT_LOGIN: + if ( + event_name == "pull_request_review" + and (review_event.get("user") or {}).get("login") == BOT_LOGIN + ): + if not review_matches_head(review_event, head_sha): + print("REACT → stale connector review ignored") + step_summary( + "## Codex advisory ignored\n\n" + "The submitted connector review does not match the current PR head." + ) + return 0 rc, title, summary = app_decision(review_event) print(f"REACT → exit={rc}: {title}") step_summary(f"## {title}\n\n{summary}") @@ -227,7 +249,7 @@ def main() -> int: # WAIT: poll for existing or upcoming review try: - existing = get_codex_review(token, repo, pr_number) + existing = get_codex_review(token, repo, pr_number, head_sha) except RuntimeError: existing = None @@ -237,26 +259,11 @@ def main() -> int: step_summary(f"## {title}\n\n{summary}") return rc - poll_s = env_int("CODEX_GATE_POLL_SECONDS", 30) - max_w = env_int("CODEX_GATE_MAX_WAIT_MINUTES", 5) - deadline = time.time() + max_w * 60 - print(f"WAIT → polling every {poll_s}s for up to {max_w}min") - - while time.time() < deadline: - time.sleep(poll_s) - try: - review = get_codex_review(token, repo, pr_number) - except RuntimeError: - continue - if review is not None: - rc, title, summary = app_decision(review) - print(f"WAIT → found review → exit={rc}: {title}") - step_summary(f"## {title}\n\n{summary}") - return rc - - # Timeout - print(f"TIMEOUT → Codex did not respond in {max_w}min; passing through") - step_summary(f"## Codex: timeout after {max_w}min\n\nPassed through to avoid blocking development.") + print("ADVISORY → no current-head connector review; not waiting") + step_summary( + "## Codex advisory pending\n\n" + "No current-head connector review is available. The event-driven review hook will report it when submitted." + ) return 0 diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py index fdf1ae1a..765a4c59 100644 --- a/scripts/run_codex_pr_review.py +++ b/scripts/run_codex_pr_review.py @@ -325,12 +325,15 @@ 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. Review the entire diff holistically and report all independent actionable findings in one response. Do not stop after the first blocking issue. +4. Review the entire diff holistically and report all independent reachable 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: +6. Emit a finding only when the current diff causes or exposes a defect on a supported input through a repository-backed caller or a declared public untrusted boundary. Encode machine-checkable evidence as `kind|path|line|symbol`, where `kind` is `repository_call` or `public_boundary`, `path` is repository-relative, and `symbol` appears on that exact line in the current checkout. Free-form evidence is advisory only. +7. Do not invent a raw JSON/parser boundary for private typed values. Do not treat `object.__new__`, `object.__setattr__`, `dataclasses.replace`, custom stateful mappings, corrupted private files, or similar escape hatches as reachable unless the repository or PR contract explicitly exposes them. +8. For JSON/wire code, check only requirements evidenced by the changed public boundary and its real callers. Do not expand the task into a generic canonicalization or adversarial-parser checklist. +9. Do not flag hypothetical future consumers, general defense-in-depth, or robustness outside the PR goal. If reachability cannot be proven from repository or PR evidence, omit the finding. +10. 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 + - **high**: concrete supported call path produces wrong results, reachable API break, memory/connection leak - **medium**: missing error handling, performance degradation, race condition - **low**: misleading comment, unclear variable name, redundant code @@ -347,6 +350,7 @@ def build_review_prompt(diff: str, pr_title: str, pr_body: str, repo: str) -> st "category": "security|bug|performance|logic|reliability", "file": "relative/path/to/file.py", "line": 42, + "evidence": "repository_call|service/handler.py|42|review(request.body)", "description": "What the problem is", "suggestion": "How to fix it" } @@ -813,13 +817,15 @@ def blocking_finding_fingerprint(findings: list[dict[str, Any]]) -> str: for finding in findings: if not isinstance(finding, dict): continue - normalized.append( - { - "category": str(finding.get("category") or "").strip().lower(), - "file": str(finding.get("file") or "").strip(), - "severity": str(finding.get("severity") or "").strip().lower(), - } - ) + identity = { + "category": str(finding.get("category") or "").strip().lower(), + "file": str(finding.get("file") or "").strip(), + "severity": str(finding.get("severity") or "").strip().lower(), + } + evidence = finding.get("evidence") + if type(evidence) is str and evidence.strip(): + identity["evidence"] = re.sub(r"\s+", " ", evidence).strip().lower() + normalized.append(identity) if not normalized: return "" payload = json.dumps(sorted(normalized, key=lambda item: json.dumps(item, sort_keys=True)), sort_keys=True) @@ -944,10 +950,41 @@ def apply_arbitration_failure( # --------------------------------------------------------------------------- +def _blocking_evidence_matches_repository(value: Any, *, repo_root: Path) -> bool: + """Accept only evidence that resolves to an exact line in this checkout.""" + if type(value) is not str or len(value) > FINDING_HISTORY_TEXT_LIMIT: + return False + parts = value.split("|", 3) + if len(parts) != 4: + return False + kind, raw_path, raw_line, symbol = (part.strip() for part in parts) + if kind not in {"repository_call", "public_boundary"}: + return False + if not raw_path or Path(raw_path).is_absolute() or ".." in Path(raw_path).parts: + return False + if not raw_line.isascii() or not raw_line.isdecimal() or raw_line.startswith("0"): + return False + line_number = int(raw_line) + if line_number < 1 or not symbol or any(ord(char) < 32 for char in symbol): + return False + try: + root = repo_root.resolve(strict=True) + candidate = (root / raw_path).resolve(strict=True) + candidate.relative_to(root) + if not candidate.is_file(): + return False + lines = candidate.read_text(encoding="utf-8").splitlines() + except (OSError, UnicodeError, ValueError): + return False + return line_number <= len(lines) and symbol in lines[line_number - 1] + + def evaluate_findings( findings: list[dict[str, Any]], changed_files: list[dict[str, Any]], policy: dict[str, Any], + *, + repo_root: Path | None = None, ) -> dict[str, Any]: """Evaluate Codex findings against the risk policy. @@ -976,6 +1013,11 @@ def evaluate_findings( severity = str(finding.get("severity", "")).strip().lower() file_path = str(finding.get("file", "")).strip() + evidence = finding.get("evidence") + has_blocking_evidence = _blocking_evidence_matches_repository( + evidence, + repo_root=repo_root or ROOT, + ) # Classify the file's risk level if file_path not in file_risk_cache: @@ -987,12 +1029,14 @@ def evaluate_findings( severity in BLOCK_SEVERITIES and file_risk == "high" and file_path in changed_paths # only block on actually changed files + and has_blocking_evidence ) enriched = { **finding, "file_risk": file_risk, "file_risk_reason": file_risk_reason, + "blocking_evidence": has_blocking_evidence, } if should_block: @@ -1069,6 +1113,7 @@ def _history_finding(finding: dict[str, Any]) -> dict[str, str]: "severity": _sanitize_history_text(finding.get("severity"), 20).lower(), "category": _sanitize_history_text(finding.get("category"), 80).lower(), "file": _sanitize_history_path(finding.get("file")), + "evidence": _sanitize_history_text(finding.get("evidence")), "description": _sanitize_history_text(finding.get("description")), "suggestion": _sanitize_history_text(finding.get("suggestion")), } @@ -1178,9 +1223,13 @@ def parse_finding_history(body: str) -> tuple[list[dict[str, Any]], bool]: "severity": 20, "category": 80, "file": 300, + "evidence": FINDING_HISTORY_TEXT_LIMIT, "description": FINDING_HISTORY_TEXT_LIMIT, "suggestion": FINDING_HISTORY_TEXT_LIMIT, } + legacy_field_limits = { + field: limit for field, limit in field_limits.items() if field != "evidence" + } for round_state in rounds: if not isinstance(round_state, dict): return [], False @@ -1197,11 +1246,14 @@ def parse_finding_history(body: str) -> tuple[list[dict[str, Any]], bool]: return [], False checked_findings: list[dict[str, str]] = [] for finding in findings: - if not isinstance(finding, dict) or set(finding) != set(field_limits): + if not isinstance(finding, dict): + return [], False + limits = field_limits if set(finding) == set(field_limits) else legacy_field_limits + if set(finding) != set(limits): return [], False if any( not isinstance(finding[field], str) or len(finding[field]) > limit - for field, limit in field_limits.items() + for field, limit in limits.items() ): return [], False checked_findings.append(dict(finding)) @@ -1361,6 +1413,7 @@ def _format_finding(index: int, finding: dict[str, Any]) -> list[str]: file_path = finding.get("file", "?") line = finding.get("line") description = finding.get("description", "No description") + evidence = finding.get("evidence", "") suggestion = finding.get("suggestion", "") emoji = {"critical": "🔴", "high": "🟠", "medium": "🟡", "low": "🔵"}.get(severity, "⚪") @@ -1372,6 +1425,8 @@ def _format_finding(index: int, finding: dict[str, Any]) -> list[str]: ] if line: lines[-1] += f" (line {line})" + if evidence: + lines.extend(["", f"**Evidence:** {evidence}"]) if suggestion: lines.extend(["", f"**Suggestion:** {suggestion}"]) lines.append("") diff --git a/tests/test_gate_codex_app_review.py b/tests/test_gate_codex_app_review.py index 923403c1..b2dbf2fe 100644 --- a/tests/test_gate_codex_app_review.py +++ b/tests/test_gate_codex_app_review.py @@ -5,6 +5,7 @@ import sys import unittest from pathlib import Path +from unittest.mock import patch ROOT = Path(__file__).resolve().parents[1] @@ -23,6 +24,56 @@ class GateCodexAppReviewTest(unittest.TestCase): + def test_connector_changes_requested_is_advisory(self) -> None: + exit_code, title, summary = gate_codex_app_review.app_decision( + { + "state": "CHANGES_REQUESTED", + "body": "Consider another validation layer.", + "submitted_at": "2026-07-17T00:00:00Z", + "html_url": "https://example.test/review/1", + } + ) + + self.assertEqual(exit_code, 0) + self.assertIn("advisory", title.lower()) + self.assertIn("does not block", summary.lower()) + + def test_connector_review_is_bound_to_current_head(self) -> None: + reviews = [ + { + "user": {"login": gate_codex_app_review.BOT_LOGIN}, + "commit_id": "old-head", + "state": "CHANGES_REQUESTED", + }, + { + "user": {"login": gate_codex_app_review.BOT_LOGIN}, + "commit_id": "current-head", + "state": "COMMENTED", + }, + ] + with patch.object(gate_codex_app_review, "github_request", return_value=reviews): + current = gate_codex_app_review.get_codex_review( + "token", "org/repo", 7, "current-head" + ) + missing = gate_codex_app_review.get_codex_review( + "token", "org/repo", 7, "other-head" + ) + + self.assertEqual(current, reviews[1]) + self.assertIsNone(missing) + + def test_review_event_is_ignored_when_commit_is_not_current_head(self) -> None: + self.assertFalse( + gate_codex_app_review.review_matches_head( + {"commit_id": "old-head"}, "current-head" + ) + ) + self.assertTrue( + gate_codex_app_review.review_matches_head( + {"commit_id": "current-head"}, "current-head" + ) + ) + def test_wrapper_exports_shared_static_functions(self) -> None: sample = "diff --git a/example.py b/example.py\n+++ b/example.py\n+api_key= \"x\"" files = [{"filename": "example.py", "status": "modified", "additions": 0, "deletions": 0}] diff --git a/tests/test_run_codex_pr_review.py b/tests/test_run_codex_pr_review.py index 09ff08e5..0ebeed20 100644 --- a/tests/test_run_codex_pr_review.py +++ b/tests/test_run_codex_pr_review.py @@ -36,19 +36,91 @@ 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: + def test_review_prompt_requires_reachable_current_diff_evidence(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("report all independent reachable 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) + self.assertIn("repository-backed caller or a declared public untrusted boundary", prompt) + self.assertIn("kind|path|line|symbol", prompt) + self.assertIn("Free-form evidence is advisory only", prompt) + self.assertIn("Do not invent a raw JSON/parser boundary", prompt) + self.assertIn('"evidence"', prompt) + self.assertNotIn("systematically check optional-key presence", prompt) + + def test_findings_without_concrete_reachability_evidence_cannot_block(self) -> None: + policy = run_codex_pr_review.load_policy() + changed_files = [{"filename": "service/review.py", "status": "modified"}] + finding = { + "severity": "high", + "category": "logic", + "file": "service/review.py", + "line": 11, + "description": "A hypothetical caller could forge private state.", + "suggestion": "Add another public parser.", + } + + with tempfile.TemporaryDirectory() as tmpdir: + caller = Path(tmpdir) / "service" / "handler.py" + caller.parent.mkdir(parents=True) + caller.write_text("result = review(request.body)\n", encoding="utf-8") + unproven = run_codex_pr_review.evaluate_findings( + [{**finding, "evidence": "A public handler calls review()."}], + changed_files, + policy, + repo_root=Path(tmpdir), + ) + proven = run_codex_pr_review.evaluate_findings( + [ + { + **finding, + "evidence": "repository_call|service/handler.py|1|review(request.body)", + } + ], + changed_files, + policy, + repo_root=Path(tmpdir), + ) + + self.assertFalse(unproven["blocked"]) + self.assertEqual(unproven["non_blocking_findings"][0]["blocking_evidence"], False) + self.assertTrue(proven["blocked"]) + self.assertEqual(proven["blocking_findings"][0]["blocking_evidence"], True) + + def test_blocking_evidence_must_match_repository_line(self) -> None: + policy = run_codex_pr_review.load_policy() + changed_files = [{"filename": "service/review.py", "status": "modified"}] + finding = { + "severity": "high", + "category": "logic", + "file": "service/review.py", + "line": 11, + "description": "Wrong result.", + "suggestion": "Fix it.", + } + with tempfile.TemporaryDirectory() as tmpdir: + caller = Path(tmpdir) / "service" / "handler.py" + caller.parent.mkdir(parents=True) + caller.write_text("result = review(request.body)\n", encoding="utf-8") + decision = run_codex_pr_review.evaluate_findings( + [ + { + **finding, + "evidence": "repository_call|service/handler.py|1|submit(request.body)", + } + ], + changed_files, + policy, + repo_root=Path(tmpdir), + ) + + self.assertFalse(decision["blocked"]) + self.assertFalse(decision["non_blocking_findings"][0]["blocking_evidence"]) def test_review_script_never_imports_from_the_pr_checkout(self) -> None: source = Path(run_codex_pr_review.__file__).read_text(encoding="utf-8") @@ -125,6 +197,28 @@ def test_repeated_findings_are_fingerprinted_before_arbitration(self) -> None: self.assertTrue(run_codex_pr_review.should_arbitrate(blocked=True, streak=2, repeated=True, new_head=True)) self.assertFalse(run_codex_pr_review.should_arbitrate(blocked=True, streak=2, repeated=True, new_head=False)) + def test_fingerprint_distinguishes_different_reachability_evidence(self) -> None: + base = { + "severity": "high", + "category": "logic", + "file": "service/review.py", + "description": "Wrong result.", + "suggestion": "Fix it.", + } + first = [{**base, "evidence": "public submit() passes an empty account id"}] + reworded = [ + { + **base, + "description": "The result is incorrect.", + "evidence": "public submit() passes an empty account id", + } + ] + different = [{**base, "evidence": "scheduler retry calls close() twice"}] + + fingerprint = run_codex_pr_review.blocking_finding_fingerprint(first) + self.assertEqual(fingerprint, run_codex_pr_review.blocking_finding_fingerprint(reworded)) + self.assertNotEqual(fingerprint, run_codex_pr_review.blocking_finding_fingerprint(different)) + def test_parse_arbitration_output_requires_supported_verdict(self) -> None: self.assertEqual( run_codex_pr_review.parse_arbitration_output('{"verdict":"clear","reason":"covered by the regression test"}'), @@ -223,6 +317,7 @@ def test_finding_history_round_trips_sanitized_blocking_contracts(self) -> None: "severity": "high", "category": "contract", "file": "service/review.py", + "evidence": "service/handler.py passes the public request body to review().", "description": "Missing panel returns a structured result; token=secret-value", "suggestion": "Return ReviewResult(blocked=True).", } @@ -234,9 +329,32 @@ def test_finding_history_round_trips_sanitized_blocking_contracts(self) -> None: self.assertTrue(valid) self.assertEqual(history[0]["head_sha"], "abc1234") self.assertEqual(history[0]["findings"][0]["file"], "service/review.py") + self.assertIn("handler.py", history[0]["findings"][0]["evidence"]) self.assertIn("[REDACTED]", history[0]["findings"][0]["description"]) self.assertNotIn("secret-value", marker) + def test_review_comment_exposes_blocking_evidence(self) -> None: + body = run_codex_pr_review.build_pr_comment( + { + "summary": "blocked", + "blocking_findings": [ + { + "severity": "high", + "category": "logic", + "file": "service/review.py", + "line": 7, + "evidence": "public submit() calls review() with request.body", + "description": "The supported request returns the wrong result.", + "suggestion": "Use the validated value.", + } + ], + "non_blocking_findings": [], + }, + "https://example.test/pr/7", + ) + + self.assertIn("**Evidence:** public submit() calls review() with request.body", body) + def test_finding_history_is_bounded_and_legacy_comments_remain_compatible(self) -> None: history: list[dict[str, object]] = [] finding = { @@ -1081,6 +1199,7 @@ def test_main_clears_repeated_finding_only_after_arbitration(self) -> None: "category": "security", "file": "scripts/run_codex_pr_review.py", "line": 1, + "evidence": "public_boundary|scripts/run_codex_pr_review.py|1|#!/usr/bin/env python3", "description": "example blocking finding", "suggestion": "fix it", } @@ -1301,6 +1420,7 @@ def test_main_ignores_legacy_bypass_policy_fields(self) -> None: "category": "security", "file": "scripts/run_codex_pr_review.py", "line": 1, + "evidence": "public_boundary|scripts/run_codex_pr_review.py|1|#!/usr/bin/env python3", "description": "example blocking finding", "suggestion": "fix it", }