From e0d344782c9da67c883eba4c04e2abf34b321309 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Thu, 30 Apr 2026 08:27:03 +0200 Subject: [PATCH] fix: default label sync to non-destructive merge mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why `synchronizeIssueLabels` (`src/labels.js:69-78`) unconditionally deleted any repo label not in the target list. It runs on every `repository.created`, every `/sync-all-repos`, every `/configure-repo`, and every `reconcile-repo` task. **A user creating a `priority/p0` label saw it silently destroyed the next time the bot synced** — across the org, on every webhook tick, with no opt-out. This was wave-1 Bug #1 (the QA bug-hunter's #1 critical) and the highest- priority finding in `docs/agent-fleet/bugs.md`. ## What `synchronizeIssueLabels` now takes a `mode` option: - **`'merge'`** (default) — only create / update labels listed in `targetLabels`. Labels not in the target list are left untouched. Non-destructive. - **`'replace'`** — current destructive behaviour, preserved as opt-in. `config.yml` gains an `issue_labels_sync_mode: merge | replace` setting (default `merge`); `src/repository.js` reads it and threads it through. `src/schema.js` validates the new field. ## Source Wave-1 QA bug-hunter (Bug #1, `docs/agent-fleet/bugs.md`). ## Test plan - [x] 808 tests pass (was 806; added "does NOT remove in default merge mode" regression test for Bug #1; added "rejects invalid mode value"; converted the two existing replace-mode assertions to opt-in via `{mode: 'replace'}`) - [x] eslint clean ## Risk & rollout - Risk: **medium-low**. Behaviour change for existing operators: deletions stop happening unless they explicitly set `issue_labels_sync_mode: replace`. Almost everyone wants the new default. Anyone relying on the old destructive sweep will see a CHANGELOG entry and a `replace` opt-in. - Rollout: self-update on merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- __tests__/integration/labels.test.js | 27 +++++++++++++--- config.yml | 11 +++++++ src/labels.js | 46 ++++++++++++++++++++++------ src/repository.js | 8 ++++- src/schema.js | 7 +++++ 5 files changed, 84 insertions(+), 15 deletions(-) diff --git a/__tests__/integration/labels.test.js b/__tests__/integration/labels.test.js index 41ce133..e005eef 100644 --- a/__tests__/integration/labels.test.js +++ b/__tests__/integration/labels.test.js @@ -35,11 +35,11 @@ describe('synchronizeIssueLabels', () => { ); }); - it('removes labels not in target', async () => { + it('removes labels not in target when mode is replace', async () => { octokit.paginate.mockResolvedValue([{ name: 'old-label', color: 'fff', description: '' }]); const target = []; - await synchronizeIssueLabels(octokit, 'owner', 'repo', target); + await synchronizeIssueLabels(octokit, 'owner', 'repo', target, { mode: 'replace' }); expect(octokit.request).toHaveBeenCalledWith( 'DELETE /repos/{owner}/{repo}/labels/{name}', @@ -47,6 +47,25 @@ describe('synchronizeIssueLabels', () => { ); }); + it('does NOT remove labels in default merge mode (Bug #1 fix)', async () => { + octokit.paginate.mockResolvedValue([{ name: 'priority/p0', color: 'fff', description: 'user-created' }]); + const target = []; + + await synchronizeIssueLabels(octokit, 'owner', 'repo', target); + + expect(octokit.request).not.toHaveBeenCalledWith( + 'DELETE /repos/{owner}/{repo}/labels/{name}', + expect.anything() + ); + }); + + it('rejects an invalid mode value', async () => { + octokit.paginate.mockResolvedValue([]); + await expect( + synchronizeIssueLabels(octokit, 'owner', 'repo', [], { mode: 'destroy' }) + ).rejects.toThrow(/invalid mode/i); + }); + it('skips update when label matches', async () => { octokit.paginate.mockResolvedValue([{ name: 'bug', color: 'red', description: 'Bug' }]); const target = [{ name: 'bug', color: 'red', description: 'Bug' }]; @@ -71,7 +90,7 @@ describe('synchronizeIssueLabels', () => { ); }); - it('handles create, update, and delete in one pass', async () => { + it('handles create, update, and delete in one pass (replace mode)', async () => { octokit.paginate.mockResolvedValue([ { name: 'keep', color: 'aaa', description: 'Keep' }, { name: 'update-me', color: 'old', description: 'Old desc' }, @@ -83,7 +102,7 @@ describe('synchronizeIssueLabels', () => { { name: 'create-me', color: 'ccc', description: 'Created' } ]; - await synchronizeIssueLabels(octokit, 'owner', 'repo', target); + await synchronizeIssueLabels(octokit, 'owner', 'repo', target, { mode: 'replace' }); // Should not update 'keep' (unchanged) expect(octokit.request).not.toHaveBeenCalledWith( diff --git a/config.yml b/config.yml index d6da25f..b13fdac 100644 --- a/config.yml +++ b/config.yml @@ -131,6 +131,17 @@ scheduler: max_tasks_per_tick: 5 rate_limit_threshold: 100 +# How `synchronizeIssueLabels` reconciles a repo's labels with `issue_labels`: +# - `merge` (default, non-destructive): create or update labels listed below, +# but never delete labels that are not in this list. Safe for repos where +# users have created their own custom labels (per-issue tags, milestones, +# etc.). +# - `replace` (destructive, opt-in): also delete any label that is not in +# this list. This was the behaviour prior to fixing Bug #1 +# (`docs/agent-fleet/bugs.md`); set this only if you explicitly want every +# repo's labels reduced to exactly `issue_labels`. +issue_labels_sync_mode: merge + issue_labels: - name: "bug" color: "d73a4a" diff --git a/src/labels.js b/src/labels.js index 9bc6e8e..18df3f4 100644 --- a/src/labels.js +++ b/src/labels.js @@ -27,9 +27,33 @@ async function ensureLabelsExist(octokit, owner, repo, labelNames) { } } -async function synchronizeIssueLabels(octokit, owner, repo, targetLabels) { +const VALID_SYNC_MODES = new Set(['merge', 'replace']); + +/** + * Synchronize a repo's issue labels with `targetLabels`. + * + * @param {object} octokit + * @param {string} owner + * @param {string} repo + * @param {Array<{name: string, color: string, description?: string}>} targetLabels + * @param {object} [options] + * @param {'merge'|'replace'} [options.mode='merge'] - synchronization mode: + * - `'merge'` (default): only create / update labels listed in `targetLabels`. + * Labels not in the target list are left untouched. Non-destructive. + * - `'replace'`: in addition to create/update, deletes any label that is not + * in the target list. Destructive — the historical (pre-Bug #1) behaviour, + * preserved as opt-in. + */ +async function synchronizeIssueLabels(octokit, owner, repo, targetLabels, options = {}) { + const mode = options.mode || 'merge'; + if (!VALID_SYNC_MODES.has(mode)) { + throw new Error( + `synchronizeIssueLabels: invalid mode "${mode}" (expected "merge" or "replace")` + ); + } + try { - getLogger().info(`Synchronizing labels for ${owner}/${repo}`); + getLogger().info(`Synchronizing labels for ${owner}/${repo} (mode=${mode})`); const currentLabels = await octokit.paginate('GET /repos/{owner}/{repo}/labels', { owner, @@ -66,14 +90,16 @@ async function synchronizeIssueLabels(octokit, owner, repo, targetLabels) { } } - for (const currentLabel of currentLabels) { - if (!targetLabels.some((tl) => tl.name === currentLabel.name)) { - await octokit.request('DELETE /repos/{owner}/{repo}/labels/{name}', { - owner, - repo, - name: currentLabel.name - }); - getLogger().info(`Removed label: ${currentLabel.name}`); + if (mode === 'replace') { + for (const currentLabel of currentLabels) { + if (!targetLabels.some((tl) => tl.name === currentLabel.name)) { + await octokit.request('DELETE /repos/{owner}/{repo}/labels/{name}', { + owner, + repo, + name: currentLabel.name + }); + getLogger().info(`Removed label: ${currentLabel.name}`); + } } } diff --git a/src/repository.js b/src/repository.js index 289ff04..9331261 100644 --- a/src/repository.js +++ b/src/repository.js @@ -111,7 +111,13 @@ async function configureRepository( const targetLabels = getTargetIssueLabels(); if (targetLabels.length > 0) { // Labels don't need a default branch — safe even on empty repos. - await synchronizeIssueLabels(octokit, owner, repo, targetLabels); + // Default sync mode is 'merge' (non-destructive). Set + // `issue_labels_sync_mode: replace` in config.yml to opt back into the + // pre-Bug-#1 behaviour that deletes labels not in the target list. + const labelSyncMode = config?.issue_labels_sync_mode || 'merge'; + await synchronizeIssueLabels(octokit, owner, repo, targetLabels, { + mode: labelSyncMode + }); } if (skipBranchScopedWork) { diff --git a/src/schema.js b/src/schema.js index 5d38aeb..782a8b5 100644 --- a/src/schema.js +++ b/src/schema.js @@ -49,6 +49,13 @@ export function validateConfig(config) { } } + if (config.issue_labels_sync_mode !== undefined) { + const validModes = ['merge', 'replace']; + if (!validModes.includes(config.issue_labels_sync_mode)) { + errors.push('issue_labels_sync_mode must be one of: merge, replace'); + } + } + if (config.dependabot !== undefined) { if (typeof config.dependabot.version !== 'number') { errors.push('dependabot.version must be a number');