Phase CI gate: phase-pr restarts CI on every resume, cannot resume on Forgejo, and expires against a green pipeline #437

New issue

Closed

opened 2026-08-23 19:28:38 +00:00 by mph · 0 comments

mph commented

2026-08-23 19:28:38 +00:00

Owner

1. A 409 is not recognised as "already exists"

./iw phase-pr created PR 434, waited 481s, and exited 3 asking to be run again. The second run failed:

Error: The branch 'IW-423-phase-01' was already pushed. You can create the PR manually.
Error: Failed to create merge request: Forgejo API error: 409

PhasePr.resolvePullRequest (core/commands/PhasePr.scala:139-154) is written for exactly this case — on a duplicate it falls back to the PR url on the card. It gates that fallback on describesExisting (:157-158), which matches err.toLowerCase.contains("already exists"). But ForgejoClient.createPullRequest (core/adapters/ForgejoClient.scala:391-399) collapses every unhandled status into s"Forgejo API error: ${response.code}", so a 409 Conflict arrives as the string Forgejo API error: 409.

The comment at PhasePr.scala:131-137 ("A gate that ends unfinished asks to be called again... The card holds the url the first invocation recorded") describes behaviour the code does not deliver on this forge.

Fix: handle StatusCode.Conflict explicitly in ForgejoClient.createPullRequest, and match on the status in PhasePr rather than on prose. A retry path keyed on the text of an error message is only as good as the adapter that formats it. Wants a test: a 409 must reach describesExisting, whatever wording the forge uses.

2. The CI wait budget is wall-clock since the gate first ran, not time spent waiting

CI on PR 434 was green — combined state success on the branch tip, verified through /api/v1/repos/.../commits/<sha>/status. ./iw phase-merge --max-retries 0 nonetheless refused with:

Timed out waiting for CI checks after 30m.

The 30m --timeout had been consumed by the earlier phase-pr gate plus the manual waiting that defect 1 forced — none of which was time this invocation spent waiting.

Fix: decide deliberately whether the budget measures elapsed wall-clock or accumulated polling, and whether a gate should refuse at all once the forge reports a terminal green. A budget measured from the first gate run expires against a green pipeline whenever anything delays the caller between the gate and the merge.

3. `phase-pr` restarts CI every time it hands the wait back, so the wait never converges

This is the defect that makes the exit-3 loop non-terminating. It is forge-independent.

phase-pr commits and pushes review-state.json when its poll window ends. The push moves the head of the pull request branch, so the forge starts CI again on a new commit. The path:

CiGate.markPending (core/commands/CiGate.scala:240) writes the card, and heartbeat (:265) rewrites it every poll interval. ReviewStateUpdater always refreshes last_updated (core/model/ReviewStateUpdater.scala:85), so the file is always dirty.
The poll window (8 min default) ends. The verdict is CiVerdict.Running.
PhasePr.finish calls persist for that verdict (core/commands/PhasePr.scala:234).
persist (:336) runs commitFileWithRetry and then git push on the phase branch.
The new head cancels the run in flight (concurrency: cancel-in-progress, .github/workflows/ci.yml:19) and starts a fresh one.
The agent obeys exit 3 and runs the same command again. It waits 8 min on a run that is only 8 min old, then repeats step 3.

Any pipeline longer than the poll window can therefore never be proved green. The 30m budget survives across invocations, so the phase ends as ci_failed although nothing failed.

--batch has the same defect through a different door: the gate policy is CommitPolicy.CommitBeforeGate (:190), and the entry git push of the next invocation (:115) pushes that commit.

Raising --poll-window is not a way out. A foreground tool call is capped at 600000 ms, so the window can never exceed about 9 minutes for an agent.

Confirmed. A probe assertion added to the existing exit-3 harness test fails:

==> X PhasePrHarnessTest.the poll window elapses: exit 3 ...
=> Obtained
List("chore(TEST-100): update review-state for phase 01")

The push buys nothing. The dashboard reads review-state.json from the worktree path (dashboard/jvm/src/ReviewStateService.scala:152), not from the forge. A wait whose card stays local is still visible. iw ci-gate is already correct and is the model to copy: policy DeferToCaller, and the Running case only prints the resume report (core/commands/CiGateCommand.scala:135, :147).

Fix. Hold this invariant: while a CI wait is unfinished, phase-pr must not move the head of the branch.

In PhasePr.finish, remove persist(r, env) from the CiVerdict.Running case. Keep the card write. The file stays uncommitted until the verdict is terminal.
In PhasePr.gate, use CommitPolicy.DeferToCaller for --batch too. Move the card commit into doBatchMerge, immediately before mergeSquashAndDelete — the only step that needs a clean tree, because gh pr merge --delete-branch does a checkout.

The terminal paths (green, red, no-CI) keep their commit and push.

Two alternatives were considered and rejected:

paths-ignore on the workflow. Per project, and it leaves the new head with no run at all, so the gate then reports RunsForOtherShaOnly.
Commit locally, skip the entry push while resuming. Keeps the tree clean, but the "am I resuming" test is subtle. If it is wrong, a real CI fix is never pushed and the gate waits on a stale commit. phase-merge is safe today only because it has no entry push.

Tests to add:

PhasePrHarnessTest: the exit-3 path makes no commit (the probe above).
PhasePrHarnessTest: the exit-3 path under --batch makes no commit either.
PhasePrHarnessTest: a green --batch run still commits the card before the merge.

Defects 1 and 2 recorded during IW-423; see project-management/issues/IW-423/LESSONS.md entries 10 and 12.

Three defects in the phase CI gate. Defects 1 and 2 were hit while shipping Phase 1 of IW-423. Defect 3 was found by reading the code afterwards and is confirmed by a harness probe. Together they make the gate unable to finish a phase without manual intervention. ## 1. A 409 is not recognised as "already exists" `./iw phase-pr` created PR 434, waited 481s, and exited 3 asking to be run again. The second run failed: ``` Error: The branch 'IW-423-phase-01' was already pushed. You can create the PR manually. Error: Failed to create merge request: Forgejo API error: 409 ``` `PhasePr.resolvePullRequest` (`core/commands/PhasePr.scala:139-154`) is written for exactly this case — on a duplicate it falls back to the PR url on the card. It gates that fallback on `describesExisting` (`:157-158`), which matches `err.toLowerCase.contains("already exists")`. But `ForgejoClient.createPullRequest` (`core/adapters/ForgejoClient.scala:391-399`) collapses every unhandled status into `s"Forgejo API error: ${response.code}"`, so a 409 Conflict arrives as the string `Forgejo API error: 409`. The comment at `PhasePr.scala:131-137` ("A gate that ends unfinished asks to be called again... The card holds the url the first invocation recorded") describes behaviour the code does not deliver on this forge. **Fix:** handle `StatusCode.Conflict` explicitly in `ForgejoClient.createPullRequest`, and match on the status in `PhasePr` rather than on prose. A retry path keyed on the text of an error message is only as good as the adapter that formats it. Wants a test: a 409 must reach `describesExisting`, whatever wording the forge uses. ## 2. The CI wait budget is wall-clock since the gate first ran, not time spent waiting CI on PR 434 was green — combined state `success` on the branch tip, verified through `/api/v1/repos/.../commits/<sha>/status`. `./iw phase-merge --max-retries 0` nonetheless refused with: ``` Timed out waiting for CI checks after 30m. ``` The 30m `--timeout` had been consumed by the earlier `phase-pr` gate plus the manual waiting that defect 1 forced — none of which was time this invocation spent waiting. **Fix:** decide deliberately whether the budget measures elapsed wall-clock or accumulated polling, and whether a gate should refuse at all once the forge reports a terminal green. A budget measured from the first gate run expires against a green pipeline whenever anything delays the caller between the gate and the merge. ## 3. `phase-pr` restarts CI every time it hands the wait back, so the wait never converges This is the defect that makes the exit-3 loop non-terminating. It is forge-independent. `phase-pr` commits and pushes `review-state.json` when its poll window ends. The push moves the head of the pull request branch, so the forge starts CI again on a new commit. The path: 1. `CiGate.markPending` (`core/commands/CiGate.scala:240`) writes the card, and `heartbeat` (`:265`) rewrites it every poll interval. `ReviewStateUpdater` always refreshes `last_updated` (`core/model/ReviewStateUpdater.scala:85`), so the file is always dirty. 2. The poll window (8 min default) ends. The verdict is `CiVerdict.Running`. 3. `PhasePr.finish` calls `persist` for that verdict (`core/commands/PhasePr.scala:234`). 4. `persist` (`:336`) runs `commitFileWithRetry` and then `git push` on the phase branch. 5. The new head cancels the run in flight (`concurrency: cancel-in-progress`, `.github/workflows/ci.yml:19`) and starts a fresh one. 6. The agent obeys exit 3 and runs the same command again. It waits 8 min on a run that is only 8 min old, then repeats step 3. Any pipeline longer than the poll window can therefore never be proved green. The 30m budget survives across invocations, so the phase ends as `ci_failed` although nothing failed. `--batch` has the same defect through a different door: the gate policy is `CommitPolicy.CommitBeforeGate` (`:190`), and the entry `git push` of the next invocation (`:115`) pushes that commit. Raising `--poll-window` is not a way out. A foreground tool call is capped at 600000 ms, so the window can never exceed about 9 minutes for an agent. **Confirmed.** A probe assertion added to the existing exit-3 harness test fails: ``` ==> X PhasePrHarnessTest.the poll window elapses: exit 3 ... => Obtained List("chore(TEST-100): update review-state for phase 01") ``` **The push buys nothing.** The dashboard reads `review-state.json` from the worktree path (`dashboard/jvm/src/ReviewStateService.scala:152`), not from the forge. A wait whose card stays local is still visible. `iw ci-gate` is already correct and is the model to copy: policy `DeferToCaller`, and the `Running` case only prints the resume report (`core/commands/CiGateCommand.scala:135`, `:147`). **Fix.** Hold this invariant: while a CI wait is unfinished, `phase-pr` must not move the head of the branch. 1. In `PhasePr.finish`, remove `persist(r, env)` from the `CiVerdict.Running` case. Keep the card write. The file stays uncommitted until the verdict is terminal. 2. In `PhasePr.gate`, use `CommitPolicy.DeferToCaller` for `--batch` too. Move the card commit into `doBatchMerge`, immediately before `mergeSquashAndDelete` — the only step that needs a clean tree, because `gh pr merge --delete-branch` does a checkout. The terminal paths (green, red, no-CI) keep their commit and push. Two alternatives were considered and rejected: - **`paths-ignore` on the workflow.** Per project, and it leaves the new head with no run at all, so the gate then reports `RunsForOtherShaOnly`. - **Commit locally, skip the entry push while resuming.** Keeps the tree clean, but the "am I resuming" test is subtle. If it is wrong, a real CI fix is never pushed and the gate waits on a stale commit. `phase-merge` is safe today only because it has no entry push. **Tests to add:** - `PhasePrHarnessTest`: the exit-3 path makes no commit (the probe above). - `PhasePrHarnessTest`: the exit-3 path under `--batch` makes no commit either. - `PhasePrHarnessTest`: a green `--batch` run still commits the card before the merge. --- Defects 1 and 2 recorded during IW-423; see `project-management/issues/IW-423/LESSONS.md` entries 10 and 12.

mph referenced this issue from a commit

2026-08-23 20:00:59 +00:00

workflow: Triage lessons for IW-423

mph changed title from ~~Phase CI gate: phase-pr cannot resume on Forgejo, phase-merge expires against a green pipeline~~ to Phase CI gate: phase-pr restarts CI on every resume, cannot resume on Forgejo, and expires against a green pipeline

2026-08-25 11:39:21 +00:00

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

2026-08-28 07:22:08 +00:00

IW-437: The phase CI gate finishes a phase without manual intervention #475

mph closed this issue

2026-08-28 07:55:46 +00:00