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');