fix(review): require reachability evidence for blockers

[Pigbibi]

Summary

• require Critical/High findings to prove a current caller or explicit public untrusted boundary, current reachability, and concrete impact
• downgrade or omit hypothetical future-consumer, forged-state, and generic defense-in-depth findings
• prohibit review-driven parser/store/registry/event-persistence expansion without a reachable current boundary

Scope

Prompt, prompt-construction text, focused tests, and architecture documentation only. No parser, evidence store, event persistence, workflow, permission, deployment, or runtime changes.

Verification

• python3 -m pytest -q tests/test_run_codex_pr_review.py -> 57 passed
• python3 -m pytest -q -> 655 passed, 1 skipped, 50 subtests
• python3 -m compileall -q scripts tests
• actionlint
• git diff --check
• ruff unavailable locally

Review contract

One tests-first concentrated closure maximum. Findings that do not prove a current caller/public boundary and reachability are advisory, not blockers.

[github-actions]

🤖 Codex PR Review

🚫 Merge blocked: 1 serious issue(s) found in high-risk files

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in prompts/pr_review.md

The new reachability gate rule states: assign critical/high 'only when the supplied PR context proves … a current caller or entry point proven by the supplied PR context.' This is self-referential: it conditions the severity on what is present in the PR context supplied to the AI reviewer. A malicious or incomplete PR description that omits caller/entry-point information would cause the AI reviewer to systematically downgrade genuine critical vulnerabilities to medium/low rather than flagging missing evidence as itself suspicious. The rule as written incentivizes omission over disclosure, creating a systematic blind spot for security findings in PRs with sparse context. (line 23)

Suggestion: Add an explicit instruction: if the PR context lacks caller/entry-point evidence but the code change touches a known public boundary (e.g., HTTP handler, CLI entrypoint, deserialization path), the reviewer must flag the missing evidence as a medium finding and may still escalate to high/critical when the code path is self-evidently public. Distinguish between 'evidence is absent from PR description' and 'evidence proves non-reachability.'

ℹ️ Other Findings

1. 🟠 [HIGH] Logic in tests/test_run_codex_pr_review.py

The existing test test_review_prompt_requires_holistic_contract_review asserts that the prompt contains 'every identity-bearing integer' and 'one canonical timestamp representation' as unconditional requirements. However, the updated prompt (rule 8) now gates those JSON/wire contract checks behind the reachability rule 4, making them conditional. The test still passes because the strings are present in the prompt text, but the semantic contract the test is meant to enforce (that these checks are always required) is now broken—the test no longer validates the intended invariant. A future refactor that removes those strings from the conditional clause would still satisfy rule 8 while breaking the old guarantee, and this test would fail to catch it. (line 50)

Suggestion: Update test_review_prompt_requires_holistic_contract_review to assert that the JSON/wire contract checks appear in the conditional/gated form (i.e., the string 'Only for a public JSON/wire contract proven by' precedes those check descriptions), or rename/repurpose the test to reflect the new conditional semantics. Add a separate test asserting the conditionality is present.

2. 🟡 [MEDIUM] Logic in scripts/run_codex_pr_review.py

Rule 5 in the updated prompt prohibits requesting 'a new parser, store, registry, or event-persistence layer unless the changed code already exposes that current boundary.' This prohibition is absolute and applies even when the AI reviewer identifies a real security defect (e.g., unsanitized input fed into an existing parser) that genuinely requires a validation layer. The rule conflates 'do not invent new infrastructure for hypothetical risks' with 'never recommend any new protective layer,' which could suppress valid remediation suggestions for real findings. (line 334)

Suggestion: Narrow rule 5 to: 'Do not request a new parser, store, registry, or event-persistence layer solely to address a hypothetical future consumer or forged internal state.' Preserve the reviewer's ability to recommend protective layers (input validation, sanitization wrappers) when the defect is proven reachable under rule 4.

---

Review by Codex PR Review bot • PR

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b046033d32

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Pigbibi]

Goal Driver reachability arbitration

Current head: bd432fead44ed89842bc84519996cc00a5f31200

• The supplied PR context includes the unified diff, not only the PR description. A changed HTTP/CLI/deserialization entry point that is self-evidently public is therefore a current entry point proven by the supplied context and may still receive High/Critical severity. Sparse prose does not suppress code evidence.
• The existing holistic-contract test checks prompt content, while the new focused tests check that those wire checks are conditional on a proven public boundary. The reported test-name concern does not change runtime behavior or leave the new contract untested.
• Rule 5 already permits a protective layer when the changed code exposes the current boundary and the defect is reachable; it only rejects infrastructure requested for hypothetical risks.

Required actionlint, test, and Codex Review Gate checks are successful; current unresolved threads are zero. These repository-review findings are advisory under the merged authority split and do not justify a second closure.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd432fead4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep crash bugs eligible for blocker severity

For PRs where the proven impact is a reachable production crash, this new gate allows critical/high only for correctness, security, or data-integrity impact and then says to downgrade or omit when any element is missing. That conflicts with the prompt’s own crash-risk priority and critical severity definition, and because evaluate_findings blocks solely on critical/high, a concrete null dereference or resource-exhaustion regression can now become non-blocking. Include crash/reliability impact in the allowed blocker criteria.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep API breaks eligible for blocker severity

When the supplied diff itself proves a changed public entry point or config format, this rule still prevents high unless the reviewer can frame the impact as correctness, security, or data-integrity. A signature/schema/config-format break is explicitly listed as a review priority and high-severity example below, but under this new gate the model can downgrade it, and evaluate_findings then lets the PR pass despite a reachable downstream compatibility break.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Do not require callers for credential leaks

For PRs that add a plaintext token or secret to a tracked file, the security impact is the exposure in the diff itself, not a reachable runtime caller or public input boundary. This new rule still requires a current caller/entry point or declared public untrusted boundary before allowing critical/high, so a credential leak can be downgraded to non-blocking even though credential leaks are the top review priority and evaluate_findings only blocks those two severities.

Useful? React with 👍 / 👎.