feat(labels): add gated migrate and delete paths to labels-sync.rb#466
Conversation
Closes #465. labels-sync.rb could create and update canonical labels but had no way to remove the legacy taxonomy it replaces, so repositories accumulate both sets indefinitely. lib/labels.yml already declared the intended policy; nothing executed it. Adds two modes behind the existing two-step preview/confirm pattern: --migrate-legacy adds the mapped canonical label to every item carrying an in-use legacy label, verifies it landed, and only then removes the legacy label. Order is load-bearing: deleting first would strip the association with nothing to replace it. --delete-unused-legacy removes legacy labels attached to nothing. Behavior is read from sync_policy rather than hardcoded, so labels.yml stays the single source of truth. Unknown labels are never deleted, and usage is measured live at run time rather than read from a cached audit, so a label that gained an item since the last scan counts as in use. Both modes reuse the apply guardrails: a confirm flag is inert without its preview flag, neither may fan out with --all-repos, and confirmed runs stay inside the pilot allowlist unless explicitly waived. Tests cover the refusal paths and both load-bearing invariants. The in-use guard and the migration ordering were each verified by mutation: the suite fails when the guard is removed and when the delete is moved ahead of the relabel. The stub records calls to a file rather than stderr, which the script captures.
There was a problem hiding this comment.
Pull request overview
This PR extends scripts/labels-sync.rb to support removing the legacy label taxonomy by adding two new, tightly gated destructive workflows (legacy migration and unused-legacy deletion), and expands the dry-run test harness to assert the new refusal paths and ordering invariants.
Changes:
- Add
--migrate-legacy/--confirm-migrate-legacyand--delete-unused-legacy/--confirm-delete-unused-legacyflags with guardrails consistent with the existing apply pilot. - Implement planning + execution for legacy label migration (relabel then delete) and deletion of unused legacy labels, driven by
sync_policyfromlib/labels.yml. - Add shell-based refusal-path and invariant tests (read-only previews, no
--all-repos, pilot allowlist enforcement, and migration ordering).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/labels-sync.rb | Adds destructive legacy migration/deletion modes, policy gating, and JSON/Markdown output updates. |
| scripts/test-labels-dry-run.sh | Adds regression tests for new destructive-mode guardrails and load-bearing invariants (no deletes for in-use labels; relabel-before-delete ordering; preview stays read-only). |
The verify step only checked items that carried the legacy label at plan time, but delete_label strips the label from every holder at the moment it runs. An item that acquired the legacy label between planning and deletion would lose it with no canonical replacement added — silently destroying the association the migration exists to preserve. Replace the single relabel-then-verify pass with a loop that re-reads current holders and relabels stragglers until a pass finds nothing pending, bounded by MAX_MIGRATION_PASSES so a label that keeps gaining items aborts instead of deleting. The ordering test could not catch this: its stub returns a fixed carrying set. The new stub introduces a second item only after the first relabel lands. Verified by mutation — restoring the plan-time filter fails the suite.
Review note: these tests do not run in CI
sh scripts/test-labels-dry-run.shWiring it into CI is worth doing but is deliberately not bundled into this PR. Follow-up commit: closed a race the first version had
The verify step only checked items carrying the legacy label at plan time, but Now the relabel step loops: re-read current holders, relabel stragglers, repeat until a pass finds nothing pending, bounded by The original ordering test could not have caught this — its stub returns a fixed carrying set. The new stub introduces a second item only after the first relabel lands, and the fix is mutation-verified: restoring the plan-time filter fails the suite with |
Two defects from review. Confirmed legacy runs printed "This is a read-only dry run. No labels or issues were changed." and used the dry-run heading, because the status block branched on options.apply, which is false in migrate and delete modes. A run that had just deleted labels described itself as read-only, which is worse than unhelpful in an operator log. Heading and status are now derived from the computed mode. The two legacy modes could also be passed together: both would execute but the payload reports a single mode, so a machine consumer would see "migrate" for a run that also deleted. Rather than widen the mode field, refuse the combination — a single invocation should carry out one destructive operation, and migrate-then-delete is an ordering the operator should choose deliberately. Both fixes are mutation-verified: restoring the old status block and removing the exclusivity guard each fail the suite.
Closes #469. No workflow executed scripts/, so test-labels-dry-run.sh only ran when someone remembered to. That suite caught two real defects in #466, either of which would have destroyed label associations org-wide. Runs on changes to the label tooling or lib/labels.yml, with contents: read and issues: read for the suite's read-only preview calls. Verified the job is not a no-op: the same invocation exits 1 against a broken script and 0 against the restored one.
Closes #465.
What
labels-sync.rbcould create and update canonical labels but had no way to remove the legacy taxonomy it replaces. After the org-wide canonical rollout, most repositories now carry both taxonomies at once.lib/labels.ymlalready declared the cleanup policy; nothing executed it.Two new modes, behind the existing preview/confirm two-step:
--migrate-legacy--delete-unused-legacyDesign notes
sync_policyis the source of truth. The three keys are read, not reimplemented.delete_unknown_labels: falsemeans unknown labels are never deletable; the other two keys gate their respective modes and the script refuses to run when they are off.--all-repos, and confirmed runs stay insidePILOT_APPLY_REPOSunless explicitly waived.Testing
scripts/test-labels-dry-run.shgains refusal-path tests plus coverage of both load-bearing invariants. Both were verified by mutation rather than assumed:if true→ suite fails (delete path issued DELETE for an in-use legacy label)migration deleted the legacy label before relabelling items)An earlier version of the in-use test passed against the broken implementation, because the stub reported via stderr, which the script captures into its JSON error array. The stub now records calls to a file.
Preview modes are asserted to issue zero write calls.
Not in this PR
Running the cleanup. That is roughly 2,400 irreversible write operations across the org — 1,405 unused-legacy deletions and 1,002 item migrations across 31 repos — and needs its own approval with figures re-measured at run time.