fix(ci): stop fork GPT lane hiding a verdict behind an incomplete run (#8292)

[bolichen97]

Summary

Follow-up to #8342. The same verdict-visibility bug (#8292) also affects the fork lane: fork-gpt-review.yml's Post/update summary comment step PATCHed the bot's marker comment unconditionally, so an incomplete rerun overwrote a posted [GPT-REVIEWED]/[BLOCK-MERGE] verdict with a short "review incomplete" body. The prior verdict survives only in the GraphQL userContentEdits edit history, which the REST comments API (what tooling and gh api reach for) does not expose. The finding becomes recoverable in principle and invisible in practice.

This is a visibility bug, not a data-loss or merge-safety bug: the fail-closed check-run gate already refuses to pass on an incomplete verdict.

What changed

.github/workflows/fork-gpt-review.yml — Post/update summary comment step only:

• An incomplete run never modifies an existing comment. When a github-actions[bot] marker comment already exists and this run's kind is incomplete, the step emits a diagnostic log line and makes no edit of any kind — no PATCH, no create — regardless of whether the existing body carries [GPT-REVIEWED].
• This is stricter than a preserve-and-prepend approach on purpose: overlapping runs for different SHAs can read the comment before a newer run PATCHes its verdict in, so an incomplete run that read verdict V1 must not PATCH V1 back over a newer run's V2 that landed in between — and even a PATCH that only preserved the verdict and prepended a notice would restore the stale body it read.
• Only blocked/clear kinds PATCH the full new body; the create-new gh pr comment path (no existing marker) is unchanged.
• Lookup-failure handling (incomplete-only). The bot-comment lookup captures the paginated query's full output before selecting the first id (so head -n1 cannot SIGPIPE the API call under set -o pipefail and be misread as a failure). A genuine lookup failure suppresses the update only for an incomplete run — where posting would risk a spurious marker over a live verdict. A completed (blocked/clear) run whose lookup fails falls through to create and publishes the verdict rather than staying silent, since a duplicate comment is more recoverable than an unposted verdict.
• The existing github-actions[bot] author filter on the comment lookup is preserved, so a PR commenter cannot plant the marker and have this step PATCH their comment with the write-scoped token.

The Finalize check-run (fail closed) step (the fork analogue of Gate on findings) is untouched, so the merge gate stays fail-closed and decoupled from the comment — an incomplete run is never mistaken for an approval.

docs/ci/ci-and-reviews.md — a paragraph documenting this lane's no-touch-on-incomplete behavior and why the overlapping-run race makes even a preserving PATCH unsafe.

test/test_ai_review_workflows.py — new TestForkGptVerdictVisibility running the real step bash with a stubbed gh; the step's hardcoded /tmp/fork-codex-comment.md write is redirected under tmp_path so the test touches no shared host file.

Scope

This PR changes only the fork lane (fork-gpt-review.yml). It does not touch codex-review.yml — that same-repo lane's #8292 fix is sibling PR #8342, which owns it.

Testing

• python -m pytest test/test_ai_review_workflows.py -q => all green (adjudication tests from main coexist with the new fork-lane class).
• test/test_workflow_secret_and_cache_scope.py => all green (the workflow edit does not alter secret/cache scope).
• Test-quality check: reverting the workflow guard makes the new contract cases fail (a PATCH is recorded), confirming they are load-bearing.
• flake8 / isort clean on the test file; YAML parse of fork-gpt-review.yml OK.

Follow-up (not in this PR's scope)

The same unconditional-PATCH-on-incomplete construct still lives in seven other review lanes: fork-opus-review.yml, design-review.yml, fork-design-review.yml, ux-review.yml, fork-ux-review.yml, first-principles-review.yml, and fork-first-principles-review.yml (plus codex-review.yml, owned by sibling PR #8342; claude-review.yml already posts nothing on an incomplete run). Sweeping those sibling lanes is a tracked follow-up, kept out of this PR to hold its diff to the fork GPT lane and avoid re-arming the Fork workflow-change guard on unrelated workflow files.

Pattern harvest

Rule candidate: when one lane of a duplicated CI mechanism is fixed, sweep every sibling lane that carries the same construct in the same or an immediately-tracked follow-up — a fix landing on only one of N copies leaves the invariant false everywhere else while reading as "fixed." Here the unconditional PATCH .../issues/comments/$existing on an incomplete run is shared by fork-gpt-review.yml (this PR), codex-review.yml (sibling PR #8342), and fork-opus-review.yml (tracked follow-up). A cross-lane census at fix time (grep the shared Post/update comment step and the unconditional PATCH) surfaces every copy up front rather than relying on a hand-filed note.

Not in scope

The gate's fail-closed behaviour on an incomplete verdict is correct and is unchanged.

no linked issue: #8292 is closed by sibling PR #8342 (the same-repo lane); this fork-lane follow-up intentionally carries no closing keyword to avoid a duplicate/premature close.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 3511c9498e57466f6fe44b827c9a4f73eb353166 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real verdict-visibility bug, solved at the right layer with a fail-safe asymmetry, and the merge gate stays untouched.

The no-touch-on-incomplete choice is structurally justified for this lane: its concurrency group is keyed per head_sha, so runs for different SHAs on one PR genuinely overlap and even a preserve-and-prepend PATCH is a stale-read write-back. The lookup-failure asymmetry (incomplete suppresses, completed falls through to create) fails toward the recoverable outcome in both directions, and the fail-closed check-run keeps merge safety independent of the comment.

Suggestions

• The new docs paragraph argues a preserving PATCH is unsafe, while the same-repo lane (fix(ci): never let an incomplete review body overwrite a posted verdict (#8292) #8342, same file) ships exactly that; add the reconciling fact — fork lane concurrency is per-SHA (overlap possible), same-repo is per-PR with cancel-in-progress — so the tracked fork-opus-review.yml sweep knows which semantics to copy for which concurrency shape.

[DESIGN-REVIEWED] 3511c94

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 3511c9498e57466f6fe44b827c9a4f73eb353166 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. The verification confirmed: (1) claude-review.yml already ships a smaller shape for this exact job — an incomplete run posts nothing at all (line 456–459); (2) the author's sibling census undercounts — the unconditional-PATCH-of-a-non-verdict-body construct lives in seven other lanes, not the one named in the follow-up; (3) the PR's own rationale for never-touch contradicts the preserve-and-prepend fix #8342 landed in codex-review.yml. Final review follows.

First-Principles-Verdict: CONCERNS

The fix is real and cause-level, but half its new machinery guards a placeholder claude-review.yml already deletes, and the sibling census names 1 of 7 unfixed lanes.

What this change ships

Intent: stop an incomplete fork-GPT rerun from burying a posted verdict comment (#8292) — a FIX.

1. Incomplete rerun leaves the existing verdict comment untouched — justified (reported defect GPT review verdict lives in one mutable comment, so an incomplete run hides a blocking finding #8292).
2. Incomplete run whose comment lookup fails posts nothing — declared; exists only to protect item 4.
3. Completed verdict whose lookup fails posts a fresh comment, never stays silent — justified.
4. First incomplete run still posts a "review incomplete" placeholder — rides along; smaller sibling exists (claude-review.yml:456).
5. Lookup captures all pages before picking the first id — mechanism detail serving item 2 only.
6. Doc paragraph on the no-touch rule — justified (same-commit spec invariant).
7. Six tests executing the real step bash — justified, load-bearing per revert check.

Watch

• Sibling undercount. The harvest claims the construct "is shared by fork-gpt-review.yml, codex-review.yml, and fork-opus-review.yml." Grepping the unconditional PATCH .../issues/comments/$existing beside an incomplete/"could not complete" body finds 7 unfixed lanes: fork-opus-review.yml:471, design-review.yml:522, fork-design-review.yml:590 (read-confirmed), ux-review.yml:693, fork-ux-review.yml:792, first-principles-review.yml:570 (read-confirmed), fork-first-principles-review.yml:687. The tracked follow-up covers one.
• The doc's own rationale — "even a PATCH that only preserved the verdict … would restore the stale body it read" — condemns the preserve-and-prepend fix fix(ci): never let an incomplete review body overwrite a posted verdict (#8292) #8342 already landed at codex-review.yml:1084–1105. One bug class now carries two contradictory contracts; the deferral note should say so.

Subtractions

• Drop the incomplete-run create path and mirror claude-review.yml:456–459 ("incomplete: post NOTHING"). With it go lookup_ok, the capture-before-head -n1 rework, the lookup-failure branch, and the two lookup-failure tests — roughly half the workflow diff — while the named harm stays removed.

[FIRST-PRINCIPLES-REVIEWED] 3511c94

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 3511c9498e57466f6fe44b827c9a4f73eb353166 — this comment is updated in place on each push.

Review details

No findings.

The lone candidate requires all_matches to exceed the pipe buffer (~64KB, i.e. thousands of matching bot comments) for head -n1 to close the pipe and SIGPIPE printf. The step maintains exactly ONE in-place marker comment per PR under the github-actions[bot] author filter, so a real run yields zero or one match — the >6000-match trigger has no input that occurs in practice, failing (a). The candidate itself concedes it could not construct a realistic path. Dropped below 80.

[OPUS-REVIEWED] 3511c94

Verdict parsed from the review's SHA-scoped output markers for commit 3511c9498e57466f6fe44b827c9a4f73eb353166.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 3511c9498e57466f6fe44b827c9a4f73eb353166: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3511c9498e57466f6fe44b827c9a4f73eb353166 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3511c94

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 3511c9498e57466f6fe44b827c9a4f73eb353166: <one-sentence reason>

[bolichen97]

Reply to First Principles Review (advisory CONCERNS on 3511c9498) — answering each point; all verified against the tree.

1. Sibling census undercount (7 lanes, not 3). Accepted — corrected in the PR body. The unconditional-PATCH-on-incomplete construct lives in fork-opus-review.yml, design-review.yml, fork-design-review.yml, ux-review.yml, fork-ux-review.yml, first-principles-review.yml, fork-first-principles-review.yml (plus codex-review.yml, owned by fix(ci): never let an incomplete review body overwrite a posted verdict (#8292) #8342). The cross-lane sweep is the tracked follow-up; this PR stays scoped to the fork GPT lane to avoid re-arming the Fork workflow-change guard on unrelated workflow files.

2. "Two contradictory contracts" vs fix(ci): never let an incomplete review body overwrite a posted verdict (#8292) #8342's preserve-and-prepend. The divergence is deliberate, not a contradiction: the fork lane runs under workflow_run where overlapping runs for different SHAs can read the marker before a newer run publishes, so even a preserving PATCH restores a stale body — never-touch is the correct floor there. The same-repo codex-review.yml (fix(ci): never let an incomplete review body overwrite a posted verdict (#8292) #8342) does not face the same cross-SHA overlap in the same way. The PR body/doc no longer compares against codex-review.yml's mechanism (that comparison was removed in an earlier round), so the doc asserts only this lane's own behavior.

3. Subtraction: drop the incomplete-run create path, mirror claude-review.yml:456 "post NOTHING". This is a real, simpler alternative. I am not adopting it unilaterally because it changes pre-existing behavior beyond this PR's stated scope: the fork GPT lane already posts a "review incomplete" placeholder on a first incomplete run (that predates this PR), and "post nothing instead" is a separate behavior decision for the maintainer, not part of "stop an incomplete rerun from burying a posted verdict." The lookup_ok/capture-before-head machinery guards the completed-verdict path too (item 3 in your own list — a completed verdict whose lookup fails must still publish), so it is not solely in service of the placeholder. If the maintainer prefers the claude-review.yml shape lane-wide, that is best done as the tracked cross-lane sweep (which would converge all 8 lanes on one shape) rather than diverging this one lane now.

Readiness is green; this reply answers the advisory verdict. Happy to adopt the "post nothing on incomplete" shape here if a maintainer prefers it over the placeholder.

[chenmingwei23]

Approving: PR Readiness green (the repo's only required check), no failing lanes, MERGEABLE.