fix: keep PR review blockers evidence-based

[Pigbibi]

Summary

• keep the repository-owned Codex PR Review as the only AI blocking authority
• retain blocking static secret/path/metadata checks while making the official Codex connector advisory and event-driven (no polling)
• require concrete repository caller/public-boundary evidence before a Critical/High finding can block
• include reachability evidence in finding identity/history so unrelated findings in the same file do not share a repeated fingerprint

Scope

• no model/provider/dependency/permission/deploy changes
• no merge, remediation, or live execution automation changes
• no integration with PR feat: emit PR review completion events #90 notification state

Validation

• python3 -m pytest -q tests/test_run_codex_pr_review.py tests/test_gate_codex_app_review.py — 65 passed
• python3 -m pytest -q — 654 passed, 1 skipped, 50 subtests
• python3 -m compileall -q scripts tests
• actionlint .github/workflows/codex_review_gate.yml
• git diff --check

Risk

The official connector can no longer block merge by itself. The static security guard remains blocking, and the repository-owned Codex PR Review remains the authoritative AI review gate.

[github-actions]

🤖 Codex PR Review

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

⚖️ Codex Review Arbitration

🚫 block: At least three current blockers remain valid against the shown PR diff. In .github/workflows/codex_review_gate.yml the same Codex Review Gate workflow is repurposed for pull_request_review reporting, while scripts/gate_codex_app_review.py only calls run_static_guard() on pull_request and returns success for advisory connector outcomes on review events, so the required check context can be re-reported green without rerunning the blocking static guard. In scripts/run_codex_pr_review.py, _blocking_evidence_matches_repository() still accepts any repository_call|... or public_boundary|... string whose symbol is merely a substring of the cited line; it does not verify an actual call site or a declared boundary, and the added tests explicitly preserve acceptance of public_boundary|scripts/run_codex_pr_review.py|1|#!/usr/bin/env python3, which is not semantic reachability evidence. Also in scripts/run_codex_pr_review.py, _format_finding() renders evidence verbatim into the PR comment, and tests/test_run_codex_pr_review.py now locks in that raw echo; combined with the new contract that blocking evidence includes an exact repository line symbol, this creates a direct comment-level secret/DSN/token exfiltration path. The prior high/logic finding on machine-checkable evidence is not contradicted: the current high/logic finding strengthens the same required behavior rather than reversing it.

🚫 Blocking Issues

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

1. 🔴 [CRITICAL] Security in .github/workflows/codex_review_gate.yml

The workflow now runs the same required Codex Review Gate check on pull_request_review events, but scripts/gate_codex_app_review.py skips run_static_guard() for review-only events and always exits success for connector reviews. That means a PR with a failing static secret/path/metadata check can be followed by a bot review on the same head SHA that produces a green run for the same check context, effectively clearing the blocking static verdict without fixing the PR. (line 15)

Suggestion: Do not reuse the required static-gate workflow for advisory review events. Move the connector observer into a separate non-required workflow/check name, or rerun the static guard on pull_request_review events before reporting success.

2. 🟠 [HIGH] Logic in scripts/run_codex_pr_review.py

_blocking_evidence_matches_repository() only verifies that the supplied symbol string appears somewhere on the cited line of some repository file. It does not verify that the line is actually a real repository caller or a declared public boundary, so evidence like public_boundary|scripts/run_codex_pr_review.py|1|#!/usr/bin/env python3 is accepted and can make any high-severity finding block. This defeats the PR's stated goal of requiring concrete reachability evidence before blocking. (line 953)

Suggestion: Validate evidence semantically, not just syntactically. At minimum, require repository_call entries to resolve to an actual call site for the reported symbol/file and public_boundary entries to resolve to an explicitly recognized boundary construct, or reject evidence that cannot be derived from structured repository analysis.

3. 🟠 [HIGH] Security in scripts/run_codex_pr_review.py

The new evidence field is rendered into the PR comment verbatim. Because the blocking format now requires an exact source-line symbol from the repository, any credential, DSN, token, or other sensitive literal present on the cited line will be copied directly into the public review comment (and the decision artifact), creating a new exfiltration path that did not exist before this PR. (line 1425)

Suggestion: Sanitize evidence before rendering or persisting it, using the same secret-redaction rules as finding history, or store only safe components such as kind/path/line plus a hashed or truncated symbol excerpt.

---

Review by Codex PR Review bot • PR

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 404908ef74

ℹ️ 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]

Re-check stale review events before passing the gate

In the pull_request_review path, main() skips run_static_guard() and returns this advisory decision directly; when a CHANGES_REQUESTED connector review for an older commit is delivered after a new push, this now creates a successful Codex Review Gate run without running the blocking static secret/path checks for the current head. In repositories that require this workflow check, that race can satisfy the required check for a head that has not passed the static guard, so the REACT branch should either verify review.commit_id == head_sha before returning success or run the static guard first.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep arbitration fingerprints stable across evidence wording

Because previous_matching_findings() and the repeated-streak logic use this fingerprint, including the free-form evidence text means the same file/category/severity blocker stops matching its trusted history whenever the reviewer cites a different caller path or simply rewrites the evidence. In that scenario the prior finding is not passed into contract arbitration, so opposing remediation suggestions for the same contract can avoid the intended contract_arbitration freeze; keep the identity stable and store evidence separately or canonicalize it before keying.

Useful? React with 👍 / 👎.

[Pigbibi]

Permanent freeze after the single concentrated closure. Current-head review found reachable Critical/High issues: required static-gate check context can be overwritten by advisory review events; evidence validation is syntactic rather than semantic; exact source evidence may be echoed into public comments. Closure is exhausted. Preserving branch, commits, checks, and review evidence; no second closure, rerun, bypass, merge, deploy, or successor.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 47579e3c62

ℹ️ 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]

Preserve machine evidence in review history

For short machine-checkable evidence strings with no spaces and a line number, such as repository_call|x.py|1|foo, the generic credential redactor rewrites the newly stored evidence field to [REDACTED]. On the next run the current finding fingerprints with the real evidence while history fingerprints with [REDACTED], so previous_matching_findings() returns no match and repeated/contract arbitration is skipped even for the same blocker; store a normalized evidence key separately or avoid applying the broad secret-token heuristic to this structured field.

Useful? React with 👍 / 👎.