The phase gate reads and writes cards that do not belong to the head under test #601

New issue

Closed

opened 2026-09-21 19:32:19 +00:00 by claude · 1 comment

claude commented

2026-09-21 19:32:19 +00:00

Member

The phase gate and the card do not agree on which head they speak about. IW-491
fixed the one case that stopped a batch run: phase-pr pushed a card-only commit
after it read a green CI result, so the green described a stale head. The same
mistake is in four other places. Each one is small. Together they are one defect
with four faces, so they belong in one investigation and one branch.

This issue replaces #592, #593, #594 and #595, which are closed as duplicates of
it. Related: #568, #569.

Phase 1 — one flag clears one wait (was #595)

ReviewStateUpdater has clearCiWaitStarted and clearCiWaitSha. The two fields
describe one wait and must be cleared together, but eight call sites (in
PhasePr, PhaseMerge, PhaseMergedCard, CiGateCommand) set the pair by hand.
A call site that sets one and not the other leaves a wait start with no head, or
a head with no wait start, and nothing catches it.

Fix: replace the pair with one flag, clearCiWait, that clears both fields.

This phase is first because the three phases after it read and clear the same
wait, and each one would otherwise add a ninth and a tenth hand-set pair.

Phase 2 — a red verdict must not move the head (was #592)

After a red verdict, PhasePr.stop (core/commands/PhasePr.scala:535-558) calls
persist, which commits and pushes the card. The push makes a new head, CI runs
again on it, and when the red was false the new head goes green while the card
still says ci_failed. Seen on IW-583, run 890.

Fix: do on the red and stop path what IW-491 did on the green path — write the
card to disk, commit nothing and push nothing.

Check before the fix: batch-implement and phase-merge must still read a dirty
ci_failed card correctly. IW-491 recorded the cost this makes on the green path:
a bare phase-commit after phase-pr stops, because it refuses unstaged changes.
The red path must not make that cost worse.

Phase 3 — judge a reading only when it is about the pushed head (was #594)

After a recovery push, phase-merge can read the forge before the forge has moved
the pull request head. The reading then returns the old head with its red verdict,
phase-merge counts it as a failure of the new head, and it spends one of its
--max-retries attempts on a result that was never about the new code.

Fix: compare the pull request head with the local pushed sha before judging a
reading. When they differ, the reading is not yet about the pushed commit: wait,
read again, and do not spend a retry.

This is the same rule IW-491 wrote into the wait card as ci_wait_sha — a verdict
belongs to one commit — applied at the other end, to the reading rather than the
wait.

Phase 4 — the state-only merge must say what it did not judge (was #593)

When only review-state.json has changed since ci_gated_sha, phase-merge takes
one CI reading and does not wait (core/commands/PhaseMerge.scala:187-196). A
Running reading is accepted, so the merge can land a head whose own run never
finished. Commit 9d577408 made this trade knowingly: the changed head differs
from the gated head only in the card.

Phase 2 removes the push that creates most of these state-only heads, so this
phase must re-measure the trade before it changes anything.

Decision (2026-09-21, Michal): keep the trade and record it. The state-only
path does not wait for a terminal verdict. It merges, and it writes on the merged
card that the merged head was never judged. Reason: phase 2 stops the red path from
pushing a card-only commit, so a state-only head becomes rare, and a wait there
would add a full CI cycle to a merge whose only change is the card.

This phase carries no open question. Implement the decision. Do not raise it as a
CLARIFY.

The phase gate and the card do not agree on which head they speak about. IW-491 fixed the one case that stopped a batch run: `phase-pr` pushed a card-only commit after it read a green CI result, so the green described a stale head. The same mistake is in four other places. Each one is small. Together they are one defect with four faces, so they belong in one investigation and one branch. This issue replaces #592, #593, #594 and #595, which are closed as duplicates of it. Related: #568, #569. ## Phase 1 — one flag clears one wait (was #595) `ReviewStateUpdater` has `clearCiWaitStarted` and `clearCiWaitSha`. The two fields describe one wait and must be cleared together, but eight call sites (in `PhasePr`, `PhaseMerge`, `PhaseMergedCard`, `CiGateCommand`) set the pair by hand. A call site that sets one and not the other leaves a wait start with no head, or a head with no wait start, and nothing catches it. Fix: replace the pair with one flag, `clearCiWait`, that clears both fields. This phase is first because the three phases after it read and clear the same wait, and each one would otherwise add a ninth and a tenth hand-set pair. ## Phase 2 — a red verdict must not move the head (was #592) After a red verdict, `PhasePr.stop` (`core/commands/PhasePr.scala:535-558`) calls `persist`, which commits and pushes the card. The push makes a new head, CI runs again on it, and when the red was false the new head goes green while the card still says `ci_failed`. Seen on IW-583, run 890. Fix: do on the red and stop path what IW-491 did on the green path — write the card to disk, commit nothing and push nothing. Check before the fix: `batch-implement` and `phase-merge` must still read a dirty `ci_failed` card correctly. IW-491 recorded the cost this makes on the green path: a bare `phase-commit` after `phase-pr` stops, because it refuses unstaged changes. The red path must not make that cost worse. ## Phase 3 — judge a reading only when it is about the pushed head (was #594) After a recovery push, `phase-merge` can read the forge before the forge has moved the pull request head. The reading then returns the old head with its red verdict, `phase-merge` counts it as a failure of the new head, and it spends one of its `--max-retries` attempts on a result that was never about the new code. Fix: compare the pull request head with the local pushed sha before judging a reading. When they differ, the reading is not yet about the pushed commit: wait, read again, and do not spend a retry. This is the same rule IW-491 wrote into the wait card as `ci_wait_sha` — a verdict belongs to one commit — applied at the other end, to the reading rather than the wait. ## Phase 4 — the state-only merge must say what it did not judge (was #593) When only `review-state.json` has changed since `ci_gated_sha`, `phase-merge` takes one CI reading and does not wait (`core/commands/PhaseMerge.scala:187-196`). A `Running` reading is accepted, so the merge can land a head whose own run never finished. Commit `9d577408` made this trade knowingly: the changed head differs from the gated head only in the card. Phase 2 removes the push that creates most of these state-only heads, so this phase must re-measure the trade before it changes anything. **Decision (2026-09-21, Michal): keep the trade and record it.** The state-only path does not wait for a terminal verdict. It merges, and it writes on the merged card that the merged head was never judged. Reason: phase 2 stops the red path from pushing a card-only commit, so a state-only head becomes rare, and a wait there would add a full CI cycle to a merge whose only change is the card. This phase carries no open question. Implement the decision. Do not raise it as a CLARIFY.

claude referenced this issue

2026-09-21 19:32:25 +00:00

phase-pr pushes a card-only commit after a red verdict, so a false red leaves a green head under a ci_failed card #592

claude referenced this issue

2026-09-21 19:32:25 +00:00

phase-merge's state-only path merges a head whose CI is still Running #593

claude referenced this issue

2026-09-21 19:32:25 +00:00

phase-merge can read the forge before a recovery push moves the pull request head, and spends a retry on the old red #594

claude referenced this issue

2026-09-21 19:32:25 +00:00

ReviewStateUpdater clears ci_wait_started and ci_wait_sha with two flags that every caller must set together #595

claude commented

2026-09-21 19:51:26 +00:00

Author

Member

Phase 4 decision (2026-09-21, Michal): keep the trade and record it.

The state-only path in phase-merge does not wait for a terminal verdict. It merges, and it writes on the merged card that the merged head was never judged.

Reason: phase 2 stops the red path from pushing a card-only commit, so a state-only head becomes rare. A wait there would add a full CI cycle to a merge whose only change is the card.

Phase 4 therefore carries no open question. It implements this decision. Do not raise it as a CLARIFY.

**Phase 4 decision (2026-09-21, Michal): keep the trade and record it.** The state-only path in `phase-merge` does not wait for a terminal verdict. It merges, and it writes on the merged card that the merged head was never judged. Reason: phase 2 stops the red path from pushing a card-only commit, so a state-only head becomes rare. A wait there would add a full CI cycle to a merge whose only change is the card. Phase 4 therefore carries no open question. It implements this decision. Do not raise it as a CLARIFY.

claude referenced this issue from a pull request that will close it,

2026-09-22 13:35:46 +00:00

IW-601: The phase gate reads and writes cards that do not belong to the head under test #609