From bf1591ca2ad629f2936e588ff44edcdf815ba71b Mon Sep 17 00:00:00 2001 From: Sarath Soman Date: Tue, 26 May 2026 01:42:05 +0100 Subject: [PATCH] fix(ship): discharge check uses PR ref presence (gh doesn't surface PR state) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The classifier was checking `ref.state === "MERGED"` against entries in `closedByPullRequestsReferences`, but `gh issue view` does not surface the linked PR's state — the array only carries id/number/url/repository. Result: every discharged sub-issue was misclassified as `closed-orphan`, halting the orchestrator on the first re-fire. Fix: presence of any PR ref on a CLOSED issue is sufficient signal. GitHub only populates `closedByPullRequestsReferences` when a PR's "Closes #N" keyword triggered the close — which implies a merge action happened. If the PR were later reverted, the issue would re-open and the `state === CLOSED` branch wouldn't fire. Discovered when re-firing `ship run 426` after #461 landed — V-1's discharge check (issue #427 closed by merged PR #448) returned `closed-orphan` instead of `discharged`, halting the orchestrator. Updated tests reflect the verbatim `gh` JSON shape (no `state` field on PR refs) and drop the "PR not merged" / "prefers first MERGED" test cases that are no longer reachable. Refs #454. Co-Authored-By: Claude Opus 4.7 --- .../engine/capability-orchestrator.test.ts | 33 ++++++++++--------- ship/src/engine/capability-orchestrator.ts | 32 ++++++++---------- 2 files changed, 31 insertions(+), 34 deletions(-) diff --git a/ship/src/engine/capability-orchestrator.test.ts b/ship/src/engine/capability-orchestrator.test.ts index a29402d..870c7ce 100644 --- a/ship/src/engine/capability-orchestrator.test.ts +++ b/ship/src/engine/capability-orchestrator.test.ts @@ -359,22 +359,31 @@ describe("classifyGhSubIssueState — sub-issue discharge classifier (pramana#45 kind: "open", }); }); - it("returns `discharged` with the merged PR url when CLOSED + linked PR MERGED", () => { + it("returns `discharged` when CLOSED + any linked PR ref (gh's closedByPullRequestsReferences populates only on close-via-PR-keyword)", () => { + // Verbatim shape `gh issue view --json state,closedByPullRequestsReferences` + // returns — the PR ref carries url/number/id/repository but NOT a state field. + // We rely on presence: a non-empty refs array implies the issue was closed + // by a PR's "Closes #N" keyword (which means a merge happened). const parsed = { state: "CLOSED", closedByPullRequestsReferences: [ - { number: 448, state: "MERGED", url: "https://github.com/owner/repo/pull/448" }, + { + id: "PR_kwDORfza0s7fMaua", + number: 448, + url: "https://github.com/lambda-brahman/pramana/pull/448", + repository: { name: "pramana", owner: { login: "lambda-brahman" } }, + }, ], }; expect(classifyGhSubIssueState(parsed)).toEqual({ kind: "discharged", - mergedPrUrl: "https://github.com/owner/repo/pull/448", + mergedPrUrl: "https://github.com/lambda-brahman/pramana/pull/448", }); }); it("falls back to `#N` shorthand when url is missing but number is present", () => { const parsed = { state: "CLOSED", - closedByPullRequestsReferences: [{ number: 448, state: "MERGED" }], + closedByPullRequestsReferences: [{ number: 448 }], }; expect(classifyGhSubIssueState(parsed)).toEqual({ kind: "discharged", mergedPrUrl: "#448" }); }); @@ -385,25 +394,17 @@ describe("classifyGhSubIssueState — sub-issue discharge classifier (pramana#45 kind: "closed-orphan", }); }); - it("returns `closed-orphan` when CLOSED but linked PR is not MERGED", () => { - const parsed = { - state: "CLOSED", - closedByPullRequestsReferences: [{ number: 100, state: "CLOSED" }], - }; - expect(classifyGhSubIssueState(parsed)).toEqual({ kind: "closed-orphan" }); - }); - it("prefers the first MERGED entry when multiple PRs closed the issue", () => { + it("prefers the first PR ref when multiple PRs are linked", () => { const parsed = { state: "CLOSED", closedByPullRequestsReferences: [ - { number: 1, state: "CLOSED" }, - { number: 2, state: "MERGED", url: "https://github.com/owner/repo/pull/2" }, - { number: 3, state: "MERGED", url: "https://github.com/owner/repo/pull/3" }, + { number: 1, url: "https://github.com/owner/repo/pull/1" }, + { number: 2, url: "https://github.com/owner/repo/pull/2" }, ], }; expect(classifyGhSubIssueState(parsed)).toEqual({ kind: "discharged", - mergedPrUrl: "https://github.com/owner/repo/pull/2", + mergedPrUrl: "https://github.com/owner/repo/pull/1", }); }); it("returns `unknown` for malformed input (defensive — orchestrator falls through to spawn)", () => { diff --git a/ship/src/engine/capability-orchestrator.ts b/ship/src/engine/capability-orchestrator.ts index cc6a382..1a4fc7d 100644 --- a/ship/src/engine/capability-orchestrator.ts +++ b/ship/src/engine/capability-orchestrator.ts @@ -349,33 +349,29 @@ export type SubIssueDischargeOutcome = * Pure classifier — exposed for testing without spawning gh. Takes the * parsed JSON shape `gh issue view --json state,closedByPullRequestsReferences` * returns and maps to a SubIssueDischargeOutcome. + * + * Note on the PR `state` field: `gh issue view`'s + * `closedByPullRequestsReferences` array does NOT surface each PR's + * state — only `id` / `number` / `url` / `repository`. We rely instead + * on presence: GitHub only populates this array when a PR's + * "Closes #N" keyword triggered the issue close, which means a merge + * action happened. If the PR were later reverted, the issue would + * re-open and the `state: CLOSED` branch wouldn't fire. So + * `CLOSED + any-PR-ref` is a sound proxy for `discharged`. */ export function classifyGhSubIssueState(parsed: unknown): SubIssueDischargeOutcome { if (!parsed || typeof parsed !== "object") return { kind: "unknown" }; const p = parsed as { state?: unknown; closedByPullRequestsReferences?: unknown }; if (p.state !== "CLOSED") return { kind: "open" }; const closedBy = p.closedByPullRequestsReferences; - if (!Array.isArray(closedBy)) return { kind: "closed-orphan" }; + if (!Array.isArray(closedBy) || closedBy.length === 0) return { kind: "closed-orphan" }; + // Prefer the first ref's `url`; fall back to `#N` synthesized from `number`. for (const ref of closedBy) { - if ( - ref && - typeof ref === "object" && - "state" in ref && - ref.state === "MERGED" && - "url" in ref && - typeof ref.url === "string" - ) { + if (!ref || typeof ref !== "object") continue; + if ("url" in ref && typeof ref.url === "string") { return { kind: "discharged", mergedPrUrl: ref.url }; } - // Fallback: synthesize URL from number if `url` isn't surfaced. - if ( - ref && - typeof ref === "object" && - "state" in ref && - ref.state === "MERGED" && - "number" in ref && - typeof ref.number === "number" - ) { + if ("number" in ref && typeof ref.number === "number") { return { kind: "discharged", mergedPrUrl: `#${ref.number}` }; } }