fix: converge repeated PR review findings#77
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review🚫 Merge blocked: 2 serious issue(s) found in high-risk files ⚖️ Codex Review Arbitration🚫 block: Both current findings remain valid against the cumulative diff. First, 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🔴 [CRITICAL] Security in
|
Co-Authored-By: Codex <noreply@openai.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
AIAuditBridge/scripts/run_codex_pr_review.py
Lines 2676 to 2679 in 3e0b1f7
Preserve human-resolution decisions after serializing history
When a finding exceeds the remediation limit and arbitration confirms it, the code sets next_action to human_contract_resolution and writes a blocked_human_contract history round, but this validation immediately treats that expected status as a history-confirmation failure and overwrites the decision with contract_arbitration. In that limit-reached path, the comment, artifact, and step outputs point automation/humans at another arbitration instead of the required human contract resolution.
ℹ️ 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".
| if ( | ||
| history_requires_confirmation | ||
| or (active_blocking_history and not decision["blocked"] and not clearance_marker) | ||
| ) and not arbitration_cleared: |
There was a problem hiding this comment.
When a remediation review has active blocking history but the primary reviewer returns no blockers, a non-clear arbitration re-blocks the decision; because this condition then excludes decision["blocked"], the code falls through and appends an arbitration_required round with an empty findings list. Since has_active_blocking_history() only considers the latest round active when it has findings, the next run can treat the unresolved blocker as inactive and pass a clean primary review without the required independent clearance.
Useful? React with 👍 / 👎.
| status = latest.get("status", "blocking") | ||
| return finding_history_requires_confirmation(history) or ( | ||
| status == "blocking" and bool(latest.get("findings")) | ||
| status in {"blocking", "remediation", "arbitration_required", "blocked_human_contract"} |
There was a problem hiding this comment.
When the first blocking review is recorded, history_status is initial_review, but this active-state check does not include that status. On the next author push, active_blocking_history becomes false, so a clean primary review can clear the PR without the independent source-of-truth clearance required for an outstanding blocker, and the remediation-scoped review is skipped.
Useful? React with 👍 / 👎.
| for round_state in reversed(history): | ||
| prior = round_state.get("findings") | ||
| if not isinstance(prior, list): | ||
| continue | ||
| for finding in prior: |
There was a problem hiding this comment.
When a previous finding was cleared by arbitration, its round is still stored with status cleared, but this conflict scan walks every historical round without skipping cleared findings. A later new finding on the same contract surface can therefore be marked as a contract conflict against a false-positive finding that was already cleared, disabling normal remediation and reviving history that should no longer block.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1198ec4f8
ℹ️ 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".
| history_valid=False, | ||
| ) | ||
|
|
||
| clearance_marker_out = clearance_marker_text if clearance_marker and not decision["blocked"] else "" |
There was a problem hiding this comment.
When a previous comment contains a syntactically valid clearance marker that no longer matches the current head/diff, this still copies it into clearance_marker_out whenever the primary review is clean. In an active-history run, that non-empty value makes the later active_blocking_history and not clearance_marker_out guard skip re-blocking, so an arbiter verdict of block can publish “Merge allowed” and even carry forward the stale clearance marker; this happens on a new PR head after an older clearance marker remains in the bot comment.
Useful? React with 👍 / 👎.
| run.get("event") == "pull_request_target" | ||
| and run.get("conclusion") == "success" | ||
| and str(run.get("head_sha") or "").strip().lower() | ||
| == str(clearance.get("head_sha") or "").strip().lower() |
There was a problem hiding this comment.
For pull_request_target runs, GitHub executes the workflow on the default-branch commit, while this marker stores the pull request head SHA. This check therefore rejects the successful run that just produced a valid clearance on the next same-head retry, so a persistent false-positive finding cannot be reused from clearance.v1 and the review falls back to blocking/arbitrating again despite an unchanged head and diff.
Useful? React with 👍 / 👎.
| current_fingerprint = str(current.get("fingerprint_v2") or _fingerprint_v2(current)) | ||
| prior_fingerprint = str(prior.get("fingerprint_v2") or _fingerprint_v2(prior)) | ||
| if current_fingerprint != prior_fingerprint: |
There was a problem hiding this comment.
If the same blocking finding is reclassified between high and critical while keeping the same required behavior, this comparison still treats it as a contract conflict because fingerprint_v2 includes severity. That sends an ordinary severity wording drift into contract_arbitration with auto-fix disabled, and if arbitration capacity is unavailable the PR fails closed even though no behavior reversed or contradicted the prior contract.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6b74e63bd
ℹ️ 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".
| if run_id <= 0: | ||
| return False | ||
| try: | ||
| run = github_request(token, "GET", f"/repos/{repo}/actions/runs/{run_id}") |
There was a problem hiding this comment.
In the actual workflow this call is made with GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}, but .github/workflows/codex_pr_review.yml only grants contents, id-token, issues, and pull-requests, not actions: read. The workflow-run endpoint needs Actions read permission on private repos, so github_request will 403 and this helper will always return false, causing otherwise valid clearance markers to be ignored on same-head reruns.
Useful? React with 👍 / 👎.
| and str(run.get("head_sha") or "").strip().lower() | ||
| == str(clearance.get("head_sha") or "").strip().lower() | ||
| and str(run_repo.get("full_name") or "") == repo | ||
| and workflow_path.startswith(".github/workflows/codex_pr_review.yml@") |
There was a problem hiding this comment.
For workflow_call consumers, GITHUB_RUN_ID belongs to the caller workflow run, so the run's path is the caller's workflow while this reusable codex_pr_review.yml is recorded under referenced_workflows. Because this workflow explicitly supports consumer repos via workflow_call, checking only the top-level path rejects clearances emitted from those repos and same-head reruns re-review/re-arbitrate instead of reusing the trusted clearance.
Useful? React with 👍 / 👎.
| findings = review.get("findings", []) | ||
| if not isinstance(findings, list): | ||
| findings = [] | ||
| findings = [_contract_finding(finding) for finding in findings if isinstance(finding, dict)] |
There was a problem hiding this comment.
When the backend returns a finding with a line, this replaces the whole finding with _contract_finding(), which only keeps severity/category/file/description/suggestion and contract hashes. evaluate_findings() therefore never receives the original line, so _format_finding() can no longer include (line n) in PR comments or artifacts for any finding; keep the original dict and merge the contract fields instead of discarding it.
Useful? React with 👍 / 👎.
| if not isinstance(comparison, dict) or comparison.get("status") != "ahead": | ||
| raise ReviewError("remediation head is not a strict descendant of the trusted reviewed head") |
There was a problem hiding this comment.
When an author rebases, amends, or force-pushes after a blocking review, the previous reviewed SHA is no longer an ancestor of the new head, so the compare status becomes non-ahead and this raises. The caller then publishes review_retry while preserving the old previous_head_sha, so every rerun hits the same failure and the PR cannot get reviewed again until the branch is rebuilt on top of the abandoned head; fall back to a cumulative review/final sanity pass for diverged heads instead.
Useful? React with 👍 / 👎.
| def remediation_round_count(history: list[dict[str, Any]]) -> int: | ||
| return sum(1 for round_state in history if round_state.get("status") == "remediation") |
There was a problem hiding this comment.
This counts every historical remediation round globally, but the new contract state is tracked per finding/contract. If contract A used two remediation rounds and is then fixed while an unrelated contract B becomes the current blocker, B is immediately routed to final sanity/contract arbitration because the old rounds are still counted; filter the count to the current unresolved contract keys so unrelated history does not exhaust B's remediation budget.
Useful? React with 👍 / 👎.
| new_head=new_head, | ||
| ) and not (history_requires_confirmation and not previous_findings) | ||
| if confirmation_arbitration_required or history_arbitration_required or repeated_arbitration_required: | ||
| if confirmation_arbitration_required or active_history_arbitration_required or repeated_arbitration_required: |
There was a problem hiding this comment.
When conflicting_contract_findings() detects the same contract surface with a changed behavior digest, the code sets next_action=contract_arbitration but this condition never actually invokes the arbiter unless the finding also meets the repeated-finding threshold or has empty current findings. A first-time opposite-contract finding therefore gets stuck publishing an arbitration-required block with no arbitration verdict or clearance path on reruns; include contract_conflicts in the arbitration trigger.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de8cd1151f
ℹ️ 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".
| for token in re.findall(r"[a-z0-9]+", text) | ||
| if token not in {"the", "and", "for", "from", "into", "that", "this", "with", "must", "should", "will", "need", "return"} | ||
| } | ||
| return " ".join(sorted(tokens)) |
There was a problem hiding this comment.
When two findings for the same contract use the same words or numbers in a different order to require opposite behavior (for example, "copy A to B" vs "copy B to A", or "validate before cache" vs "cache before validate"), this normalization sorts the token set before hashing. Those suggestions produce the same behavior_digest, so conflicting_contract_findings() will not mark the later finding as a contract conflict and can allow normal auto-remediation to reverse the earlier contract.
Useful? React with 👍 / 👎.
| return anchored_subject or _normalize_contract_text( | ||
| finding.get("suggestion") or text | ||
| ) |
There was a problem hiding this comment.
When a finding does not include a backticked or dotted code anchor, this fallback derives the contract subject from the suggestion/description, and _contract_key() hashes that subject. If a later finding on the same plain-English contract changes the required behavior (for example from “return a blocked result” to “raise an error”), it gets a different contract_key instead of just a different behavior_digest, so conflicting_contract_findings() never compares the two rounds and the opposite remediation can remain auto-fixable instead of going to contract arbitration.
Useful? React with 👍 / 👎.
| contract_key = str(finding.get("contract_key") or _contract_key(finding)) | ||
| if contract_key not in resolved_contracts and contract_key not in latest_by_contract: | ||
| latest_by_contract[contract_key] = { |
There was a problem hiding this comment.
When a contract conflict has been recorded as arbitration_required and the author pushes another head, this reverse scan treats that latest conflicting finding as the contract source and then skips the older finding that held the original required behavior. If the same contradictory finding appears again, conflicting_contract_findings() now sees matching behavior against the arbitration-required round, forgets the original conflict, and can re-enable normal auto-remediation instead of keeping the contract in arbitration.
Useful? React with 👍 / 👎.
1 participant
Summary
clearance.v1Validation
python3 -m unittest tests.test_run_codex_pr_review(57 tests)python3 -m unittest discover tests(623 tests, 1 skipped)python3 -m ruff check scripts/run_codex_pr_review.py tests/test_run_codex_pr_review.pypython3 -m compileall -q scripts testsactionlint .github/workflows/codex_pr_review.ymlgit diff --checkSafety