fix(work-resolve-issue): skip an issue an open pull request already closes

.changes/pr-pipeline/fix-resolve-issue-skip-open-pr-2d0f317a.md

@@ -0,0 +1,7 @@
+---
+bump: patch
+---
+
+### Fixed
+
+- `work-resolve-issue` no longer opens a second PR for an issue that another open PR already closes. A PR can gain its closing keyword while the model runs, after the `issue-prior` check read none, as happened with [#10698](https://github.com/melodic-software/claude-code-plugins/pull/10698) and [#10686](https://github.com/melodic-software/claude-code-plugins/pull/10686). `report-outcome` now reads the issue's open closing PRs again just before it opens a PR, applying the same trust rule as `issue-prior` (opened by the lanes App or a listed human). When one closes the issue, it opens no PR, logs that PR's number and stops `closed-by-open-pr`, a new `retry` route in `stop-routes.json`.

.github/scripts/work-resolve-issue/report-outcome.mjs

@@ -1116,6 +1116,17 @@ export function readBranch(dir, issueNumber, key = "branch") {
   }
 }
 
+// The first of the issue's open closing PRs (closingPulls) in this repository
+// that the lanes App or a listed human opened: anyone can write `Closes #N`, so
+// an outsider's PR would let anyone stop the lane.
+export const trustedCloser = (closing, { repo, bot, humans }) =>
+  closing.find(
+    ({ author, repository }) =>
+      String(repository?.nameWithOwner ?? "").toLowerCase() === repo.toLowerCase() &&
+      ((author?.__typename === "Bot" && (author.login === bot || `${author.login}[bot]` === bot)) ||
+        (author?.__typename === "User" && humans.has(author.databaseId))),
+  );
+
 // Whether an earlier run already did the issue's work, from facts read before
 // the resolve-issue mint:
 //   stop   a lanes-App PR from a lane branch is open, or an open PR in this
@@ -1163,12 +1174,7 @@ export function findPrior({ issueNumber, repo, bot, humans, branches, closing, m
   if (open) {
     return { action: "stop", reason: `the lanes App's PR from ${open.name} is open` };
   }
-  const trusted = closing.find(
-    ({ author, repository }) =>
-      String(repository?.nameWithOwner ?? "").toLowerCase() === repo.toLowerCase() &&
-      ((author?.__typename === "Bot" && (author.login === bot || `${author.login}[bot]` === bot)) ||
-        (author?.__typename === "User" && humans.has(author.databaseId))),
-  );
+  const trusted = trustedCloser(closing, { repo, bot, humans });
   if (trusted) {
     return { action: "stop", reason: `open PR #${trusted.number} closes the issue` };
   }
@@ -1780,23 +1786,49 @@ export async function main({ env = process.env, fetch = globalThis.fetch, now =
           (candidate) => candidate.user?.login === bot,
         );
       let pull = await findOpen();
-      if (!pull) {
-        const fields = { title: plan.title, body: plan.body, head: plan.branch, base: base || (await github("GET", repo)).default_branch, draft: false };
-        try {
-          pull = await github("POST", `${repo}/pulls`, fields);
-        } catch (error) {
-          // A create that failed on the network may have opened the PR, so
-          // it is read back first, and created once more only when absent.
-          if (!networkError(error)) {
-            throw error;
+      // A PR that closes the issue may have opened, or gained its closing
+      // keyword, while the model ran, after the earlier-work check
+      // (--find-prior) read none, so the closers are read again just before
+      // the open. A failed read throws, like this script's other reads, and
+      // opens no PR.
+      const closer = pull
+        ? undefined
+        : trustedCloser(await closingPulls(github, { owner, name: env.GITHUB_REPOSITORY.split("/")[1], number: issueNumber }), {
+            repo: env.GITHUB_REPOSITORY,
+            bot,
+            humans: listedHumans(),
+          });
+      if (closer) {
+        console.log(`report-outcome: open PR #${closer.number} already closes issue #${issueNumber}, so no PR opens from ${plan.branch}`);
+        plan = routeStop("closed-by-open-pr", {
+          outcome: stopped(`Open PR #${closer.number} already closes this issue, so no second PR opened from \`${plan.branch}\`.`),
+          issueNumber,
+          branch: plan.branch,
+          humanLabel,
+          runUrl,
+          runId,
+        });
+      } else {
+        if (!pull) {
+          const fields = { title: plan.title, body: plan.body, head: plan.branch, base: base || (await github("GET", repo)).default_branch, draft: false };
+          try {
+            pull = await github("POST", `${repo}/pulls`, fields);
+          } catch (error) {
+            // A create that failed on the network may have opened the PR, so
+            // it is read back first, and created once more only when absent.
+            if (!networkError(error)) {
+              throw error;
+            }
+            pull = (await findOpen()) ?? (await github("POST", `${repo}/pulls`, fields));
           }
-          pull = (await findOpen()) ?? (await github("POST", `${repo}/pulls`, fields));
         }
-      }
-      // A plan with a tip opens only at it: a PR whose head moved off the
-      // checked tip is closed, and the issue goes to a person. Closing a
-      // closed PR changes nothing, so the close is resent.
-      if (plan.tip && pull.head?.sha !== plan.tip) {
+        // A plan with a tip opens only at it: a PR whose head moved off the
+        // checked tip is closed, and the issue goes to a person. Closing a
+        // closed PR changes nothing, so the close is resent.
+        if (!plan.tip || pull.head?.sha === plan.tip) {
+          console.log(`report-outcome: opened PR #${pull.number} for issue #${issueNumber}`);
+          return 0;
+        }
         await github("PATCH", `${repo}/pulls/${pull.number}`, { state: "closed" }, { safeToResend: true });
         console.log(`report-outcome: closed PR #${pull.number}: its head is not ${plan.tip}, the tip the runner checked`);
         plan = routeStop("pr-head-moved", {
@@ -1809,9 +1841,6 @@ export async function main({ env = process.env, fetch = globalThis.fetch, now =
           runUrl,
           runId,
         });
-      } else {
-        console.log(`report-outcome: opened PR #${pull.number} for issue #${issueNumber}`);
-        return 0;
       }
     }
     await finishStop(github, repo, plan, { bot, humans: listedHumans(), runId, limits });

.github/scripts/work-resolve-issue/report-outcome.test.mjs

@@ -761,7 +761,9 @@ function fakeFetch(routes) {
     const method = init.method ?? "GET";
     const path = new URL(url).pathname;
     calls.push({ method, path, body: init.body ? JSON.parse(init.body) : undefined });
-    const route = routes.find(([m, p]) => m === method && p.test(path));
+    // An issue no open PR closes, unless a route says otherwise.
+    const none = path === "/graphql" ? [method, /./, 200, { data: { repository: { issue: { closedByPullRequestsReferences: { nodes: [], pageInfo: { hasNextPage: false } } } } } }] : null;
+    const route = routes.find(([m, p]) => m === method && p.test(path)) ?? none;
     const [, , status, body] = route ?? [method, /./, 404, { message: "Not Found" }];
     return { ok: status < 300, status, json: async () => body, text: async () => JSON.stringify(body) };
   };
@@ -1114,7 +1116,7 @@ test("main on done with Refs opens the PR and writes nothing to the issue", asyn
         ["POST", /\/pulls$/, 201, { number: 90 }],
       ]);
       assert.equal(await main({ env: env(dir), fetch }), 0);
-      const writes = calls.filter((c) => c.method !== "GET").map((c) => `${c.method} ${c.path}`);
+      const writes = calls.filter((c) => c.method !== "GET" && c.path !== "/graphql").map((c) => `${c.method} ${c.path}`);
       assert.deepEqual(writes, ["POST /repos/o/r/pulls"]);
       assert.ok(calls.find((c) => c.path.endsWith("/pulls") && c.method === "POST").body.body.startsWith("Refs: #77\n"));
     },
@@ -3422,3 +3424,95 @@ test("scan-reuse.mjs prints failed when the scan cannot finish, and refuses argu
     rmSync(root, { recursive: true, force: true });
   }
 });
+
+// #10698 (run 40732936415): the earlier-work check read no closing PR at
+// 17:11, a listed human's open #10686 gained a closing keyword at 17:13, and the
+// lane still opened a second PR at 17:19. report-outcome reads the closers
+// again before the open.
+async function openWithClosers(nodes, { own = [] } = {}) {
+  return withAsyncDir(
+    {
+      "work-outcome.json": JSON.stringify(done),
+      "lane.json": JSON.stringify({ branch: BRANCH }),
+      "runner-checks": checksFile(),
+      "trusted.json": TRUSTED_LIST,
+    },
+    async (dir) => {
+      const { fetch, calls } = fakeFetch([
+        ["GET", /\/issues\/77$/, ...ISSUE.slice(1)],
+        branchTip(),
+        ["GET", /\/repos\/o\/r$/, 200, { default_branch: "main" }],
+        ["GET", /\/compare\//, 200, { files: [{ filename: "README.md" }] }],
+        ["GET", /\/pulls$/, 200, own],
+        ["POST", /^\/graphql$/, 200, graphql(nodes)],
+        ["POST", /\/pulls$/, 201, { number: 90 }],
+        ...HISTORY,
+        ["POST", /\/issues\/77\/labels$/, 200, []],
+        ["DELETE", /\/labels\//, 200, []],
+        ["POST", /\/issues\/77\/comments$/, 201, {}],
+      ]);
+      const lines = [];
+      const log = console.log;
+      console.log = (line) => lines.push(line);
+      try {
+        const code = await main({ env: { ...env(dir), TRUSTED_ACTORS: join(dir, "trusted.json") }, fetch });
+        return { code, calls, lines };
+      } finally {
+        console.log = log;
+      }
+    },
+  );
+}
+const opened = (calls) => calls.some((c) => c.method === "POST" && c.path === "/repos/o/r/pulls");
+
+test("main on done opens no PR when a listed human's open PR already closes the issue, and keeps agent-ready", async () => {
+  const { code, calls, lines } = await openWithClosers([{ ...closer("User", "kyle-sexton", OWNER_ID), number: 10686 }]);
+  assert.equal(code, 0);
+  assert.ok(!opened(calls), JSON.stringify(calls));
+  assert.ok(lines.some((line) => line.includes("#10686")), lines.join("\n"));
+  assert.ok(!calls.some((c) => c.path.endsWith("/issues/77/labels") || c.method === "DELETE"));
+  const comment = calls.find((c) => c.method === "POST" && c.path.endsWith("/issues/77/comments")).body.body;
+  assert.ok(comment.startsWith("<!-- work-resolve-issue outcome=retry run=123 -->"), comment);
+  assert.match(comment, /`closed-by-open-pr`/);
+  // Quoted as the stop's words, so its `#` is neutralized like any run text.
+  assert.match(comment, /PR .10686 already closes this issue/);
+  assert.ok(comment.includes(BRANCH), comment);
+});
+
+test("main on done still opens the PR when no open PR closes the issue, or only an outsider's does", async () => {
+  for (const nodes of [[], [closer("User", "stranger", 42)], [closer("User", "kyle-sexton", OWNER_ID, "fork/r")]]) {
+    const { code, calls } = await openWithClosers(nodes);
+    assert.equal(code, 0, JSON.stringify(nodes));
+    assert.ok(opened(calls), JSON.stringify(nodes));
+    assert.ok(!calls.some((c) => c.path.endsWith("/issues/77/comments")), JSON.stringify(nodes));
+  }
+});
+
+// A rerun of the report finds the lanes App's own PR from this branch, which
+// closes the issue itself, so it is never read as a second PR.
+test("main on done keeps the lanes App's own open PR from the branch without reading the closers", async () => {
+  const { code, calls } = await openWithClosers([closer("Bot", BOT)], { own: [{ number: 90, user: { login: BOT }, head: { sha: CHECKED_TIP } }] });
+  assert.equal(code, 0);
+  assert.ok(!calls.some((c) => c.path === "/graphql"));
+  assert.ok(!opened(calls));
+  assert.ok(!calls.some((c) => c.path.endsWith("/issues/77/comments")));
+});
+
+test("main opens no PR when the closing PRs cannot be read", async () => {
+  await withAsyncDir(
+    { "work-outcome.json": JSON.stringify(done), "lane.json": JSON.stringify({ branch: BRANCH }), "runner-checks": checksFile() },
+    async (dir) => {
+      const { fetch, calls } = fakeFetch([
+        ["GET", /\/issues\/77$/, ...ISSUE.slice(1)],
+        branchTip(),
+        ["GET", /\/repos\/o\/r$/, 200, { default_branch: "main" }],
+        ["GET", /\/compare\//, 200, { files: [{ filename: "README.md" }] }],
+        ["GET", /\/pulls$/, 200, []],
+        ["POST", /^\/graphql$/, 200, { errors: [{ message: "boom" }] }],
+        ["POST", /\/pulls$/, 201, { number: 90 }],
+      ]);
+      assert.equal(await main({ env: env(dir), fetch }), 1);
+      assert.ok(!opened(calls));
+    },
+  );
+});

.github/scripts/work-resolve-issue/stop-routes.json

@@ -16,6 +16,7 @@
   "check-failed": { "route": "retry", "why": "a check the runner ran on the branch failed or could not be read, so the next run continues the branch" },
   "required-undone": { "route": "retry", "why": "the run left a required check undone, so the next run continues the branch" },
   "pr-head-moved": { "route": "retry", "why": "the PR opened at a head other than the tip the runner checked, a race, not a finding about the issue" },
+  "closed-by-open-pr": { "route": "retry", "why": "an open PR the lanes App or a listed human opened already closes the issue, a race with the earlier-work check, not a finding about the issue; while it stays open the next run stops before any model turn" },
   "turn-cap-spent": { "route": "fail", "why": "the run reached its turn cap with no restart left" },
   "retry-spent": { "route": "fail", "why": "the lane has retried this issue as many times as limits.stop-retries allows" },
   "pushback": { "route": "hold", "why": "the issue leaves a decision open, an intent question only a person answers" },

docs/conventions/pr-pipeline/pr-run-activity.md

@@ -2138,6 +2138,12 @@ step under the escalation marker and an `outcome=<reason>` marker. What changes
   record) is missing or not `clean`. The summary never decides the hold or reaches a status:
   `report-statuses` then posts only `pipeline/report-outcome` on that branch, so any statuses the
   earlier run posted on the same head stand.
+- Just before it opens a PR (a `done` run's or a reuse's), and only when the lanes App has none
+  open from the branch, `report-outcome` reads the issue's open closing PRs again, with the same
+  trust rule as `issue-prior`: a closing keyword can land on another PR while the model runs. When
+  one closes the issue, it opens no PR, logs that PR's number and stops `closed-by-open-pr`, which
+  routes to `retry`, so the next run stops in `issue-prior` while that PR stays open. A failed read
+  fails the job and opens no PR.
 - No check run is posted: there is no PR head. The lane's `report-statuses` job (in
   `work-resolve-issue.yml`, the lane's only `statuses: write`, on its own `GITHUB_TOKEN`) posts a
   `pipeline/<activity>` commit status on the lane branch's head for `resolve-issue` and