diff --git a/.github/scripts/label-hygiene.mjs b/.github/scripts/label-hygiene.mjs index 114f37ac..5c5d2254 100644 --- a/.github/scripts/label-hygiene.mjs +++ b/.github/scripts/label-hygiene.mjs @@ -39,6 +39,23 @@ export function plan(pages) { const realGh = (...a) => execFileSync('gh', a, { encoding: 'utf8', maxBuffer: 16 << 20 }) +// What a refused DELETE says, in the order the log needs it: the status line `gh` writes to stderr, +// then the JSON body it writes to stdout (#1418). A bare status line cannot tell a refused request +// apart from a race, so the body is kept with the item that hit it. +const describe = (error) => { + const status = String(error?.stderr ?? '') + .split('\n').map(line => line.trim()).filter(Boolean).pop() + ?.replace(/^gh:\s*/, '') + const body = String(error?.stdout ?? '').trim().replace(/\s+/g, ' ') + const line = status ?? String(error?.message ?? error) + return body ? `${line} — ${body}` : line +} + +// A 404 is the one benign answer: the label was already gone before this call. A 422 is a refusal — +// the live #1356/#1359 runs show the label still on the item afterwards — so it must not be read as +// "already gone"; it goes to `failed` like any other write failure. +const alreadyGone = (error) => /\(HTTP 404\)|Label does not exist/.test(String(error?.stderr ?? error?.message ?? error)) + // `gh` and `env` are injectable so the whole run can be driven without the network, the way // merge-gate.mjs is tested. export function main({ gh = realGh, env = process.env, repo: repoOption, apply: applyOption } = {}) { @@ -54,23 +71,45 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, apply: const items = plan(pages) const labels = items.reduce((n, item) => n + item.stale.length, 0) console.log(`label-hygiene: before: ${items.length} item(s) / ${labels} label(s) not open`) + // Each removal stands on its own: one refused label is reported and the items behind it are still + // cleaned (#1418). Before this, the first throw escaped main() and the run processed only 1 item. + const skipped = [] + const failed = [] for (const item of items) { for (const name of item.stale) console.log(`#${item.number} ${item.title} -${name}`) if (!apply) continue - for (const name of item.stale) gh('api', '-X', 'DELETE', `repos/${repo}/issues/${item.number}/labels/${encodeURIComponent(name)}`) + for (const name of item.stale) { + try { + gh('api', '-X', 'DELETE', `repos/${repo}/issues/${item.number}/labels/${encodeURIComponent(name)}`) + } catch (error) { + if (alreadyGone(error)) { + skipped.push({ number: item.number, name }) + console.log(`label-hygiene: skip #${item.number} ${name}: already gone`) + continue + } + const reason = describe(error) + failed.push({ number: item.number, name, reason }) + console.log(`label-hygiene: FAILED #${item.number} ${name}: ${reason}`) + } + } } if (!apply) { console.log('label-hygiene: dry run; add --apply to remove these labels') - return { ok: items.length === 0 || !apply, items, applied: false } + return { ok: items.length === 0 || !apply, items, skipped, failed, applied: false } } + // Print the collected failures before the read-back, so the line survives the normal case where the + // refused label is still on the item and the read-back answers first. + if (failed.length) console.log(`label-hygiene: FAILED: ${failed.map(f => `#${f.number} (${f.name}: ${f.reason})`).join(', ')}`) // Read back rather than assume: the run reports its own result so the cleanup is verified, not trusted. const after = plan(STALE_LABELS.flatMap(name => list(`repos/${repo}/issues?state=closed&labels=${encodeURIComponent(name)}&per_page=100`))) console.log(`label-hygiene: after: ${after.length} item(s)`) if (after.length) { console.log(`label-hygiene: STILL STALE: ${after.map(item => `#${item.number} (${item.stale.join(', ')})`).join(', ')}`) - return { ok: false, items, after, applied: true } + return { ok: false, items, after, skipped, failed, applied: true } } - return { ok: true, items, after, applied: true } + // A write that was refused keeps the run red even if another actor cleared the label meanwhile: this + // sweep did not do the work it set out to do, and the log names it. A 404 is skipped, not a failure. + return { ok: failed.length === 0, items, after, skipped, failed, applied: true } } if (process.argv[1] === fileURLToPath(import.meta.url)) { diff --git a/.github/scripts/label-hygiene.test.mjs b/.github/scripts/label-hygiene.test.mjs index e30a0a37..1f6fd724 100644 --- a/.github/scripts/label-hygiene.test.mjs +++ b/.github/scripts/label-hygiene.test.mjs @@ -11,7 +11,9 @@ const issue = (number, state, labels, title = `Item ${number}`) => // A fake API over a page store: `GET repos/x/y/issues?state=closed&labels=` filters like GitHub // (state and one label), and a DELETE removes that label from the item. Every call is recorded. -const fakeGh = (items) => { +// `failures` maps `/