phase-pr must attach to an existing PR on Forgejo instead of failing on 409 #431

New issue

Open

opened 2026-08-21 15:58:36 +00:00 by mph · 0 comments

mph commented

2026-08-21 15:58:36 +00:00

Owner

Problem

On a Forgejo remote, re-running phase-pr against a branch that already carries
an open PR fails instead of attaching to it and resuming the CI wait.

Observed on IW-422 phase 1: ./iw phase-pr --title "Phase 1: ..." created PR
#424, then hit the tool's 10-minute timeout while waiting for CI. Re-running the
same command — the documented recovery — printed:

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

review-state stayed pinned at ci_pending and had to be moved to
awaiting_review by hand, after reading CI with fj pr status 424 --wait.

Root cause

The recovery path exists and is correct. PhasePr.resolvePullRequest falls back
to the PR url recorded by the first invocation when it sees a duplicate-PR
failure. Its detection predicate is the problem:

core/commands/PhasePr.scala:157
private def describesExisting(err: String): Boolean =
  err.toLowerCase.contains("already exists")

ForgejoClient never produces those words. Every failure is rendered as a bare
status code:

core/adapters/ForgejoClient.scala:178 (and 222, 399, 444, 599, 646, 705, 756)
case _ => Left(s"Forgejo API error: ${response.code}")

So Forgejo's duplicate-PR answer arrives as Forgejo API error: 409,
describesExisting returns false, and the command errors out. GitHub and GitLab
are unaffected — their clients say "already exists" in words.

Fix

Preferred: have ForgejoClient carry the response body into the error message
instead of discarding it. Today every Forgejo failure loses its reason, not only
this one, so this repairs a whole class of unhelpful errors. Then
describesExisting matches as intended.

Narrower alternative: make describesExisting recognise the 409 status.

Either way, add a test that drives the duplicate-PR path on the Forgejo backend —
there is none today, which is why this shipped.

Note on the skill

The wf-implementation-workflow skill documents exit 3 as "run the same command
again — it finds the pull request it already created and continues waiting."
That describes the intended behaviour correctly. The code does not match it on
Forgejo. Fix the code; the skill needs no change.

Why it matters

The window is worst in an unattended batch-implement run, where the exit-3
loop is the documented recovery path and no human is present to move
review-state by hand.

Origin

Found while working IW-422 and recorded in its LESSONS.md; root cause identified
during lessons triage.

## Problem On a Forgejo remote, re-running `phase-pr` against a branch that already carries an open PR fails instead of attaching to it and resuming the CI wait. Observed on IW-422 phase 1: `./iw phase-pr --title "Phase 1: ..."` created PR #424, then hit the tool's 10-minute timeout while waiting for CI. Re-running the same command — the documented recovery — printed: Error: The branch 'IW-422-phase-01' was already pushed. You can create the PR manually. Error: Failed to create merge request: Forgejo API error: 409 `review-state` stayed pinned at `ci_pending` and had to be moved to `awaiting_review` by hand, after reading CI with `fj pr status 424 --wait`. ## Root cause The recovery path exists and is correct. `PhasePr.resolvePullRequest` falls back to the PR url recorded by the first invocation when it sees a duplicate-PR failure. Its detection predicate is the problem: core/commands/PhasePr.scala:157 private def describesExisting(err: String): Boolean = err.toLowerCase.contains("already exists") `ForgejoClient` never produces those words. Every failure is rendered as a bare status code: core/adapters/ForgejoClient.scala:178 (and 222, 399, 444, 599, 646, 705, 756) case _ => Left(s"Forgejo API error: ${response.code}") So Forgejo's duplicate-PR answer arrives as `Forgejo API error: 409`, `describesExisting` returns false, and the command errors out. GitHub and GitLab are unaffected — their clients say "already exists" in words. ## Fix Preferred: have `ForgejoClient` carry the response body into the error message instead of discarding it. Today every Forgejo failure loses its reason, not only this one, so this repairs a whole class of unhelpful errors. Then `describesExisting` matches as intended. Narrower alternative: make `describesExisting` recognise the 409 status. Either way, add a test that drives the duplicate-PR path on the Forgejo backend — there is none today, which is why this shipped. ## Note on the skill The `wf-implementation-workflow` skill documents exit 3 as "run the same command again — it finds the pull request it already created and continues waiting." That describes the intended behaviour correctly. The code does not match it on Forgejo. Fix the code; the skill needs no change. ## Why it matters The window is worst in an unattended `batch-implement` run, where the exit-3 loop is the documented recovery path and no human is present to move `review-state` by hand. ## Origin Found while working IW-422 and recorded in its LESSONS.md; root cause identified during lessons triage.

mph referenced this issue from a commit

2026-08-21 20:59:52 +00:00

workflow: triage lessons for IW-422 (entries 7-8)