iterative-works/iw-cli

Watch

Fork 0

Phase 2: A 409 Conflict is not recognised as a duplicate pull request #454

Merged

mph merged 3 commits from IW-437-phase-02 into IW-437

2026-08-26 12:40:09 +00:00

mph commented

2026-08-26 11:36:53 +00:00

Owner

The defect

iw phase-pr is built to be re-entered: a gate that ends unfinished exits 3 and asks to be called
again, and the second invocation is meant to recognise that the pull request already exists and read
its url off the review-state card. That recovery was gated on prose — PhasePr.describesExisting
tested the error message for "already exists" — while ForgejoClient.createPullRequest collapsed
every unhandled status, 409 included, into Forgejo API error: 409.

GitHub and GitLab worked by accident of vocabulary, because gh and glab put those words in their
own stderr. Forgejo does not. On this project's own forge the resume the exit-3 contract asks for
could not re-enter at all, and the phase had to be merged by hand.

The cause was the port, not the adapter: TrackerOps.createPullRequest returned
Either[String, String], and a String error channel cannot carry a status.

The open investigation task, answered

Does Forgejo 16.0.1 answer 409 to anything besides a duplicate pull request?

No. In Forgejo v16.0.1, routers/api/v1/repo/pull.go, the CreatePullRequest handler returns
http.StatusConflict from exactly one branch — the duplicate check, GetUnmergedPullRequest →
issues_model.ErrPullRequestAlreadyExists → ctx.Error(http.StatusConflict, ...). No other path in
that handler returns 409. Source: tag v16.0.1 on codeberg.org/forgejo/forgejo.

code.iterative.works was confirmed to run 16.0.1 via unauthenticated GET /api/v1/version.

Consequence: the fix classifies a 409 on the status alone and does not inspect the response body.
Testing-requirement item 6 of the phase context (a 409 whose body is not a duplicate classifies as
Other) was explicitly conditional on this answer, so it is not needed.

The fix

A duplicate is now recognised by what the forge said, not by how the adapter phrased it.

core/model/ForgeCreateError.scala (new) — enum ForgeCreateError with Duplicate and
Other(message). Duplicate is a payload-less marker: the fact of the duplicate is the whole
signal. Its companion carries fromCliMessage, the one home for the predicate the CLI adapters
share.
CommandEnv.scala — the port returns Either[ForgeCreateError, String].
ForgejoClient.scala — StatusCode.Conflict maps to Duplicate. An unmapped status keeps
both the code and a body snippet, so the next surprise stays diagnosable.
GitHubClient.scala, GitLabClient.scala — each classifies its own CLI's wording through
ForgeCreateError.fromCliMessage. The prose match did not disappear; it moved to the layer that
owns the tool's vocabulary, exactly as broad as it was before (case-insensitive
contains("already exists")). It was not narrowed: no tool-contract test in this repository pins
what gh and glab actually emit, so a tighter match would be a guess.
PhasePr.scala — resolvePullRequest matches Duplicate; describesExisting is deleted with
its comment. The false "the branch was already pushed, you can create the PR manually" line no
longer prints on a failure that is not a duplicate. When a duplicate meets a card that records no
url, the message names the branch the forge refused, so the user has something to search by.

Scope statements the phase context asked for

Only one of the eight Forgejo API error fall-throughs changed. ForgejoClient has eight, not
the six the analysis counted: fetchIssue, createIssue, createPullRequest, mergePullRequest,
fetchPullRequestForBranch, fetchPullRequestByNumber, fetchPullRequestState, fetchPrHeadSha.
Only createPullRequest is in scope; the other seven are untouched by design, because no caller
matches on their text. The asymmetry is deliberate, not an oversight.
The gh/glab duplicate wording is an assumption, not a fact. Nothing in this repository pins
it, and no tool-contract test covers it. Moving the match into the adapters puts that assumption
where core/CLAUDE.md says API clients belong, but it remains an assumption.
Unchanged by this phase: core/commands/CiGate.scala, core/commands/CiGateCommand.scala,
core/commands/PhaseMerge.scala, core/model/CiGate.scala. The test contracts that invert belong
to phase 3.

Tests

The regression test fails before the fix and passes after: a 409 resumes off the card and exits 0.

ForgejoClientTest — a 409 classifies as Duplicate however the body is worded (the stubbed
body is deliberately not "already exists", so the test cannot pass under a regression to prose
matching); an unmapped status keeps code and body; the snippet strips control characters and caps
at 500; the snippet strips Unicode bidirectional formatting.
PhasePrHarnessTest — a Duplicate resumes off the card and gates; a duplicate the card cannot
answer names the branch it was refused for; the control still exits 1, still makes no CI snapshot
call, and its stderr no longer claims manual creation.
GitHubClientTest, GitLabClientTest — each CLI's duplicate wording classifies as Duplicate.
ForgeCreateErrorTest (new) — the predicate is case-insensitive whatever case the CLI wrote, and
an unrelated message keeps its text verbatim.

./mill core.test — 2499 assertions, 0 failures. ./iw ./test — exit 0.

Review

Two iterations, ten reviewer passes, zero critical issues in either round. Six warnings in
iteration 1 and three in iteration 2, all resolved; full record in
review-phase-02-20260826-091102.md. Two findings were worth the rounds: the 409 test as first
written could not have caught a regression to prose matching, and the typed error silently dropped
the forge's own diagnostic on the duplicate-with-unreadable-card path.

🤖 Generated with Claude Code

## The defect `iw phase-pr` is built to be re-entered: a gate that ends unfinished exits 3 and asks to be called again, and the second invocation is meant to recognise that the pull request already exists and read its url off the review-state card. That recovery was gated on prose — `PhasePr.describesExisting` tested the error message for `"already exists"` — while `ForgejoClient.createPullRequest` collapsed every unhandled status, 409 included, into `Forgejo API error: 409`. GitHub and GitLab worked by accident of vocabulary, because `gh` and `glab` put those words in their own stderr. Forgejo does not. On this project's own forge the resume the exit-3 contract asks for could not re-enter at all, and the phase had to be merged by hand. The cause was the port, not the adapter: `TrackerOps.createPullRequest` returned `Either[String, String]`, and a `String` error channel cannot carry a status. ## The open investigation task, answered **Does Forgejo 16.0.1 answer 409 to anything besides a duplicate pull request?** **No.** In Forgejo `v16.0.1`, `routers/api/v1/repo/pull.go`, the `CreatePullRequest` handler returns `http.StatusConflict` from exactly one branch — the duplicate check, `GetUnmergedPullRequest` → `issues_model.ErrPullRequestAlreadyExists` → `ctx.Error(http.StatusConflict, ...)`. No other path in that handler returns 409. Source: tag `v16.0.1` on `codeberg.org/forgejo/forgejo`. `code.iterative.works` was confirmed to run 16.0.1 via unauthenticated `GET /api/v1/version`. **Consequence:** the fix classifies a 409 on the status alone and does not inspect the response body. Testing-requirement item 6 of the phase context (a 409 whose body is not a duplicate classifies as `Other`) was explicitly conditional on this answer, so it is not needed. ## The fix A duplicate is now recognised by what the forge said, not by how the adapter phrased it. - **`core/model/ForgeCreateError.scala`** (new) — `enum ForgeCreateError` with `Duplicate` and `Other(message)`. `Duplicate` is a payload-less marker: the fact of the duplicate is the whole signal. Its companion carries `fromCliMessage`, the one home for the predicate the CLI adapters share. - **`CommandEnv.scala`** — the port returns `Either[ForgeCreateError, String]`. - **`ForgejoClient.scala`** — `StatusCode.Conflict` maps to `Duplicate`. An unmapped status keeps both the code and a body snippet, so the next surprise stays diagnosable. - **`GitHubClient.scala`, `GitLabClient.scala`** — each classifies its own CLI's wording through `ForgeCreateError.fromCliMessage`. The prose match did not disappear; it moved to the layer that owns the tool's vocabulary, **exactly as broad as it was before** (case-insensitive `contains("already exists")`). It was not narrowed: no tool-contract test in this repository pins what `gh` and `glab` actually emit, so a tighter match would be a guess. - **`PhasePr.scala`** — `resolvePullRequest` matches `Duplicate`; `describesExisting` is deleted with its comment. The false "the branch was already pushed, you can create the PR manually" line no longer prints on a failure that is not a duplicate. When a duplicate meets a card that records no url, the message names the branch the forge refused, so the user has something to search by. ## Scope statements the phase context asked for - **Only one of the eight `Forgejo API error` fall-throughs changed.** `ForgejoClient` has eight, not the six the analysis counted: `fetchIssue`, `createIssue`, `createPullRequest`, `mergePullRequest`, `fetchPullRequestForBranch`, `fetchPullRequestByNumber`, `fetchPullRequestState`, `fetchPrHeadSha`. Only `createPullRequest` is in scope; the other seven are untouched by design, because no caller matches on their text. The asymmetry is deliberate, not an oversight. - **The `gh`/`glab` duplicate wording is an assumption, not a fact.** Nothing in this repository pins it, and no tool-contract test covers it. Moving the match into the adapters puts that assumption where `core/CLAUDE.md` says API clients belong, but it remains an assumption. - **Unchanged by this phase:** `core/commands/CiGate.scala`, `core/commands/CiGateCommand.scala`, `core/commands/PhaseMerge.scala`, `core/model/CiGate.scala`. The test contracts that invert belong to phase 3. ## Tests The regression test fails before the fix and passes after: a 409 resumes off the card and exits 0. - `ForgejoClientTest` — a 409 classifies as `Duplicate` **however the body is worded** (the stubbed body is deliberately *not* "already exists", so the test cannot pass under a regression to prose matching); an unmapped status keeps code and body; the snippet strips control characters and caps at 500; the snippet strips Unicode bidirectional formatting. - `PhasePrHarnessTest` — a `Duplicate` resumes off the card and gates; a duplicate the card cannot answer names the branch it was refused for; the control still exits 1, still makes no CI snapshot call, and its stderr no longer claims manual creation. - `GitHubClientTest`, `GitLabClientTest` — each CLI's duplicate wording classifies as `Duplicate`. - `ForgeCreateErrorTest` (new) — the predicate is case-insensitive whatever case the CLI wrote, and an unrelated message keeps its text verbatim. `./mill core.test` — 2499 assertions, 0 failures. `./iw ./test` — exit 0. ## Review Two iterations, ten reviewer passes, **zero critical issues in either round**. Six warnings in iteration 1 and three in iteration 2, all resolved; full record in `review-phase-02-20260826-091102.md`. Two findings were worth the rounds: the 409 test as first written could not have caught a regression to prose matching, and the typed error silently dropped the forge's own diagnostic on the duplicate-with-unreadable-card path. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

mph added 2 commits

2026-08-26 11:36:54 +00:00

chore(IW-437): update review-state for phase 02 7a6a4849a1

A 409 Conflict is not recognised as a duplicate pull request

CI / contract (pull_request) Has been skipped

Details

CI / format (pull_request) Successful in 7s

Details

CI / lint (pull_request) Successful in 7s

Details

CI / compile (pull_request) Successful in 1m25s

Details

CI / dashboard-build (pull_request) Successful in 59s

Details

CI / test (pull_request) Successful in 9m30s

Details

e7d20f56b0

- Type the createPullRequest error channel as ForgeCreateError, so a duplicate
  is recognised by what the forge said and not by how the adapter phrased it
- Classify Forgejo's 409 by status alone - v16.0.1 answers it only for a duplicate
- Move the gh/glab prose match into the adapters that own each CLI's vocabulary
- Stop claiming manual PR creation is the next step on a failure that is not a duplicate
- Sanitise and cap the response body that reaches the terminal

Issue: IW-437
Phase: 2 - A 409 Conflict is not recognised as a duplicate pull request

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019Pwo2RcHqPuziyjj49RMFp

mph added 1 commit

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

chore(IW-437): update review-state for phase 02

CI / contract (pull_request) Has been skipped

Details

CI / format (pull_request) Successful in 7s

Details

CI / lint (pull_request) Successful in 7s

Details

CI / compile (pull_request) Successful in 1m23s

Details

CI / dashboard-build (pull_request) Successful in 58s