diff --git a/docs/ai_autonomy_architecture.md b/docs/ai_autonomy_architecture.md index 769ce8c..a36d7ea 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、当前 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 层。只有当前变更已经暴露对应真实边界，而且缺陷能从该边界到达时，才允许提出此类修改建议。 + ### P1：强烈建议补的缺口 #### 3.4 缺少统一的任务状态机 diff --git a/prompts/pr_review.md b/prompts/pr_review.md index 40dec18..7f5240d 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 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. -- 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..f69229b 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 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. +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..f91ee76 100644 --- a/tests/test_run_codex_pr_review.py +++ b/tests/test_run_codex_pr_review.py @@ -50,6 +50,31 @@ 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("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) + 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("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) + 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)