diff --git a/.changes/pr-pipeline/fix-strip-state-on-close-8c41d2e7.md b/.changes/pr-pipeline/fix-strip-state-on-close-8c41d2e7.md new file mode 100644 index 00000000000..68e883e744c --- /dev/null +++ b/.changes/pr-pipeline/fix-strip-state-on-close-8c41d2e7.md @@ -0,0 +1,12 @@ +--- +bump: patch +--- + +### Fixed + +- A closed issue loses its `status: confirmed`, `status: needs-decision`, `status: needs-info` + and `host-run` labels too, not only the queue labels, whoever closed it. Where the lanes App + closes the issue, these come off after the close, so a failed close leaves an open issue in its + state. `attended` stays: only a listed human lifts it. +- The README says that only an issue a listed human opened gives work a Bot filed a spec: a + listed human's comment on, or edit of, a Bot-filed issue never makes it trusted. diff --git a/.github/scripts/work-resolve-issue/report-outcome.mjs b/.github/scripts/work-resolve-issue/report-outcome.mjs index db190bceeae..dd31f44e076 100755 --- a/.github/scripts/work-resolve-issue/report-outcome.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.mjs @@ -695,6 +695,25 @@ export async function stripQueueLabels(github, base, had) { } return removed; } +// The state labels a closed issue no longer needs: triage's and the host +// queue's. Removed only once the issue is closed, so a close that fails leaves +// an open issue in its state. `attended` stays: only a listed human lifts it. +export const STATE_LABELS = ["status: confirmed", "status: needs-decision", "status: needs-info", HOST_RUN_LABEL]; + +// Removes the state labels `had` holds from the closed issue at `base`, and +// returns their names. A 404 counts as removed. +export async function stripStateLabels(github, base, had) { + const present = new Set(had.map((name) => String(typeof name === "string" ? name : name?.name).toLowerCase())); + const removed = STATE_LABELS.filter((name) => present.has(name)); + for (const name of removed) { + await github("DELETE", `${base}/labels/${encodeURIComponent(name)}`).catch((error) => { + if (error?.status !== 404) { + throw error; + } + }); + } + return removed; +} const ROUTE_REMOVES = { intake: ADMISSION_LABELS, park: ADMISSION_LABELS }; const ROUTE_TEXT = { "host-run": `It added \`${HOST_RUN_LABEL}\` and removed \`agent-ready\`: an agent run on a host builds it, and it waits on no decision from a person.`, @@ -2277,9 +2296,11 @@ async function finishStop(github, repo, plan, { bot, humans, runId, limits, lane // A label DELETE is idempotent (a 404 counts as removed), so it is resent // after a 5xx; only a removal that keeps failing leaves the issue open. const resend = (method, path) => github(method, path, undefined, { safeToResend: true }); - const removed = await stripQueueLabels(resend, base, (await github("GET", base))?.labels ?? []); + const had = (await github("GET", base))?.labels ?? []; + const removed = await stripQueueLabels(resend, base, had); await github("PATCH", base, { state: "closed", state_reason: "completed" }, { safeToResend: true }); - console.log(`report-outcome: closed issue #${plan.issueNumber} (${plan.stop}); removed ${removed.length} queue label(s)`); + const states = await stripStateLabels(resend, base, had); + console.log(`report-outcome: closed issue #${plan.issueNumber} (${plan.stop}); removed ${removed.length} queue label(s) and ${states.length} state label(s)`); } } diff --git a/.github/scripts/work-resolve-issue/report-outcome.test.mjs b/.github/scripts/work-resolve-issue/report-outcome.test.mjs index 539de9ddf09..9f72827629e 100644 --- a/.github/scripts/work-resolve-issue/report-outcome.test.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.test.mjs @@ -4809,6 +4809,18 @@ test("main's close removes the queue labels the issue carries, before the close assert.deepEqual(labelWrites(gone.calls), ["DELETE /labels/status%3A%20ready", "DELETE /labels/agent-ready", "PATCH "]); }); +// The state labels come off only after the close, so a failed close never +// leaves an open issue without its state; attended stays. +test("main's close removes the state labels after the close and keeps attended", async () => { + const run = await stopRun({ + reason: "report-delivered: the audit found three stale rows", + routes: [], + issue: withLabels("status: confirmed", "host-run", "attended", "agent-ready"), + }); + assert.equal(run.code, 0); + assert.deepEqual(labelWrites(run.calls), ["DELETE /labels/agent-ready", "PATCH ", "DELETE /labels/status%3A%20confirmed", "DELETE /labels/host-run"]); +}); + // A label DELETE is idempotent, so a 5xx is resent (three tries in all); one // that keeps failing fails the run before the close: the issue stays open with // agent-ready, for work-unblock's sweep. diff --git a/.github/scripts/work-unblock/label-unblocked.mjs b/.github/scripts/work-unblock/label-unblocked.mjs index e13947826ce..60971301e57 100755 --- a/.github/scripts/work-unblock/label-unblocked.mjs +++ b/.github/scripts/work-unblock/label-unblocked.mjs @@ -293,7 +293,7 @@ import { notBornTriaged } from "../../actions/check-born-triaged/check-born-tria import { createGitHub, isTrustedClose as closedByHumanOrMerge, loadTrustedList, paginate, readRerunner } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { writeOutputs } from "../../actions/check-kill-switch/check-kill-switch.mjs"; import { admissionLine, checkWip, searchRateLimit } from "../../actions/check-wip-cap/check-wip-cap.mjs"; -import { clampReset, isLaneBranch, LANE_HOLDS, LANE_WORKFLOWS, parseOutcomeMarker, REFIRED, refireComment as refireRecord, refsHoldMarker, stripQueueLabels } from "../work-resolve-issue/report-outcome.mjs"; +import { clampReset, isLaneBranch, LANE_HOLDS, LANE_WORKFLOWS, parseOutcomeMarker, REFIRED, refireComment as refireRecord, refsHoldMarker, stripQueueLabels, stripStateLabels } from "../work-resolve-issue/report-outcome.mjs"; import { readLimits, readWip, resolveStrandedBackoff, STRANDED_BACKOFF_MAX } from "../work-resolve-issue/limits.mjs"; import { ADMIT_MARKER, admissionFacts, admissionParked, admissionSettled, ATTENDED, authorAdmitted, attendedHold, blockersOf, CONFIRMED, confirmedOnly, decide, deferralLapsed, loadRules, NEEDS_DECISION, openPullsFor, openSubIssues, READY, refusalText, removalStands, sayWhy, SIZE_SPLIT_DONE, SPLIT_PENDING, specOf, stillBlocking, subIssueWait, UNTRUSTED, untrustedHold } from "../intake-triage/admit.mjs"; import { subIssueLeftOpen } from "../lib/done-evidence.mjs"; @@ -920,8 +920,8 @@ async function stripClosed({ github, repo, number, read }) { console.log(`label-unblocked: #${number} is a post-merge fix issue; its queue labels stay`); return 0; } - const removed = await stripQueueLabels(github, base, labelNames(issue)); - console.log(`label-unblocked: #${number} closed; removed ${removed.length} queue label(s)${removed.length > 0 ? `: ${removed.join(", ")}` : ""}`); + const removed = [...(await stripQueueLabels(github, base, labelNames(issue))), ...(await stripStateLabels(github, base, labelNames(issue)))]; + console.log(`label-unblocked: #${number} closed; removed ${removed.length} queue and state label(s)${removed.length > 0 ? `: ${removed.join(", ")}` : ""}`); return 0; } catch (error) { reportError(number, error); @@ -2205,7 +2205,8 @@ async function closeDoneParent({ github, repo, parent, humans, closes, bot, mirr // before the close: a failed removal leaves the parent open. const removed = await stripQueueLabels(github, base, labelNames(parent)); await github("PATCH", base, { state: "closed", state_reason: "completed" }); - console.log(`label-unblocked: split parent #${parent.number} closed as completed; removed ${removed.length} queue label(s)`); + const states = await stripStateLabels(github, base, labelNames(parent)); + console.log(`label-unblocked: split parent #${parent.number} closed as completed; removed ${removed.length} queue label(s) and ${states.length} state label(s)`); } // Takes status: confirmed off `parent` (an open issue as read, with its diff --git a/.github/scripts/work-unblock/label-unblocked.test.mjs b/.github/scripts/work-unblock/label-unblocked.test.mjs index 64803c6e22d..488f2c23add 100644 --- a/.github/scripts/work-unblock/label-unblocked.test.mjs +++ b/.github/scripts/work-unblock/label-unblocked.test.mjs @@ -260,7 +260,7 @@ function world({ if (method === "POST" && (match = route.match(/\/issues\/(\d+)\/labels$/))) { return []; } - if (method === "DELETE" && route.match(/\/issues\/(\d+)\/labels\/(agent-ready|needs-human|admission-deferred|status%3A%20ready|status%3A%20confirmed)$/)) { + if (method === "DELETE" && route.match(/\/issues\/(\d+)\/labels\/(agent-ready|needs-human|admission-deferred|status%3A%20ready|status%3A%20confirmed|status%3A%20needs-decision|status%3A%20needs-info|host-run)$/)) { return []; } const lookup = (number) => @@ -417,6 +417,18 @@ test("a closed issue loses the queue labels it carries, agent-ready last, and ke assert.deepEqual(writes(deferred.calls), [unlabeled("status: ready"), unlabeled("admission-deferred")]); }); +test("a closed issue also loses its state labels, after the queue labels, and keeps attended", async () => { + const { github, calls } = closedSelf(["status: confirmed", "host-run", "attended", "agent-ready", "status: needs-decision", "status: needs-info"]); + assert.equal(await main({ env: envFor(), github }), 0); + assert.deepEqual(writes(calls), [ + unlabeled("agent-ready"), + unlabeled("status: confirmed"), + unlabeled("status: needs-decision"), + unlabeled("status: needs-info"), + unlabeled("host-run"), + ]); +}); + test("a closed issue with no queue label gets no write", async () => { const { github, calls } = closedSelf(["priority: high", "bug"]); assert.equal(await main({ env: envFor(), github }), 0); @@ -4814,6 +4826,20 @@ test("closing a split parent removes the queue labels it carries before the clos ]); }); +// A parked parent carries status: confirmed; its state labels come off only +// after the close, and attended stays. +test("closing a split parent removes its state labels after the close and keeps attended", async () => { + const { github, calls } = splitWorld({ parent: issue(30, { labels: ["agent-ready", "status: confirmed", "host-run", "attended"] }) }); + assert.equal(await sweep({ github }), 0); + assert.deepEqual(writtenPaths(calls), [ + closeParentWrites[0], + `DELETE /repos/${REPO}/issues/30/labels/agent-ready`, + closeParentWrites[1], + `DELETE /repos/${REPO}/issues/30/labels/status%3A%20confirmed`, + `DELETE /repos/${REPO}/issues/30/labels/host-run`, + ]); +}); + // A failed removal stops the close, so the parent stays open with agent-ready // for the next sweep. test("a failed queue-label removal leaves the split parent open", async (t) => { diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 527305a06b3..35ba28e70f9 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -2573,7 +2573,9 @@ request whose closing issue the owner wrote is judged against that issue as its lanes App or any other Bot or App wrote, or one an untrusted account wrote or last edited, still reads `verify:untrusted-spec`, except a size split's sub-issue whose lines are its listed-human parent's task-list lines or those of intake's refinement on it (below), so a lane pull request -never supplies its own spec. +never supplies its own spec. A listed human's comment on such an issue reaches the verifier, but it +never makes the issue trusted, and neither does a listed human's edit of its body. To give work a Bot +filed a spec, a listed human opens an issue that restates it, and the pull request links that one. A trusted edit to the spec after a pass, an issue body edit or a spec comment added or edited, makes the gate refuse that pass `verify:spec-changed`. The resolver then judges the head again: