label-hygiene: isolate a refused label instead of abandoning the sweep (#1418)

.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)) {

.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=<name>` 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 `<number>/<label>` to a refusal: `stderr` is the status line `gh` writes, `stdout`
+// the JSON body it writes, and `remove: false` keeps the label on the item like a real refusal does.
+const fakeGh = (items, failures = {}) => {
   const store = items.map(item => ({ ...item, labels: item.labels }))
   const calls = []
   const gh = (...args) => {
@@ -21,7 +23,14 @@ const fakeGh = (items) => {
       const m = /\/issues\/(\d+)\/labels\/(.+)$/.exec(path)
       const item = store.find(i => String(i.number) === m[1])
       const name = decodeURIComponent(m[2])
-      item.labels = item.labels.filter(l => l.name !== name)
+      const failure = failures[`${m[1]}/${name}`]
+      if (failure?.remove !== false) item.labels = item.labels.filter(l => l.name !== name)
+      if (failure) {
+        const error = new Error(failure.stderr)
+        error.stderr = failure.stderr
+        error.stdout = failure.stdout ?? ''
+        throw error
+      }
       return ''
     }
     const url = new URL('https://api.github.com/' + path)
@@ -41,6 +50,15 @@ const fakeGh = (items) => {
 
 const run = (gh, apply) => main({ gh, env: { GH_REPO: 'ThinkFlowLab/FlyBot' }, repo: 'ThinkFlowLab/FlyBot', apply })
 
+// Capture what the run prints: the failure line and the refusal's response body are the evidence, so
+// the tests pin them the way the live log reads.
+const capture = (fn) => {
+  const lines = []
+  const log = console.log
+  console.log = (...args) => lines.push(args.join(' '))
+  try { return { result: fn(), lines } } finally { console.log = log }
+}
+
 test('#738: a dry run prints what it would do and removes nothing', () => {
   const gh = fakeGh([issue(602, 'closed', ['agent-working', 'bug', 'risk:T1'])])
   const r = run(gh, false)
@@ -131,3 +149,72 @@ test('#1401: closing an item is a trigger, and the daily sweep stays', () => {
   assert.match(wf, /^ {2}cancel-in-progress: false$/m, 'a close must not cancel the sweep it needs')
   assert.match(wf, /run: node \.github\/scripts\/label-hygiene\.mjs --apply$/m, 'the close event runs the same --apply as the sweep')
 })
+
+test('#1418: a 422 is a refusal, not "already gone": the sweep continues and the read-back keeps the run red', () => {
+  // The live shape from #1418: the label is still on the item after a 422 (job 122128075345, #1359).
+  const gh = fakeGh([
+    issue(1356, 'closed', ['agent-working']),
+    issue(1359, 'closed', ['agent-pr']),
+    issue(1400, 'closed', ['agent-working', 'bug']),
+  ], {
+    '1356/agent-working': {
+      remove: false,
+      stderr: 'gh: Validation Failed (HTTP 422)',
+      stdout: '{"message":"Validation Failed","errors":[{"resource":"Issue","field":"labels","code":"unprocessable"}]}',
+    },
+  })
+  const { result: r, lines } = capture(() => run(gh, true))
+  assert.equal(gh.deletes().length, 3, 'the first refusal must not abandon the items behind it')
+  assert.deepEqual(gh.store.find(i => i.number === 1356).labels.map(l => l.name), ['agent-working'], 'a 422 leaves its label on the item')
+  assert.deepEqual(gh.store.filter(i => i.number !== 1356).map(i => i.labels.map(l => l.name)), [[], ['bug']], 'the other items are still cleaned')
+  assert.deepEqual(r.skipped, [], 'a 422 is never reported as already gone')
+  assert.deepEqual(r.failed.map(f => [f.number, f.name]), [[1356, 'agent-working']])
+  assert.equal(r.ok, false, 'the label is still stale at the read-back')
+  const text = lines.join('\n')
+  assert.match(text, /label-hygiene: FAILED #1356 agent-working: Validation Failed \(HTTP 422\)/)
+  assert.match(text, /"message":"Validation Failed"/, 'the response body is printed, not only the status line')
+  assert.doesNotMatch(text, /label-hygiene: skip #1356/)
+  assert.match(text, /label-hygiene: FAILED: #1356 \(agent-working:/, 'the failed summary is printed before the read-back')
+  assert.match(text, /STILL STALE: #1356 \(agent-working\)/)
+})
+
+test('#1418: a 404 means the label is already gone: one skip line and a clean board is green', () => {
+  const gh = fakeGh([
+    issue(1356, 'closed', ['agent-working']),
+    issue(1359, 'closed', ['agent-pr']),
+  ], {
+    '1356/agent-working': {
+      stderr: 'gh: Not Found (HTTP 404)',
+      stdout: '{"message":"Not Found","documentation_url":"https://docs.github.com/rest"}',
+    },
+  })
+  const { result: r, lines } = capture(() => run(gh, true))
+  assert.equal(gh.deletes().length, 2)
+  assert.deepEqual(r.skipped.map(s => [s.number, s.name]), [[1356, 'agent-working']])
+  assert.deepEqual(r.failed, [])
+  assert.equal(r.ok, true, 'a label that was gone before the call does not fail the sweep')
+  const text = lines.join('\n')
+  assert.match(text, /label-hygiene: skip #1356 agent-working: already gone/)
+  assert.doesNotMatch(text, /STILL STALE/)
+})
+
+test('#1418: an unexpected write failure is collected, the rest is cleaned, and the run names it and exits non-zero', () => {
+  const gh = fakeGh([1354, 1356, 1358].map(n => issue(n, 'closed', ['agent-working', 'bug'])), {
+    '1356/agent-working': {
+      remove: false,
+      stderr: 'gh: Server Error (HTTP 500)',
+      stdout: '{"message":"Server Error"}',
+    },
+  })
+  const { result: r, lines } = capture(() => run(gh, true))
+  assert.equal(gh.deletes().length, 3)
+  assert.deepEqual(r.failed.map(f => [f.number, f.name]), [[1356, 'agent-working']])
+  assert.deepEqual(r.skipped, [])
+  assert.equal(r.ok, false)
+  assert.deepEqual(r.after.map(i => i.number), [1356], 'the label that really failed stays on the board')
+  assert.deepEqual(gh.store.filter(i => i.number !== 1356).map(i => i.labels.map(l => l.name)), [['bug'], ['bug']])
+  const text = lines.join('\n')
+  assert.match(text, /FAILED #1356 agent-working: Server Error \(HTTP 500\)/)
+  assert.match(text, /label-hygiene: FAILED: #1356 \(agent-working:/)
+  assert.match(text, /STILL STALE: #1356 \(agent-working\)/)
+})

changelog.d/1418-label-hygiene-resilient-apply.fixed.md

@@ -0,0 +1 @@
+The label-hygiene sweep no longer stops at the first label it cannot remove. A `404` on one DELETE means that label was already gone and is skipped; any other refusal — including the `422` that the live `#1356`/`#1359` runs returned, with the label still on the item — is collected together with the response body the request answered, the items behind it are still cleaned, and the run exits non-zero at the end naming what failed. The read-back after the sweep stays the criterion. ([#1418](https://github.com/ThinkFlowLab/FlyBot/issues/1418))