fix(ci): bind fork AI-review readiness reads to PR+attempt external_id (#9302)

[bolichen97]

Fixes #9302.

Problem

The five Stage-2 fork AI-review lanes (Opus, GPT, Design, UX, First Principles) read their verdict from the head SHA's check-runs by name only. Two open PRs can share a head SHA, so a sibling PR's check-run could answer for the wrong PR (a race that fails open or closed depending on the sibling's verdict). A rerun on an unchanged head also left the previous attempt's completed row in place, so a PR-scoped-but-attempt-blind read would accept a verdict computed against an older ruleset. The fallback POST, which omits external_id when the PR number cannot be resolved, read as a pass by name.

Fix

Replicate the two-dimensional binding that PR #9214 (commit a4e6944e3) established for the Internal Content Scan lane, applied to the five sibling lanes:

• Each lane stamps external_id=<lane>-pr-$PR-$WR_RUN_ID-$WR_RUN_ATTEMPT at both the open and fallback POST (env gains WR_RUN_ID/WR_RUN_ATTEMPT from github.event.workflow_run.id/.run_attempt).
• pr-readiness.yml four-field checkrun: specs (<Name>|<Name>|<prefix>-pr-|fast-gate.yml); the reader resolves Fast Gate's newest run for the head, filtered to THIS PR's (head repository, head branch) so a sibling PR's trigger run on the same head SHA can never be selected, builds want=<prefix>-pr-<PR>-<runid>-<attempt>, and filters check-runs to that exact id. Trigger-not-run-yet reads pending, holding the merge.
• The two live consumers of the old id (each lane's finalize stranded-run sweep and ai-review-human-override.yml) switch from exact match to a PR-dimension prefix match (startswith("<lane>-pr-$PR-"), trailing hyphen so -pr-9 != -pr-99) so they still span attempts.
• Fallback POST resolved: stamps the id when the PR is known; when unresolved it stamps nothing and is documented as intentionally pending-on-invisible (fail-closed).

Audit findings

• GPT lane posts exactly one idded check-run row per run (open POST or fallback) and PATCHes that same row; its "several passes under one name" are internal model passes into the same check-run, not extra un-idded rows. The binding filter does not blind it (except the documented no-PR fallback).
• Attempt binding (run id + attempt), not just PR binding, is required, per the scope correction on Fork AI-review lanes: readiness reads check-runs by name, so a sibling PR's verdict can answer for the wrong PR #9302: a rerun on an unchanged head would otherwise reuse the previous attempt's verdict.

Note on the reference

PR #9214 (commit a4e6944e3) merged into main while this branch was rebasing and is now an ancestor of HEAD, so the Internal Content Scan lane's own binding (its checkrun: spec, its TestForkScanVerdictIsBoundToItsPullRequest test class, and its (head repository, head branch) trigger-run filter) already exists in the working tree unmodified. This PR generalizes that exact proven pattern -- including the trigger-run repo/branch filter, which #9214 added to close a sibling-PR collision that the original commit 43d28c515 had not yet accounted for -- to the five sibling review lanes, and adds a shared _ForkLaneVerdictBinding test base class (subclassed per lane) rather than restating the four per-lane test methods five times.

Testing

• All .github/workflows/*.yml parse via yaml.safe_load.
• Targeted suites (test_pr_readiness_evaluate.py, test_ai_review_workflows.py, plus the touched-file suites test_acp_client.py, test_bench_kb_retrieval.py, test_conductor_agent.py, test_hooks_coverage.py, test_knowledge_search_upgrade.py, test_security_posture.py, test_session_pool.py): 1657 passed, 6 failed. The 6 failures are all in test_acp_client.py's tool-results/session-resume tests, unrelated to this PR's diff (workflow YAML + readiness test files) -- confirmed by running the identical 6 tests against a clean origin/main checkout, where they fail identically with the same assertion.
• scripts/check_black_formatting.py exit 0; isort --check-only and flake8 clean on the touched test files; scripts/docs_lint.py "All documentation checks passed".
• No stale exact-match sweeps remain (grep-verified against ai-review-human-override.yml and all five fork-*-review.yml finalize steps).

Pattern harvest

Not generalizable: this replicates an already-landed, already-reviewed binding pattern (PR #9214) onto five sibling lanes rather than introducing a new defect class.

Files changed (10)

• .github/workflows/pr-readiness.yml -- four-field fork specs + attempt-binding reader for the five sibling lanes, sharing the (head repository, head branch) trigger-run filter Internal Content Scan already uses.
• .github/workflows/fork-{opus,gpt,design,ux,first-principles}-review.yml -- two-dimensional open/fallback stamp; prefix-match stranded-run sweep.
• .github/workflows/ai-review-human-override.yml -- prefix-match on the second consumer.
• test/test_pr_readiness_evaluate.py -- shared _ForkLaneVerdictBinding base class + five per-lane subclasses (sibling / stale-attempt / sibling-trigger-run / own-row).
• test/test_ai_review_workflows.py -- pins updated with negative guards on the old exact match.
• docs/ci/ci-and-reviews.md -- fork readiness read documented as external_id-bound (PR + run + attempt).

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 383f674cc7e7d81fd508f376a6bba6c468fb663d — 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.

All facts I need are verified by reading: the trigger wiring (types: [in_progress, completed] on all six Fork * workflows), the exact Fork $cname name match for every lane, zero remaining name-only checkrun: specs, and the pre-existing max_by(.id) collapse pattern at three prior sites in the same file. Emitting the review.

First-Principles-Verdict: PASS

Verify on a live override rerun that a fresh check-run POST gets a higher id than the stale row — the one platform premise the collapse rests on, unverifiable from the repo.

Not justified as shipped

• Item 6 — undeclared: the WR_NAME/WR_STATUS "rerun starting" pending guard appears nowhere in the description's Fix bullets, only in diff comments.
• Item 7 — undeclared: the collapse of same-id rows to newest check-run id (which also changes the already-landed Internal Content Scan read) is likewise described only inside the diff.

Both are derived, not speculative: pr-readiness.yml:75 really is wired at in_progress, the six workflow names really match Fork $cname, and max_by(.id) already exists at 3 sibling collapse sites in the same file. Undeclared is an inventory tag here, not a concern.

What this change ships

Intent: stop a fork PR's merge gate from accepting an AI-review verdict computed for a different PR or a previous attempt — a FIX (linked issue #9302, replicating the merged #9214 pattern).

1. A sibling PR sharing the head SHA can no longer answer any of the five lanes' verdicts — justified
2. A rerun on an unchanged head can no longer reuse the previous attempt's verdict — justified
3. A lane that has not run yet now reads pending and holds the merge — justified
4. The no-PR fallback check-run now carries no id and reads pending (fail-closed) — justified
5. Finalize sweeps and the override lookup now match the PR dimension by prefix, spanning attempts — justified
6. Readiness triggered by a lane rerun's own start reads pending without querying check-runs — undeclared
7. Multiple rows sharing one external_id collapse to the newest check-run id, Internal Content Scan included — undeclared
8. Fork readiness docs state the bound read — justified

Sibling count: grep checkrun: in .github/workflows — 6 specs, all four-field; 0 name-only readers remain. No existing mechanism duplicated: this extends the one #9214 landed rather than spelling a second one, and the shared _ForkLaneVerdictBinding base avoids five restatements. The rewritten test pins each carry a negative guard on the old exact match, with the defect readable in the old three-field specs themselves.

[FIRST-PRINCIPLES-REVIEWED] 383f674

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 383f674cc7e7d81fd508f376a6bba6c468fb663d and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 383f674

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

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

The rerun-race guard is event-scoped, so the stale-verdict window it names stays open to every readiness evaluation the rerun itself didn't trigger.

Watch

The in_progress guard only fires when readiness was triggered by that lane's own rerun event (WR_NAME = "Fork $cname"). A human-override rerun keeps the same external_id, so from the rerun POST through queue time until the rerun's "Open check-run" step, the stale completed row is still the highest-id match — and a readiness evaluation triggered by any other monitored workflow in that window reads it as current and can publish it ("reading check-runs at that exact moment would still see only the OLD completed verdict" — true, but only the self-triggered path is closed). On a green-lane rerun with auto-merge armed, that is the exact merge-safety class the workflow's own in_progress comment says it protects. A state-based close is available and composes with this PR's own mechanism: the override handler already resolves the lane's check-run for details_url, so it can POST/PATCH an in_progress row with the same external_id before rerunning — the max_by(.id) collapse then reads pending for every trigger, and the WR_NAME string-coupling guard (which a lane rename silently breaks) becomes deletable.
Clears when: the override handler marks the lane's check-run in-progress before issuing the rerun, or a comment/test documents the residual window as accepted.

Suggestions

• ai-review-human-override.yml picks the newest attempt by sort_by(.started_at) while readiness picks by max_by(.id); converging both consumers on check-run id removes a second ordering definition for the same question.

[DESIGN-REVIEWED] 383f674

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 383f674cc7e7d81fd508f376a6bba6c468fb663d — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 383f674

Verdict parsed from the review's SHA-scoped output markers for commit 383f674cc7e7d81fd508f376a6bba6c468fb663d.

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

[bolichen97]

F1 (.github/workflows/fork-opus-review.yml:194 — Stage-2 reruns reuse the previous verdict identity): fixed.

Legitimate. WR_RUN_ID/WR_RUN_ATTEMPT are the triggering Fast Gate run's id and attempt, taken from github.event.workflow_run. A human-override rerun (gh api .../runs/<id>/rerun) re-executes the Stage-2 lane's own run directly, without Fast Gate re-running, so the trigger-bound id stays identical between the stale failed attempt and the fresh rerun. The reader's exact-id match then saw both rows and let the stale failure outvote the fresh success on fail-precedence — a real readiness-availability defect, reachable on every override rerun of these five lanes.

Fix: each lane's external_id now appends its own github.run_attempt as a trailing segment (<lane>-pr-<PR>-<WR_RUN_ID>-<WR_RUN_ATTEMPT>-<LANE_RUN_ATTEMPT>), and pr-readiness.yml's reader matches the trigger-bound id (the scan lane's existing shape, unsuffixed) OR that id plus any lane-attempt suffix, then collapses to the newest check-run by id — so a fresh rerun is never outvoted by the stale attempt it replaces. This mirrors the same-repo lanes' newest-run-wins collapse used elsewhere in this file. Covers all 5 sibling lanes (opus/gpt/design/ux/first-principles); the pre-existing scan lane (merged main PR #9214, out of this PR's scope) is unaffected and untouched.

New test test_a_stale_failure_does_not_outvote_a_human_override_rerun on the shared _ForkLaneVerdictBinding base class (covers all 5 subclasses) fixtures a stale-failure + fresh-success pair sharing one trigger-bound id, distinguished only by the lane-attempt suffix, and asserts the fresh success wins.

self-added: yes
mechanism: LANE_RUN_ATTEMPT suffix on external_id + newest-by-id collapse in the readiness reader, to survive a Stage-2-only rerun that leaves the Fast Gate trigger id unchanged.

[bolichen97]

Backend Lint & Type Check (3.12) — comment-history gate FAILED "grew from 49 to 56" on test/test_ai_review_workflows.py: inherited main-red, not caused by this PR.

check_comment_history.py diffs this file's total violation count against comment-history-baseline.json's stored value (49), not against this PR's own added lines — the gate's own output confirms "0 new offender(s), 1 grown count(s)". This PR's diff to that file adds zero lines matching the narration patterns (grep-verified: no used to/no longer/previously/#issue references added by this PR).

Running the checker directly against a clean origin/main (7177fa4) checkout shows the file's real current count is already 56 — the baseline (49) went stale from other PRs that landed on main after it was last regenerated, unrelated to this branch. An open PR, #9364 ("chore(comments): drop the history narration main outgrew its baseline on"), already tracks and fixes exactly this drift on main. Once #9364 merges and this branch rebases past it, this check clears without any change needed here.

[bolichen97]

Item 5 (LANE_RUN_ATTEMPT undeclared and inert): fixed.

Legitimate. Verified concretely: a fixture with two check-run rows sharing the exact same external_id (no suffix), one failure at check-run id 1 and one success at check-run id 2, resolves correctly to the fresh success under select(.external_id == $x) | max_by(.id) alone -- the check-run's own .id is distinct per POST and monotonically increasing, independent of what external_id it carries, so the collapse-by-id already solves the human-override-rerun case without any suffix.

Reverted LANE_RUN_ATTEMPT from all 10 stamp sites and the startswith/OR-branch in pr-readiness.yml. The five sibling lanes' id shape is now byte-identical to the scan lane's (<lane>-pr-<PR>-<run>-<attempt>, no fourth segment), and the reader is back to a single exact-match branch, collapsed to the newest check-run by id. Test test_a_stale_failure_does_not_outvote_a_human_override_rerun (on the shared _ForkLaneVerdictBinding base class, covers all 5 lanes) now fixtures two rows sharing one identical external_id and asserts the newer row's verdict wins -- the scenario your Watch item asked for, confirming max_by(.id) alone resolves it.

self-added: no
mechanism: reverted -- LANE_RUN_ATTEMPT and the OR-branch it required are removed; only the existing check-run-id collapse remains.

[bolichen97]

F1 (.github/workflows/pr-readiness.yml:719 -- rerun startup reuses the previous successful verdict): fixed.

Legitimate and distinct from the earlier stale-attempt finding. pr-readiness.yml's workflow_run trigger fires at BOTH in_progress and completed (confirmed at the top of the file) for each fork lane's own workflow display name (e.g. Fork Opus 4.8 Review, which a fork's fork-opus-review.yml declares as its name:). A human-override rerun (gh api .../runs/<id>/rerun) therefore fires readiness the INSTANT the rerun starts, before that rerun's own "Open check-run (in progress)" step has posted a fresh row -- so the read at that moment sees only the OLD completed+success verdict and can publish a stale success, a real (if narrow) merge-race window.

Fix: readiness now reads github.event.workflow_run.name and .status (as WR_NAME/WR_STATUS) into the evaluate step's own env. In the fork-lane loop, when this evaluation's own trigger is that exact lane's in_progress event (WR_NAME == "Fork $cname" and WR_STATUS == in_progress), it reads pending directly -- no check-runs API call at all -- rather than risking a stale read. The rerun's own completed event re-triggers a real evaluation once it finishes.

Two new tests on the shared _ForkLaneVerdictBinding base class (covers all 5 lanes): test_a_rerun_starting_reads_pending_not_the_stale_check_run fixtures a stale-success check-run plus this lane's own in_progress trigger, and asserts pending (not the stale success); test_a_different_lanes_rerun_starting_does_not_force_this_lane_pending asserts the guard is scoped to THIS lane's own fork workflow name, so a sibling lane's rerun starting cannot blind an unrelated lane to its own genuine verdict.

self-added: yes
mechanism: WR_NAME/WR_STATUS env vars + an early-continue guard in the checkrun-lane loop, forcing pending when this evaluation's own trigger is the lane's just-started rerun.