ci: distinguish provider refusal from crash in the fork GPT review lane

[javenciu]

Problem / Motivation

When the review provider refuses to review a fork PR's diff — declines because of what the content is, as opposed to crashing or timing out — the fork GPT lane (fork-gpt-review.yml) reports the generic "review incomplete" state and advises re-running the workflow. Issue #8739 tracks porting the refusal/crash distinction that landed for the same-repo lane in #8738 (issue #8685).

The two failure modes need opposite remedies: a crash is fixed by re-running, while a refusal is caused by the diff itself and is empirically sticky — 12 consecutive identical refusals across 10 heads were measured on one PR (#8685). Fork PRs are where this matters most: an external contributor's security fix whose evidence is a working exploit is exactly the diff a provider content filter refuses, and today that contributor (and the maintainer triaging the red check) is told to re-run forever.

Why it matters

A maintainer looking at "review incomplete" on a fork PR has no way to tell a transient infrastructure failure from a permanent content refusal. The advised remedy (re-run) has never been observed to clear a refusal, so the check stays red with a misleading title, the contributor's PR looks broken rather than needing-human-review, and the one remedy that works — a maintainer reading the change directly — is never named.

What changed (motivation → approach → change)

Observed symptom: a provider refusal on a fork PR surfaces as "review incomplete" with re-run advice that does not work.

Root cause: the fork lane discards each pass's output stream, so when a pass fails (rc != 0 or empty pass file) there is nothing to distinguish the provider's refusal line from a crash — the same gap #8738 closed for codex-review.yml.

The change, a mechanical port of #8738's classification with one deliberate fork-specific difference:

• Each review pass's combined stream is now tee'd into RUNNER_TEMP (never the checkout, so a PR cannot plant a symlink at the log's name) where the exit code is captured.
• A failed pass is classified as refused only when the provider's own error line appears anchored at line start and only in the 4000-byte tail of that pass's captured stream. Both constraints are load-bearing here — this lane hands the model more attacker-controlled text than the same-repo lane (the prompt embeds the fork author's title/body, and the reviewer echoes the diff, which for a PR touching this workflow contains the signature verbatim) — so an echoed copy of the signature must not reclassify an ordinary crash.
• The classification rides the verdict assembly's refused step output into both publishers (summary comment and fail-closed check-run), never a grep of the review body: on the clean path that body is model prose, and a verdict that merely quotes the refusal wording must not be reclassified.
• The refused terminal state still fails closed — no [GPT-REVIEWED] marker, so a declined review can never read as an approval — but the check-run title says "reviewer refused this diff" and the comment names the honest remedy.
• Fork-specific difference, and why this is not a blind copy: the same-repo lane's refused copy routes to /ai-review override. This lane resolves no override record, so promising that command here would route a maintainer to a clearance path that cannot clear this check. The refused copy routes to direct maintainer review instead.
• A refused run rides the shared guarded_comment_upsert withhold path (fork review lanes: incomplete review body still overwrites a posted verdict (fork-lane twins of #8292) #8344, fix(ci): guard the review lanes against verdict overwrite (#8344) #8450): the refused body carries no [GPT-REVIEWED] proof marker, so it never modifies an existing comment and only creates the first placeholder when a successful lookup confirms none exists. This PR pins that behavior for the refused kind with tests rather than growing a posting branch of its own.
• A refusal dominates a mixed failure (one pass crashed, the other refused), because a re-run only helps if the refusal does not recur.

Alternatives considered: grepping the review body for a refusal marker downstream (rejected — the body on the clean path is model prose, and a marker nothing may parse would invite prose-grepping); matching the signature anywhere in the whole stream (rejected — an echoed copy in PR-controlled text would reclassify crashes); porting the same-repo lane's /ai-review override remedy text verbatim (rejected — this lane cannot resolve an override record, see above).

The claude-review.yml half of #8739 is deliberately not included: that lane's output is action-mediated, so refusal classification needs its own signature source rather than this mechanical port. See Pattern harvest.

docs/ci/ci-and-reviews.md (the owning doc for the reviewer lanes) is updated in the same commit: the refusal paragraph now covers both GPT lanes and names each lane's remedy.

Tests

New class TestForkGptRefusalTerminalState (12 tests) in test/test_ai_review_workflows.py, mirroring the same-repo lane's coverage plus the fork-specific attack surface:

• test_refusal_is_classified_where_rc_is_captured — both passes tee into RUNNER_TEMP, match line-anchored against the tail, and record into the re-initialized refused-passes file; the signature carries the provider's ERROR: prefix and stays free of regex metacharacters (it is interpolated into an anchored grep pattern).
• test_signature_stays_in_sync_with_the_same_repo_lane — both lanes classify against one identical signature; drift would silently split refusal semantics between lanes.
• test_provider_emitted_refusal_line_classifies_as_refused — executes the workflow's real classification block (extracted from the YAML, run under bash) against a fabricated stream carrying the provider's line: classified refused.
• test_echoed_signature_in_pr_controlled_text_stays_a_crash — the signature quoted/indented in PR-controlled text (exactly what this PR's own diff looks like to the reviewer) does not classify: a crash on such a PR stays re-runnable.
• test_signature_outside_the_stream_tail_stays_a_crash — an early echoed copy scrolled past by 8000 bytes of output does not classify.
• test_refused_pass_publishes_its_own_terminal_state — refused body carries no [GPT-REVIEWED]/[GPT-REFUSED] marker, no re-run advice, no /ai-review override, routes to direct maintainer review, and emits refused=true to GITHUB_OUTPUT.
• test_crashed_pass_keeps_the_rerunnable_incomplete_state — the pre-existing incomplete state and its re-run advice are untouched for genuine crashes.
• test_refusal_dominates_a_mixed_failure — one crashed + one refused ⇒ refused wins.
• test_clean_run_still_publishes_pass_2_verbatim — the happy path is byte-identical.
• test_refused_run_never_modifies_an_existing_comment — the GPT review verdict lives in one mutable comment, so an incomplete run hides a blocking finding #8292 stale-read guard covers the refused kind (no PATCH, no create with an existing comment).
• test_refused_run_with_no_existing_comment_creates_the_refused_placeholder — first comment carries the refused verdict and fork-honest remedy.
• test_refused_classification_rides_step_outputs_into_both_publishers — comment and finalize steps read steps.gpt_pass2.outputs.refused through env, never a body grep; the check-run title changes but its failure conclusion does not.

Fails-before, run at base d4c2cbf22 with the new tests committed onto the pristine tree (full targeted file, tests only):

11 failed, 443 passed
(the new class alone: 11 failed, 1 passed — the 1 pass is the
 stale-read-guard control, which holds via the shared
 guarded_comment_upsert withhold path (#8344, #8450) — correct
 control behavior)

With the fix: 12 passed; full targeted file: 454 passed.

Manual verification

N/A — the workflow's classification and publication logic is executed directly by the tests (the classification block and verdict assembly are extracted from the YAML and run under bash with fabricated streams), which is the same technique the file's existing suites use for this lane. A live refusal requires the provider to refuse a real diff on a real fork PR, which cannot be staged deterministically.

Related Issues

Fixes #8739 (the fork-gpt-review.yml half; the claude-review.yml half needs its own signature source — see Pattern harvest). Ports #8738 (#8685). Interacts with the shared guarded_comment_upsert withhold path (#8344, #8450), which this pins for the refused kind with tests.

Pattern harvest

Rule candidate: review-prompt
Pattern: "failure-state classification must ride step outputs, never a grep of content that is model prose or attacker-influenced on other paths"

The remaining half of #8739 — claude-review.yml's action-mediated lane, where the provider's stderr is not directly capturable and refusal classification needs its own signature source — is a distinct follow-up, not covered by this port.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

Per the template, the CLA wording is a placeholder pending OSPO/Legal; nothing to affirm beyond the checklist until it is supplied.

[bolichen97]

Rebased onto main 9c2b4b678 by a maintainer as part of the 2026-09-08 open-PR audit (was 172 commits behind, mergeable_state: dirty).

Conflicts and resolution:

• test/test_ai_review_workflows.py: this PR appends TestForkGptRefusalTerminalState at the end of the file, and main appended new classes there too. Took main's file and re-appended this PR's 314 lines unchanged, so both sets of classes are present.
• .github/workflows/fork-gpt-review.yml and docs/ci/ci-and-reviews.md auto-merged; main's Opus 4.8 / model-name churn is preserved alongside this PR's refusal handling.

Gates run locally on the changed file: black, isort, flake8 clean; pytest test/test_ai_review_workflows.py 536 passed.

Please review the resolution. A maintainer push makes the maintainer the last pusher, so under the repo's last-push rule a second approver is needed. Reply if anything looks wrong.