From 162d4bb5462aee91215c5be82621b72a86c27dfb Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Tue, 27 Oct 2026 17:42:16 +0800 Subject: [PATCH] Run label hygiene when an item closes, not only at the daily sweep A status label answers "what is happening right now", so it must not outlive the item. The 19:30Z sweep left up to a day where a closed issue still read as in flight; the close is the moment the label stops being true. The same --apply now also runs on `issues: closed` and on `pull_request_target: closed` -- a `pull_request` run from a fork is read-only, so it could not remove the label it came for. Two tests pin the triggers, the unchanged permissions and the keep-list, including hold. Fixes #1401 Docs-not-needed: workflow trigger plus its tests; no architecture or subsystem ownership change. --- .github/scripts/label-hygiene.test.mjs | 25 +++++++++++++++++++++++++ .github/workflows/label-hygiene.yml | 10 ++++++++++ 2 files changed, 35 insertions(+) diff --git a/.github/scripts/label-hygiene.test.mjs b/.github/scripts/label-hygiene.test.mjs index 66a207306..e30a0a37c 100644 --- a/.github/scripts/label-hygiene.test.mjs +++ b/.github/scripts/label-hygiene.test.mjs @@ -3,6 +3,7 @@ // merge-gate.mjs is tested: the fixtures are real API pages, and every removal is a recorded call. import { test } from 'node:test' import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' import { main, plan, planItem, STALE_LABELS } from './label-hygiene.mjs' const issue = (number, state, labels, title = `Item ${number}`) => @@ -106,3 +107,27 @@ test('#738: the keep-list is a closed set, so a new label is not removed by acci assert.deepEqual(STALE_LABELS, ['agent-working', 'agent-pr', 'agent-ready', 'agent-task', 'awaiting-approval']) for (const keep of ['bug', 'area:ui', 'risk:T3', 'hold', 'needs-human', 'status:done']) assert.equal(STALE_LABELS.includes(keep), false) }) + +test('#1401: a closed item that still carries hold loses only its status label', () => { + const gh = fakeGh([issue(800, 'closed', ['hold', 'agent-working', 'bug', 'area:automation'])]) + const r = run(gh, true) + assert.deepEqual(r.items.map(i => [i.number, i.stale]), [[800, ['agent-working']]]) + assert.deepEqual(gh.store[0].labels.map(l => l.name), ['hold', 'bug', 'area:automation'], 'hold is human-only, in both directions') + assert.deepEqual(gh.deletes(), ['api -X DELETE repos/ThinkFlowLab/FlyBot/issues/800/labels/agent-working']) +}) + +test('#1401: closing an item is a trigger, and the daily sweep stays', () => { + const wf = readFileSync(new URL('../workflows/label-hygiene.yml', import.meta.url), 'utf8') + const triggers = wf.slice(wf.indexOf('\non:\n'), wf.indexOf('\nconcurrency:')) + assert.match(triggers, /^ {2}issues:\n {4}types: \[closed\]$/m, 'a closed issue runs the same cleanup') + assert.match(triggers, /^ {2}pull_request_target:\n {4}types: \[closed\]$/m, 'a closed pull request too, with the write token') + // A `pull_request` run from a fork is read-only and could not remove the label it came for. + assert.doesNotMatch(triggers, /^ {2}pull_request:\s*$/m) + assert.match(triggers, /^ {2}schedule:\n {4}- cron: '30 19 \* \* \*'/m, 'the daily sweep stays') + assert.match(triggers, /^ {2}workflow_dispatch:$/m, 'and the manual backfill stays') + const perms = wf.slice(wf.indexOf('\npermissions:\n'), wf.indexOf('\njobs:')).trim().split('\n').map(line => line.trim()) + assert.deepEqual(perms, ['permissions:', 'contents: read', 'issues: write', 'pull-requests: write'], 'the close event adds no permission') + assert.match(wf, /^ {2}group: label-hygiene$/m, 'one sweep at a time') + 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') +}) diff --git a/.github/workflows/label-hygiene.yml b/.github/workflows/label-hygiene.yml index 25856fbc0..cb0b41e3d 100644 --- a/.github/workflows/label-hygiene.yml +++ b/.github/workflows/label-hygiene.yml @@ -6,10 +6,20 @@ name: Label hygiene # prints `before:`/`after:` counts so the cleanup is verified rather than trusted. # # `hold` is untouched: only a human removes it (AGENTS.md → Merge gates). +# +# A close is the moment the label stops being true, so the same job also runs on it (#1401): +# otherwise the board reads a finished item as in flight until the next 19:30Z sweep, up to 24 hours. +# The pull-request side is `pull_request_target`, not `pull_request`, for the write token: a +# `pull_request` run from a fork is read-only, so it could not remove the label it came for +# (gate-check.test.mjs carries the same rule for #812). on: schedule: - cron: '30 19 * * *' # 03:30 Asia/Shanghai, a quiet hour workflow_dispatch: + issues: + types: [closed] + pull_request_target: + types: [closed] concurrency: group: label-hygiene