phase-pr cannot tell a forge that is down from a forge that refused, so both stop a batch

mph commented

2026-09-03 08:37:03 +00:00

Owner

phase-pr's duplicate path refuses with exit 1 on three conditions that are not alike:

the branch carries a merged or closed pull request — genuinely resolved, a person must look
the create said duplicate and the forge then lists none for the branch — a real contradiction
the forge would not answer — a 502, a rate limit, a network blip

The third is Left, not Right(Some(resolved)). Nothing is resolved and nothing contradicts, yet an unattended batch stops on it, while the CI wait treats an equivalent "cannot say yet" as exit 3 and polls. The asymmetry is real and was raised by the error-handling review and independently by the plugin consumer.

Why exit 3 is not the fix as things stand

Checked before answering, and both checks fail.

There is no budget at that point in the command. ci_wait_started is written inside the gate (core/commands/CiGate.scala:249). The duplicate path runs in resolvePullRequest, which createPrAndGate calls before gateAndFinish (core/commands/PhasePr.scala:115-124). So when this refusal happens, no wait has started and no budget exists. PhaseOutcome.AwaitCI deliberately does not consume the recovery budget — core/model/BatchImplement.scala:11-15 says the wait must not be paid for out of it — so an exit 3 here would be bounded by nothing. Against a forge that is down, the batch would re-dispatch forever, and each iteration re-runs env.git.push (PhasePr.scala:115) before reaching the refusal again.

Left is not reliably transient. It carries permanent configuration failures alongside network ones:

GitHubClient.requireGh returns "gh not installed" and "gh not authenticated" (GitHubClient.scala:1003-1015)
GitLabClient.requireGlab does the same for glab
Forgejo returns "API token is invalid or expired" on 401, and a bare status line on anything else

Splitting on Left alone would put a missing binary or a bad token into an unbounded retry, which is a worse failure than stopping: the batch never ends and never says why.

What the fix actually needs

Both, and in this order:

A transient/permanent distinction the caller can read. Today every forge failure is Either[String, A], so the only thing a caller can do is match on message text. The error-handling review named this as the one place the codebase's stringly-typed errors cost something real. A small error type on the tracker port — enough to separate "could not reach the forge" from "the forge refused you" — is the prerequisite.
A budget for the pre-gate phase, or an explicit attempt count carried on the card, so a retryable refusal is bounded by something. Without it, exit 3 anywhere before the gate is unbounded by construction.

Only then does exit 3 become the right answer for case 3, and it needs no fourth exit code: the 0/1/3 contract already means "nothing failed, run this again".

Until then

Exit 1 stands, and the trade-off is recorded in pullRequestOnBranch's own docstring. A batch stopped on a transient blip is recoverable by a person; a batch that loops forever, or one that merges against a stale reading, is not.

Origin

Raised by the error-handling review of PR #511 and refined by the kanon session, which noticed it had asked for exit 1 here while mapping the same condition to "resume normal flow" in its own catchUpMergedPhases.

`phase-pr`'s duplicate path refuses with exit 1 on three conditions that are not alike: 1. the branch carries a merged or closed pull request — genuinely resolved, a person must look 2. the create said duplicate and the forge then lists none for the branch — a real contradiction 3. **the forge would not answer** — a 502, a rate limit, a network blip The third is `Left`, not `Right(Some(resolved))`. Nothing is resolved and nothing contradicts, yet an unattended batch stops on it, while the CI wait treats an equivalent "cannot say yet" as exit 3 and polls. The asymmetry is real and was raised by the error-handling review and independently by the plugin consumer. ## Why exit 3 is not the fix as things stand Checked before answering, and both checks fail. **There is no budget at that point in the command.** `ci_wait_started` is written inside the gate (`core/commands/CiGate.scala:249`). The duplicate path runs in `resolvePullRequest`, which `createPrAndGate` calls *before* `gateAndFinish` (`core/commands/PhasePr.scala:115-124`). So when this refusal happens, no wait has started and no budget exists. `PhaseOutcome.AwaitCI` deliberately does not consume the recovery budget — `core/model/BatchImplement.scala:11-15` says the wait must not be paid for out of it — so an exit 3 here would be bounded by nothing. Against a forge that is down, the batch would re-dispatch forever, and each iteration re-runs `env.git.push` (`PhasePr.scala:115`) before reaching the refusal again. **`Left` is not reliably transient.** It carries permanent configuration failures alongside network ones: - `GitHubClient.requireGh` returns "gh not installed" and "gh not authenticated" (`GitHubClient.scala:1003-1015`) - `GitLabClient.requireGlab` does the same for glab - Forgejo returns "API token is invalid or expired" on 401, and a bare status line on anything else Splitting on `Left` alone would put a missing binary or a bad token into an unbounded retry, which is a worse failure than stopping: the batch never ends and never says why. ## What the fix actually needs Both, and in this order: 1. **A transient/permanent distinction the caller can read.** Today every forge failure is `Either[String, A]`, so the only thing a caller can do is match on message text. The error-handling review named this as the one place the codebase's stringly-typed errors cost something real. A small error type on the tracker port — enough to separate "could not reach the forge" from "the forge refused you" — is the prerequisite. 2. **A budget for the pre-gate phase**, or an explicit attempt count carried on the card, so a retryable refusal is bounded by something. Without it, exit 3 anywhere before the gate is unbounded by construction. Only then does exit 3 become the right answer for case 3, and it needs no fourth exit code: the 0/1/3 contract already means "nothing failed, run this again". ## Until then Exit 1 stands, and the trade-off is recorded in `pullRequestOnBranch`'s own docstring. A batch stopped on a transient blip is recoverable by a person; a batch that loops forever, or one that merges against a stale reading, is not. ## Origin Raised by the error-handling review of PR #511 and refined by the kanon session, which noticed it had asked for exit 1 here while mapping the same condition to "resume normal flow" in its own `catchUpMergedPhases`.

mph added the

needs-human

label

2026-09-03 21:32:24 +00:00

mph commented

2026-09-03 21:32:24 +00:00

Author

Owner

Intake: needs-human
Workflow: waterfall
Reason: G1 — one CLARIFY marker (analysis.md:260): the analysis refuses the typed-error-plus-persisted-budget scope the issue states and proposes a bounded in-process retry instead; a human picks the scope
Analysis: project-management/issues/IW-513/analysis.md on branch IW-513

Intake: needs-human Workflow: waterfall Reason: G1 — one CLARIFY marker (analysis.md:260): the analysis refuses the typed-error-plus-persisted-budget scope the issue states and proposes a bounded in-process retry instead; a human picks the scope Analysis: project-management/issues/IW-513/analysis.md on branch IW-513

mph removed the

needs-human

label

2026-09-05 19:46:49 +00:00

claude referenced this issue

2026-09-15 01:44:15 +00:00

phase-pr exits 0 on a green gate, then pushes a commit the gate never validated #536

claude commented

2026-09-15 02:46:23 +00:00

Member

Intake: needs-input

Every technical claim in this issue verifies (line numbers have drifted, the facts hold): ci_wait_started is written in markPending at CiGate.scala:264; resolvePullRequest runs before gateAndFinish at PhasePr.scala:217-226 with env.git.push at :221; BatchImplement.scala:13-14 says "the recovery budget must not pay for the wait"; and requireGh (GitHubClient.scala:982-994) / requireGlab (GitLabClient.scala:1010-1022) return "not installed" and "not authenticated" through the same Left channel. The docstring claim is true too — pullRequestOnBranch (PhasePr.scala:286-305) states every refusal there is terminal, and PhasePrHarnessTest.scala:889 pins it.

The asymmetry is real today: commit 984c5c0b on main made a silent forge in the gate return CiVerdict.Unreadable, which PhasePr.scala:428-440 turns into exit 3. (An earlier analysis on an unmerged branch denied this premise; that denial is now out of date.)

The blocker is size, not doubt. Typing fetchPullRequestForBranch alone touches 13 files (CommandEnv, LiveCommandEnv, ForgeGateway, three clients, PhasePr, dashboard PullRequestReader, FakeCommandEnv, four test files); part 2 adds a review-state.json field plus ReviewStateUpdater, BatchImplement and the 0/1/3 contract docs. Honest total: 4 phases, above the 3-phase ceiling the unattended loop will run.

1. This issue specifies a two-part fix (typed transient/permanent error on the tracker port, then a pre-gate budget). A narrower alternative exists: keep Either[String, A] and give the duplicate path the same bounded in-process retry the CI gate already uses (~3 attempts, 30s apart, then exit 1). Which do you want built — the narrow retry (~1 phase), the full two-part fix (~4 phases, needing a split into separate issues), or the narrow retry now with the typed error deferred until a real outage proves 90 seconds too thin?

2. Typing the error on fetchPullRequestForBranch changes a signature in the published core jar, which the dashboard (PullRequestReader) and the kanon plugin both link against. Is breaking that published signature acceptable here, or must the typed error be added without changing existing callers?

3. Should the typed error cover only fetchPullRequestForBranch, or every forge call that currently returns Either[String, A] (~84 such returns across the three clients)? The error-handling review this issue cites asked for the general case; the issue text asks for "a small error type ... enough to separate".

Not a duplicate: #485 (no-pipeline-yet passes the gate) and #491 (push after reading CI) are different phase-pr defects, and #400 is about the batch loop rather than the pre-gate path.

Note: an earlier intake reached the same conclusion on 2026-09-03 (commit b96f2048, "intake analysis — needs-human") via a CLARIFY marker. That commit is not on main and no branch contains it, so its analysis is orphaned; the question it raised was never answered.

Intake: needs-input Every technical claim in this issue verifies (line numbers have drifted, the facts hold): `ci_wait_started` is written in `markPending` at `CiGate.scala:264`; `resolvePullRequest` runs before `gateAndFinish` at `PhasePr.scala:217-226` with `env.git.push` at :221; `BatchImplement.scala:13-14` says "the recovery budget must not pay for the wait"; and `requireGh` (`GitHubClient.scala:982-994`) / `requireGlab` (`GitLabClient.scala:1010-1022`) return "not installed" and "not authenticated" through the same `Left` channel. The docstring claim is true too — `pullRequestOnBranch` (`PhasePr.scala:286-305`) states every refusal there is terminal, and `PhasePrHarnessTest.scala:889` pins it. The asymmetry is real **today**: commit `984c5c0b` on main made a silent forge in the gate return `CiVerdict.Unreadable`, which `PhasePr.scala:428-440` turns into exit 3. (An earlier analysis on an unmerged branch denied this premise; that denial is now out of date.) The blocker is size, not doubt. Typing `fetchPullRequestForBranch` alone touches 13 files (`CommandEnv`, `LiveCommandEnv`, `ForgeGateway`, three clients, `PhasePr`, dashboard `PullRequestReader`, `FakeCommandEnv`, four test files); part 2 adds a `review-state.json` field plus `ReviewStateUpdater`, `BatchImplement` and the 0/1/3 contract docs. Honest total: **4 phases**, above the 3-phase ceiling the unattended loop will run. **1.** This issue specifies a two-part fix (typed transient/permanent error on the tracker port, then a pre-gate budget). A narrower alternative exists: keep `Either[String, A]` and give the duplicate path the same bounded in-process retry the CI gate already uses (~3 attempts, 30s apart, then exit 1). Which do you want built — the narrow retry (~1 phase), the full two-part fix (~4 phases, needing a split into separate issues), or the narrow retry now with the typed error deferred until a real outage proves 90 seconds too thin? **2.** Typing the error on `fetchPullRequestForBranch` changes a signature in the published `core` jar, which the dashboard (`PullRequestReader`) and the kanon plugin both link against. Is breaking that published signature acceptable here, or must the typed error be added without changing existing callers? **3.** Should the typed error cover only `fetchPullRequestForBranch`, or every forge call that currently returns `Either[String, A]` (~84 such returns across the three clients)? The error-handling review this issue cites asked for the general case; the issue text asks for "a small error type ... enough to separate". Not a duplicate: #485 (no-pipeline-yet passes the gate) and #491 (push after reading CI) are different `phase-pr` defects, and #400 is about the batch loop rather than the pre-gate path. Note: an earlier intake reached the same conclusion on 2026-09-03 (commit `b96f2048`, "intake analysis — needs-human") via a CLARIFY marker. That commit is not on main and no branch contains it, so its analysis is orphaned; the question it raised was never answered.

claude added the

needs-input

label

2026-09-15 02:46:23 +00:00

mph commented

2026-10-10 20:10:42 +00:00

Author

Owner