From 303b42692cc594a7584e9b594dcaf95548bc96a9 Mon Sep 17 00:00:00 2001 From: Sall Date: Tue, 21 Jul 2026 20:39:48 +0100 Subject: [PATCH 1/3] feat(labels): add gated migrate and delete paths to labels-sync.rb 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. --- scripts/labels-sync.rb | 301 ++++++++++++++++++++++++++++++++- scripts/test-labels-dry-run.sh | 183 ++++++++++++++++++++ 2 files changed, 478 insertions(+), 6 deletions(-) diff --git a/scripts/labels-sync.rb b/scripts/labels-sync.rb index da16763b4..12423d1eb 100755 --- a/scripts/labels-sync.rb +++ b/scripts/labels-sync.rb @@ -34,6 +34,10 @@ :apply, :confirm_apply, :allow_non_pilot_repo, + :migrate_legacy, + :confirm_migrate_legacy, + :delete_unused_legacy, + :confirm_delete_unused_legacy, keyword_init: true ) @@ -46,7 +50,11 @@ include_clean: false, apply: false, confirm_apply: false, - allow_non_pilot_repo: false + allow_non_pilot_repo: false, + migrate_legacy: false, + confirm_migrate_legacy: false, + delete_unused_legacy: false, + confirm_delete_unused_legacy: false ) parser = OptionParser.new do |opts| @@ -88,6 +96,22 @@ options.allow_non_pilot_repo = true end + opts.on("--migrate-legacy", "Preview migrating in-use legacy labels onto their canonical replacement") do + options.migrate_legacy = true + end + + opts.on("--confirm-migrate-legacy", "Actually relabel items and remove the migrated legacy labels") do + options.confirm_migrate_legacy = true + end + + opts.on("--delete-unused-legacy", "Preview deleting legacy labels that are attached to nothing") do + options.delete_unused_legacy = true + end + + opts.on("--confirm-delete-unused-legacy", "Actually delete the unused legacy labels") do + options.confirm_delete_unused_legacy = true + end + opts.on("-h", "--help", "Show this help") do puts opts exit 0 @@ -135,6 +159,40 @@ end end +# Destructive modes reuse the apply guardrails: a confirm flag is inert without +# its preview flag, neither mode may fan out across the whole org, and a +# confirmed run stays inside the pilot allowlist unless explicitly waived. +DESTRUCTIVE_MODES = { + "migrate-legacy" => %i[migrate_legacy confirm_migrate_legacy], + "delete-unused-legacy" => %i[delete_unused_legacy confirm_delete_unused_legacy] +}.freeze + +DESTRUCTIVE_MODES.each do |name, (preview_flag, confirm_flag)| + preview = options.public_send(preview_flag) + confirm = options.public_send(confirm_flag) + + if confirm && !preview + warn parser + warn "\nerror: --confirm-#{name} requires --#{name}" + exit 2 + end + + if preview && options.all_repos + warn parser + warn "\nerror: --#{name} is only allowed with explicit --repo values" + exit 2 + end + + next unless preview && confirm && !options.allow_non_pilot_repo + + outside_pilot = options.repos.reject { |repo| PILOT_APPLY_REPOS.include?(repo) } + next if outside_pilot.empty? + + warn "error: --#{name} pilot is limited to: #{PILOT_APPLY_REPOS.join(', ')}" + warn "rerun with --allow-non-pilot-repo only after maintainer approval" + exit 2 +end + def gh_json(*args) stdout, stderr, status = Open3.capture3("gh", *args) unless status.success? @@ -186,6 +244,39 @@ def update_label(owner_repo, label) ) end +def delete_label(owner_repo, name) + gh_api_mutation( + "repos/#{owner_repo}/labels/#{label_path_segment(name)}", + "--method", "DELETE" + ) +end + +def add_label_to_item(owner_repo, number, name) + gh_api_mutation( + "repos/#{owner_repo}/issues/#{number}/labels", + "--method", "POST", + "-f", "labels[]=#{name}" + ) +end + +# Every issue and pull request in any state, with the labels each one carries. +# Closed items count: deleting a label strips it from them too. +def repo_items(owner_repo) + gh_paginated_array("repos/#{owner_repo}/issues?state=all&per_page=100").map do |item| + { + "number" => item.fetch("number"), + "labels" => (item["labels"] || []).map { |label| label.is_a?(Hash) ? label.fetch("name") : label } + } + end +end + +# name => number of items carrying it. Absent names are unused. +def label_usage(items) + items.each_with_object(Hash.new(0)) do |item, counts| + item.fetch("labels").each { |name| counts[name] += 1 } + end +end + def repo_list(org) gh_json("repo", "list", org, "--limit", "1000", "--json", "nameWithOwner").map { |repo| repo.fetch("nameWithOwner") } end @@ -289,6 +380,117 @@ def planned_label_operations(result, canonical) } end +# Plan the destructive work for one repo. Reads live usage rather than any +# cached audit, so a label that gained an item since the last scan is seen as +# in use. Unknown labels are never planned for deletion. +def planned_legacy_operations(result, canonical, items) + usage = label_usage(items) + + migrations = [] + deletions = [] + blocked = [] + + result.fetch("legacy_present").each do |entry| + legacy = entry.fetch("legacy") + replacement = entry.fetch("replacement") + count = usage[legacy] + + unless canonical.key?(replacement) + blocked << { + "legacy" => legacy, + "reason" => "replacement #{replacement} is not a canonical label" + } + next + end + + if count.zero? + deletions << { "legacy" => legacy, "items" => 0 } + next + end + + carrying = items.select { |item| item.fetch("labels").include?(legacy) } + .map { |item| item.fetch("number") } + .sort + + migrations << { + "legacy" => legacy, + "replacement" => replacement, + "items" => carrying, + "count" => carrying.length + } + end + + { + "would_migrate" => migrations.sort_by { |item| item.fetch("legacy") }, + "would_delete_unused" => deletions.sort_by { |item| item.fetch("legacy") }, + "blocked" => blocked.sort_by { |item| item.fetch("legacy") }, + "protected_unknown" => result.fetch("unknown") + } +end + +# Order is load-bearing. The canonical label goes onto every carrying item and is +# verified to have landed before the legacy label is removed; deleting first +# would strip the association with nothing to replace it. +def migrate_legacy_labels(owner_repo, operations) + result = { "relabelled" => [], "removed" => [], "errors" => [] } + + operations.fetch("would_migrate").each do |migration| + legacy = migration.fetch("legacy") + replacement = migration.fetch("replacement") + relabelled = [] + + begin + migration.fetch("items").each do |number| + add_label_to_item(owner_repo, number, replacement) + relabelled << number + end + + still_missing = repo_items(owner_repo).select do |item| + migration.fetch("items").include?(item.fetch("number")) && + !item.fetch("labels").include?(replacement) + end + + unless still_missing.empty? + raise "#{replacement} did not land on items #{still_missing.map { |i| i.fetch('number') }.join(', ')}" + end + + delete_label(owner_repo, legacy) + result.fetch("removed") << legacy + result.fetch("relabelled") << { "legacy" => legacy, "replacement" => replacement, "items" => relabelled } + rescue StandardError => e + result.fetch("errors") << { + "operation" => "migrate", + "label" => legacy, + "message" => e.message + } + return result + end + end + + result +end + +def delete_unused_legacy_labels(owner_repo, operations) + result = { "deleted" => [], "errors" => [] } + + operations.fetch("would_delete_unused").each do |entry| + legacy = entry.fetch("legacy") + begin + delete_label(owner_repo, legacy) + result.fetch("deleted") << legacy + rescue StandardError => e + result.fetch("errors") << { + "operation" => "delete", + "label" => legacy, + "message" => e.message + } + return result + end + end + + result +end + def apply_label_operations(owner_repo, operations) result = { "created" => [], @@ -355,6 +557,22 @@ def print_apply_errors(errors) end canonical, legacy_migrations, sync_policy = canonical_label_map(options.labels_file) + +# sync_policy is the source of truth for what the destructive modes may do. The +# script only ever implements delete-when-unused, so a false value for that key +# disables the mode outright rather than widening it. +if options.delete_unused_legacy && !sync_policy.fetch("delete_legacy_labels_only_when_unused", false) + warn "error: sync_policy.delete_legacy_labels_only_when_unused is not enabled in #{options.labels_file}" + warn "the delete mode implements unused-only deletion and refuses to run without it" + exit 2 +end + +if options.migrate_legacy && !sync_policy.fetch("preserve_labels_on_open_items_before_removal", false) + warn "error: sync_policy.preserve_labels_on_open_items_before_removal is not enabled in #{options.labels_file}" + warn "migration exists to preserve labelling before removal and refuses to run without it" + exit 2 +end + repos = options.all_repos ? repo_list(options.org) : options.repos results = repos.sort.map { |repo| diff_repo(repo, canonical, legacy_migrations) } @@ -364,6 +582,14 @@ def print_apply_errors(errors) end end +legacy_mode = options.migrate_legacy || options.delete_unused_legacy +if legacy_mode + results.each do |result| + items = repo_items(result.fetch("repo")) + result["legacy_operations"] = planned_legacy_operations(result, canonical, items) + end +end + apply_failed = false if options.apply && options.confirm_apply results.each do |result| @@ -372,9 +598,37 @@ def print_apply_errors(errors) end end +legacy_failed = false +if options.migrate_legacy && options.confirm_migrate_legacy + results.each do |result| + outcome = migrate_legacy_labels(result.fetch("repo"), result.fetch("legacy_operations")) + result["migrated"] = outcome + legacy_failed ||= !outcome.fetch("errors").empty? + end +end + +if options.delete_unused_legacy && options.confirm_delete_unused_legacy + results.each do |result| + outcome = delete_unused_legacy_labels(result.fetch("repo"), result.fetch("legacy_operations")) + result["deleted"] = outcome + legacy_failed ||= !outcome.fetch("errors").empty? + end +end + +mode = + if options.migrate_legacy + options.confirm_migrate_legacy ? "migrate" : "migrate-preview" + elsif options.delete_unused_legacy + options.confirm_delete_unused_legacy ? "delete" : "delete-preview" + elsif options.apply + options.confirm_apply ? "apply" : "apply-preview" + else + "dry-run" + end + payload = { - "mode" => options.apply ? (options.confirm_apply ? "apply" : "apply-preview") : "dry-run", - "confirmed" => options.confirm_apply, + "mode" => mode, + "confirmed" => options.confirm_apply || options.confirm_migrate_legacy || options.confirm_delete_unused_legacy, "labels_file" => options.labels_file, "canonical_labels" => canonical.length, "legacy_migrations" => legacy_migrations.length, @@ -387,7 +641,7 @@ def print_apply_errors(errors) if options.json puts JSON.pretty_generate(payload) - exit(apply_failed ? 1 : 0) + exit(apply_failed || legacy_failed ? 1 : 0) end heading = options.apply ? "# Label sync apply preview" : "# Label sync dry-run" @@ -409,7 +663,13 @@ def print_apply_errors(errors) else puts "This is a read-only dry run. No labels or issues were changed." end -puts "Legacy and unknown labels are skipped/preserved; no labels are deleted." +if legacy_mode + puts "Unknown labels are never deleted. Legacy labels are removed only after " \ + "their canonical replacement is verified on every carrying item, or when " \ + "they are attached to nothing." +else + puts "Legacy and unknown labels are skipped/preserved; no labels are deleted." +end puts puts "## Sync policy" @@ -485,6 +745,35 @@ def print_apply_errors(errors) result.fetch("unknown").each { |name| puts "- #{name}" } puts end + + next unless legacy_mode + + legacy_operations = result.fetch("legacy_operations") + + unless legacy_operations.fetch("would_migrate").empty? + puts "### Would migrate (relabel items, then remove legacy)" + legacy_operations.fetch("would_migrate").each do |item| + puts "- #{item.fetch('legacy')} -> #{item.fetch('replacement')} (#{item.fetch('count')} item(s))" + end + puts + end + + unless legacy_operations.fetch("would_delete_unused").empty? + puts "### Would delete (unused legacy labels)" + legacy_operations.fetch("would_delete_unused").each { |item| puts "- #{item.fetch('legacy')}" } + puts + end + + unless legacy_operations.fetch("blocked").empty? + puts "### Blocked" + legacy_operations.fetch("blocked").each do |item| + puts "- #{item.fetch('legacy')}: #{item.fetch('reason')}" + end + puts + end + + print_apply_errors(result.dig("migrated", "errors") || []) + print_apply_errors(result.dig("deleted", "errors") || []) end -exit(apply_failed ? 1 : 0) +exit(apply_failed || legacy_failed ? 1 : 0) diff --git a/scripts/test-labels-dry-run.sh b/scripts/test-labels-dry-run.sh index 4e2b35312..8b2f18e7b 100755 --- a/scripts/test-labels-dry-run.sh +++ b/scripts/test-labels-dry-run.sh @@ -103,4 +103,187 @@ set -e grep -q '^### Apply errors' "$OUT" || fail "expected markdown apply errors section" grep -q '^- create test-label: .*forced create failure' "$OUT" || fail "expected formatted apply error in markdown" +# Migrate and delete guardrails: confirm flags require their preview flag. +assert_exit 2 "$SCRIPT" --repo z-shell/.github --confirm-migrate-legacy +assert_exit 2 "$SCRIPT" --repo z-shell/.github --confirm-delete-unused-legacy + +# Neither destructive mode may run against --all-repos. +assert_exit 2 "$SCRIPT" --all-repos --migrate-legacy +assert_exit 2 "$SCRIPT" --all-repos --delete-unused-legacy +assert_exit 2 "$SCRIPT" --all-repos --migrate-legacy --confirm-migrate-legacy +assert_exit 2 "$SCRIPT" --all-repos --delete-unused-legacy --confirm-delete-unused-legacy + +# Confirmed destructive runs stay inside the pilot allowlist. +assert_exit 2 "$SCRIPT" --repo z-shell/zi --migrate-legacy --confirm-migrate-legacy +assert_exit 2 "$SCRIPT" --repo z-shell/zi --delete-unused-legacy --confirm-delete-unused-legacy + +# Preview modes are read-only and must report their own mode. +assert_json_field migrate-preview "$SCRIPT" --repo z-shell/.github --migrate-legacy --json +assert_json_field delete-preview "$SCRIPT" --repo z-shell/.github --delete-unused-legacy --json + +# sync_policy is the source of truth: a policy that forbids the operation refuses +# it even when every flag is supplied correctly. +POLICY_OFF="$TEST_TMP/policy-off.yml" +cat >"$POLICY_OFF" <<'YAML' +labels: + - name: type:bug + color: ff0000 + description: Bug +legacy_migrations: + "bug 🐞": type:bug +sync_policy: + delete_unknown_labels: false + delete_legacy_labels_only_when_unused: false + preserve_labels_on_open_items_before_removal: true +YAML +assert_exit 2 "$SCRIPT" --labels-file "$POLICY_OFF" --repo z-shell/.github \ + --delete-unused-legacy --confirm-delete-unused-legacy + +# A legacy label that is still in use must never be deleted directly. The fake gh +# reports one carrying item, so the delete path has to refuse it. +INUSE_BIN="$TEST_TMP/inuse-bin" +DELETE_MARKER="$TEST_TMP/delete-attempted" +export DELETE_MARKER +mkdir -p "$INUSE_BIN" +cat >"$INUSE_BIN/gh" <<'GH' +#!/usr/bin/env sh +set -eu +case "$*" in + *"/labels?per_page=100"*) + printf '{"name":"bug 🐞","color":"d73a4a","description":"legacy"}\n' + exit 0 + ;; + *"issues?state=all"*) + printf '{"number":7,"labels":[{"name":"bug 🐞"}]}\n' + exit 0 + ;; + *--method\ DELETE*) + # Record out-of-band: the script captures our stderr, so a message there + # would never reach the test. + printf 'delete attempted\n' >>"$DELETE_MARKER" + exit 90 + ;; +esac +printf '\n' +exit 0 +GH +chmod +x "$INUSE_BIN/gh" + +INUSE_LABELS="$TEST_TMP/inuse.yml" +cat >"$INUSE_LABELS" <<'YAML' +labels: + - name: type:bug + color: d73a4a + description: Bug +legacy_migrations: + "bug 🐞": type:bug +sync_policy: + delete_unknown_labels: false + delete_legacy_labels_only_when_unused: true + preserve_labels_on_open_items_before_removal: true +YAML + +set +e +PATH="$INUSE_BIN:$PATH" "$SCRIPT" \ + --labels-file "$INUSE_LABELS" \ + --repo z-shell/.github \ + --delete-unused-legacy \ + --confirm-delete-unused-legacy \ + --json >"$OUT" 2>"$ERR" +code=$? +set -e + +# The guard: an in-use legacy label must never reach the delete path. +if [ -e "$DELETE_MARKER" ]; then + fail "delete path issued DELETE for an in-use legacy label" +fi + +# ...and it must be routed to migration instead, with nothing queued for deletion. +ruby -rjson -e ' + data = JSON.parse(File.read(ARGV.fetch(0))) + ops = data.fetch("results").fetch(0).fetch("legacy_operations") + unless ops.fetch("would_delete_unused").empty? + abort "in-use legacy label was queued for deletion: #{ops.fetch('"'"'would_delete_unused'"'"').inspect}" + end + migrating = ops.fetch("would_migrate").map { |m| m.fetch("legacy") } + unless migrating.include?("bug 🐞") + abort "in-use legacy label was not queued for migration: #{migrating.inspect}" + end +' "$OUT" || fail "in-use legacy label was not handled correctly" + +[ "$code" = 0 ] || fail "expected clean exit for in-use legacy preview, got $code" + +# Migration order is load-bearing: the canonical label must be added to every +# carrying item BEFORE the legacy label is deleted. Deleting first would strip +# the association with nothing to replace it. This stub logs call order. +ORDER_BIN="$TEST_TMP/order-bin" +ORDER_LOG="$TEST_TMP/order.log" +export ORDER_LOG +mkdir -p "$ORDER_BIN" +: >"$ORDER_LOG" +cat >"$ORDER_BIN/gh" <<'GH' +#!/usr/bin/env sh +set -eu +case "$*" in + *"/labels?per_page=100"*) + printf '{"name":"bug 🐞","color":"d73a4a","description":"legacy"}\n' + exit 0 + ;; + *"issues?state=all"*) + # After the relabel has been recorded, report the canonical label as present + # so the script's own verification step can succeed. + if grep -q '^ADD' "$ORDER_LOG" 2>/dev/null; then + printf '{"number":7,"labels":[{"name":"bug 🐞"},{"name":"type:bug"}]}\n' + else + printf '{"number":7,"labels":[{"name":"bug 🐞"}]}\n' + fi + exit 0 + ;; + *"/issues/7/labels"*--method\ POST*) + printf 'ADD\n' >>"$ORDER_LOG" + printf '\n' + exit 0 + ;; + *--method\ DELETE*) + printf 'DELETE\n' >>"$ORDER_LOG" + printf '\n' + exit 0 + ;; +esac +printf '\n' +exit 0 +GH +chmod +x "$ORDER_BIN/gh" + +assert_success env PATH="$ORDER_BIN:$PATH" "$SCRIPT" \ + --labels-file "$INUSE_LABELS" \ + --repo z-shell/.github \ + --migrate-legacy \ + --confirm-migrate-legacy \ + --json + +grep -q '^ADD' "$ORDER_LOG" || fail "migration never added the canonical label" +grep -q '^DELETE' "$ORDER_LOG" || fail "migration never removed the legacy label" +[ "$(head -n 1 "$ORDER_LOG")" = "ADD" ] || { + cat "$ORDER_LOG" >&2 + fail "migration deleted the legacy label before relabelling items" +} + +# Preview modes must be read-only: no POST, no DELETE, for either mode. +: >"$ORDER_LOG" +assert_success env PATH="$ORDER_BIN:$PATH" "$SCRIPT" \ + --labels-file "$INUSE_LABELS" --repo z-shell/.github --migrate-legacy --json +[ -s "$ORDER_LOG" ] && { + cat "$ORDER_LOG" >&2 + fail "--migrate-legacy preview issued write calls" +} + +: >"$ORDER_LOG" +assert_success env PATH="$ORDER_BIN:$PATH" "$SCRIPT" \ + --labels-file "$INUSE_LABELS" --repo z-shell/.github --delete-unused-legacy --json +[ -s "$ORDER_LOG" ] && { + cat "$ORDER_LOG" >&2 + fail "--delete-unused-legacy preview issued write calls" +} + printf 'labels-sync smoke tests passed\n' From deabe4318d9857168269a75a92f023e395e5e48f Mon Sep 17 00:00:00 2001 From: Sall Date: Tue, 21 Jul 2026 20:50:02 +0100 Subject: [PATCH 2/3] fix(labels): close the migration race before deleting a legacy label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- scripts/labels-sync.rb | 40 +++++++++++++++++------- scripts/test-labels-dry-run.sh | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 12 deletions(-) diff --git a/scripts/labels-sync.rb b/scripts/labels-sync.rb index 12423d1eb..31abe7a8f 100755 --- a/scripts/labels-sync.rb +++ b/scripts/labels-sync.rb @@ -24,6 +24,10 @@ "z-shell/.github" ].freeze +# Relabel passes allowed before a migration gives up rather than delete a legacy +# label that is still gaining items. +MAX_MIGRATION_PASSES = 5 + Options = Struct.new( :labels_file, :org, @@ -440,20 +444,32 @@ def migrate_legacy_labels(owner_repo, operations) relabelled = [] begin - migration.fetch("items").each do |number| - add_label_to_item(owner_repo, number, replacement) - relabelled << number - end - - still_missing = repo_items(owner_repo).select do |item| - migration.fetch("items").include?(item.fetch("number")) && - !item.fetch("labels").include?(replacement) - end - - unless still_missing.empty? - raise "#{replacement} did not land on items #{still_missing.map { |i| i.fetch('number') }.join(', ')}" + # Relabel every item that currently carries the legacy label, then look + # again, until a pass finds nothing left to do. The planned item list is + # only a preview: deleting a label strips it from every holder at that + # moment, so anything that acquires the legacy label between planning and + # deletion would lose it with no replacement. Converging here closes that + # window instead of trusting the plan. + passes = 0 + loop do + passes += 1 + pending = repo_items(owner_repo).select do |item| + item.fetch("labels").include?(legacy) && !item.fetch("labels").include?(replacement) + end + break if pending.empty? + + if passes > MAX_MIGRATION_PASSES + raise "#{legacy} kept gaining items across #{MAX_MIGRATION_PASSES} passes; " \ + "aborting before delete so nothing loses its labelling" + end + + pending.each do |item| + add_label_to_item(owner_repo, item.fetch("number"), replacement) + relabelled << item.fetch("number") + end end + relabelled.uniq! delete_label(owner_repo, legacy) result.fetch("removed") << legacy result.fetch("relabelled") << { "legacy" => legacy, "replacement" => replacement, "items" => relabelled } diff --git a/scripts/test-labels-dry-run.sh b/scripts/test-labels-dry-run.sh index 8b2f18e7b..cc27e4815 100755 --- a/scripts/test-labels-dry-run.sh +++ b/scripts/test-labels-dry-run.sh @@ -286,4 +286,61 @@ assert_success env PATH="$ORDER_BIN:$PATH" "$SCRIPT" \ fail "--delete-unused-legacy preview issued write calls" } +# An item that acquires the legacy label between planning and deletion must not +# lose it. Deleting a label strips it from every holder at that moment, so the +# migration has to re-check for late arrivals before it deletes. +RACE_BIN="$TEST_TMP/race-bin" +RACE_LOG="$TEST_TMP/race.log" +export RACE_LOG +mkdir -p "$RACE_BIN" +: >"$RACE_LOG" +cat >"$RACE_BIN/gh" <<'GH' +#!/usr/bin/env sh +set -eu +case "$*" in + *"/labels?per_page=100"*) + printf '{"name":"bug 🐞","color":"d73a4a","description":"legacy"}\n' + exit 0 + ;; + *"issues?state=all"*) + # Item 7 is known at plan time. Item 8 shows up carrying the legacy label + # only after the first relabel — the race this guards against. + if grep -q '^ADD 7' "$RACE_LOG" 2>/dev/null; then + printf '{"number":7,"labels":[{"name":"bug 🐞"},{"name":"type:bug"}]}\n' + if grep -q '^ADD 8' "$RACE_LOG" 2>/dev/null; then + printf '{"number":8,"labels":[{"name":"bug 🐞"},{"name":"type:bug"}]}\n' + else + printf '{"number":8,"labels":[{"name":"bug 🐞"}]}\n' + fi + else + printf '{"number":7,"labels":[{"name":"bug 🐞"}]}\n' + fi + exit 0 + ;; + *"/issues/7/labels"*--method\ POST*) printf 'ADD 7\n' >>"$RACE_LOG"; printf '\n'; exit 0 ;; + *"/issues/8/labels"*--method\ POST*) printf 'ADD 8\n' >>"$RACE_LOG"; printf '\n'; exit 0 ;; + *--method\ DELETE*) printf 'DELETE\n' >>"$RACE_LOG"; printf '\n'; exit 0 ;; +esac +printf '\n' +exit 0 +GH +chmod +x "$RACE_BIN/gh" + +assert_success env PATH="$RACE_BIN:$PATH" "$SCRIPT" \ + --labels-file "$INUSE_LABELS" \ + --repo z-shell/.github \ + --migrate-legacy \ + --confirm-migrate-legacy \ + --json + +grep -q '^ADD 8' "$RACE_LOG" || { + cat "$RACE_LOG" >&2 + fail "late-arriving item 8 never received the canonical label" +} +[ "$(grep -n '^DELETE' "$RACE_LOG" | head -n 1 | cut -d: -f1)" -gt \ + "$(grep -n '^ADD 8' "$RACE_LOG" | head -n 1 | cut -d: -f1)" ] || { + cat "$RACE_LOG" >&2 + fail "legacy label was deleted before the late-arriving item was relabelled" +} + printf 'labels-sync smoke tests passed\n' From d265e86ec995f1665a5b23e90cba728adf7a2810 Mon Sep 17 00:00:00 2001 From: Sall Date: Tue, 21 Jul 2026 21:19:02 +0100 Subject: [PATCH 3/3] fix(labels): report destructive modes honestly in Markdown output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- scripts/labels-sync.rb | 35 ++++++++++++++++++++++++++++++---- scripts/test-labels-dry-run.sh | 26 +++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/scripts/labels-sync.rb b/scripts/labels-sync.rb index 31abe7a8f..1290ab92c 100755 --- a/scripts/labels-sync.rb +++ b/scripts/labels-sync.rb @@ -171,6 +171,16 @@ "delete-unused-legacy" => %i[delete_unused_legacy confirm_delete_unused_legacy] }.freeze +# One destructive operation per invocation. Both modes can run in the same +# process, but a single run reports a single mode, and migrate-then-delete is an +# ordering an operator should carry out deliberately rather than have inferred. +if options.migrate_legacy && options.delete_unused_legacy + warn parser + warn "\nerror: use either --migrate-legacy or --delete-unused-legacy, not both" + warn "run them as separate invocations so each reports its own mode and is confirmed on its own" + exit 2 +end + DESTRUCTIVE_MODES.each do |name, (preview_flag, confirm_flag)| preview = options.public_send(preview_flag) confirm = options.public_send(confirm_flag) @@ -660,8 +670,16 @@ def print_apply_errors(errors) exit(apply_failed || legacy_failed ? 1 : 0) end -heading = options.apply ? "# Label sync apply preview" : "# Label sync dry-run" -heading = "# Label sync apply result" if options.apply && options.confirm_apply +heading = + case mode + when "migrate" then "# Label sync migration result" + when "migrate-preview" then "# Label sync migration preview" + when "delete" then "# Label sync deletion result" + when "delete-preview" then "# Label sync deletion preview" + when "apply" then "# Label sync apply result" + when "apply-preview" then "# Label sync apply preview" + else "# Label sync dry-run" + end puts heading puts puts "Mode: #{payload.fetch('mode')}" @@ -672,9 +690,18 @@ def print_apply_errors(errors) puts "Repos with drift: #{payload.fetch('repos_with_drift')}" puts -if options.apply && options.confirm_apply +case mode +when "migrate" + puts "Confirmed migration: items were relabelled and migrated legacy labels were removed." +when "migrate-preview" + puts "Migration preview only. Pass --confirm-migrate-legacy to relabel items and remove legacy labels." +when "delete" + puts "Confirmed deletion: unused legacy labels were removed." +when "delete-preview" + puts "Deletion preview only. Pass --confirm-delete-unused-legacy to remove unused legacy labels." +when "apply" puts "Confirmed apply mode: canonical labels may have been created or updated." -elsif options.apply +when "apply-preview" puts "Apply preview only. Pass --confirm-apply to create/update canonical labels." else puts "This is a read-only dry run. No labels or issues were changed." diff --git a/scripts/test-labels-dry-run.sh b/scripts/test-labels-dry-run.sh index cc27e4815..47f65a6ee 100755 --- a/scripts/test-labels-dry-run.sh +++ b/scripts/test-labels-dry-run.sh @@ -269,6 +269,32 @@ grep -q '^DELETE' "$ORDER_LOG" || fail "migration never removed the legacy label fail "migration deleted the legacy label before relabelling items" } +# One destructive operation per invocation: the two legacy modes are exclusive. +assert_exit 2 "$SCRIPT" --repo z-shell/.github --migrate-legacy --delete-unused-legacy + +# Confirmed destructive runs must never describe themselves as a read-only dry +# run in the Markdown output; that would misrepresent what happened in logs. +: >"$ORDER_LOG" +assert_success env PATH="$ORDER_BIN:$PATH" "$SCRIPT" \ + --labels-file "$INUSE_LABELS" --repo z-shell/.github \ + --migrate-legacy --confirm-migrate-legacy +grep -q 'read-only dry run' "$OUT" && { + cat "$OUT" >&2 + fail "confirmed migration described itself as a read-only dry run" +} +grep -q '^# Label sync migration result' "$OUT" || { + cat "$OUT" >&2 + fail "confirmed migration did not use the migration heading" +} + +: >"$ORDER_LOG" +assert_success env PATH="$ORDER_BIN:$PATH" "$SCRIPT" \ + --labels-file "$INUSE_LABELS" --repo z-shell/.github --delete-unused-legacy +grep -q 'read-only dry run' "$OUT" && { + cat "$OUT" >&2 + fail "deletion preview described itself as a read-only dry run" +} + # Preview modes must be read-only: no POST, no DELETE, for either mode. : >"$ORDER_LOG" assert_success env PATH="$ORDER_BIN:$PATH" "$SCRIPT" \