fix(review): honor trusted advisory dispositions

[Pigbibi]

Summary

• require exact repository-backed current-caller or public/untrusted-boundary evidence before HIGH/CRITICAL findings can block
• load exact-head resolved advisory dispositions only from authenticated GitHub review metadata; fail closed when that metadata cannot be verified
• suppress unchanged-head advisory repeats unless materially distinct current reachability evidence exists, while preserving blocking for real reachable impact and contract conflicts
• add PR #271 fact-fixture regressions for portability, clean-revision validation, trusted disposition authority, and repeat handling

Validation

• focused PR #271 fixture replay: 5 passed
• python3 -m unittest tests.test_run_codex_pr_review: 62 passed
• python3 -m pytest tests ops/quant-monitor/tests -q: 660 passed, 1 skipped, 50 subtests passed
• python3 -m ruff check .
• python3 -m compileall -q scripts service tests ops
• actionlint
• Cloudflare Node tests: 13 + 15 passed
• git diff --check

Boundaries

• no dependency, workflow, branch-protection, deployment, or service changes
• PR #271 and its frozen head were not modified

[github-actions]

🤖 Codex PR Review

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

⚖️ Codex Review Arbitration

🚫 block: The current blockers remain valid against the cumulative diff. First, _finding_matches_trusted_advisory() only binds suppression to advisory_provenance + file + overlapping advisory line range; it never checks that the current finding is the same defect semantically, so a different high/critical bug on the same line can still be marked trusted_advisory_suppressed and stop blocking. That is the same required behavior as the prior blocker, not a contract conflict. Second, evaluate_findings() accepts new_reachability_evidence whenever _parse_blocking_evidence() can parse any different existing repo line; it does not verify that the new caller/boundary belongs to the same advisory or scope, and the added test test_genuinely_different_reachability_identity_is_new explicitly codifies re-blocking from an unrelated entrypoint. Third, _history_finding_identity() still omits line/advisory identity and remaining_history_after_trusted_advisory_suppression() removes active history by that coarse key, so same-file same-description unresolved blockers can be dropped together when only one advisory was actually suppressed.

🚫 Blocking Issues

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

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

On unchanged heads, _finding_matches_trusted_advisory() treats a finding as the same resolved advisory when advisory_provenance, file, and line range match, but it never checks that the current finding is semantically the same issue. That means a new reachable defect on the same line can copy a valid authenticated provenance from the prompt, get marked trusted_advisory_suppressed, and main() will stop blocking on a real current bug. (line 1020)

Suggestion: Bind suppression to a bridge-generated stable advisory identity for the exact prior finding (for example thread id plus classification and prior fingerprint), not just file/line/provenance echoed back by the model.

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

new_reachability_evidence is accepted whenever it names any other existing repository line with a valid kind|path|line|symbol shape. In the unchanged-head path, a repeated advisory can therefore become blocking again just by emitting an unrelated caller/boundary from elsewhere in the checkout, even though there is no materially new reachability for that advisory under the current PR. (line 1092)

Suggestion: Validate new_reachability_evidence against the same advisory identity and finding scope, or only honor new reachability evidence that is authenticated by the bridge instead of any syntactically valid repository line.

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

_history_finding_identity() collapses findings by (severity, category, file, normalized description) and omits the line number and advisory identity. When one unchanged-head advisory is suppressed, remaining_history_after_trusted_advisory_suppression() can also remove a different unresolved blocker in the same file that happens to share the same description text, which skips arbitration and incorrectly clears active history. (line 1235)

Suggestion: Include the finding line and stable advisory identity/provenance in _history_finding_identity() so suppression only exempts the exact previously resolved blocker.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Reliability in scripts/run_codex_pr_review.py

The GraphQL query only loads comments(last: 100) for each review thread. Any resolved thread with more than 100 comments can have its authenticated disposition comment paged out of this window, so exact-head trusted advisory metadata is silently dropped and unchanged-head repeats start blocking again or triggering contract-conflict handling even though a valid disposition still exists. (line 1708)

Suggestion: Paginate thread comments or fetch disposition comments separately instead of hard-capping each thread to the last 100 comments.

---

Review by Codex PR Review bot • PR

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 482595b918

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 94dea8e159

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

Clear advisory history even when Codex omits the repeat

When the head is unchanged and the trusted advisory context makes the primary reviewer omit the resolved finding instead of re-emitting it as a suppressed non-blocker, this list is empty because it only inspects current non_blocking_findings. remaining_history_after_trusted_advisory_suppression then keeps the prior active history, so the run still blocks/arbitrates without the advisory context even though the advisory has been resolved; advisory-covered history needs to be cleared independently of whether Codex repeats the finding.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Key history clearance on advisory identity

When a trusted advisory suppresses a repeated finding but Codex rephrases the description, this set no longer matches the stored active history because _history_finding_identity includes the sanitized description. Fresh evidence is that the new exemption key uses current finding text even after _finding_matches_trusted_advisory has already proved the exact advisory via provenance/thread, so same-advisory repeats with wording drift still fall through to active-history blocking/arbitration.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Validate the finding line before blocking

For a high/critical finding in a changed file, has_blocking_evidence only proves that the separate caller/boundary evidence line exists; the reported finding["line"] is never checked to be present or part of the patch. If Codex returns a valid current_caller but points the bug at an unchanged or nonexistent line in the changed file, this still blocks and can drive remediation against a hallucinated location, contrary to the exact changed path/line gate added in the prompt.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Anchor contract-conflict markers before parsing

This counts the marker prefix anywhere in the previous bot comment, but finding descriptions and suggestions are rendered back into that same comment verbatim. If a reviewer finding quotes this marker-like text from the diff, the next run sees multiple prefixes and treats it as a trusted contract conflict even when the real machine marker is false, which can leave the PR blocked for contract arbitration based only on displayed finding text; parse only the dedicated marker line instead.

Useful? React with 👍 / 👎.