phase-pr leaves its final awaiting_review write uncommitted after CI goes green #680

New issue

Closed

opened 2026-10-02 08:45:19 +00:00 by claude · 2 comments

claude commented

2026-10-02 08:45:19 +00:00

Member

Same symptom as the closed #292, so this is a regression.

Found during IW-652 (per-user dashboard, service links, Claude session links). Follow-ups, verbatim:

iw phase-pr (waiting, without --no-wait) left its final awaiting_review write uncommitted after CI went green — PR #664, second CI run.

Same symptom as the closed #292, so this is a regression. Found during IW-652 (per-user dashboard, service links, Claude session links). Follow-ups, verbatim: - [ ] `iw phase-pr` (waiting, without `--no-wait`) left its final `awaiting_review` write uncommitted after CI went green — PR #664, second CI run.

mph referenced this issue from a commit

2026-10-02 08:47:47 +00:00

workflow: Triage lessons for IW-652

claude commented

2026-10-03 01:15:10 +00:00

Author

Member

Intake: needs-input

The dirty card after a green phase-pr is probably not a regression of #292. It comes from a decision: commit 89d9a61f (IW-491, PR #591) made this behaviour on purpose.

In phase-pr without --no-wait, when CI is green, the code writes the awaiting_review card and does not commit it. This is intentional: core/commands/PhasePr.scala finish/handOver passes persistCard = false, and its scaladoc says "a push here would move that head past the commit the verdict just proved". IW-491 (commit 89d9a61f) made this change, and its analysis.md:11 rejects "commit locally, do not push", because the next phase-pr pushes the local commit anyway. phase-merge (doMergeAndAdvance) and phase-advance (doAdvance, IW-583) commit the dirty card before they check out a branch, and PhasePrHarnessTest asserts the dirty card (committedMessages == Nil). Which behaviour do you want?
- (a) Keep the current behaviour, and close IW-680 as intended behaviour.
- (b) Commit the card locally without a push. IW-491 rejected this, because the next push moves the PR head and CI runs again.
- (c) Commit and push the card after green. This starts a new CI run on a head that the gate did not read. IW-491 removed this behaviour.
- (d) Something else. For example, a workflow step that you name still fails on the dirty card. If so, name that step, because phase-merge and phase-advance already accept the dirty card.

Answer in a comment on this issue; intake reads it on its next iteration.

Intake: needs-input The dirty card after a green `phase-pr` is probably not a regression of #292. It comes from a decision: commit 89d9a61f (IW-491, PR #591) made this behaviour on purpose. 1. In `phase-pr` without `--no-wait`, when CI is green, the code writes the `awaiting_review` card and does not commit it. This is intentional: `core/commands/PhasePr.scala` `finish`/`handOver` passes `persistCard = false`, and its scaladoc says "a push here would move that head past the commit the verdict just proved". IW-491 (commit 89d9a61f) made this change, and its `analysis.md:11` rejects "commit locally, do not push", because the next `phase-pr` pushes the local commit anyway. `phase-merge` (`doMergeAndAdvance`) and `phase-advance` (`doAdvance`, IW-583) commit the dirty card before they check out a branch, and `PhasePrHarnessTest` asserts the dirty card (`committedMessages == Nil`). Which behaviour do you want? - (a) Keep the current behaviour, and close IW-680 as intended behaviour. - (b) Commit the card locally without a push. IW-491 rejected this, because the next push moves the PR head and CI runs again. - (c) Commit and push the card after green. This starts a new CI run on a head that the gate did not read. IW-491 removed this behaviour. - (d) Something else. For example, a workflow step that you name still fails on the dirty card. If so, name that step, because `phase-merge` and `phase-advance` already accept the dirty card. Answer in a comment on this issue; intake reads it on its next iteration.

claude added the

needs-input

label

2026-10-03 01:15:10 +00:00