From b046033d32815e40b8abb3e96af8f4a7292eee3b Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Fri, 17 Jul 2026 03:26:35 +0800 Subject: [PATCH 1/2] fix(review): require reachability evidence for blockers Co-Authored-By: Claude Co-Authored-By: Codex --- docs/ai_autonomy_architecture.md | 6 ++++++ prompts/pr_review.md | 4 +++- scripts/run_codex_pr_review.py | 10 ++++++---- tests/test_run_codex_pr_review.py | 23 +++++++++++++++++++++++ 4 files changed, 38 insertions(+), 5 deletions(-) diff --git a/docs/ai_autonomy_architecture.md b/docs/ai_autonomy_architecture.md index 769ce8c..b9081be 100644 --- a/docs/ai_autonomy_architecture.md +++ b/docs/ai_autonomy_architecture.md @@ -223,6 +223,12 @@ trusted review comment 只保存最近固定轮数、固定字节上限且脱敏 已 `cleared` 的 finding key 是历史匹配边界，不得继续回溯并复活更旧的同 key blocker；未被 clear 的多个 current finding key 则必须从最近历史轮分别聚合后统一交给仲裁，不能只取第一个命中的 round。 +#### 3.3.2 Blocking finding 的可达性证据 + +repository Review 只有在 PR 上下文能够同时证明 exact changed path/line、merged current caller 或明确声明的 public untrusted boundary、当前配置和输入下可达，以及具体 correctness/security/data-integrity 影响时，才能给出 `critical` 或 `high`。证据不足的 finding 必须降为 `medium/low` 或省略，不能依靠 future consumer、伪造内部对象或通用 defense-in-depth 推测阻塞当前 PR。 + +Review 不得为了 hypothetical 风险要求新增 parser、store、registry 或 event-persistence 层。只有当前变更已经暴露对应真实边界，而且缺陷能从该边界到达时，才允许提出此类修改建议。 + ### P1：强烈建议补的缺口 #### 3.4 缺少统一的任务状态机 diff --git a/prompts/pr_review.md b/prompts/pr_review.md index 40dec18..e9ab47b 100644 --- a/prompts/pr_review.md +++ b/prompts/pr_review.md @@ -20,9 +20,11 @@ You are reviewing a pull request for a **production quantitative trading and dat ## Review completeness +- Assign **critical** or **high** only when the supplied PR context proves an exact changed path/line, a merged current caller or explicitly declared public untrusted boundary, reachability under current configuration and inputs, and concrete correctness, security, or data-integrity impact. State the reachability and impact in the description. If any element is missing, downgrade it to medium or low or omit it. +- Do not block on a hypothetical future consumer, forged internal object state, or generic defense-in-depth concern. Do not request a new parser, store, registry, or event-persistence layer unless the changed code already exposes that current boundary and the defect is reachable through it. - 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. +- Only for a public JSON/wire contract proven by the reachability rule above, check optional-key presence versus explicit null, recursive JSON-safe types, every identity-bearing integer range, one canonical timestamp representation, deterministic round-trips and digests, immutability, and identifier/path safety. ## Severity definitions diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py index fdf1ae1..6b5f931 100644 --- a/scripts/run_codex_pr_review.py +++ b/scripts/run_codex_pr_review.py @@ -325,10 +325,12 @@ 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. -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: +4. Assign **critical** or **high** only when the supplied PR context proves all of the following: an exact changed path and line; a merged current caller or an explicitly declared public untrusted boundary; reachability under the current configuration and inputs; and a concrete correctness, security, or data-integrity impact. State that reachability and impact in the finding description. If any element is missing, downgrade it to medium or low or omit it. +5. Do not block on a hypothetical future consumer, forged internal object state, or generic defense-in-depth concern. Do not request a new parser, store, registry, or event-persistence layer unless the changed code already exposes that current boundary and the defect is reachable through it. +6. Review the entire diff holistically and report all independent actionable findings in one response. Do not stop after the first blocking issue. +7. 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. +8. Only for a public JSON/wire contract proven by rule 4, check optional-key presence versus explicit null, recursive JSON-safe types, every identity-bearing integer range, one canonical timestamp representation, deterministic round-trips and digests, immutability, and identifier/path safety. +9. 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 09ff08e..bf9a330 100644 --- a/tests/test_run_codex_pr_review.py +++ b/tests/test_run_codex_pr_review.py @@ -50,6 +50,29 @@ def test_review_prompt_requires_holistic_contract_review(self) -> None: self.assertIn("every identity-bearing integer", prompt) self.assertIn("one canonical timestamp representation", prompt) + def test_review_prompt_requires_reachability_evidence_for_blockers(self) -> None: + prompt = run_codex_pr_review.build_review_prompt( + "diff", + "bounded implementation", + "Future consumers are out of scope.", + "org/repo", + ) + + self.assertIn("merged current caller", prompt) + self.assertIn("explicitly declared public untrusted boundary", prompt) + self.assertIn("current configuration and inputs", prompt) + self.assertIn("downgrade it to medium or low", prompt) + self.assertIn("hypothetical future consumer", prompt) + self.assertIn("Do not request a new parser, store, registry, or event-persistence layer", prompt) + + def test_repository_review_template_uses_the_same_reachability_gate(self) -> None: + template = run_codex_pr_review.PROMPT_TEMPLATE_PATH.read_text(encoding="utf-8") + + self.assertIn("merged current caller", template) + self.assertIn("explicitly declared public untrusted boundary", template) + self.assertIn("downgrade it to medium or low", template) + self.assertIn("hypothetical future consumer", template) + 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) From bd432fead44ed89842bc84519996cc00a5f31200 Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Fri, 17 Jul 2026 03:34:21 +0800 Subject: [PATCH 2/2] fix(review): include current PR entry points Co-Authored-By: Claude Co-Authored-By: Codex --- docs/ai_autonomy_architecture.md | 2 +- prompts/pr_review.md | 2 +- scripts/run_codex_pr_review.py | 2 +- tests/test_run_codex_pr_review.py | 6 ++++-- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/ai_autonomy_architecture.md b/docs/ai_autonomy_architecture.md index b9081be..a36d7ea 100644 --- a/docs/ai_autonomy_architecture.md +++ b/docs/ai_autonomy_architecture.md @@ -225,7 +225,7 @@ trusted review comment 只保存最近固定轮数、固定字节上限且脱敏 #### 3.3.2 Blocking finding 的可达性证据 -repository Review 只有在 PR 上下文能够同时证明 exact changed path/line、merged current caller 或明确声明的 public untrusted boundary、当前配置和输入下可达，以及具体 correctness/security/data-integrity 影响时，才能给出 `critical` 或 `high`。证据不足的 finding 必须降为 `medium/low` 或省略，不能依靠 future consumer、伪造内部对象或通用 defense-in-depth 推测阻塞当前 PR。 +repository Review 只有在 PR 上下文能够同时证明 exact changed path/line、当前 caller/entry point（可为既有路径或本 PR 新增路径）或明确声明的 public untrusted boundary、当前配置和输入下可达，以及具体 correctness/security/data-integrity 影响时，才能给出 `critical` 或 `high`。证据不足的 finding 必须降为 `medium/low` 或省略，不能依靠 future consumer、伪造内部对象或通用 defense-in-depth 推测阻塞当前 PR。 Review 不得为了 hypothetical 风险要求新增 parser、store、registry 或 event-persistence 层。只有当前变更已经暴露对应真实边界，而且缺陷能从该边界到达时，才允许提出此类修改建议。 diff --git a/prompts/pr_review.md b/prompts/pr_review.md index e9ab47b..7f5240d 100644 --- a/prompts/pr_review.md +++ b/prompts/pr_review.md @@ -20,7 +20,7 @@ You are reviewing a pull request for a **production quantitative trading and dat ## Review completeness -- Assign **critical** or **high** only when the supplied PR context proves an exact changed path/line, a merged current caller or explicitly declared public untrusted boundary, reachability under current configuration and inputs, and concrete correctness, security, or data-integrity impact. State the reachability and impact in the description. If any element is missing, downgrade it to medium or low or omit it. +- Assign **critical** or **high** only when the supplied PR context proves an exact changed path/line, a current caller or entry point proven by the supplied PR context whether pre-existing or introduced by this PR or an explicitly declared public untrusted boundary, reachability under current configuration and inputs, and concrete correctness, security, or data-integrity impact. State the reachability and impact in the description. If any element is missing, downgrade it to medium or low or omit it. - Do not block on a hypothetical future consumer, forged internal object state, or generic defense-in-depth concern. Do not request a new parser, store, registry, or event-persistence layer unless the changed code already exposes that current boundary and the defect is reachable through it. - 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. diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py index 6b5f931..f69229b 100644 --- a/scripts/run_codex_pr_review.py +++ b/scripts/run_codex_pr_review.py @@ -325,7 +325,7 @@ 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. Assign **critical** or **high** only when the supplied PR context proves all of the following: an exact changed path and line; a merged current caller or an explicitly declared public untrusted boundary; reachability under the current configuration and inputs; and a concrete correctness, security, or data-integrity impact. State that reachability and impact in the finding description. If any element is missing, downgrade it to medium or low or omit it. +4. Assign **critical** or **high** only when the supplied PR context proves all of the following: an exact changed path and line; a current caller or entry point proven by the supplied PR context, whether pre-existing or introduced by this PR, or an explicitly declared public untrusted boundary; reachability under the current configuration and inputs; and a concrete correctness, security, or data-integrity impact. State that reachability and impact in the finding description. If any element is missing, downgrade it to medium or low or omit it. 5. Do not block on a hypothetical future consumer, forged internal object state, or generic defense-in-depth concern. Do not request a new parser, store, registry, or event-persistence layer unless the changed code already exposes that current boundary and the defect is reachable through it. 6. Review the entire diff holistically and report all independent actionable findings in one response. Do not stop after the first blocking issue. 7. 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. diff --git a/tests/test_run_codex_pr_review.py b/tests/test_run_codex_pr_review.py index bf9a330..f91ee76 100644 --- a/tests/test_run_codex_pr_review.py +++ b/tests/test_run_codex_pr_review.py @@ -58,7 +58,8 @@ def test_review_prompt_requires_reachability_evidence_for_blockers(self) -> None "org/repo", ) - self.assertIn("merged current caller", prompt) + self.assertIn("current caller or entry point proven by the supplied PR context", prompt) + self.assertIn("introduced by this PR", prompt) self.assertIn("explicitly declared public untrusted boundary", prompt) self.assertIn("current configuration and inputs", prompt) self.assertIn("downgrade it to medium or low", prompt) @@ -68,7 +69,8 @@ def test_review_prompt_requires_reachability_evidence_for_blockers(self) -> None def test_repository_review_template_uses_the_same_reachability_gate(self) -> None: template = run_codex_pr_review.PROMPT_TEMPLATE_PATH.read_text(encoding="utf-8") - self.assertIn("merged current caller", template) + self.assertIn("current caller or entry point proven by the supplied PR context", template) + self.assertIn("introduced by this PR", template) self.assertIn("explicitly declared public untrusted boundary", template) self.assertIn("downgrade it to medium or low", template) self.assertIn("hypothetical future consumer", template)