Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 36 additions & 6 deletions .github/scripts/work-resolve-issue/report-outcome.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,28 @@ const ROUTE_LABEL = { "host-run": HOST_RUN_LABEL, attended: ATTENDED_LABEL, park
const KEPT = new Set(["retry", "wait"]);
// The routes whose stops are counted against a cap: past it, retry-spent's.
const COUNTED = new Set([...KEPT, "intake"]);
const ADMISSION_LABELS = ["agent-ready", "status: ready", "admission-deferred"];
// The queue labels: admission's, which stripQueueLabels removes from a closed
// issue and from one its lane PR handed back to triage.
export const ADMISSION_LABELS = ["agent-ready", "status: ready", "admission-deferred"];
// Agent-ready last: work-unblock's sweep lists open agent-ready issues, so a
// failed write leaves the issue for it. A caller that closes the issue itself
// strips first, so the issue is still open when a write fails.
const QUEUE_LABELS = [...ADMISSION_LABELS.filter((name) => name !== "agent-ready"), "agent-ready"];

// Removes the queue labels `had` holds from the issue at `base`, and returns
// their names. A 404 is a label already gone, so it counts as removed.
export async function stripQueueLabels(github, base, had) {
const present = new Set(had.map((name) => String(typeof name === "string" ? name : name?.name).toLowerCase()));
const removed = QUEUE_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.`,
Expand Down Expand Up @@ -1886,7 +1907,7 @@ function api({ fetch: rawFetch, apiUrl, token, sleep }) {
...(body === undefined ? {} : { body: JSON.stringify(body) }),
});
if (!response.ok) {
throw new Error(`${method} ${path} answered ${response.status}`);
throw Object.assign(new Error(`${method} ${path} answered ${response.status}`), { status: response.status });
}
return response.status === 204 ? null : response.json();
};
Expand Down Expand Up @@ -2127,7 +2148,7 @@ const openPull = (github, repo, pr, { bot, humans }) =>
// carry over: intake triage re-judged it before admitting the issue again. A
// retry or wait under the cap posts its comment once per run (a rerun of this
// report finds its own marker) and keeps agent-ready. A close comments,
// removes agent-ready, then closes the issue as completed.
// removes the queue labels, then closes the issue as completed.
async function finishStop(github, repo, plan, { bot, humans, runId, limits, lane = {}, verifyClose = "" }) {
const waited = plan.action === "wait" ? await openPull(github, repo, plan.pr, { bot, humans }) : "";
if (plan.action === "wait" && waited !== "open") {
Expand Down Expand Up @@ -2197,10 +2218,19 @@ async function finishStop(github, repo, plan, { bot, humans, runId, limits, lane
const base = `${repo}/issues/${plan.issueNumber}`;
await sayWhy(github, repo.slice("/repos/".length), base, (await read()).comments, refusalText(plan.admission), bot);
}
await applyHandBack(github, repo, plan);
// A close leaves its labels to stripQueueLabels below, which throws on a
// failed removal where applyHandBack only logs it.
await applyHandBack(github, repo, plan.action === "close" ? { ...plan, removeLabels: [] } : plan);
if (plan.action === "close") {
await github("PATCH", `${repo}/issues/${plan.issueNumber}`, { state: "closed", state_reason: "completed" }, { safeToResend: true });
console.log(`report-outcome: closed issue #${plan.issueNumber} (${plan.stop})`);
const base = `${repo}/issues/${plan.issueNumber}`;
// The App's close starts no work-unblock close run, so the queue labels
// come off here, before the close: a failed removal leaves the issue open.
// 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 ?? []);
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)`);
}
}

Expand Down
49 changes: 47 additions & 2 deletions .github/scripts/work-resolve-issue/report-outcome.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -4380,12 +4380,12 @@ test("route close: report-delivered retries on a resumed branch when the run com

// main against a fake API for the routes whose effect needs a read first.
// `trusted` adds the trusted-actor list, whose humans' pull requests a wait accepts.
const stopRun = async ({ reason, summary = "", routes, trusted = false, extraEnv = {}, extraFiles = {} }) => {
const stopRun = async ({ reason, summary = "", routes, trusted = false, issue = ISSUE[2], extraEnv = {}, extraFiles = {} }) => {
let result;
const files = { "work-outcome.json": JSON.stringify({ exit: "stopped", reason, summary }), ...(trusted && { "trusted.json": TRUSTED_LIST }), ...extraFiles };
await withAsyncDir(files, async (dir) => {
const { fetch, calls } = fakeFetch([
["GET", /\/issues\/77$/, ...ISSUE.slice(1)],
["GET", /\/issues\/77$/, 200, issue],
...routes,
["GET", /\/issues\/77\/comments/, 200, []],
["GET", /\/issues\/77\/events/, 200, []],
Expand Down Expand Up @@ -4715,6 +4715,51 @@ test("main posts a delivered report and closes the issue", async () => {
assert.deepEqual(run.calls.find((c) => c.method === "PATCH").body, { state: "closed", state_reason: "completed" });
});

// The App's close starts no work-unblock close run, so the queue labels the
// issue carries come off before the close, agent-ready last.
const withLabels = (...names) => ({ ...ISSUE[2], labels: names.map((name) => ({ name })) });
const labelWrites = (calls) => calls.filter((c) => c.method === "DELETE" || c.method === "PATCH").map((c) => `${c.method} ${c.path.replace(/^.*\/issues\/77/, "")}`);
test("main's close removes the queue labels the issue carries, before the close and agent-ready last", async () => {
const reason = "report-delivered: the audit found three stale rows";
const run = await stopRun({ reason, routes: [], issue: withLabels("status: ready", "admission-deferred", "agent-ready", "priority: low") });
assert.equal(run.code, 0);
assert.deepEqual(labelWrites(run.calls), ["DELETE /labels/status%3A%20ready", "DELETE /labels/admission-deferred", "DELETE /labels/agent-ready", "PATCH "]);
// None on the issue: the close is the only write.
const bare = await stopRun({ reason, routes: [], issue: withLabels("priority: low") });
assert.equal(bare.code, 0);
assert.deepEqual(labelWrites(bare.calls), ["PATCH "]);
// A label already gone (404) counts as removed.
const gone = await stopRun({ reason, routes: [["DELETE", /\/labels\/status%3A%20ready$/, 404, {}]], issue: withLabels("status: ready", "agent-ready") });
assert.equal(gone.code, 0);
assert.deepEqual(labelWrites(gone.calls), ["DELETE /labels/status%3A%20ready", "DELETE /labels/agent-ready", "PATCH "]);
});

// 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.
const FAILED_STRIP = Array(3).fill("DELETE /labels/status%3A%20ready");
test("main's close resends a failed queue-label removal, then leaves the issue open", async () => {
const run = await stopRun({
reason: "report-delivered: the audit found three stale rows",
routes: [["DELETE", /\/labels\/status%3A%20ready$/, 502, {}]],
issue: withLabels("status: ready", "agent-ready"),
});
assert.notEqual(run.code, 0);
assert.deepEqual(labelWrites(run.calls), FAILED_STRIP);
});

// A verified-done parent closes through the same route, so its queue labels
// come off first too, and a failed removal leaves it open.
test("main's verified-done close removes the queue labels before the close, and stays open when one fails", async () => {
const opts = { reason: VERIFIED, extraEnv: { VERIFY_CLOSE: "80,81" }, issue: withLabels("status: ready", "agent-ready") };
const run = await stopRun({ ...opts, routes: [subs(doneSub(80), doneSub(81))] });
assert.equal(run.code, 0);
assert.deepEqual(labelWrites(run.calls), ["DELETE /labels/status%3A%20ready", "DELETE /labels/agent-ready", "PATCH "]);
const failed = await stopRun({ ...opts, routes: [subs(doneSub(80), doneSub(81)), ["DELETE", /\/labels\/status%3A%20ready$/, 502, {}]] });
assert.notEqual(failed.code, 0);
assert.deepEqual(labelWrites(failed.calls), FAILED_STRIP);
});

test("retries count the App's retry markers since a listed human last added agent-ready, not this run's", () => {
const marker = (outcome, run, at) => ({ user: { login: "melodic-automation-lanes[bot]" }, body: `<!-- work-resolve-issue outcome=${outcome} run=${run} -->`, created_at: at });
const comments = [marker("retry", "1", "2026-10-17T01:00:00Z"), marker("retry", "2", "2026-10-17T03:00:00Z"), marker("turn-cap", "3", "2026-10-17T04:00:00Z"), marker("retry", "555", "2026-10-17T05:00:00Z")];
Expand Down
Loading
Loading