fix(ci): guard the review lanes against verdict overwrite (#8344) by bolichen97 · Pull Request #8450 · kirodotdev/KiroCrew

bolichen97 · 2026-09-04T12:37:03Z

Summary

A lane's summary comment is one slot shared by every run on the PR, and it is upserted in place. The REST comments API has no If-Match, so a write to that slot is last-writer-wins, and it exposes no edit history, so the loss is undetectable afterwards. A failed review run replaced a posted [GPT-REVIEWED] pass verdict on PR #7845 a day later, recoverable only from GraphQL edit history (#8292). First-principles is the highest-priority lane: its BLOCK verdict gates PR readiness, so a buried BLOCK is exactly that harm. Merged #8342 addressed this in codex-review.yml only.

This PR makes exactly one kind of run allowed to claim that slot — a completed verdict for the PR's current head — on eight of the ten lanes that upsert a verdict comment (nine call sites, because fork first-principles upserts from two places). The other two lanes stay as they are and are now both declared in the doc: codex-review.yml (#8342's merged preserve-and-prepend shape) and claude-review.yml (a plain lookup-then-PATCH whose incomplete path posts nothing at all). Before this PR the doc's census named only the first, which is what First Principles' Watch item caught; see "The tenth site" below.

Closes #8344

Two starting states, and the distinction matters because a reviewer asked for it:

Seven lanes were genuinely unguarded (fork Opus, design, UX, first-principles, and the fork design / fork UX / fork first-principles twins): they PATCHed unconditionally, and they are what fork review lanes: incomplete review body still overwrites a posted verdict (fork-lane twins of #8292) #8344 is about.
fork-gpt-review.yml was already guarded, with the strongest posture in the repo — an incomplete run never modified an existing comment at all. That arm is what the other seven now adopt, so it is not a newly-guarded site for the fork review lanes: incomplete review body still overwrites a posted verdict (fork-lane twins of #8292) #8344 defect and this PR claims no fix there. It is not left untouched, though, and the earlier wording overstated that (Design Review and First Principles both flagged it): refactoring it onto the shared function also gives it the superseded-head and unreadable-head arms below, neither of which it had. Strengthening, not a no-op.

The guard: two ways to lose a verdict, one no-touch answer

A run stands down — leaves an existing comment untouched, no PATCH at all — in both of the cases that can lose a live verdict:

1. The body carries no "<stamp> <head>" proof marker. A review failure. PATCHing a "review incomplete" / "could not complete" body over a completed verdict buries that verdict, blocking findings included.

Preserving the verdict and prepending a dated staleness notice — #8342's shape — is not a safe alternative, and this is the substantive correction this PR took from review: it reads the body and writes a merge of it back, so a verdict published between the read and the write is restored away. concurrency does not cancel a run for a different head, so that window is real, and on the first-principles lane the thing restored away can be a BLOCK.

2. The body is a completed verdict, but for a superseded head. The same loss arriving late. An older run can finish after a newer one published, because the fork lanes' concurrency group is keyed per head so they are not cancelled — and a cancelled same-repo run still executes its if: always() posting step. So the step confirms this run is still the PR's head before claiming the slot.

3. The PR's head cannot be read. Writing is the destructive half of the guard, so it does not proceed on an unknown: a head unreadable after three attempts is not confirmed current and the slot is left alone. The read retries first — for attempt in 1 2 3 with sleep "$attempt", the same bounded shape this lane's own intent fetch and finalize PATCH already use — because one blip is not evidence about the PR, and an API broken enough to fail all three would fail the write too.

Around all three arms:

Whether a comment exists is the other gating input, so that read retries too, on the same bounded backoff. A lookup still erroring afterwards counts as "a comment may exist", never as "none does" — a withheld run posts nothing rather than plant a second marker comment over a possibly-live verdict, which no later run undoes.
When the lookup succeeded and found nothing, there is no verdict to lose, so the body is posted as a new comment.
A completed verdict for a confirmed current head whose comment lookup failed still CREATEs rather than stay silent: a duplicate comment is recoverable, an unposted verdict is not (fix(ci): stop fork GPT lane hiding a verdict behind an incomplete run (#8292) #8350).
Human overrides and skip notices are current-head determinations, not review failures, so their upsert sites keep their own unconditional PATCH and never route through this function — guarding them would pin a stale verdict onto a revision the lane has ruled out of scope.

Nothing is lost by standing down. The comment names the head it reviewed; each head's own check-run is finalized fail-CLOSED by the step below the upsert; and pr_status.py's evaluate_reviewer_markers matches reviewer stamps against the current head, so a comment left in place for an older head reads as stale, never as an approval of this one.

Because nothing is merged, the function reads only the comment id. No body capture, no notice markers persisted into comment bodies, no notice-stripping pass. Lookup captures the full paginated output first and selects off the captured value (awk 'NR == 1'), never ... | head -n1 inside the pipeline, which lets head's early exit SIGPIPE the api call and misread a successful lookup as failure under pipefail (#8350).

What changed after review (seven rounds; four produced a fix, and the rest are answered)

Round 1 — the merge is the bug. GPT 5.6 (fork-gpt-review.yml:1067, "incomplete runs can overwrite newer verdicts") and First Principles ("this port silently deletes the fork GPT lane's stronger guard and reinstalls the exact PATCH-back its own comments call the bug") converged on the same defect from two directions, and the second named the right resolution: base fork-gpt's no-touch posture was the stronger of the two, so it is now canonical for all eight lanes rather than the one that got overwritten. The notice machinery, its persisted stale-notice-begin/end markers, and the position-anchored strip an earlier pre-push round had to harden are all deleted — a net subtraction from the function.

Round 2 — the same loss arriving late. GPT 5.6 (fork-opus-review.yml:534, "older completed runs can erase newer-head verdicts across all fork lanes"): head B completes, then an already-running head A completes and PATCHes the shared slot, and B's current verdict disappears. Correct, and it is the completed path, which round 1 left as-is. Fixed as the second withhold arm above rather than by the suggested remedies: per-branch serialization would break the fork lanes' deliberate per-head keying (two PRs can share a commit, which is why those groups key on head repository + head branch), and head-specific comments would replace the one-slot upsert with one comment per push.

Round 3 — the guard must not resolve its own unknown in favour of writing. GPT 5.6 (design-review.yml:560): round 2's head check failed open, so an older completed run plus a transient PR-head read failure still PATCHed the slot. Correct, and the general principle is the one this whole PR is built on — the destructive action needs a positive answer, not the absence of a negative one. Fixed by retrying the read on the repo's established bounded-backoff shape and then treating an unreadable head as a third withhold reason. Not fixed by the suggested "create a separate completed comment", which plants a duplicate marker comment on every transient blip and hands the next run's startswith(marker) finder two slots to choose between.

The result is simpler than round 2's version: one comparison decides, and every arm of the guard is now the same sentence — am I the current head, and do I have a completed verdict?

Round 4 — partially accepted, and the second half declined on merit. GPT 5.6 (design-review.yml:596): a same-head BLOCK already posted, a cleared re-run, and a comment-lookup failure produce a duplicate comment, and evaluate_reviewer_markers then still sees the old [BLOCK-MERGE] and keeps the PR blocked. Its requested fix has two halves.

Accepted: "retry until the existing-comment state is known" — the comment lookup now retries on the same bounded backoff as the head read, so both gating inputs are treated the same way and the duplicate becomes correspondingly rarer. The identity test pins that both reads retry.

Declined, with the reasoning: "never CREATE when lookup failed" reverses #8350 and trades a recoverable failure for an unrecoverable one.

The consequence is narrower than the finding states, and round 7's adjudication pass is what established it: the comment slot is not the merge gate. PR Readiness resolves each reviewer lane from the lane's check-run / workflow run, never from the comment — pr-readiness.yml lists codex-review.yml|GPT 5.6 Review for a same-repo PR and checkrun:GPT 5.6 Review for a fork one, and the fork lane's fail-closed Finalize check-run step sets that check-run from the on-disk codex-review-output.md, independent of the slot. The comment reader, evaluate_reviewer_markers, lives in pr_status.py — the prepare-pr skill's status readout, an agent-side helper, not a gate. So a stale same-head [BLOCK-MERGE] comment misleads that readout and nothing else, and it self-corrects on the next run that patches the slot. Earlier rounds of this PR conceded the finding's "the PR stays blocked" premise; that concession was wrong and is withdrawn.
The direction of the described failure is fail-CLOSED even in that readout. The outcome is a PR that stays blocked while a clear verdict exists, not one that merges while a BLOCK exists. pr_status.py documents that asymmetry as deliberate at the site GPT's scenario runs through: "a [BLOCK-MERGE] for the current head gates from ANY trusted comment, bound or not — injection can deny a review, never forge one." Over-blocking is the designed behaviour of that reader, and it clears on the next run.
Never CREATE means the clear verdict is never published at all — not to the comment, and the reader sees the stale BLOCK anyway. The PR is blocked in both variants; only one of them also loses the new verdict. fix(ci): stop fork GPT lane hiding a verdict behind an incomplete run (#8292) #8350 weighed exactly this and chose CREATE, because a duplicate comment is recoverable and an unposted verdict is not.
The behaviour is unchanged from main. Every base lane already created when the lookup returned no id, and codex-review.yml still does. This finding describes a standing property of the upsert model, not something this diff introduces — so accepting it here would leave the same path live on the ninth site and on main.

Round 5 — declined: the ask is a redesign of the comment model, and the residual window is the floor. GPT 5.6 (design-review.yml:609): run A confirms it is the head, pauses, B publishes, A resumes and PATCHes. True, and irreducible with the tools available. Its own suggested remedies name why it is out of scope: "use head-specific comment slots or serialize each lane's writes per PR."

The window is already the theoretical minimum for a read-then-write. The confirmation is the statement immediately before the write, separated by one if test. Closing it needs compare-and-swap, and the REST comments API has none — no If-Match, no ETag precondition. Every round above narrowed the window; this round asks to eliminate it, which no amount of ordering achieves.
Head-specific comment slots means one comment per push. That is the model fix(ci): never let an incomplete review body overwrite a posted verdict (#8292) #8342 and fix(ci): stop fork GPT lane hiding a verdict behind an incomplete run (#8292) #8350 chose against, and it would land on all nine sites plus every other marker comment in the repo.
Per-PR serialization is not available to the lanes that matter. The fork lanes key concurrency on head repository + head branch precisely because two PRs can share a commit; collapsing them to one group per PR reintroduces the cross-PR mix-up those groups exist to prevent, and workflow_run-triggered runs cannot be ordered against each other anyway.
The residual harm is bounded, fail-CLOSED, and confined to the human surface. Both racers hold completed verdicts. If the older one wins, the slot holds a verdict stamped for an older head, which pr_status.py reads as stale — readiness stays blocked, never passes — while the newer head's own check-run, the actual merge gate, is untouched and correct. The cost is a comment that needs a re-run to refresh, not an un-gated merge and not a lost gate signal. The docs state this floor plainly: "the comments API has no If-Match, so a write to that slot is last-writer-wins, and it exposes no edit history, so the loss is undetectable afterwards."

Redesigning the reviewer comment model is a maintainer's call, not a rider on this port. The lane's /ai-review override gpt <sha>: <reason> is the right instrument if a maintainer agrees.

Not fixed here, and now declared rather than implied: codex-review.yml keeps #8342's merged preserve-and-prepend shape and is the one remaining upsert site with a read-modify-write window on its slot. docs/ci/ci-and-reviews.md says so explicitly. Reversing a merged, human-reviewed decision on that lane is a separate call and does not belong as a rider on this port.

Round 6 — declined: this is round 4's declined half restated. GPT 5.6 (design-review.yml:618, "failed lookup preserves stale same-head BLOCK"): an existing same-head BLOCK, a clearing re-run whose comment lookup fails, CREATE succeeds, both comments remain, and evaluate_reviewer_markers keeps the PR blocked. Its requested fix — "publish only after successfully locating and replacing or retiring every same-head marker comment" — is "never publish when the lookup failed" with a stronger precondition, and the round-4 answer applies unchanged: the outcome is fail-CLOSED (blocked while a clear verdict exists, never merged while a BLOCK exists); making publication conditional on the lookup does not unblock that PR, because the stale [BLOCK-MERGE] is still there and the reader still blocks — the only difference is that the clearing verdict is then published nowhere; and reaching that branch already means three retried reads failed, a state in which the proposed "retire" writes would very likely fail too. The path is also unchanged from main and identical on codex-review.yml.

Round 7 (post-rebase, 894f5fd) — the same two asks, now raised together, and the lane's own adjudicator agrees on one of them. Two fenced findings:

F1 design-review.yml:574 is round 5 restated: head-confirm races the PATCH. The annotate-only adjudication pass added upstream in ai-review: give the stage-3 arbiter annotate-only voice on security-class findings and require a trigger path for the withholding exemption #8693 independently reached the same conclusion this PR argued in round 5 and machine-FLAGGED it as a likely edge case: "The newer verdict can only be overwritten if a full review run publishes within the sub-second gap between the head read and the PATCH, which a minutes-long run cannot do, so the reaching timing is self-contradicting." It also pre-drafted the override rationale. The fence still blocks regardless of that flag, by design.
F2 fork-gpt-review.yml:1247 is round 4/6 restated, with a new suggested remedy: not "never CREATE", but "reconcile every matching marker comment after CREATE, neutralizing superseded duplicates before returning." The adjudicator UPHELD this one as a genuine transient scenario, and it is right that the scenario is reachable. A third run on the identical head then FLAGGED the same finding and named the reason: "Readiness is check-run-driven (pr-readiness.yml:556; finalize sets it from on-disk output at fork-gpt-review.yml:1268-1284), so a stale/duplicate lane comment cannot block readiness." That is verifiable and correct — see the corrected bullet in round 4 above — so the finding's stated consequence ("keeps readiness blocked", "falsely reports the head blocked") is about the wrong surface. The remedy is also not available, for a reason specific to the branch it lands on: reaching it means the paginated comment lookup failed three consecutive times, so there is no id to reconcile — the reconcile pass needs the very lookup that just failed. Doing it anyway (retry the lookup after the CREATE, then rewrite the older comment's [BLOCK-MERGE] to a defused marker, mirroring the existing [BLOCK-MERGE-DOWNGRADED] sed) is worse on the axis that matters: it is a read-modify-write on a slot we have just established we cannot read reliably, so a verdict published between the failed lookup and the reconcile would be defused — converting an over-block into an under-block. That is the direction pr_status.py documents as the one it will not trade: "a [BLOCK-MERGE] for the current head gates from ANY trusted comment, bound or not — injection can deny a review, never forge one." Over-blocking is designed behaviour of that reader and clears on the next run; under-blocking is the class the fence exists to stop.

The lane is not converging, and that is worth recording as data for whoever adjudicates. On the identical head 3fe6c067, two runs produced two different blocking findings (design-review.yml:609, then :618) with no diff change between them. On 894f5fd it raised both of those asks at once, and a third run on that same head raised only one of them — the same finding the previous run's adjudicator had UPHELD, this time FLAGGED. So neither the reviewer nor its adjudicator is deterministic on this diff. Rounds 1–3 and round 4's retry half each produced a real fix and are shipped. Rounds 4-declined, 5, 6 and 7 all reduce to the same two asks — stop publishing when a read fails, or replace the one-slot upsert model — and both reverse a merged, human-reviewed decision (#8342, #8350) or the documented fail-closed asymmetry in pr_status.py. Neither is a call this port should make, and neither is made here.

The tenth site (`claude-review.yml`), and the census the doc was carrying

First Principles' Watch item was right: the title said "every review lane" while claude-review.yml kept an unguarded unconditional PATCH, undeclared — unlike codex-review.yml, whose exclusion the doc already named. The doc's census said "nine upsert sites"; there are ten. Fixed here as a documentation and framing correction, not a tenth port:

docs/ci/ci-and-reviews.md now counts ten lanes that upsert a verdict comment and declares both exclusions in one list, with each one's residual named.
The commit subject drops the "every review lane" overclaim.
claude-review.yml's residual is genuinely narrower than the others': its incomplete path posts nothing at all (if [ "$kind" = "incomplete" ]; then ... exit 0), so the GPT review verdict lives in one mutable comment, so an incomplete run hides a blocking finding #8292 class — a failure notice burying a verdict — cannot reach it. What remains is only the superseded-completed window: its concurrency group is per-PR with cancel-in-progress: true, but the posting step is if: always(), which a cancelled run still executes, so an older run holding a completed verdict can still claim the slot.
Porting the guard onto it is not in this PR, because that lane's single upsert serves both the verdict and the human-override body. Routing it through guarded_comment_upsert would first require splitting those two paths — overrides are current-head determinations that keep their unconditional PATCH by declared design — which is a design change on a lane outside the fork review lanes: incomplete review body still overwrites a posted verdict (fork-lane twins of #8292) #8344 census, on top of an eight-workflow diff. Same reasoning the maintainer already recorded for codex-review.yml in ci: converge codex-review.yml onto the shared guarded_comment_upsert shape #8462.

Rebase onto current `main`

The branch went dirty and is rebased twice, now onto 9b46746e3. Two upstream commits touched these files:

#8693/#8707 7ef2209cb (annotate-only adjudication voice on security-class findings):

fork-gpt-review.yml — resolved by keeping both sides: upstream's new "Fenced finding(s) machine-flagged as likely edge case" render block stays inside the comment body, and the tail of the step is this PR's guarded_comment_upsert. Upstream's block already writes to $RUNNER_TEMP, so it needed no adaptation.
test_ai_review_workflows.py — upstream's only change inside TestForkGptVerdictVisibility was black reflow, and this PR deletes that class in favour of the per-lane parameterized TestReviewLaneVerdictVisibility, so the deletion is the correct resolution. Upstream also left a cross-reference to the deleted class in a comment, which now points at the assertion that actually pins the behaviour.

#8738 8c7ad0441 (a GPT reviewer refusal is its own terminal state):

test_ai_review_workflows.py — its new TestGptRefusalTerminalState class is kept in full and lands after this PR's class; the conflict was only that git anchored it against the tail of the class this PR removes.
docs/ci/ci-and-reviews.md — auto-merged; its addition is in the marker-contract list, a different section from the upsert paragraph this PR rewrites.

Both upstream surfaces are green in the post-rebase run (541 tests, 0 failures), and the new adjudication surface is what produced the round-7 flag above.

One incidental fix, worth flagging to a maintainer: at 9b46746e3, test/test_ai_review_workflows.py is not black-clean and is not in .github/black-baseline.txt (8c7ad0441 added a signature black collapses back to one line). Verify with git show 9b46746e3:test/test_ai_review_workflows.py > /tmp/f.py && black --target-version py310 --check /tmp/f.py. Since the black gate is diff-scoped, that drift lands on any PR touching the file; this PR formats it, which is the three-line hunk at test_completed_verdict_quoting_the_marker_is_not_reclassified. main-ratchet-audit.yml runs whole-tree and should be red on main until something formats it.

Why a byte-identical inline function, not a shared script

The issue thread suggested extracting one shared script under .github/scripts/. Deliberately not done, for an availability reason specific to these lanes:

Fork lanes read their workflow definition from the default branch, but execute scripts from the base_sha checkout. A guard shipped as a checkout script would leave every currently-open fork PR (whose base_sha predates this merge) unguarded until rebase — and the five fork lanes are the majority of the sites. Inline in the workflow definition, the guard covers ALL fork PRs the moment this merges.
The posting steps run if: always(), so they must work when checkout failed; a missing script would force either a create-only fallback (comment spam per run) or an unguarded-PATCH fallback (the bug, back).

Single-source-of-truth is enforced mechanically instead: all 8 lanes define the SAME guarded_comment_upsert() bash function and test_guard_function_is_byte_identical_across_all_lanes pins every copy to one canonical body, so per-lane drift is a test failure.

Also folded in: the six lanes that wrote their comment body to a fixed /tmp/<lane>-comment.md now write under ${RUNNER_TEMP:-/tmp} like the merged codex template — on a runner these are equivalent (RUNNER_TEMP is always set), and it makes the steps' bash parallel-safe for the bash-level tests.

Tests (bash-level, per lane, against each lane's real posting step)

TestReviewLaneVerdictVisibility runs each lane's REAL posting-step bash with a stubbed gh. The finder branch executes the step's actual --jq filter through real jq, so filter drift fails the tests instead of hiding; the stub also answers the PR-object read, with knobs for "the PR has moved on" and "the read failed".

test_incomplete_run_never_overwrites_a_posted_verdict ×8 — no PATCH call and no created comment at all, and the step names the comment id it left alone; that id came through the real author-guarded filter, so it is the bot's comment (123) and never the marker-planting impostor's (999)
test_a_superseded_completed_verdict_leaves_the_comment_alone ×8 — round 2's finding, pinned
test_an_unreadable_pr_head_retries_then_withholds ×8 — round 3's finding: exactly three reads are recorded, and nothing is written
test_incomplete_run_with_a_failed_lookup_posts_nothing ×8 — exactly three comment reads are recorded, and a lookup error is still not read as "no comment exists"
test_completed_verdict_still_replaces_the_comment ×8 and test_completed_verdict_creates_when_the_lookup_fails ×8 — the asymmetry that keeps a real verdict from being dropped
test_incomplete_with_no_existing_comment_creates_as_before ×8
fork-FP withheld-site leaves a posted verdict alone (that lane's second call site); UX skip-notice still replaces wholesale (pins the deliberate non-guard)
identity test (byte-identical function across all 8; it asserts the body is not captured and that the withhold block reaches no PATCH, so neither a merge-and-PATCH shape nor a dropped arm can come back unnoticed) + wiring test (every lane calls it; fork-FP covers both its sites)

The notice-strip regression tests from the first draft are removed along with the notice machinery they pinned; the invariant they protected (never delete verdict content) is now structural — nothing is rewritten, so nothing can be truncated.

Mutation-verified four times on the shipped shape, each reverting exactly the intended tests to red and green again on restore:

turning the ux lane's no-touch arm back into a PATCH → test_incomplete_run_never_overwrites_a_posted_verdict[ux] + the identity test
deleting the design lane's lookup_ok fail-closed arm → test_incomplete_run_with_a_failed_lookup_posts_nothing[design] + the identity test
inverting the fork-opus lane's superseded-head comparison → test_a_superseded_completed_verdict_leaves_the_comment_alone[fork-opus] + the identity test
restoring the fail-open behaviour on an unreadable head in the ux lane → test_an_unreadable_pr_head_retries_then_withholds[ux] + the identity test

The incomplete-case fixtures use a verdict header with a stale proof marker (the live #7845 shape). A header-LESS summary crashes the design-family posting steps earlier, at the grep -iE '^...-Verdict:' pipeline under set -o pipefail — a pre-existing lane defect outside this PR's scope (in production those runs post nothing at all, which cannot bury a verdict).

Verification

Post-rebase: test_ai_review_workflows.py, plus test_prepare_pr_findings.py and test_pr_quality_gates.py — 541 passed, 0 failed. That run covers this PR's per-lane guard tests, the adjudication/fenced-flag tests 7ef2209cb added upstream, and the refusal-state tests 8c7ad0441 added.
black --check clean on test_ai_review_workflows.py (upstream dropped it from .github/black-baseline.txt, so it is now gated) / flake8 src/kiro_crew test / mypy --platform linux src/kiro_crew (1294 files, no issues) / black + subprocess-encoding ratchets / docs-lint / brand / harness-parity / changelog-history all pass
YAML parses for all ten review-lane workflows and bash -n is clean on the merged fork-gpt post step; the 8 function bodies verified byte-identical by the identity test after YAML dedent
scrub-lint --no-history reports one pre-existing hit in test/test_atomic_write_named_duplicates.py, a file this PR does not touch and whose content is byte-identical to main — the De-Amazon Scrub Lint check was green on the previous head with the same file in the tree
docs/ci/ci-and-reviews.md updated in the same commit

Screenshots

No visual change — this PR touches only GitHub Actions workflow definitions, one CI doc, and a backend test module. No user-facing surface, no website/ file, and no rendered output is affected.

Pattern harvest

The bug and all three near-misses are one shape: a mutable shared slot with no compare-and-swap. #8292 was "a failed run's body replaces a verdict". Draft 1 of this fix was "a failed run's merge of a verdict replaces a newer verdict". Draft 2 was "an older completed run replaces a newer verdict". Draft 3 was "a run that could not tell whether it was current replaces it anyway". Each round the reviewers moved the same question one step earlier, and the terminal answer is the same one: on an API with no If-Match, the only race-free write a run that is not provably authoritative can make is no write at all. So the guard is a return, and every arm of it is one sentence: am I confirmed to be the current head, and do I have a completed verdict?

The corollary is the part worth keeping: a guard whose own input is unknown must not resolve the unknown in favour of the destructive action. Draft 2 failed open on an unreadable head out of a real fear — dropping a verdict is unrecoverable — but the fix for that fear is to make the unknown rare (retry) rather than to make it permissive.

The informational bit given up (this verdict is stale) is recovered from an immutable per-head record that already existed — the fail-closed check-run — rather than by writing harder into the mutable one.

Rule candidate: when N workflows must enforce one invariant inside steps that run if: always(), define it as a byte-identical bash function in each workflow and pin every copy to one canonical body with a test — never a checkout-loaded shared script, whose availability dies with a failed checkout and whose base_sha pin leaves already-open fork PRs unguarded until they rebase.

Third rule candidate, from the tenth-site miss: a census stated as a number in prose is a claim no test reads, so it goes stale in exactly the direction that flatters the change. The doc said "nine upsert sites" and the subject said "every review lane" while ten lanes upsert and only eight were guarded — and the one left out was the one whose narrower residual made it easy to overlook. The durable form is to declare each exclusion by name with its own residual, which is what the doc now does; a bare count invites the reader to assume the remainder is empty.

Second rule candidate: porting a guard onto a lane that already has one must compare postures first. Draft 1 downgraded fork-gpt because the merged lane was treated as the reference implementation by default; the deleted hunk's own comment said why it was stronger. When a "port" produces a deletion in the target, that deletion is the thing to justify.

github-actions · 2026-09-04T12:39:42Z

Design Review (Fable 5) — ✅ PASS

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

The claims verify: eight lanes carry the byte-identical guarded_comment_upsert, the identity and wiring tests exist (test/test_ai_review_workflows.py:5178), the doc census declares both excluded lanes with their named residuals, and fork-gpt's stronger no-touch posture became the canonical shape rather than being downgraded. The residual read-then-write race is genuinely irreducible without If-Match, and its failure direction is fail-closed (stale comment, per-head check-runs untouched). The excluded lanes' remaining windows are declared with rationale, and the machine gate (pr_status.py head-scoped stamps, fail-closed check-runs) stays correct even where the comment slot loses a race.

Design-Verdict: PASS

No-write-unless-provably-authoritative is the correct terminal design for a slot with no compare-and-swap; every residual window is named, bounded, and fail-closed.

Suggestions

The two exclusions (codex-review.yml, claude-review.yml) are again a prose-only census — the failure mode this PR's own harvest names; a small test asserting exactly which lanes define (and don't define) the guard would pin it.

[DESIGN-REVIEWED] 894f5fd

github-actions · 2026-09-04T12:40:48Z

GPT 5.6 Review — ✅ human override accepted

Human judgment by @bolichen97 overrides the GPT 5.6 finding for 894f5fd65a9bb3ac980c6f529a439196dada9362; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

github-actions · 2026-09-04T12:41:17Z

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 894f5fd65a9bb3ac980c6f529a439196dada9362 — 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.

First-Principles-Verdict: CONCERNS

The guard is cause-level and earns its nine sites, but codex-review.yml keeps the exact read-modify-write shape this PR's own canonical comment indicts.

What this change ships

Intent: stop any review run from erasing a live verdict in the one shared PR comment slot — a FIX (#8344, defect #8292).

A failed run no longer replaces a posted verdict; it leaves the comment untouched — justified (the fix, 7 lanes)
A completed verdict for a superseded head no longer claims the slot — justified, declared
An unreadable PR head withholds the write after 3 retries — justified, declared
Both gating reads retry transient failures before deciding — justified, declared
Fork GPT lane rebuilt onto the shared function, gaining arms 2–3 — declared strengthening
Withheld run whose comment lookup failed posts nothing — justified, declared
Comment temp files move /tmp → RUNNER_TEMP on every touched lane — rides along, derived (test-isolation invariant)
Doc census rewritten to ten lanes with two declared exclusions — mandated same-commit doc rule
Fork-only test class replaced by an 8-lane parametrized suite plus byte-identity pin — fix verification
codex-review.yml and claude-review.yml stay outside the guard — declared exclusion, counted below

Watch

The PR's canonical comment says preserve-and-prepend "is not a safe alternative… a verdict published between the read and the write is restored away," yet codex-review.yml:1328–1361 keeps exactly that shape (grep stale-notice|prepend, 1 site) — on the same-repo GPT lane where GPT review verdict lives in one mutable comment, so an incomplete run hides a blocking finding #8292 actually happened. Its only stated support is that fix(ci): never let an incomplete review body overwrite a posted verdict (#8292) #8342 just merged and is test-pinned: recency, not a derived constraint. Accepted-and-deferred, but a human should own that residue.
claude-review.yml:499 still runs | head -n1 inside the lookup pipeline — the fix(ci): stop fork GPT lane hiding a verdict behind an incomplete run (#8292) #8350 pattern this PR replaced with awk in 8 lanes (1 unfixed sibling; its failure direction is a duplicate comment, not a lost verdict) — plus the declared superseded-head window.

Subtractions

Converge codex-review.yml onto guarded_comment_upsert and delete its codex-stale-notice-begin/end machinery and strip pass (codex-review.yml:1328–1361) — the same net deletion round 1 performed on fork-gpt, and it removes the repo's last read-modify-write window on a verdict slot.

[FIRST-PRINCIPLES-REVIEWED] 894f5fd

github-actions · 2026-09-04T12:43:57Z

Opus 4.8 Review — ✅ no blocking findings

Reviewed 894f5fd65a9bb3ac980c6f529a439196dada9362 — this comment is updated in place on each push.

Review details

I've read the full diff, the test rewrite, and checked the base AUTOSDE rules against the changed files. My analysis:

The candidate discovery pass reported no candidates. Re-deriving independently: the shared guarded_comment_upsert function's control flow is sound — the lookup and head reads are inside if-conditions (safe under bash -e), matches is captured before awk 'NR == 1' selection (no SIGPIPE), withhold paths all return 0 before any PATCH, and the common single-run completed-verdict case still PATCHes/creates as before (confirmed by test_completed_verdict_still_replaces_the_comment across all eight lanes).
Blocking base rules: only no-test-side-effects matches the changed files (test/**/*.py). The new _run_step spawns bash with cwd= under tmp_path, redirects RUNNER_TEMP under tmp_path, stubs gh to write only under $STUB_CALLS (tmp_path), and every completed/incomplete lambda writes under cwd/runner_temp. No host writes. The old hardcoded /tmp/*.md paths are now ${RUNNER_TEMP:-/tmp}. No violation.
No other blocking rule's file-patterns match the workflow YAML or docs changes. CHANGELOG.md is untouched.

I could not ground any of (a) concrete input, (b) call path, (c) observable wrong outcome for any defect at 80+ confidence.

No findings.

[OPUS-REVIEWED] 894f5fd

Verdict parsed from the review's SHA-scoped output markers for commit 894f5fd65a9bb3ac980c6f529a439196dada9362.

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

bolichen97 · 2026-09-04T13:45:04Z

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97#bb3ad1ca]

accepted-and-deferred — the Watch item and its Subtraction (converge codex-review.yml's inline sed guard onto guarded_comment_upsert and add it to the identity test) are tracked in #8462 (label deferred-finding, assignee @bolichen97, Due: 2026-09-18).

Why deferred rather than folded in: codex-review.yml was explicitly outside the #8344 census as the already-fixed lane; its merged shape is pinned byte-for-byte by TestGptVerdictVisibility, so the convergence moves those tests too, and stacking that churn onto an 8-workflow PR would re-arm every review lane for a change with no behavioral delta on the 8 lanes this PR fixes. The residual codex exposure is the bounded 3-line head-window case, not the unbounded #8292 class. The follow-up is smaller than this PR and is due within two weeks.

chenmingwei23

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

bolichen97 · 2026-09-05T05:18:46Z

Round 6 — already argued in round 4; declined for the same reasons

design-review.yml:618, "failed lookup preserves stale same-head BLOCK": an existing same-head BLOCK, a clearing re-run whose comment lookup fails, CREATE succeeds, both comments remain, and evaluate_reviewer_markers keeps the PR blocked. This is round 4's declined half restated, and its requested fix — "publish only after successfully locating and replacing or retiring every same-head marker comment" — is "never publish when the lookup failed", now with a stronger precondition.

The round-4 answer stands unchanged, and the third point is the one that matters most here:

The outcome is fail-CLOSED: the PR stays blocked while a clear verdict exists. It cannot merge with a live BLOCK.
Making publication conditional on a successful lookup does not unblock that PR — the stale [BLOCK-MERGE] comment is still there and the reader still blocks. The only thing that changes is that the clearing verdict is now published nowhere at all. Both variants leave the PR blocked; one of them also loses the new verdict. fix(ci): stop fork GPT lane hiding a verdict behind an incomplete run (#8292) #8350 weighed exactly this and chose to publish.
The comment lookup already retries three times with backoff as of 3fe6c06 (round 4's accepted half), so reaching this branch means the API failed three times in a row — a state in which the proposed PATCH/"retire" writes would very likely fail too, arriving at the same blocked PR with the verdict additionally lost.
The path is unchanged from main and identical on codex-review.yml.

Rounds 1–3 were accepted and fixed; round 4's retry half was accepted and shipped. Rounds 4-declined, 5 and 6 all reduce to the same two requests — stop publishing when a read fails, or replace the one-slot upsert model — and both reverse merged, human-reviewed decisions (#8342, #8350) or the documented fail-closed asymmetry in pr_status.py. I am not making either call from inside this port.

For a maintainer who agrees the port should land on its merits: /ai-review override gpt 3fe6c067573c64b72f031d61d669c4e514c063d0: <reason>. Every other check on this head is green.

A lane's summary comment is ONE slot shared by every run on the PR. The REST comments API has no If-Match, so a write to it is last-writer-wins, and it exposes no edit history, so the loss is undetectable afterwards: a failed run's "review incomplete" body replaced a posted verdict and a [BLOCK-MERGE] finding vanished from every surface a reader or tool checks (#8292). Merged Let exactly one kind of run claim the slot on the eight lanes that still had no guard — a COMPLETED verdict for the PR's CURRENT head — as one byte-identical guarded_comment_upsert() bash function per lane, pinned to a single canonical body by test so it cannot drift lane by lane. The two other kinds each lose a live verdict, so each leaves an existing comment untouched: - No "<stamp> <head>" proof marker: a review failure. Preserving the verdict and prepending a staleness notice is NOT a safe alternative, since it reads the body and writes a merge of it back, so a verdict published between the read and the write is restored away. fork-gpt-review.yml already refused to touch an existing comment for this reason, so its posture is what the other seven lanes adopt rather than the one that got overwritten; it is not left untouched itself, since it gains the two arms below. - A completed verdict for a SUPERSEDED head: the same loss arriving late. An older run can finish after a newer one published, because the fork lanes' concurrency group is keyed per head so they are not cancelled, and a cancelled same-repo run still executes its if:always() posting step. So the step confirms this run is still the PR's head before claiming the slot. - A head that cannot be READ. Writing is the destructive half of the guard, so it does not proceed on an unknown: a head unreadable after three attempts is not confirmed current and the slot is left alone. The read retries first, with the same bounded backoff this lane's other gating reads use, because one blip is not evidence about the PR and an API broken enough to fail all three would fail the write too. Whether a comment exists is the other gating input, so that read retries the same way. A lookup still erroring afterwards counts as "a comment may exist", not as "none does", so a withheld run posts nothing rather than plant a second marker comment over a possibly-live verdict; when the lookup succeeded and found nothing there is no verdict to lose, so the body is posted as a new comment. Nothing is lost by standing down: the comment names the head it reviewed, each head's own check-run is finalized fail-CLOSED, and pr_status.py matches reviewer stamps against the current head, so a comment left in place for an older head reads as stale and never as an approval of this one. Human overrides and skip notices are current-head determinations rather than review failures, so their upsert sites keep their own unconditional PATCH and never route through this function, and a completed verdict for a CONFIRMED current head whose comment lookup failed still CREATEs rather than stay silent (#8350). A checkout-loaded shared script was rejected because a fork lane executes scripts from base_sha, which would leave every open fork PR unguarded until rebase, and the posting steps run if:always() so they must work when checkout failed. Lookup captures the paginated output first and selects off the captured value, never `| head -n1` inside the pipeline (SIGPIPE under pipefail, #8350). Comment body files move from fixed /tmp paths to RUNNER_TEMP, matching the codex template. Two of the ten lanes that upsert a verdict comment stay outside the function, and the docs now declare both rather than only the first: - codex-review.yml keeps #8342's own inline preserve-and-prepend shape, unchanged and still pinned byte-for-byte by its own tests. It is the one remaining site with a read-modify-write window on the slot. - claude-review.yml keeps a plain lookup-then-PATCH. Its incomplete path posts nothing at all, so the #8292 class cannot reach it; what remains is only the superseded-completed window, because its per-PR concurrency group cancels an older run but the posting step is if:always(), which a cancelled run still executes. docs/ci/ci-and-reviews.md updated in the same commit. Closes #8344 Co-authored-by: Kiro Crew <kirocrew@users.noreply.github.com>

bolichen97 · 2026-09-05T20:15:41Z

Round 7 on `894f5fd` — both findings answered, and what a maintainer would need to verify before overriding

Rebased twice (now onto 9b46746e3); the merge conflict is cleared and mergeable is true. Every check is green except GPT 5.6 Review, which raised two fenced (security-class) findings. Neither is fixable inside this port, and the reasons are different for each.

F1 design-review.yml:574 — head-confirm races the PATCH. This is round 5 restated. The lane's own annotate-only adjudication pass (added upstream in #8693) reached the same conclusion this PR argued in round 5, independently, and machine-FLAGGED it:

The newer verdict can only be overwritten if a full review run publishes within the sub-second gap between the head read (574) and the PATCH (609), which a minutes-long run cannot do, so the reaching timing is self-contradicting.

Independently checkable: the confirmation and the write are two adjacent gh statements separated by one if test, so the window is the theoretical minimum for a read-then-write; closing it needs compare-and-swap, and the REST comments API has no If-Match or ETag precondition — which is the premise the whole PR is built on. The finding's own suggested remedies ("use head-specific comments or serialize validation and update within one cross-run lock") are the two designs #8342 and #8350 already chose against, and per-PR serialization is unavailable to the fork lanes because their concurrency groups key on head repository + head branch precisely so two PRs sharing a commit do not collide (test_lane_does_not_rerun_on_a_description_edit pins that keying and says why).

F2 fork-gpt-review.yml:1247 — a failed lookup leaves a stale same-head BLOCK next to a fresh clear verdict. The adjudicator UPHELD this one, and it is right that the scenario is reachable. Its suggested remedy is new and better than round 4/6's "never CREATE" — "reconcile every matching marker comment after CREATE, neutralizing superseded duplicates before returning" — but it is still not available, for a reason specific to the branch it lands on:

Reaching that branch means the paginated comment lookup failed three consecutive times (lookup_ok=0 after for attempt in 1 2 3). There is no comment id to reconcile: the reconcile pass needs the very lookup that just failed.
Doing it anyway — retry the lookup after the CREATE, then rewrite the older comment's [BLOCK-MERGE] to a defused marker, mirroring the existing [BLOCK-MERGE-DOWNGRADED] sed — is a read-modify-write on a slot we have just established we cannot read reliably. A verdict published between the failed lookup and the reconcile would be defused, converting an over-block into an under-block.
That is the one direction pr_status.py documents as non-negotiable, at the exact line F2's scenario runs through: "Fail-closed asymmetry: a [BLOCK-MERGE] for the current head gates from ANY trusted comment, bound or not — injection can deny a review, never forge one." Over-blocking is that reader's designed behaviour and clears on the next run; under-blocking is the class the fence exists to stop.
The path is also unchanged from main and identical on codex-review.yml, so accepting the remedy here would leave the same property live on the other two sites.

The lane is not converging on this diff, which is worth weighing alongside the findings. On the identical head 3fe6c067, two runs produced two different blocking findings — design-review.yml:609, then design-review.yml:618 — with no change to the diff between them. On 894f5fd it raised both of those asks at once. Rounds 1–3 and round 4's retry half each produced a real fix and are shipped; rounds 4-declined, 5, 6 and 7 all reduce to the same two asks, and both reverse a merged human-reviewed decision (#8342, #8350) or the documented fail-closed asymmetry above.

Also fixed on this push, from First Principles' Watch item: the doc's census said "nine upsert sites" when ten lanes upsert a verdict comment — claude-review.yml was the undeclared tenth. docs/ci/ci-and-reviews.md now counts ten and declares both exclusions by name with each one's residual, and the subject drops the "every review lane" overclaim. That lane's residual is genuinely narrower: its incomplete path posts nothing at all, so the #8292 class cannot reach it, and only the superseded-completed window remains.

Unrelated, but a maintainer should know: at 9b46746e3, test/test_ai_review_workflows.py is not black-clean and is not in .github/black-baseline.txt (8c7ad0441 added a signature black collapses back to one line). Verify with git show 9b46746e3:test/test_ai_review_workflows.py > /tmp/f.py && black --target-version py310 --check /tmp/f.py. The PR-side black gate is diff-scoped, so that drift lands on any PR touching the file; this rebase formats it. main-ratchet-audit.yml runs whole-tree and should be red on main until something does.

I am not posting an override — that is a repository writer's security action, and the repo says in writing that the writer must independently verify a machine-authored rationale before recording one. The evidence above is what to verify.

bolichen97 · 2026-09-05T20:25:39Z

Correction, from the lane's own third run on `894f5fd`: the comment slot is not the merge gate

The GPT 5.6 Review lane ran a third time on this unchanged head. It produced one finding — the same fork-gpt-review.yml:1247 one its previous run's adjudicator had UPHELD — and this time the adjudicator FLAGGED it, on a ground neither the finding nor my earlier answer had:

Readiness is check-run-driven (pr-readiness.yml:556; finalize sets it from on-disk output at fork-gpt-review.yml:1268-1284), so a stale/duplicate lane comment cannot block readiness; the display self-corrects next run.

That is verifiable and it is correct, so I am withdrawing a concession I made in rounds 4, 6 and 7. I had accepted the finding's premise that the stale comment "keeps the PR blocked". It does not:

pr-readiness.yml resolves each reviewer lane from the lane's check-run or workflow run: codex-review.yml|GPT 5.6 Review for a same-repo PR, checkrun:GPT 5.6 Review for a fork one (.github/workflows/pr-readiness.yml, the workflow_specs block around line 556). Neither spec reads a comment.
The fork lane's fail-CLOSED Finalize check-run step sets that check-run from the on-disk codex-review-output.md, independent of the comment slot.
The comment reader the finding names, evaluate_reviewer_markers, is in src/kiro_crew/builtin_skills/kirocrew-dev/prepare-pr/scripts/pr_status.py — the prepare-pr skill's status readout. grep -rn evaluate_reviewer_markers src/ .github/ docs/ scripts/ returns only that file: it is an agent-side helper, not a gate.

So the residual F2/F1 describes is: prepare-pr's readout shows a stale block for one run, in the fail-closed direction, self-correcting as soon as any later run patches the slot — and only after the paginated comment lookup has failed three consecutive backed-off times. It cannot hold up a merge, and it cannot let one through.

Three runs on one unchanged head, for the record: two findings (one FLAGGED, one UPHELD) → one finding (FLAGGED) → and on the previous head, two runs with two different single findings. Neither the reviewer nor its adjudicator is deterministic on this diff, so a fourth run is a coin flip rather than convergence. The PR body now carries this correction in the round-4 and round-7 sections.

Still not posting an override — a repository writer must verify a machine-authored rationale independently, and the three checks above are what to verify. Every other check on 894f5fd is green.

bolichen97 · 2026-09-05T20:36:00Z

Fourth run on `894f5fd`, and the one falsifiable half of its finding

The lane ran a fourth time on this unchanged head and swung back to the design-review.yml:574 race (round 5's ask). Its adjudicator FLAGGED it again, and independently reached the same two conclusions this PR argued:

For run B's current-head verdict to be lost, B must complete a full Bedrock LLM review and PATCH inside that millisecond window while run A simultaneously read the older head as still current at line 574 — a mutually-contradicting timing requirement… Recovery: the merge gate is the per-run status check (design-review.yml:638-669, reading steps.post.outputs.verdict of B's own run), independent of the comment slot.

The finding's second half is checkable and is not a defect: "fork UX skip also PATCHes unguarded at line 725." The line exists (the adjudicator looked at design-review.yml's 669 lines, not fork-ux-review.yml's 1011), and it is the skip-notice upsert:

724:  } > "${RUNNER_TEMP:-/tmp}/ux-comment.md"
725:  gh api --method PATCH "repos/$REPO/issues/comments/$existing" \
727:  echo "Updated existing ux-review comment #$existing to skip notice"

A skip notice is a current-head determination ("this revision touches no user-facing surface"), not a review failure, so it keeps its unconditional PATCH by declared design — guarding it would make it unable to replace a stale verdict for the very head it just ruled out of scope, which is the opposite of the intent. docs/ci/ci-and-reviews.md states it, the PR body states it, and test_skip_notice_still_replaces_the_comment_wholesale pins it. The guarded verdict site in that same file is line 906, inside guarded_comment_upsert.

Four runs, one unchanged head

Run	Findings	Adjudication
19:51	`design:574` + `fork-gpt:1247`	FLAG + UPHOLD
20:20	`fork-gpt:1247`	FLAG
20:30	`design:574` (+ an unverifiable second half)	FLAG

Plus two runs on the previous head that produced two different single findings. The reviewer is not deterministic on this diff and neither is its adjudicator — the same finding was UPHELD at 19:51 and FLAGGED at 20:20 — so a fifth run is a coin flip, not convergence. Both surviving asks are the two this PR has answered in every round: stop publishing when a read fails, or replace the one-slot upsert model. Each reverses a merged, human-reviewed decision (#8342, #8350) or the fail-closed asymmetry in pr_status.py, and the consequence both findings state — a blocked merge — runs through a surface that is not the merge gate.

PR Readiness reports exactly 1 blocking item, this lane. Everything else on 894f5fd is green, the merge conflict is cleared, and mergeable is true. Over to a maintainer for the override call; the evidence to verify is in this comment and the two above it.

bolichen97 · 2026-09-05T21:20:43Z

/ai-review override gpt 894f5fd: residual on a path this PR strictly narrows -- main's design-review.yml:518-528 PATCHes the verdict comment with no head confirmation at all. The harm is comment text only: pr-readiness.yml resolves lanes from check-runs and never reads comments, and the sole comment reader, prepare-pr/scripts/pr_status.py:988, fails closed.

github-actions · 2026-09-05T21:20:54Z

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 894f5fd65a9bb3ac980c6f529a439196dada9362.

residual on a path this PR strictly narrows -- main's design-review.yml:518-528 PATCHes the verdict comment with no head confirmation at all. The harm is comment text only: pr-readiness.yml resolves lanes from check-runs and never reads comments, and the sole comment reader, prepare-pr/scripts/pr_status.py:988, fails closed.

This decision applies only to this commit. A new push requires a new judgment.

bolichen97 requested a review from a team as a code owner September 4, 2026 12:37

bolichen97 requested a review from CrysisDeu September 4, 2026 12:37

github-actions Bot added the readiness: checking Automated validation is still running label Sep 4, 2026

bolichen97 mentioned this pull request Sep 4, 2026

fork review lanes: incomplete review body still overwrites a posted verdict (fork-lane twins of #8292) #8344

Closed

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026

bolichen97 mentioned this pull request Sep 4, 2026

ci: converge codex-review.yml onto the shared guarded_comment_upsert shape #8462

Open

bolichen97 enabled auto-merge (squash) September 4, 2026 17:04

iamwhatever previously approved these changes Sep 4, 2026

View reviewed changes

chenmingwei23 previously approved these changes Sep 4, 2026

View reviewed changes

github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 4, 2026

bolichen97 dismissed stale reviews from chenmingwei23 and iamwhatever via a7d2099 September 4, 2026 19:13

bolichen97 force-pushed the fix/review-lane-verdict-overwrite-8344 branch from 50c1fc1 to a7d2099 Compare September 4, 2026 19:13

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 4, 2026

github-actions Bot added the readiness: action required A blocking check or review needs attention label Sep 5, 2026

github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 5, 2026

bolichen97 force-pushed the fix/review-lane-verdict-overwrite-8344 branch from 3fe6c06 to 85d4c79 Compare September 5, 2026 19:32

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026

bolichen97 changed the title ~~fix(ci): guard every review lane against verdict overwrite (#8344)~~ fix(ci): guard the review lanes against verdict overwrite (#8344) Sep 5, 2026

bolichen97 force-pushed the fix/review-lane-verdict-overwrite-8344 branch from 85d4c79 to 894f5fd Compare September 5, 2026 19:45

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026

iamwhatever approved these changes Sep 6, 2026

View reviewed changes

JiaDe-Wu mentioned this pull request Sep 6, 2026

docs: make the README's channel list and capability claims match the code #7845

Merged

javenciu mentioned this pull request Sep 6, 2026

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

Open

5 tasks

Navigation Menu

Uh oh!

fix(ci): guard the review lanes against verdict overwrite (#8344) - #8450

fix(ci): guard the review lanes against verdict overwrite (#8344)#8450
bolichen97 merged 1 commit into
mainfrom
fix/review-lane-verdict-overwrite-8344

bolichen97 commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

bolichen97 commented Sep 4, 2026

Uh oh!

chenmingwei23 left a comment

Uh oh!

bolichen97 commented Sep 5, 2026

Uh oh!

bolichen97 commented Sep 5, 2026

Uh oh!

bolichen97 commented Sep 5, 2026

Uh oh!

bolichen97 commented Sep 5, 2026

Uh oh!

bolichen97 commented Sep 5, 2026

Uh oh!

github-actions Bot commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

bolichen97 commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

The guard: two ways to lose a verdict, one no-touch answer

What changed after review (seven rounds; four produced a fix, and the rest are answered)

The tenth site (claude-review.yml), and the census the doc was carrying

Rebase onto current main

Why a byte-identical inline function, not a shared script

Tests (bash-level, per lane, against each lane's real posting step)

Verification

Screenshots

Pattern harvest

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design Review (Fable 5) — ✅ PASS

Suggestions

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GPT 5.6 Review — ✅ human override accepted

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

First Principles Review (Fable 5) — 🟡 CONCERNS

What this change ships

Watch

Subtractions

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Opus 4.8 Review — ✅ no blocking findings

Uh oh!

bolichen97 commented Sep 4, 2026

Uh oh!

chenmingwei23 left a comment

Choose a reason for hiding this comment

Uh oh!

bolichen97 commented Sep 5, 2026

Round 6 — already argued in round 4; declined for the same reasons

Uh oh!

bolichen97 commented Sep 5, 2026

Round 7 on 894f5fd — both findings answered, and what a maintainer would need to verify before overriding

Uh oh!

bolichen97 commented Sep 5, 2026

Correction, from the lane's own third run on 894f5fd: the comment slot is not the merge gate

Uh oh!

bolichen97 commented Sep 5, 2026

Fourth run on 894f5fd, and the one falsifiable half of its finding

Four runs, one unchanged head

Uh oh!

bolichen97 commented Sep 5, 2026

Uh oh!

github-actions Bot commented Sep 5, 2026

Human judgment recorded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bolichen97 commented Sep 4, 2026 •

edited

Loading

The tenth site (`claude-review.yml`), and the census the doc was carrying

Rebase onto current `main`

github-actions Bot commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Round 7 on `894f5fd` — both findings answered, and what a maintainer would need to verify before overriding

Correction, from the lane's own third run on `894f5fd`: the comment slot is not the merge gate

Fourth run on `894f5fd`, and the one falsifiable half of its finding