diff --git a/.changes/pr-pipeline/fix-resolve-issue-skip-open-pr-2d0f317a.md b/.changes/pr-pipeline/fix-resolve-issue-skip-open-pr-2d0f317a.md new file mode 100644 index 00000000000..7356e0de033 --- /dev/null +++ b/.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`. diff --git a/.github/scripts/work-resolve-issue/report-outcome.mjs b/.github/scripts/work-resolve-issue/report-outcome.mjs index 2bba3ffe703..8741302eede 100755 --- a/.github/scripts/work-resolve-issue/report-outcome.mjs +++ b/.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 }); diff --git a/.github/scripts/work-resolve-issue/report-outcome.test.mjs b/.github/scripts/work-resolve-issue/report-outcome.test.mjs index 9a1af943712..c17622231d3 100644 --- a/.github/scripts/work-resolve-issue/report-outcome.test.mjs +++ b/.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(""), 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)); + }, + ); +}); diff --git a/.github/scripts/work-resolve-issue/stop-routes.json b/.github/scripts/work-resolve-issue/stop-routes.json index 9fe582b0441..84d628e0474 100644 --- a/.github/scripts/work-resolve-issue/stop-routes.json +++ b/.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" }, diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 4fb9737af77..908d2de17cb 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -2138,6 +2138,12 @@ step under the escalation marker and an `outcome=` 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/` commit status on the lane branch's head for `resolve-issue` and