iterative-works/iw-cli

Watch

Fork 0

phase-pr pushes a commit after reading CI, so its green can describe a stale head #491

New issue

Closed

opened 2026-09-01 14:03:27 +00:00 by mph · 1 comment

mph commented

2026-09-01 14:03:27 +00:00

Owner

iw phase-pr reads the CI verdict, then creates and pushes a commit of its own. That push moves the pull request's head, so the green it records describes a commit the reviewer will not see.

Observed

Opening PR 483 for IW-476 phase 2:

./iw phase-pr waited for CI and saw run 580 pass on the phase commit 2ec9d276.
It then wrote and pushed chore(IW-476): update review-state for phase 02 (3915100f).
That push started run 581, whose format job failed.
The command had already exited 0 and written status: awaiting_review with ci_gated_sha: 2ec9d276.

So the card claimed a green gate while fj pr status 483 showed CI / format ... Failure. The gate did not catch it; a human did.

In the code

finish (core/commands/PhasePr.scala:224-249) dispatches on the verdict. CiVerdict.Green(_, sha) calls handOver(gatedSha = Some(sha)), which calls updateReviewStateForReview and then persist. persist (:333-345) commits the review-state file and pushes the phase branch.

The CiVerdict.Running branch at :230-233 already guards against exactly this, and says so:

// The card keeps ci_pending and the wait's start: the wait is
// unfinished, not blocked on a person. It stays uncommitted: a push
// here would move the pull request's head and start the run over.

The hazard is understood on the Running path and reproduced on the Green path.

Why it matters

The gate is the signal that a phase is safe to review, and in batch mode it is the signal that a phase is safe to merge. A gate that can report green over a red head does not do the one job it exists for.

Candidate fixes

Write and push the review-state commit before entering the CI wait, so the verdict is read on the final head. This matches what the Running branch already assumes.
Or re-read CI for the new head after pushing, and only then stamp ci_gated_sha.

./iw ci-gate re-reads CI and stamps ci_gated_sha without pushing anything, which is why it was the safe way to repair PR 483's card by hand.

#485 — phase-pr treats 'no pipeline yet' as 'no CI'. That is passing the gate before a pipeline exists; this is passing the gate and then invalidating it.
#472 — ci-gate reads a commit's whole status history.
#473 — a corrupt or future ci_wait_started disables the wait budget.

Recorded as IW-476 LESSONS.md entry 15.

Defect 2: the appearance budget is measured from a head that is gone

CiGate.startOfWait (core/model/CiGate.scala:414-436) takes ci_wait_started from the card and measures the whole wait from it, across invocations. emptyVerdict (:492-508) then spends runAppearanceBudgetMs (5 minutes) against that stamp. Nothing resets the stamp when the head moves, so a push made after the stamp gets a head with no budget left: the forge has not registered a run for it yet, shaMatched is empty, and the gate answers NoRunForEvent — "CI did not run" — for a commit that is seconds old.

Observed, IW-583 (Forgejo, 2026-09-21). A batch-implement run was killed while it waited on PR #588, leaving ci_pending with ci_wait_started stamped. The session re-ran phase-pr. It pushed review-state commit e3da7326, read CI at once, reported "CI did not run" and wrote ci_failed. Forgejo run 890 for e3da7326 was already queued; a later re-run waited normally and CI passed in about 20 minutes.

Why it is the more urgent of the two. BatchImplement.decideOutcome maps ci_failed to PhaseOutcome.Fail, which halts the batch with no retry and no gate re-entry, and points the reader at job logs that do not exist for a pipeline that went green. On IW-583 only the live phase session noticed and re-ran the command; an unattended run would have stopped there. A false ci_failed is the sharpest remaining obstacle to running the loop without a person.

The two defects share a cause — a verdict read against a head the gate did not measure — and they sit in the same gate code, so one branch fixes both. Defect 1 removes the push that moves the head; defect 2 makes the budget start at the head it is measuring.

Decision on the CLARIFY (Michal, 2026-09-21): F1

A green phase-pr may leave the review-state card uncommitted. Write the card after the verdict, commit nothing, push nothing.

Evidence that settled it:

No consumer reads the card from the forge. iw-cli's dashboard reads it from worktree disk (dashboard/jvm/src/infrastructure/ReviewStateReader.scala), and every kanon plugin reader — BatchImplementation, PhaseRecovery, PhaseCatchUp, Preflight — reads disk. The persist scaladoc asserts the card must be published but names no consumer.
phase-merge and phase-advance both commit a dirty card locally before they need a clean tree, and IW-583 (PhaseAdvance.scala:228-236) has just hardened that path.
F3 (push, then re-read CI on the new head) was rejected: it doubles the CI time per phase, which is the cost 9d577408 deliberately removed.
A fourth option — commit the card locally without pushing, as --batch does — was considered and rejected. A later phase-pr run pushes the branch in createPrAndGate before the gate, so that local commit would ride out and move the head anyway. F1 cannot do that.

Accepted cost: after every green phase-pr the worktree carries an uncommitted review-state.json. An agent may ask about it, and a git stash or reset --hard loses the green record; phase-merge then does the full wait again, which is slower but safe.

Note for the analysis

An earlier analysis of defect 1 alone is committed on branch IW-491 as 56475c05. Reuse its findings rather than repeating the work. In particular it establishes, with evidence, that the red format job on run 581 was not caused by the card commit: scala-cli fmt --check crashed with SIGSEGV (exit 139) after scanning 0 of 512 files, recorded in 45bca500 and tracked as #497. The card commit only made the stale-head defect visible.

`iw phase-pr` reads the CI verdict, then creates and pushes a commit of its own. That push moves the pull request's head, so the green it records describes a commit the reviewer will not see. ## Observed Opening PR 483 for IW-476 phase 2: 1. `./iw phase-pr` waited for CI and saw run 580 pass on the phase commit `2ec9d276`. 2. It then wrote and pushed `chore(IW-476): update review-state for phase 02` (`3915100f`). 3. That push started run 581, whose `format` job failed. 4. The command had already exited `0` and written `status: awaiting_review` with `ci_gated_sha: 2ec9d276`. So the card claimed a green gate while `fj pr status 483` showed `CI / format ... Failure`. The gate did not catch it; a human did. ## In the code `finish` (`core/commands/PhasePr.scala:224-249`) dispatches on the verdict. `CiVerdict.Green(_, sha)` calls `handOver(gatedSha = Some(sha))`, which calls `updateReviewStateForReview` and then `persist`. `persist` (`:333-345`) commits the review-state file and pushes the phase branch. The `CiVerdict.Running` branch at `:230-233` already guards against exactly this, and says so: // The card keeps ci_pending and the wait's start: the wait is // unfinished, not blocked on a person. It stays uncommitted: a push // here would move the pull request's head and start the run over. The hazard is understood on the `Running` path and reproduced on the `Green` path. ## Why it matters The gate is the signal that a phase is safe to review, and in batch mode it is the signal that a phase is safe to merge. A gate that can report green over a red head does not do the one job it exists for. ## Candidate fixes - Write and push the review-state commit *before* entering the CI wait, so the verdict is read on the final head. This matches what the `Running` branch already assumes. - Or re-read CI for the new head after pushing, and only then stamp `ci_gated_sha`. `./iw ci-gate` re-reads CI and stamps `ci_gated_sha` without pushing anything, which is why it was the safe way to repair PR 483's card by hand. ## Related, but distinct - #485 — phase-pr treats 'no pipeline yet' as 'no CI'. That is passing the gate before a pipeline exists; this is passing the gate and then invalidating it. - #472 — ci-gate reads a commit's whole status history. - #473 — a corrupt or future `ci_wait_started` disables the wait budget. Recorded as IW-476 LESSONS.md entry 15. --- ## Defect 2: the appearance budget is measured from a head that is gone `CiGate.startOfWait` (`core/model/CiGate.scala:414-436`) takes `ci_wait_started` from the card and measures the whole wait from it, across invocations. `emptyVerdict` (`:492-508`) then spends `runAppearanceBudgetMs` (5 minutes) against that stamp. Nothing resets the stamp when the head moves, so a push made after the stamp gets a head with no budget left: the forge has not registered a run for it yet, `shaMatched` is empty, and the gate answers `NoRunForEvent` — "CI did not run" — for a commit that is seconds old. **Observed, IW-583 (Forgejo, 2026-09-21).** A `batch-implement` run was killed while it waited on PR #588, leaving `ci_pending` with `ci_wait_started` stamped. The session re-ran `phase-pr`. It pushed review-state commit `e3da7326`, read CI at once, reported "CI did not run" and wrote `ci_failed`. Forgejo run 890 for `e3da7326` was already queued; a later re-run waited normally and CI passed in about 20 minutes. **Why it is the more urgent of the two.** `BatchImplement.decideOutcome` maps `ci_failed` to `PhaseOutcome.Fail`, which halts the batch with no retry and no gate re-entry, and points the reader at job logs that do not exist for a pipeline that went green. On IW-583 only the live phase session noticed and re-ran the command; an unattended run would have stopped there. A false `ci_failed` is the sharpest remaining obstacle to running the loop without a person. The two defects share a cause — a verdict read against a head the gate did not measure — and they sit in the same gate code, so one branch fixes both. Defect 1 removes the push that moves the head; defect 2 makes the budget start at the head it is measuring. ## Decision on the CLARIFY (Michal, 2026-09-21): F1 A green `phase-pr` **may** leave the review-state card uncommitted. Write the card after the verdict, commit nothing, push nothing. Evidence that settled it: - No consumer reads the card from the forge. iw-cli's dashboard reads it from worktree disk (`dashboard/jvm/src/infrastructure/ReviewStateReader.scala`), and every kanon plugin reader — `BatchImplementation`, `PhaseRecovery`, `PhaseCatchUp`, `Preflight` — reads disk. The `persist` scaladoc asserts the card must be published but names no consumer. - `phase-merge` and `phase-advance` both commit a dirty card locally before they need a clean tree, and IW-583 (`PhaseAdvance.scala:228-236`) has just hardened that path. - F3 (push, then re-read CI on the new head) was rejected: it doubles the CI time per phase, which is the cost 9d577408 deliberately removed. - A fourth option — commit the card locally without pushing, as `--batch` does — was considered and rejected. A later `phase-pr` run pushes the branch in `createPrAndGate` *before* the gate, so that local commit would ride out and move the head anyway. F1 cannot do that. Accepted cost: after every green `phase-pr` the worktree carries an uncommitted `review-state.json`. An agent may ask about it, and a `git stash` or `reset --hard` loses the green record; `phase-merge` then does the full wait again, which is slower but safe. ## Note for the analysis An earlier analysis of defect 1 alone is committed on branch `IW-491` as `56475c05`. Reuse its findings rather than repeating the work. In particular it establishes, with evidence, that the red `format` job on run 581 was **not** caused by the card commit: `scala-cli fmt --check` crashed with SIGSEGV (exit 139) after scanning 0 of 512 files, recorded in `45bca500` and tracked as #497. The card commit only made the stale-head defect visible.

mph referenced this issue from a commit

2026-09-01 15:35:00 +00:00

workflow: Triage lessons for IW-476

mph referenced this issue

2026-09-01 15:35:39 +00:00

IW-476: The herdr sidebar names a worktree by its key, and a rename sticks #498

claude referenced this issue

2026-09-15 01:44:15 +00:00

phase-pr exits 0 on a green gate, then pushes a commit the gate never validated #536

claude referenced this issue

2026-09-15 02:46:23 +00:00

phase-pr cannot tell a forge that is down from a forge that refused, so both stop a batch #513

claude referenced this issue

2026-09-21 11:26:08 +00:00

phase-pr treats 'no pipeline yet' as 'no CI' and passes the gate before the pipeline is created #485

claude commented

2026-09-21 11:26:27 +00:00

Member

Still live on main (1ab91787), and this issue now carries a second defect that shares its cause. Both are in scope for the fix.

Defect 1 — the push after the verdict (this issue as filed)

persist (core/commands/PhasePr.scala:595-608) commits the card and pushes the phase branch, and every path out of the gate calls it: handOver -> reportOpenPullRequest -> persist on green, and stop -> persist on red. recordPullRequest (:581) writes the card before the wait but does not commit it, so the commit and the push still land after the verdict. The head the reviewer sees is not the head that was gated.

dev-docs#241 is the same defect seen from the other side: on a forge whose CI triggers on the merge-request event, that bookkeeping push starts a second full pipeline that gates nothing and that nobody waits for. It lists the same first fix — push the card before gating — so one fix closes both.

Defect 2 — the appearance budget is measured from a head that is gone

CiGate.startOfWait (core/model/CiGate.scala:414-436) takes ci_wait_started from the card and measures the whole wait from it, across invocations. emptyVerdict (:492-508) then spends runAppearanceBudgetMs (5 min) against that stamp. Nothing resets the stamp when the head moves, so a push made after the stamp gets a head with no budget left: the forge has not registered a run for it yet, shaMatched is empty, and the gate answers NoRunForEvent — "CI did not run" — for a commit that is seconds old.

Why it matters more than it looks. BatchImplement.decideOutcome maps ci_failed to PhaseOutcome.Fail, which halts the batch with no retry and no gate re-entry, and points the reader at job logs that do not exist for a pipeline that went green. On IW-583 only the live phase session noticed and re-ran the command. An unattended run would have stopped there. So a false ci_failed is the sharpest remaining obstacle to running the loop without a person.

What the fix has to settle

The card records the gate's outcome, so it is written after the gate; pushing it then is what moves the head. Pushing it before the gate fixes the gated-sha and the wasted pipeline, but the outcome write still has to reach the branch somehow. That choice — push the outcome with a [skip ci] subject, hold it until the merge, or something else — is the design question this issue has to answer, not a detail of it.

Whatever the answer, the appearance budget must start at the head it is measuring, not at a stamp an earlier head left behind.

#485 is closed: the premature-green half of this family was implemented in 0881a351 / 10fc4b09.

Still live on `main` (1ab91787), and this issue now carries a second defect that shares its cause. Both are in scope for the fix. ## Defect 1 — the push after the verdict (this issue as filed) `persist` (`core/commands/PhasePr.scala:595-608`) commits the card and pushes the phase branch, and every path out of the gate calls it: `handOver` -> `reportOpenPullRequest` -> `persist` on green, and `stop` -> `persist` on red. `recordPullRequest` (`:581`) writes the card before the wait but does not commit it, so the commit and the push still land after the verdict. The head the reviewer sees is not the head that was gated. dev-docs#241 is the same defect seen from the other side: on a forge whose CI triggers on the merge-request event, that bookkeeping push starts a second full pipeline that gates nothing and that nobody waits for. It lists the same first fix — push the card before gating — so one fix closes both. ## Defect 2 — the appearance budget is measured from a head that is gone `CiGate.startOfWait` (`core/model/CiGate.scala:414-436`) takes `ci_wait_started` from the card and measures the whole wait from it, across invocations. `emptyVerdict` (`:492-508`) then spends `runAppearanceBudgetMs` (5 min) against that stamp. Nothing resets the stamp when the head moves, so a push made after the stamp gets a head with no budget left: the forge has not registered a run for it yet, `shaMatched` is empty, and the gate answers `NoRunForEvent` — "CI did not run" — for a commit that is seconds old. **Observed, IW-583 (Forgejo, 2026-09-21).** A `batch-implement` run was killed while it waited on PR #588, leaving `ci_pending` with `ci_wait_started` stamped. The session re-ran `phase-pr`. It pushed review-state commit `e3da7326`, read CI at once, reported "CI did not run" and wrote `ci_failed`. Forgejo run 890 for `e3da7326` was already queued; a later re-run waited normally and CI passed in about 20 minutes. **Why it matters more than it looks.** `BatchImplement.decideOutcome` maps `ci_failed` to `PhaseOutcome.Fail`, which halts the batch with no retry and no gate re-entry, and points the reader at job logs that do not exist for a pipeline that went green. On IW-583 only the live phase session noticed and re-ran the command. An unattended run would have stopped there. So a false `ci_failed` is the sharpest remaining obstacle to running the loop without a person. ## What the fix has to settle The card records the gate's outcome, so it is written after the gate; pushing it then is what moves the head. Pushing it before the gate fixes the gated-sha and the wasted pipeline, but the outcome write still has to reach the branch somehow. That choice — push the outcome with a `[skip ci]` subject, hold it until the merge, or something else — is the design question this issue has to answer, not a detail of it. Whatever the answer, the appearance budget must start at the head it is measuring, not at a stamp an earlier head left behind. #485 is closed: the premature-green half of this family was implemented in 0881a351 / 10fc4b09.

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

2026-09-21 14:52:17 +00:00

IW-491: phase-pr pushes a commit after reading CI, so its green can describe a stale head #596

mph closed this issue

2026-09-21 16:32:13 +00:00