fix(ci): never let an incomplete review body overwrite a posted verdict (#8292)

[bolichen97]

Problem / Motivation

The Post/update review comment step in .github/workflows/codex-review.yml publishes the GPT review verdict by PATCHing a single comment in place, keyed by the hidden <!-- codex-ai-review --> marker. The step defaults kind="incomplete" and unconditionally PATCHes, so a run that fails to produce codex-review-output.md replaces a posted [BLOCK-MERGE] verdict with a short "review incomplete" body. The REST comments API exposes no edit history, so the buried verdict survives only in GraphQL userContentEdits — invisible everywhere a normal reader or tool looks. Verified by the reporter on PR #8184 comment 5528347174: 3 of its 10 revisions were incomplete bodies, each replacing a marker-present blocking verdict, including a security finding.

Why it matters

A blocking security finding silently vanishing from the one comment reviewers read defeats the purpose of the review lane. Merge safety was never at risk (the Gate on findings step fails closed on the output file, not the comment), but a human scanning the PR sees "review incomplete" where a red verdict used to be, and nothing says a verdict ever existed.

What changed (motivation → approach → change)

Symptom: incomplete bodies overwrite completed verdicts. Root cause: the upsert PATCHes unconditionally, never looking at what it is replacing. Change, scoped to the Post/update review comment step only:

• The finder query now captures the existing comment's body in the same query that finds its id ({id, body} | @json — one compact line per match, so head -n1 still selects a whole record; author guard unchanged). Captured before any PATCH decision, so there is no find/fetch race. The fetched body is CRLF-normalized (tr -d '\r') before matching, because GitHub stores comment bodies with CRLF line endings and the guard's grep and line-anchored cleanup must see the bytes the step originally wrote.
• One guarded transition: if the new body is the incomplete kind AND the existing body contains [GPT-REVIEWED] (tested with grep -Fq against a file, never in a shell string), the step does NOT replace the verdict. It re-posts the existing verdict with one dated stale-verdict notice prepended (with the /ai-review override escape hatch), replacing any notice a previous incomplete run left so notices never stack. Invariant: a verdict that exists never becomes invisible.
• The notice cleanup is line-anchored and bounded to the head window (lines 1–8, where the step's own notice always sits by construction): the preserved body embeds model-authored review prose, so an unanchored range keyed on the notice markers would let a finding that merely quotes them delete the verdict to EOF — found by both pre-push reviewers, locked in by a dedicated test.
• override / blocked / clear kinds still replace the comment exactly as before. The Gate on findings step is byte-identical: fail-closed on incomplete stays, and a preserved stale verdict is SHA-scoped so no consumer reads it as fresh (sha_matches() rejects the old SHA).
• Comment scratch files moved from hardcoded /tmp to "${RUNNER_TEMP:-/tmp}" (always set on GitHub runners; behavior-identical there) so the step's bash body is hermetically testable.
• docs/ci/ci-and-reviews.md documents the new upsert semantics, scoped to the same-repo GPT lane.

Known remaining gaps (out of scope per the issue): two fork lanes carry the same defect class — fork-gpt-review.yml's Post/update summary comment and fork-opus-review.yml's comment upsert both PATCH an incomplete body over a posted verdict unconditionally. Both are tracked in follow-up #8344. (claude-review.yml is already safe: it posts nothing on incomplete. That skip shape was considered and not chosen here because the issue names the dated notice as the preferred form — see the First Principles disposition on this PR.)

Tests

New TestGptVerdictVisibility class in test/test_ai_review_workflows.py runs the real step bash (extracted via the file's existing _step_script helper) with a stubbed gh whose finder branch executes the step's real --jq filter through real jq over an array fixture — so filter drift (dropped @json, lost author guard) fails the tests instead of hiding:

• (a) incomplete-over-verdict → verdict + markers preserved, exactly one dated notice, PATCH targets the bot's comment (not a marker-planting impostor's)
• (b) completed verdict over anything → replaced wholesale, as before
• (c) incomplete with no existing comment → created, as before
• (d) verdict prose that quotes the notice markers (inline and as a bare fenced line) → never truncated, [BLOCK-MERGE] survives
• static scope assertion: same-query capture present, Gate on findings untouched

Mutation-verified: disabling the guard condition, dropping the notice-dedup, reverting to the unanchored sed, and dropping the author guard each make the suite fail. Full backend suite: 84,514 passed; fail-set byte-identical to a clean-main control on the same host (all host-env classes).

Manual verification

Three-case transcript from running the step's bash body with stubbed gh (sha 1234…5678 = new head whose run failed; aaaa…aaaa = old completed verdict):

CASE (a): incomplete run over existing BLOCKING verdict → verdict preserved + dated notice

gh-stub: PATCH repos/example/repo/issues/comments/123
Preserved GPT 5.6 verdict in comment #123; prepended incomplete-run notice for 1234567890abcdef1234567890abcdef12345678

<!-- codex-ai-review -->
<!-- codex-stale-notice-begin -->
> ⚠️ **Stale verdict notice (2026-09-04 00:50 UTC):** a later GPT 5.6 run did not produce a completed verdict for `1234567890abcdef1234567890abcdef12345678`; the verdict below is from an earlier completed run. Inspect the GPT 5.6 Review job logs and re-run the workflow, or have a repository writer comment `/ai-review override gpt 1234567890abcdef1234567890abcdef12345678: <one-sentence reason>`.
<!-- codex-stale-notice-end -->

## GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging `aaaa567890abcdef1234567890abcdef1234aaaa`.

SECURITY finding: example.
[GPT-REVIEWED] aaaa567890abcdef1234567890abcdef1234aaaa
[BLOCK-MERGE] aaaa567890abcdef1234567890abcdef1234aaaa```

### CASE (b): completed BLOCKING verdict over existing comment → replaced wholesale (as before)
gh-stub: PATCH repos/example/repo/issues/comments/123
Updated existing GPT 5.6 comment #123
```markdown
<!-- codex-ai-review -->
## GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging `1234567890abcdef1234567890abcdef12345678`.

_This comment is updated in place on each push._

FINDINGS...
...

CASE (c): incomplete run, no existing comment → created (as before)

gh-stub: CREATE comment
Created new GPT 5.6 comment

<!-- codex-ai-review -->
## GPT 5.6 Review — ⚠️ review incomplete

GPT 5.6 did not produce a complete verdict for `1234567890abcdef1234567890abcdef12345678`; inspect the workflow logs and re-run it.
...

Related Issues

Closes #8292

Pattern harvest

Rule candidate: review-prompt — Pattern: "in-place upsert of a status artifact must compare old and new state class before replacing; a lower-information state never overwrites a higher-information one". Second and third instances of the class: fork-gpt-review.yml and fork-opus-review.yml carry the same unconditional PATCH (follow-up #8344); claude-review.yml already avoids it by posting nothing on incomplete.

Checklist

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

Why no screenshot: CI workflow + backend test + doc change only; no UI surface is touched.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root-cause fix at the exact upsert seam that lost the verdict, with the one risky part (sed surgery on model prose) bounded and mutation-tested.

[DESIGN-REVIEWED] 7d769e5

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 7d769e5b38f6f2d37ed77aa9c6341102de30450e and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 7d769e5

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 7d769e5b38f6f2d37ed77aa9c6341102de30450e — 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 evidence gathered. Composing the review.

First-Principles-Verdict: CONCERNS

The guard is right, but the stale-notice merge machinery is oversized — claude-review.yml already solves the identical problem by skipping the PATCH — and a third sibling is undeclared.

What this change ships

Intent: stop a failed review run's "incomplete" body from erasing a posted blocking verdict — a FIX (reported defect: PR #8184 comment revisions).

1. An incomplete run no longer erases a posted GPT verdict — justified, cause-level for this lane
2. A dated stale-verdict notice is prepended (deduped) onto the preserved verdict — oversized
3. Finder query now fetches the comment body with the id — justified (serves items 1–2)
4. Fetched body CRLF-normalized before matching — undeclared (diff comment only)
5. Scratch files moved /tmp → RUNNER_TEMP — rides along, declared; derived from the tests-touch-nothing invariant
6. Doc section on the new upsert semantics — mandated by AGENTS.md same-commit rule
7. Five-test class running the real step bash — justified

Watch

• Undeclared unfixed sibling. The description counts "Second instance of the class: fork-gpt-review.yml" — grep for kind="incomplete" finds four lanes: codex (fixed), claude-review.yml (already safe — exits at line 456 without posting), fork-gpt (declared), and fork-opus-review.yml:441-471, which PATCHes an incomplete body over a verdict unconditionally and is named nowhere. 2 unfixed siblings, 1 declared.
• The delete option was never weighed. claude-review.yml:452-459 removes the same harm with "Incomplete review: post NOTHING" — no body merge, no sed, no over-deletion risk. The description presents the notice-merge as the only shape.

Subtractions

• Replace the merge branch (codex-merged-comment.md construction, the two-expression sed at codex-review.yml:736-738, the notice-dedup) with the claude-review.yml shape: keep the kind=incomplete && grep -Fq "[GPT-REVIEWED]" guard, then log and skip the PATCH. The red Gate on findings check already tells the reader the current head has no verdict; deleting the sed also deletes the quoted-marker truncation risk that test_a_verdict_that_quotes_the_notice_markers_is_not_truncated exists to pin.

[FIRST-PRINCIPLES-REVIEWED] 7d769e5

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 7d769e5b38f6f2d37ed77aa9c6341102de30450e — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 7d769e5

Verdict parsed from the review's SHA-scoped output markers for commit 7d769e5b38f6f2d37ed77aa9c6341102de30450e.

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

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97#bb3ad1ca]: Per-finding response to the First Principles CONCERNS verdict on 7d769e5b3.

1. Undeclared unfixed sibling (fork-opus-review.yml:441-471) — ADOPTED (declaration). Verified: that lane defaults kind="incomplete" and PATCHes the existing <!-- claude-ai-review -->-keyed comment unconditionally, same defect shape. Follow-up #8344 has been retitled to cover both fork lanes and carries a scope addendum naming the fork-opus site; the PR body's known-gap sentence and Pattern harvest now count both siblings and note that claude-review.yml is already safe. No code change in this PR — the fork lanes run in a different trigger context (workflow_run, set -uo pipefail, PR resolved from run outputs) and porting them here would widen a CI-critical diff that three lanes have already reviewed.

2. "The delete option was never weighed" / Subtraction: replace the notice merge with claude-review.yml's skip-the-PATCH — DECLINED, with the trade-off now stated in the PR body. The skip shape does preserve the verdict, but it hides the other fact a reader needs on exactly this failure: that a newer run for the current head produced no verdict. The stale comment would keep saying "updated in place on each push" under an old sha with nothing marking it stale — readable as current. Issue #8292 names the dated notice (or a separate comment) as the preferred form for precisely this reason, and this PR implements the issue: the notice dates the failure, names the failed head sha, points at the job logs, and carries the /ai-review override escape hatch for the sha a maintainer would actually override. The over-deletion risk cited as the cost of the merge machinery is pinned to zero by construction (line-anchored patterns bounded to the head window where the step's own notice sits) and by test_a_verdict_that_quotes_the_notice_markers_is_not_truncated, which fails on the unanchored form. Cost kept: ~15 lines of bounded sed; benefit kept: the comment tells the whole truth in the one place people read.

3. CRLF normalization undeclared — ADOPTED. The PR body's change list now declares it: GitHub stores comment bodies with CRLF, and the guard's grep and line-anchored cleanup must see the bytes the step wrote.

No push was made for this response; the verdict head is unchanged.