diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fd0e56401..8ed0e20370 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Validation Small Defects Fixed** — Backup opt-out is honoured, default-only footer configs apply, and null labels no longer throw. ([#3538](https://github.com/lightspeedwp/.github/issues/3538)) - **Metrics Aggregator Tolerates Malformed Artifacts** — Branch-validation metrics now skip a malformed artifact with a warning instead of discarding the whole run. ([#3528](https://github.com/lightspeedwp/.github/issues/3528)) - **Broken Diagrams Repaired** — 123 diagrams in 36 documents render again; the diagram tidy-up bot now places accessibility titles correctly and no longer edits surrounding text. (#3490) - **PR Agent Tests Run Again** — Fixed 6 PR agent test suites that could not load, so 95 more tests now run. (#3472) diff --git a/agents/pr-agent/skills/validate-and-apply-labels/scripts/__tests__/validate-and-apply-labels.test.js b/agents/pr-agent/skills/validate-and-apply-labels/scripts/__tests__/validate-and-apply-labels.test.js index 1682169e3f..06023029fb 100644 --- a/agents/pr-agent/skills/validate-and-apply-labels/scripts/__tests__/validate-and-apply-labels.test.js +++ b/agents/pr-agent/skills/validate-and-apply-labels/scripts/__tests__/validate-and-apply-labels.test.js @@ -86,6 +86,13 @@ describe("validateAndApplyLabels", () => { expect(result.valid).toBe(false); expect(result.errors).toContain("multiple-changelog-decision-labels"); }); + + test("should treat explicit labels:null like an empty array (#3538)", async () => { + const result = await validateAndApplyLabels({ labels: null }); + + expect(result.valid).toBe(false); + expect(result.errors).toContain("missing-changelog-decision-label"); + }); }); describe("Branch Type Label Mapping", () => { @@ -575,5 +582,18 @@ describe("validateAndApplyLabels", () => { expect(result.valid).toBe(true); expect(result.appliedLabels).toContain("area:docs/api"); }); + + test("should treat explicit labels:null in branch-mapping mode (#3538)", async () => { + const result = await validateAndApplyLabels({ + branchType: "fix", + templateFile: "pr_bug.md", + labels: null, + }); + + expect(result.appliedLabels).toContain("type:bug"); + expect(result.validationErrors).toContain( + "Exactly one changelog-decision label (meta:needs-changelog or meta:no-changelog) is required", + ); + }); }); }); diff --git a/agents/pr-agent/skills/validate-and-apply-labels/scripts/validate-and-apply-labels.js b/agents/pr-agent/skills/validate-and-apply-labels/scripts/validate-and-apply-labels.js index 230a5e415a..31b4d0f1c3 100644 --- a/agents/pr-agent/skills/validate-and-apply-labels/scripts/validate-and-apply-labels.js +++ b/agents/pr-agent/skills/validate-and-apply-labels/scripts/validate-and-apply-labels.js @@ -99,12 +99,15 @@ const EXCLUSIVE_FAMILIES = { export async function validateAndApplyLabels(input) { const { - labels = [], + labels: inputLabels = [], branchType, templateFile, templateMetadata = null, config = {}, } = input; + // The destructured default covers only `undefined`; normalise an explicit + // `null` once at the boundary so both validation paths can iterate safely. + const labels = inputLabels ?? []; // Handle branchType-based label mapping (if either branchType or templateFile is explicitly provided) if ("branchType" in input || "templateFile" in input) { diff --git a/scripts/agents/__tests__/branding.agent.test.js b/scripts/agents/__tests__/branding.agent.test.js index 99fdf9db14..9f20926359 100644 --- a/scripts/agents/__tests__/branding.agent.test.js +++ b/scripts/agents/__tests__/branding.agent.test.js @@ -67,4 +67,19 @@ describe('branding.agent footer functions', () => { expect(afterFirstRun).toBe(afterSecondRun); expect(afterSecondRun.trim().split(/\n\s*\n/)).toHaveLength(3); }); + + test('getFooterPhrases honours a config with only a top-level default (#3538)', async () => { + const fixtureDir = fs.mkdtempSync(path.join(os.tmpdir(), 'branding-agent-default-only-')); + fs.mkdirSync(path.join(fixtureDir, '.github'), { recursive: true }); + fs.writeFileSync( + path.join(fixtureDir, '.github', 'footers.yml'), + ['default:', ' phrases:', ' - "Need help? Say hi."', ''].join('\n') + ); + cwdSpy = jest.spyOn(process, 'cwd').mockReturnValue(fixtureDir); + + const { getFooterPhrases } = await import('../branding.agent.js'); + + expect(getFooterPhrases('docs')).toEqual(['Need help? Say hi.']); + expect(getFooterPhrases('unknown')).toEqual(['Need help? Say hi.']); + }); }); diff --git a/scripts/agents/branding.agent.js b/scripts/agents/branding.agent.js index dc3a1dc7c4..3265bc3377 100644 --- a/scripts/agents/branding.agent.js +++ b/scripts/agents/branding.agent.js @@ -45,11 +45,11 @@ const DEFAULT_FOOTERS = [ */ function getFooterPhrases(category = 'default') { const config = loadFooterConfig(); - if (!config || !config.categories) { + if (!config) { return DEFAULT_FOOTERS; } - if (config.categories[category] && config.categories[category].phrases) { + if (config.categories && config.categories[category] && config.categories[category].phrases) { return config.categories[category].phrases; } diff --git a/scripts/agents/includes/__tests__/header-footer.test.js b/scripts/agents/includes/__tests__/header-footer.test.js index a56531e6a6..37e544618d 100644 --- a/scripts/agents/includes/__tests__/header-footer.test.js +++ b/scripts/agents/includes/__tests__/header-footer.test.js @@ -116,4 +116,19 @@ describe('header-footer', () => { const output = fs.readFileSync(filePath, 'utf8'); expect(output).toContain('Fixture default phrase.'); }); + + test('getFooterPhrases honours a config with only a top-level default (#3538)', async () => { + const fixtureDir = fs.mkdtempSync(path.join(os.tmpdir(), 'header-footer-default-only-')); + fs.mkdirSync(path.join(fixtureDir, '.github'), { recursive: true }); + fs.writeFileSync( + path.join(fixtureDir, '.github', 'footers.yml'), + ['default:', ' phrases:', ' - "Fixture default phrase."', ''].join('\n') + ); + cwdSpy = jest.spyOn(process, 'cwd').mockReturnValue(fixtureDir); + + const { getFooterPhrases } = await import('../header-footer.js'); + + expect(getFooterPhrases('docs')).toEqual(['Fixture default phrase.']); + expect(getFooterPhrases('unknown')).toEqual(['Fixture default phrase.']); + }); }); diff --git a/scripts/agents/includes/header-footer.js b/scripts/agents/includes/header-footer.js index 004f0e81d3..a73b6c0c6b 100644 --- a/scripts/agents/includes/header-footer.js +++ b/scripts/agents/includes/header-footer.js @@ -39,12 +39,12 @@ const DEFAULT_FOOTERS = [ */ function getFooterPhrases(category = 'default') { const config = loadFooterConfig(); - if (!config || !config.categories) { + if (!config) { return DEFAULT_FOOTERS; } // Try to get category-specific footers - if (config.categories[category] && config.categories[category].phrases) { + if (config.categories && config.categories[category] && config.categories[category].phrases) { return config.categories[category].phrases; } @@ -218,7 +218,7 @@ function ensureFooter(file, options = {}) { * @param {object} options - Options: { backup: boolean, category: string, seed: string } * @returns {Promise} true if successful */ -async function insertHeaderFooter(filePath, config = {}, options = {}) { +async function insertHeaderFooter(filePath, _config = {}, options = {}) { const { backup = false, category = 'default', seed = null } = options; if (!fs.existsSync(filePath)) { diff --git a/scripts/validation/__tests__/auto-fixer.test.js b/scripts/validation/__tests__/auto-fixer.test.js new file mode 100644 index 0000000000..a4d1071c70 --- /dev/null +++ b/scripts/validation/__tests__/auto-fixer.test.js @@ -0,0 +1,66 @@ +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +// Regression coverage for #3538(A): `new AutoFixer({ backup: false })` +// must not write `.backup` files. Uses temp dirs so no repo files change. +function makeFixableFile() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'auto-fixer-backup-')); + const filePath = path.join(dir, 'sample.js'); + fs.writeFileSync(filePath, "const x = require('old-pkg');\n"); + return { dir, filePath }; +} + +async function loadFixer() { + const { default: AutoFixer } = await import('../lib/auto-fixer.js'); + return AutoFixer; +} + +describe('AutoFixer backup option (#3538)', () => { + test('backup:false applies the fix without writing a .backup file', async () => { + const AutoFixer = await loadFixer(); + const { dir, filePath } = makeFixableFile(); + const fixer = new AutoFixer({ rootDir: dir, backup: false }); + + const result = fixer.applyFixToFile('sample.js', 'old-pkg', 'new-pkg', 'js-import'); + + expect(result.success).toBe(true); + expect(result.changed).toBe(true); + expect(fs.readFileSync(filePath, 'utf8')).toContain("require('new-pkg')"); + expect(fs.existsSync(`${filePath}.backup`)).toBe(false); + }); + + test('omitted backup keeps the default and writes a .backup file', async () => { + const AutoFixer = await loadFixer(); + const { dir, filePath } = makeFixableFile(); + const fixer = new AutoFixer({ rootDir: dir }); + + const result = fixer.applyFixToFile('sample.js', 'old-pkg', 'new-pkg', 'js-import'); + + expect(result.success).toBe(true); + expect(fs.existsSync(`${filePath}.backup`)).toBe(true); + expect(fs.readFileSync(`${filePath}.backup`, 'utf8')).toContain("require('old-pkg')"); + }); + + test('backup:true writes a .backup file', async () => { + const AutoFixer = await loadFixer(); + const { dir, filePath } = makeFixableFile(); + const fixer = new AutoFixer({ rootDir: dir, backup: true }); + + fixer.applyFixToFile('sample.js', 'old-pkg', 'new-pkg', 'js-import'); + + expect(fs.existsSync(`${filePath}.backup`)).toBe(true); + }); + + test('dryRun writes neither the fix nor a backup', async () => { + const AutoFixer = await loadFixer(); + const { dir, filePath } = makeFixableFile(); + const fixer = new AutoFixer({ rootDir: dir, dryRun: true }); + + const result = fixer.applyFixToFile('sample.js', 'old-pkg', 'new-pkg', 'js-import'); + + expect(result.success).toBe(true); + expect(fs.readFileSync(filePath, 'utf8')).toContain("require('old-pkg')"); + expect(fs.existsSync(`${filePath}.backup`)).toBe(false); + }); +}); diff --git a/scripts/validation/lib/auto-fixer.js b/scripts/validation/lib/auto-fixer.js index 3bfc29d923..f8ce0f8022 100644 --- a/scripts/validation/lib/auto-fixer.js +++ b/scripts/validation/lib/auto-fixer.js @@ -10,7 +10,7 @@ export class AutoFixer { constructor(options = {}) { this.rootDir = options.rootDir || process.cwd(); this.dryRun = options.dryRun || false; - this.backup = options.backup || true; + this.backup = options.backup ?? true; this.fixes = []; }