From 471a7ba439cc05c604a09c9b7abc4b3cfe076695 Mon Sep 17 00:00:00 2001 From: Cindy Zhang Date: Mon, 31 Aug 2026 20:10:26 -0700 Subject: [PATCH] fix(knowledge): reject nested theme doc candidates --- .github/scripts/change-scope.cjs | 9 ++- .github/scripts/change-scope.test.mjs | 29 ++++++++ .github/scripts/spec-owner-reconcile.test.mjs | 25 +++++++ scripts/check-knowledge.mjs | 37 ++++++++-- scripts/check-knowledge.test.mjs | 71 +++++++++++++++++++ 5 files changed, 162 insertions(+), 9 deletions(-) diff --git a/.github/scripts/change-scope.cjs b/.github/scripts/change-scope.cjs index 1c09247fe89c4..81895ba8a3303 100644 --- a/.github/scripts/change-scope.cjs +++ b/.github/scripts/change-scope.cjs @@ -23,13 +23,13 @@ const SPEC_RECORD_PATTERNS = [ const CHANGESET_PATTERN = /^\.changeset\/(?!README\.md$)[^/]+\.md$/; -const THEME_DOC_CANDIDATE = /^docs\/themes\/(?!README\.md$)[^/]+\.md$/; +const THEME_DOC_PREFIX = 'docs/themes/'; +const THEME_DOC_GUIDANCE = `${THEME_DOC_PREFIX}README.md`; const THEME_PACKAGE_CANDIDATE = /^packages\/themes\/[^/]+\/(?:.*\/)?[^/]+\.spec\.md$/; const KNOWLEDGE_RECORD_PATTERNS = [ ...SPEC_RECORD_PATTERNS, - THEME_DOC_CANDIDATE, THEME_PACKAGE_CANDIDATE, /^docs\/architecture\/(?!README\.md$)[^/]+\.md$/, /^docs\/design\/assets\//, @@ -59,8 +59,13 @@ function isPackageReleasePath(filePath) { } function isKnowledgeRecordPath(filePath) { + const isThemeDocCandidate = + filePath.startsWith(THEME_DOC_PREFIX) && + filePath.endsWith('.md') && + filePath !== THEME_DOC_GUIDANCE; return ( isSpecRecordPath(filePath) || + isThemeDocCandidate || KNOWLEDGE_RECORD_PATTERNS.some(pattern => pattern.test(filePath)) ); } diff --git a/.github/scripts/change-scope.test.mjs b/.github/scripts/change-scope.test.mjs index 9fae95f040d6e..acbad5209861b 100644 --- a/.github/scripts/change-scope.test.mjs +++ b/.github/scripts/change-scope.test.mjs @@ -106,8 +106,37 @@ describe('spec-only change scope', () => { expect(classifyChanges([{filename}]).specOnly).toBe(false); }); + it('classifies allowed, canonical, and nested theme paths distinctly', () => { + const guidance = classifyChanges([{filename: 'docs/themes/README.md'}]); + expect(guidance.touchesKnowledgeRecords).toBe(false); + expect(guidance.specOnly).toBe(false); + + const canonical = classifyChanges([ + {filename: 'packages/themes/neutral/neutral.spec.md'}, + ]); + expect(canonical.touchesKnowledgeRecords).toBe(true); + expect(canonical.specOnly).toBe(true); + + const nestedCandidate = classifyChanges([ + {filename: 'docs/themes/subdir/neutral.md'}, + ]); + expect(nestedCandidate.touchesKnowledgeRecords).toBe(true); + expect(nestedCandidate.specOnly).toBe(false); + + const minimalCandidate = classifyChanges([{filename: 'docs/themes/.md'}]); + expect(minimalCandidate.touchesKnowledgeRecords).toBe(true); + expect(minimalCandidate.specOnly).toBe(false); + + const nonMarkdown = classifyChanges([ + {filename: 'docs/themes/subdir/neutral.md.txt'}, + ]); + expect(nonMarkdown.touchesKnowledgeRecords).toBe(false); + expect(nonMarkdown.specOnly).toBe(false); + }); + it.each([ 'docs/themes/neutral.md', + 'docs/themes/subdir/neutral.md', 'packages/themes/neutral/Theme.spec.md', 'packages/themes/neutral/subdir/neutral.spec.md', ])('treats misplaced theme candidate %s as unsafe knowledge', filename => { diff --git a/.github/scripts/spec-owner-reconcile.test.mjs b/.github/scripts/spec-owner-reconcile.test.mjs index 234f950c1cddc..9b15730dc8f27 100644 --- a/.github/scripts/spec-owner-reconcile.test.mjs +++ b/.github/scripts/spec-owner-reconcile.test.mjs @@ -568,8 +568,33 @@ describe('spec owner workflow reconciliation', () => { expect(harness.state.pr.auto_merge).toBe(null); }); + it('treats the allowed theme guidance index as no knowledge change', async () => { + const harness = createHarness({ + changedFile: {filename: 'docs/themes/README.md', status: 'modified'}, + headContent: '# Theme guidance\n', + baseContent: '# Theme guidance\n', + }); + + await run( + harness, + context({ + runId: 100n, + action: 'synchronize', + actor: 'cixzhang', + }), + ); + + expect(latestGateStatus(harness.state)).toMatchObject({ + state: 'success', + description: 'No knowledge records changed.', + }); + expect(harness.state.calls).not.toContain('enable-auto-merge'); + }); + it.each([ 'docs/themes/neutral.md', + 'docs/themes/.md', + 'docs/themes/subdir/neutral.md', 'packages/themes/neutral/Theme.spec.md', 'packages/themes/neutral/subdir/neutral.spec.md', ])( diff --git a/scripts/check-knowledge.mjs b/scripts/check-knowledge.mjs index 32c81f45a88c5..c4ad42b500623 100644 --- a/scripts/check-knowledge.mjs +++ b/scripts/check-knowledge.mjs @@ -120,7 +120,7 @@ function matchingFiles(directory, predicate) { function matchingFilesRecursively( directory, predicate, - {skipDirectory = () => false} = {}, + {skipDirectory = () => false, includeSymlinks = false} = {}, ) { if (!fs.existsSync(directory)) return []; const matches = []; @@ -134,7 +134,10 @@ function matchingFilesRecursively( continue; } pending.push(candidate); - } else if (entry.isFile() && predicate(entry.name, candidate)) { + } else if ( + (entry.isFile() || (includeSymlinks && entry.isSymbolicLink())) && + predicate(entry.name, candidate) + ) { matches.push(candidate); } } @@ -146,11 +149,31 @@ export function discoverThemeRecordCandidates(root = DEFAULT_ROOT) { const records = []; const problems = []; - for (const filePath of matchingFiles( - path.join(root, 'docs/themes'), - name => name.endsWith('.md') && name !== 'README.md', - )) { - records.push(filePath); + const themeDocsDirectory = path.join(root, 'docs/themes'); + const allowedThemeDocs = new Set([ + path.join(themeDocsDirectory, 'README.md'), + ]); + let themeDocsIsDirectory = false; + try { + themeDocsIsDirectory = fs.lstatSync(themeDocsDirectory).isDirectory(); + if (!themeDocsIsDirectory) { + problems.push( + 'docs/themes: theme guidance root must be a directory and may not be a symlink.', + ); + } + } catch (error) { + if (error.code !== 'ENOENT') throw error; + } + for (const filePath of themeDocsIsDirectory + ? matchingFilesRecursively( + themeDocsDirectory, + name => name.endsWith('.md'), + {includeSymlinks: true}, + ) + : []) { + const isRegularFile = fs.lstatSync(filePath).isFile(); + if (isRegularFile && allowedThemeDocs.has(filePath)) continue; + if (isRegularFile) records.push(filePath); problems.push( `${path.relative(root, filePath)}: theme records must be placed at packages/themes//.spec.md; docs/themes contains guidance only.`, ); diff --git a/scripts/check-knowledge.test.mjs b/scripts/check-knowledge.test.mjs index 311eabbea1e05..fdeab2303eb7e 100644 --- a/scripts/check-knowledge.test.mjs +++ b/scripts/check-knowledge.test.mjs @@ -1175,12 +1175,83 @@ describe('knowledge validation', () => { expect(await validateKnowledgeRoot(root)).toEqual([]); }); + it('does not follow symlink directories while scanning theme guidance', async () => { + const root = fixtureRoot(); + const outside = path.join(root, 'outside-theme-docs'); + fs.mkdirSync(outside); + fs.writeFileSync(path.join(outside, 'neutral.md'), themeRecord()); + fs.symlinkSync(outside, path.join(root, 'docs/themes/linked'), 'dir'); + + const discovered = discoverKnowledgeRecords(root).map(filePath => + path.relative(root, filePath), + ); + expect(discovered).not.toContain('docs/themes/linked/neutral.md'); + expect(await validateKnowledgeRoot(root)).toEqual([]); + }); + + it('rejects a symlinked theme guidance root without following it', async () => { + const root = fixtureRoot(); + const outside = path.join(root, 'outside-theme-docs'); + fs.mkdirSync(outside); + fs.writeFileSync( + path.join(outside, 'README.md'), + '# Not repository guidance\n', + ); + fs.rmSync(path.join(root, 'docs/themes'), {recursive: true}); + fs.symlinkSync(outside, path.join(root, 'docs/themes'), 'dir'); + + const problems = (await validateKnowledgeRoot(root)).join('\n'); + expect(problems).toMatch( + /docs\/themes: theme guidance root must be a directory and may not be a symlink/, + ); + }); + + it('rejects Markdown symlinks without following their targets', async () => { + const root = fixtureRoot(); + const outside = path.join(root, 'outside-theme.md'); + fs.writeFileSync(outside, themeRecord()); + fs.symlinkSync(outside, path.join(root, 'docs/themes/linked.md')); + + const discovered = discoverKnowledgeRecords(root).map(filePath => + path.relative(root, filePath), + ); + expect(discovered).not.toContain('docs/themes/linked.md'); + const problems = (await validateKnowledgeRoot(root)).join('\n'); + expect(problems).toMatch( + /docs\/themes\/linked\.md: theme records must be placed/, + ); + expect(problems).not.toMatch(/outside-theme\.md/); + }); + + it('ignores nested non-Markdown files', async () => { + const root = fixtureRoot(); + const nested = path.join(root, 'docs/themes/subdir/neutral.md.txt'); + fs.mkdirSync(path.dirname(nested), {recursive: true}); + fs.writeFileSync(nested, themeRecord()); + + const discovered = discoverKnowledgeRecords(root).map(filePath => + path.relative(root, filePath), + ); + expect(discovered).not.toContain('docs/themes/subdir/neutral.md.txt'); + expect(await validateKnowledgeRoot(root)).toEqual([]); + }); + it.each([ [ 'docs guidance directory', 'docs/themes/neutral.md', /theme records must be placed at packages\/themes\/\/\.spec\.md/, ], + [ + 'minimal Markdown filename', + 'docs/themes/.md', + /theme records must be placed at packages\/themes\/\/\.spec\.md/, + ], + [ + 'nested docs guidance path', + 'docs/themes/subdir/neutral.md', + /theme records must be placed at packages\/themes\/\/\.spec\.md/, + ], [ 'wrong package filename', 'packages/themes/neutral/Theme.spec.md',