From 404908ef74ebfcd107eafa2bcfc48bf5a2837ebb Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Fri, 17 Jul 2026 01:51:23 +0800 Subject: [PATCH 1/2] fix: require reachable evidence for review blockers Co-Authored-By: Codex --- .github/workflows/codex_review_gate.yml | 15 ++-- docs/ai_autonomy_architecture.md | 6 +- prompts/pr_review.md | 11 ++- scripts/gate_codex_app_review.py | 79 ++++++++++----------- scripts/run_codex_pr_review.py | 47 +++++++++---- tests/test_gate_codex_app_review.py | 39 +++++++++++ tests/test_run_codex_pr_review.py | 91 +++++++++++++++++++++++-- 7 files changed, 215 insertions(+), 73 deletions(-) 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..23610c2b 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. Cite that path in `evidence`. +- 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": "Concrete changed call path or declared public boundary proving reachability", "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..7450ff70 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,21 @@ 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 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 r.get("commit_id") == head_sha + ): return r return None @@ -163,8 +164,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 +176,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,7 +224,7 @@ 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: @@ -227,7 +235,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 +245,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..158c19f3 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. Cite that concrete path in `evidence`. +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": "Concrete changed call path or declared public boundary proving reachability", "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) @@ -976,6 +982,8 @@ def evaluate_findings( severity = str(finding.get("severity", "")).strip().lower() file_path = str(finding.get("file", "")).strip() + evidence = finding.get("evidence") + has_blocking_evidence = type(evidence) is str and bool(evidence.strip()) # Classify the file's risk level if file_path not in file_risk_cache: @@ -987,12 +995,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 +1079,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 +1189,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 +1212,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 +1379,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 +1391,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..2b7faf69 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,44 @@ 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_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..00d26a5e 100644 --- a/tests/test_run_codex_pr_review.py +++ b/tests/test_run_codex_pr_review.py @@ -36,19 +36,52 @@ 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("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.", + } + + unproven = run_codex_pr_review.evaluate_findings([finding], changed_files, policy) + proven = run_codex_pr_review.evaluate_findings( + [ + { + **finding, + "evidence": ( + "service/handler.py calls review() with request.body from the public " + "POST /review boundary." + ), + } + ], + changed_files, + policy, + ) + + 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_review_script_never_imports_from_the_pr_checkout(self) -> None: source = Path(run_codex_pr_review.__file__).read_text(encoding="utf-8") @@ -125,6 +158,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 +278,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 +290,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 +1160,7 @@ def test_main_clears_repeated_finding_only_after_arbitration(self) -> None: "category": "security", "file": "scripts/run_codex_pr_review.py", "line": 1, + "evidence": "main() executes the changed blocking decision path.", "description": "example blocking finding", "suggestion": "fix it", } @@ -1301,6 +1381,7 @@ def test_main_ignores_legacy_bypass_policy_fields(self) -> None: "category": "security", "file": "scripts/run_codex_pr_review.py", "line": 1, + "evidence": "main() executes the changed blocking decision path.", "description": "example blocking finding", "suggestion": "fix it", } From 47579e3c62fbaa9e4812e7457b5bff3b8edf74fc Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Fri, 17 Jul 2026 02:04:22 +0800 Subject: [PATCH 2/2] fix: validate review reachability evidence Co-Authored-By: Codex --- prompts/pr_review.md | 4 +- scripts/gate_codex_app_review.py | 18 +++++++- scripts/run_codex_pr_review.py | 40 ++++++++++++++-- tests/test_gate_codex_app_review.py | 12 +++++ tests/test_run_codex_pr_review.py | 71 ++++++++++++++++++++++------- 5 files changed, 122 insertions(+), 23 deletions(-) diff --git a/prompts/pr_review.md b/prompts/pr_review.md index 23610c2b..9ed8a8f4 100644 --- a/prompts/pr_review.md +++ b/prompts/pr_review.md @@ -24,7 +24,7 @@ You are reviewing a pull request for a **production quantitative trading and dat - 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. -- 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. Cite that path in `evidence`. +- 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. @@ -50,7 +50,7 @@ Return exactly one JSON object (do not wrap in markdown fences): "category": "security", "file": "path/to/file.py", "line": 42, - "evidence": "Concrete changed call path or declared public boundary proving reachability", + "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 7450ff70..49d434d3 100644 --- a/scripts/gate_codex_app_review.py +++ b/scripts/gate_codex_app_review.py @@ -142,6 +142,10 @@ def run_static_guard(token: str, repo: str, pr_number: int) -> int: # ─── app review ────────────────────────────────────────────────────────────── +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, @@ -155,7 +159,7 @@ def get_codex_review( if ( isinstance(r, dict) and (r.get("user") or {}).get("login") == BOT_LOGIN - and r.get("commit_id") == head_sha + and review_matches_head(r, head_sha) ): return r return None @@ -227,7 +231,17 @@ def main() -> int: # ── 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}") diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py index 158c19f3..765a4c59 100644 --- a/scripts/run_codex_pr_review.py +++ b/scripts/run_codex_pr_review.py @@ -327,7 +327,7 @@ def build_review_prompt(diff: str, pr_title: str, pr_body: str, repo: str) -> st 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 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. 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. Cite that concrete path in `evidence`. +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. @@ -350,7 +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": "Concrete changed call path or declared public boundary proving reachability", + "evidence": "repository_call|service/handler.py|42|review(request.body)", "description": "What the problem is", "suggestion": "How to fix it" } @@ -950,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. @@ -983,7 +1014,10 @@ def evaluate_findings( severity = str(finding.get("severity", "")).strip().lower() file_path = str(finding.get("file", "")).strip() evidence = finding.get("evidence") - has_blocking_evidence = type(evidence) is str and bool(evidence.strip()) + 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: diff --git a/tests/test_gate_codex_app_review.py b/tests/test_gate_codex_app_review.py index 2b7faf69..b2dbf2fe 100644 --- a/tests/test_gate_codex_app_review.py +++ b/tests/test_gate_codex_app_review.py @@ -62,6 +62,18 @@ def test_connector_review_is_bound_to_current_head(self) -> None: 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 00d26a5e..0ebeed20 100644 --- a/tests/test_run_codex_pr_review.py +++ b/tests/test_run_codex_pr_review.py @@ -47,6 +47,8 @@ def test_review_prompt_requires_reachable_current_diff_evidence(self) -> None: self.assertIn("Do not stop after the first blocking issue", prompt) self.assertIn("clean-slate", 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) @@ -63,26 +65,63 @@ def test_findings_without_concrete_reachability_evidence_cannot_block(self) -> N "suggestion": "Add another public parser.", } - unproven = run_codex_pr_review.evaluate_findings([finding], changed_files, policy) - proven = run_codex_pr_review.evaluate_findings( - [ - { - **finding, - "evidence": ( - "service/handler.py calls review() with request.body from the public " - "POST /review boundary." - ), - } - ], - changed_files, - policy, - ) + 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") self.assertNotIn("SOURCE_ROOT = BRIDGE_ROOT.parent / \"source\"", source) @@ -1160,7 +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": "main() executes the changed blocking decision path.", + "evidence": "public_boundary|scripts/run_codex_pr_review.py|1|#!/usr/bin/env python3", "description": "example blocking finding", "suggestion": "fix it", } @@ -1381,7 +1420,7 @@ def test_main_ignores_legacy_bypass_policy_fields(self) -> None: "category": "security", "file": "scripts/run_codex_pr_review.py", "line": 1, - "evidence": "main() executes the changed blocking decision path.", + "evidence": "public_boundary|scripts/run_codex_pr_review.py|1|#!/usr/bin/env python3", "description": "example blocking finding", "suggestion": "fix it", }