phase-pr's duplicate fallback can return an earlier phase's PR url #467

New issue

Closed

opened 2026-08-27 08:35:15 +00:00 by mph · 1 comment

mph commented

2026-08-27 08:35:15 +00:00

Owner

Found triaging IW-437 (recorded during phase 2, deliberately not fixed there).

WHAT. resolvePullRequest's Duplicate branch reads the pull request url off review-state.json via reviewStatePathFor(r, env) (core/commands/PhasePr.scala:143-160). That path is keyed on the ISSUE, not the phase (PhasePr.reviewStatePathFor, :453).

If a create fails as a duplicate while the card still carries an earlier phase's pr_url, resolvePullRequest hands back that stale url. The CI gate then waits on the wrong pull request, and under --batch the merge acts on the wrong pull request.

AGE AND EXPOSURE. The window predates IW-437 phase 2: it existed whenever describesExisting matched. Phase 2 did not create it, but it widened what counts as a duplicate - a 409 status now qualifies on every forge, not only the forges whose CLI happened to phrase the error as "already exists" - so the window is easier to hit after that fix than before it.

WHY IT WAS NOT FIXED THERE. phase-02-context.md scoped the phase to the create-pull-request classification path and instructed: "Record the per-issue card risk; do not fix it."

DESIGN QUESTION, not a one-line fix. A guard costs a forge round trip to read the pull request's head branch before trusting the card's url. Where it belongs is open: inside resolvePullRequest, or as a check on the card itself (does this card's pr_url belong to the phase we are running?).

Severity note: this is the failure mode in which the tool merges a pull request nobody asked it to merge.

Found triaging IW-437 (recorded during phase 2, deliberately not fixed there). WHAT. resolvePullRequest's Duplicate branch reads the pull request url off review-state.json via reviewStatePathFor(r, env) (core/commands/PhasePr.scala:143-160). That path is keyed on the ISSUE, not the phase (PhasePr.reviewStatePathFor, :453). If a create fails as a duplicate while the card still carries an earlier phase's pr_url, resolvePullRequest hands back that stale url. The CI gate then waits on the wrong pull request, and under --batch the merge acts on the wrong pull request. AGE AND EXPOSURE. The window predates IW-437 phase 2: it existed whenever describesExisting matched. Phase 2 did not create it, but it widened what counts as a duplicate - a 409 status now qualifies on every forge, not only the forges whose CLI happened to phrase the error as "already exists" - so the window is easier to hit after that fix than before it. WHY IT WAS NOT FIXED THERE. phase-02-context.md scoped the phase to the create-pull-request classification path and instructed: "Record the per-issue card risk; do not fix it." DESIGN QUESTION, not a one-line fix. A guard costs a forge round trip to read the pull request's head branch before trusting the card's url. Where it belongs is open: inside resolvePullRequest, or as a check on the card itself (does this card's pr_url belong to the phase we are running?). Severity note: this is the failure mode in which the tool merges a pull request nobody asked it to merge.

mph referenced this issue

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

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

mph referenced this issue from a commit

2026-09-02 06:22:12 +00:00

fix(phase-pr): refuse a recorded pull request that is not on this branch

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

2026-09-02 06:22:57 +00:00

Squash the batch-implement blockers: nine fixes for the phase gate, the CI gate and the review-state card #501

mph closed this issue

2026-09-02 20:54:35 +00:00

mph referenced this issue from a commit

2026-09-03 05:43:37 +00:00

fix(phase-pr): identify a recorded pull request by its branch, not its head

mph referenced this issue

2026-09-03 05:43:45 +00:00

Record the card's position on write, tell a killed process from a failing one, and identify a phase's pull request by its branch #506

mph referenced this issue

2026-09-03 05:44:19 +00:00

phase-pr can ask the forge which pull request is on this branch instead of trusting the card #507

mph referenced this issue

2026-09-03 05:44:19 +00:00

A harness gate test whose head sha matches no CI run spins forever instead of failing #508

mph referenced this issue

2026-09-03 05:55:44 +00:00

tmux-dependent BATS tests fail non-deterministically under a full ./iw ./test run #468

mph commented

2026-09-03 05:55:45 +00:00

Author

Owner

The two warnings the review raised against the guard are fixed on main as of cfe98763 (PR #506), and the fix changed the question the guard asks.

Both warnings had one root: a head-sha comparison asks whether the recorded pull request points at the commit just pushed, and that is not the same question as whether it belongs to this phase.

The false refusal. A forge publishes a merge request's head commit after it takes the push. GitLab does this. So the right pull request could report the commit before the one this phase pushed and be refused — under --batch that stops the run and needs a person, for a pull request that was correct all along.
The remedy that destroyed data. The refusal told the reader to clear pr_url. On a lag that is a good url thrown away to work around a delay, and clearing it leads to the other dead end: "the review-state card records no url for it".

The branch a pull request is open on says which work it is for, and no lag moves it. TrackerOps.fetchPrHeadBranch reads it from Forgejo (head.ref), GitHub (head.ref through gh api) and GitLab (source_branch). The guard compares it against phaseBranchName. The refusal names both branches and asks for pr_url to point at the pull request open on this phase's branch, rather than for the loss of the one recorded. A forge that will not answer is still a refusal — guessing is the failure this guard exists to stop.

What this does not do is remove the need for a person when the card is genuinely wrong. The forge already knows which pull request is open on the branch, so the card could leave the merge decision entirely. That is #507.

The two warnings the review raised against the guard are fixed on `main` as of `cfe98763` (PR #506), and the fix changed the question the guard asks. Both warnings had one root: a head-sha comparison asks whether the recorded pull request points at the commit just pushed, and that is not the same question as whether it belongs to this phase. - **The false refusal.** A forge publishes a merge request's head commit after it takes the push. GitLab does this. So the right pull request could report the commit before the one this phase pushed and be refused — under `--batch` that stops the run and needs a person, for a pull request that was correct all along. - **The remedy that destroyed data.** The refusal told the reader to clear `pr_url`. On a lag that is a good url thrown away to work around a delay, and clearing it leads to the other dead end: "the review-state card records no url for it". The branch a pull request is open on says which work it is for, and no lag moves it. `TrackerOps.fetchPrHeadBranch` reads it from Forgejo (`head.ref`), GitHub (`head.ref` through `gh api`) and GitLab (`source_branch`). The guard compares it against `phaseBranchName`. The refusal names both branches and asks for `pr_url` to point at the pull request open on this phase's branch, rather than for the loss of the one recorded. A forge that will not answer is still a refusal — guessing is the failure this guard exists to stop. What this does not do is remove the need for a person when the card is genuinely wrong. The forge already knows which pull request is open on the branch, so the card could leave the merge decision entirely. That is #507.