From 5be2a965bcc0a10f5ad51d9aed6413a451fc0fa1 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sat, 3 Oct 2026 16:29:32 -0400 Subject: [PATCH 1/9] build: read frontmatter once and stop name: reading the next line The skills-roster check parsed SKILL.md with its own regex, and its `^name:\s*` let `\s` run across the newline, so an empty `name:` took the next line as the name. It was also the third frontmatter parser in these scripts. gen-mcp-index's reader moves to lib/api-page.mjs as `frontmatter`, and both it and the check use it. The Agent Skills spec requires `name`, so a missing or empty one is now its own violation instead of a confusing mismatch. --- scripts/check-conformance.mjs | 29 +++++++++++++++-------------- scripts/gen-mcp-index.mjs | 18 ++++++------------ scripts/lib/api-page.mjs | 20 +++++++++++++++++++- scripts/skills-roster.test.mjs | 28 +++++++++++++++++++++++++++- 4 files changed, 67 insertions(+), 28 deletions(-) diff --git a/scripts/check-conformance.mjs b/scripts/check-conformance.mjs index 14b59e30a..989fee523 100644 --- a/scripts/check-conformance.mjs +++ b/scripts/check-conformance.mjs @@ -95,6 +95,7 @@ import { } from './lib/pack-time-protocols.mjs'; import { fenceMask, + frontmatter, readRegions, sectionSpans, splitLines, @@ -3443,16 +3444,22 @@ export function skillsPageViolations(skills, pageFiles) { } /** - * Frontmatter `name:` must equal the directory name. Every prose roster and - * install line is held to the DIRECTORY name, while gen-mcp-index.mjs keys the - * shipped MCP manifest off the FRONTMATTER (`fm.name || name`, and promptName - * derives from it) — with no gate, one edit ships two disagreeing rosters - * while everything stays green. + * Frontmatter `name:` must be present and equal the directory name. Every + * prose roster and install line is held to the DIRECTORY name, while + * gen-mcp-index.mjs keys the shipped MCP manifest off the FRONTMATTER + * (`fm.name || name`, and promptName derives from it) — with no gate, one + * edit ships two disagreeing rosters while everything stays green. The Agent + * Skills spec also requires the field, so a missing or empty one fails too. */ export function frontmatterNameViolations(entries) { const violations = []; for (const { name, fmName } of entries) { - if (fmName && fmName !== name) { + if (!fmName) { + violations.push( + `skills/${name}/SKILL.md: frontmatter has no name. The Agent Skills ` + + `spec requires one, equal to the directory name. Add "name: ${name}".` + ); + } else if (fmName !== name) { violations.push( `skills/${name}/SKILL.md: frontmatter says "name: ${fmName}" but the ` + `directory is ${name}. The rosters follow the directory and the MCP ` + @@ -3464,12 +3471,6 @@ export function frontmatterNameViolations(entries) { return violations; } -function skillFrontmatterName(text) { - const fm = text.match(/^---\r?\n([\s\S]*?)\r?\n---/); - if (!fm) return null; - return fm[1].match(/^name:\s*['"]?([^'"\r\n]+?)['"]?\s*$/m)?.[1] ?? null; -} - async function checkSkillsRoster() { const skillsDir = join(REPO, 'skills'); @@ -3507,9 +3508,9 @@ async function checkSkillsRoster() { try { fmEntries.push({ name, - fmName: skillFrontmatterName( + fmName: frontmatter( await readFile(join(skillsDir, name, 'SKILL.md'), 'utf8') - ), + ).name, }); } catch { // The dir listing said SKILL.md exists; a read race is not this diff --git a/scripts/gen-mcp-index.mjs b/scripts/gen-mcp-index.mjs index ee11181f7..d8acdc33a 100644 --- a/scripts/gen-mcp-index.mjs +++ b/scripts/gen-mcp-index.mjs @@ -43,7 +43,12 @@ import { join, relative, dirname, basename, extname } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { createRequire } from 'node:module'; -import { sectionSpans, sectionBody, firstSentence } from './lib/api-page.mjs'; +import { + frontmatter, + sectionSpans, + sectionBody, + firstSentence, +} from './lib/api-page.mjs'; import { readSkillNames } from './lib/skills.mjs'; import { docsRoute } from './lib/docs-url.mjs'; import { extractComponent, varRootCandidates } from './lib/props-extract.mjs'; @@ -137,17 +142,6 @@ async function mdFiles(dir) { return out.sort(byCodePoint); } -function frontmatter(src) { - const m = src.match(/^---\r?\n([\s\S]*?)\r?\n---/); - if (!m) return {}; - const out = {}; - for (const line of m[1].split(/\r?\n/)) { - const kv = line.match(/^([A-Za-z_][\w-]*):[ \t]*(.*)$/); - if (kv) out[kv[1]] = kv[2].trim().replace(/^['"]|['"]$/g, ''); - } - return out; -} - /** Body of the `## Overview` section, first sentence, clipped for the catalog. */ function purposeOf(fm, sections, lines) { const explicit = collapse(fm.description); diff --git a/scripts/lib/api-page.mjs b/scripts/lib/api-page.mjs index 9813e1494..e74437def 100644 --- a/scripts/lib/api-page.mjs +++ b/scripts/lib/api-page.mjs @@ -1,6 +1,6 @@ /** * Markdown surgery for the API reference pages: generated-region markers, - * top-level section spans, and frontmatter upserts. + * top-level section spans, and frontmatter reads and upserts. * * Everything here is FENCE-AWARE. `docs/docs/api/helpers/theme.md` contains a * `---` inside a code fence, and several pages show HTML comments inside `html` @@ -222,6 +222,24 @@ export function sectionBody(lines, section) { return body.join('\n'); } +/** + * The flat `key: value` lines of a frontmatter block, values trimmed and + * unquoted, or {} when there is no block. The one reader for API pages and + * SKILL.md alike. Nested YAML is not parsed. A key with nothing after it + * reads as '', never as the line below it, and a repeated key keeps its last + * value. CRLF is tolerated. + */ +export function frontmatter(src) { + const m = src.match(/^---\r?\n([\s\S]*?)\r?\n---/); + if (!m) return {}; + const out = {}; + for (const line of m[1].split(/\r?\n/)) { + const kv = line.match(/^([A-Za-z_][\w-]*):[ \t]*(.*)$/); + if (kv) out[kv[1]] = kv[2].trim().replace(/^['"]|['"]$/g, ''); + } + return out; +} + /** * Insert or update a frontmatter key, preserving key order and the rest of the * block. Used for `description:` — `docusaurus-plugin-llms` picks each page's diff --git a/scripts/skills-roster.test.mjs b/scripts/skills-roster.test.mjs index 95a1c6004..37e5f982f 100644 --- a/scripts/skills-roster.test.mjs +++ b/scripts/skills-roster.test.mjs @@ -16,6 +16,7 @@ import { fileURLToPath } from 'node:url'; import { test } from 'node:test'; import assert from 'node:assert/strict'; +import { frontmatter } from './lib/api-page.mjs'; import { SKILL_ROSTERS, readSkillDirs, @@ -450,13 +451,38 @@ test('a frontmatter name that disagrees with the directory is caught', () => { const v = frontmatterNameViolations([ { name: 'bestax-optimize', fmName: 'bestax-css-optimize' }, { name: 'bestax-form', fmName: 'bestax-form' }, - { name: 'bestax-icons', fmName: null }, ]); assert.equal(v.length, 1, v.join('\n')); assert.match(v[0], /bestax-optimize/); assert.match(v[0], /bestax-css-optimize/); }); +test('a missing or empty frontmatter name is caught, not read off the next line', () => { + // The old reader matched `^name:\s*…$`, and `\s` crosses a newline, so an + // empty `name:` took the whole next line as the name. The shared reader + // reads it as empty, and the spec requires the field, so it fails as + // missing rather than as a confusing mismatch. + const empty = frontmatter( + '---\r\nname:\r\ndescription: Build forms.\r\n---\r\n# Form\r\n' + ); + assert.equal(empty.name, ''); + assert.equal(empty.description, 'Build forms.'); + for (const fmName of [ + empty.name, + frontmatter('---\nlicense: MIT\n---').name, + ]) { + const v = frontmatterNameViolations([{ name: 'bestax-form', fmName }]); + assert.equal(v.length, 1, v.join('\n')); + assert.match(v[0], /has no name/); + assert.match(v[0], /name: bestax-form/); + } + assert.equal( + frontmatter("---\nname: 'bestax-form'\n---").name, + 'bestax-form' + ); + assert.deepEqual(frontmatter('# no frontmatter\nname: x\n'), {}); +}); + test('the tests and the check derive the comparison set the same way', () => { // readSkillNames is the bundling view (what the sync scripts copy); // rosterSkillNames additionally drops names the prose patterns cannot From ab03abe6992dbda405997e35d00cfd9752474681 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sat, 3 Oct 2026 16:33:10 -0400 Subject: [PATCH 2/9] build: share the API page walk and its completeness guard gen-component-catalog and gen-mcp-index each carried their own category order, UNDOCUMENTED_EXPORTS, page walk, barrel parser and completeness guard. These move to scripts/lib/api-catalog.mjs, and both generators now read one list of titled pages from it. That also makes the two guards agree. The catalog generator counted a page as documented before checking its title, so an untitled page passed its guard while the catalog dropped it. gen-mcp-index counted only titled pages. One list now feeds both the output and the guard. gen-mcp-index's message said to add a page "whose frontmatter title: matches the export", but the guard matches on the file name. The shared message states the real rule. Both artifacts are byte-identical. --- scripts/api-catalog.test.mjs | 161 ++++++++++++++++++++++++++ scripts/coverage-floors.json | 1 + scripts/gen-component-catalog.mjs | 148 ++++-------------------- scripts/gen-mcp-index.mjs | 131 +++------------------ scripts/gen-mcp-index.test.mjs | 4 +- scripts/lib/api-catalog.mjs | 186 ++++++++++++++++++++++++++++++ 6 files changed, 388 insertions(+), 243 deletions(-) create mode 100644 scripts/api-catalog.test.mjs create mode 100644 scripts/lib/api-catalog.mjs diff --git a/scripts/api-catalog.test.mjs b/scripts/api-catalog.test.mjs new file mode 100644 index 000000000..1ef438c04 --- /dev/null +++ b/scripts/api-catalog.test.mjs @@ -0,0 +1,161 @@ +/** + * Holds scripts/lib/api-catalog.mjs, the API page walk and completeness guard + * that gen-component-catalog.mjs and gen-mcp-index.mjs share. The fixtures + * build a small docs/docs/api tree, because the real one has no untitled page + * and no unknown category, so the branches that matter would never run. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { readFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { + CATEGORY_ORDER, + mdFiles, + missingApiPages, + missingApiPagesMessage, + orderCategories, + parseExportedComponents, + readApiPages, +} from './lib/api-catalog.mjs'; + +const repo = rel => fileURLToPath(new URL(`../${rel}`, import.meta.url)); + +/** A docs/docs/api tree in a temp dir, from { 'cat/file.md': source }. */ +function fixture(t, files) { + const dir = mkdtempSync(join(tmpdir(), 'bestax-api-catalog-')); + t.after(() => rmSync(dir, { recursive: true, force: true })); + for (const [rel, src] of Object.entries(files)) { + mkdirSync(join(dir, rel, '..'), { recursive: true }); + writeFileSync(join(dir, rel), src); + } + return dir; +} + +const page = title => `---\ntitle: ${title}\n---\n\n## Overview\n`; + +test('an untitled page documents nothing, in the catalog or the guard', async t => { + // The catalog generator used to count a page for the guard before checking + // its title, so an untitled page passed the guard and was dropped from the + // catalog. One list now feeds both. + const dir = fixture(t, { + 'elements/button.md': page('Button'), + 'elements/box.md': '# Box, with no frontmatter\n', + 'helpers/config.md': page('ConfigProvider'), + }); + const categories = await readApiPages(dir); + const elements = categories.find(c => c.dir === 'elements'); + assert.deepEqual( + elements.pages.map(p => p.relPath), + ['elements/button.md'] + ); + const barrel = [ + "export * from './elements/Button';", + "export * from './elements/Box';", + // Keyed on the file name, so a page titled differently still counts. + "export * from './helpers/Config';", + ].join('\n'); + assert.deepEqual(missingApiPages(barrel, categories), ['elements/Box']); +}); + +test('the guard exempts hooks, Base variants and pages documented elsewhere', async t => { + const dir = fixture(t, { 'form/input.md': page('Input') }); + const barrel = [ + "export * from './form/Input';", + "export * from './form/InputBase';", + "export { useThing } from './helpers/useThing';", + "export { Thead, Tr } from './elements/Table';", + "export type { InputProps } from './form/Input';", + "export { Missing } from './layout/Missing';", + ].join('\r\n'); + assert.deepEqual(missingApiPages(barrel, await readApiPages(dir)), [ + 'layout/Missing', + ]); +}); + +test('the barrel parser reads both export forms and takes the alias', () => { + assert.deepEqual( + parseExportedComponents( + [ + "export * from './elements/Button';", + "export { A, B as C } from './components/Mod';", + "export * from './index';", + ].join('\n') + ), + [ + { name: 'Button', cat: 'elements' }, + { name: 'A', cat: 'components' }, + { name: 'C', cat: 'components' }, + ] + ); +}); + +test('the guard message states the rule the guard applies', () => { + const msg = missingApiPagesMessage(['form/Foo', 'layout/Bar'], 'the index'); + assert.match(msg, /2 exported component\(s\)/); + assert.match(msg, /missing from the index:\n {2}form\/Foo\n {2}layout\/Bar/); + // It matches on the file name, so the message must not say title. + assert.match(msg, /file name is the export's name/); + assert.match(msg, /frontmatter title:/); + assert.match(msg, /UNDOCUMENTED_EXPORTS in scripts\/lib\/api-catalog\.mjs/); +}); + +test('known categories keep their order and an unknown one is appended', async t => { + assert.deepEqual(orderCategories(['zeta', 'helpers', 'elements', 'beta']), [ + ['elements', 'Elements'], + ['helpers', 'Helpers'], + ['beta', 'Beta'], + ['zeta', 'Zeta'], + ]); + const dir = fixture(t, { + 'zeta/z.md': page('Z'), + 'elements/nested/deep.md': page('Deep'), + 'elements/a.md': page('A'), + }); + const categories = await readApiPages(dir); + assert.deepEqual( + categories.map(c => [c.dir, c.label]), + [ + ['elements', 'Elements'], + ['zeta', 'Zeta'], + ] + ); + assert.deepEqual( + categories[0].pages.map(p => p.slug), + ['elements/a', 'elements/nested/deep'] + ); +}); + +test('mdFiles walks every depth, sorts by code point, and takes a pattern', async t => { + const dir = fixture(t, { + 'b.md': '', + 'a/Z.md': '', + 'a/c.mdx': '', + 'a/notes.txt': '', + }); + const rel = files => files.map(f => f.slice(dir.length + 1)); + assert.deepEqual(rel(await mdFiles(dir)), ['a/Z.md', 'b.md']); + assert.deepEqual(rel(await mdFiles(dir, /\.mdx?$/)), [ + 'a/Z.md', + 'a/c.mdx', + 'b.md', + ]); +}); + +test('every exported component of the real library has a page', async () => { + const categories = await readApiPages(repo('docs/docs/api')); + assert.deepEqual( + categories.slice(0, CATEGORY_ORDER.length).map(c => c.dir), + CATEGORY_ORDER.map(([dir]) => dir) + ); + assert.deepEqual( + missingApiPages( + await readFile(repo('bulma-ui/src/index.ts'), 'utf8'), + categories + ), + [] + ); +}); diff --git a/scripts/coverage-floors.json b/scripts/coverage-floors.json index 076a48dcd..b48d105a9 100644 --- a/scripts/coverage-floors.json +++ b/scripts/coverage-floors.json @@ -13,6 +13,7 @@ "scripts/gen-mcp-index.mjs": { "lines": 86, "branches": 85, "functions": 88 }, "scripts/gen-skills-rosters.mjs": { "lines": 72, "branches": 20, "functions": 50 }, "scripts/handoff-message.mjs": { "lines": 100, "branches": 100, "functions": 100 }, + "scripts/lib/api-catalog.mjs": { "lines": 100, "branches": 100, "functions": 100 }, "scripts/lib/api-page.mjs": { "lines": 69, "branches": 76, "functions": 44 }, "scripts/lib/api-sources.mjs": { "lines": 100, "branches": 100, "functions": 100 }, "scripts/lib/bypass-annotations.mjs": { "lines": 100, "branches": 89, "functions": 100 }, diff --git a/scripts/gen-component-catalog.mjs b/scripts/gen-component-catalog.mjs index 1c14ab49d..44705a321 100644 --- a/scripts/gen-component-catalog.mjs +++ b/scripts/gen-component-catalog.mjs @@ -26,11 +26,17 @@ * deterministic across machines/Node versions, so sorting is by code point (not * locale collation) and parsing tolerates CRLF. */ -import { readFile, readdir, writeFile } from 'node:fs/promises'; -import { join, relative, dirname, basename } from 'node:path'; +import { readFile, writeFile } from 'node:fs/promises'; +import { join, relative, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; import { docsRoute } from './lib/docs-url.mjs'; +import { + missingApiPages, + missingApiPagesMessage, + readApiPages, +} from './lib/api-catalog.mjs'; +import { byCodePoint } from './lib/skills.mjs'; const HERE = dirname(fileURLToPath(import.meta.url)); const REPO = join(HERE, '..'); @@ -46,61 +52,6 @@ const OUT = join( const DOCS_BASE = 'https://bestax.io/docs/api'; const MAX_PURPOSE = 160; -// Preferred display order + human labels for the category dirs under -// docs/docs/api. Categories NOT listed here are still included (appended, -// alphabetically, with a title-cased label) so a new api category is never -// silently dropped. -const CATEGORY_ORDER = [ - ['elements', 'Elements'], - ['components', 'Components'], - ['form', 'Form'], - ['columns', 'Columns'], - ['grid', 'Grid'], - ['layout', 'Layout'], - ['helpers', 'Helpers'], -]; - -// Exported names that intentionally have NO standalone API page (they're -// documented on a parent page). A NEW component missing its page will NOT be -// here, so it gets flagged by the completeness guard. (`*Base` escape-hatch -// variants are excluded by rule, not listed here.) -const UNDOCUMENTED_EXPORTS = new Set([ - 'Tbody', - 'Td', - 'Tfoot', - 'Th', - 'Thead', - 'Tr', // documented on the Table page -]); - -// Deterministic, locale-independent comparator (code-point order). localeCompare -// varies with the runtime's ICU version and would make CI's regenerate-and-diff -// check flake across Node versions. -const byCodePoint = (a, b) => (a < b ? -1 : a > b ? 1 : 0); - -async function subdirs(dir) { - const entries = await readdir(dir, { withFileTypes: true }); - return entries.filter(e => e.isDirectory()).map(e => e.name); -} - -async function mdFiles(dir) { - const out = []; - for (const entry of await readdir(dir, { withFileTypes: true })) { - const full = join(dir, entry.name); - if (entry.isDirectory()) out.push(...(await mdFiles(full))); - else if (entry.name.endsWith('.md')) out.push(full); - } - return out; -} - -function frontmatterTitle(src) { - // Tolerate CRLF: match up to the closing `---` on its own line. - const m = src.match(/^---\r?\n([\s\S]*?)\r?\n---/); - if (!m) return null; - const t = m[1].match(/^title:[ \t]*(.+?)[ \t]*$/m); - return t ? t[1].replace(/^['"]|['"]$/g, '') : null; -} - // Drop a trailing unbalanced inline-code backtick left by truncation. function balanceBackticks(s) { if ((s.match(/`/g) || []).length % 2 === 0) return s; @@ -155,62 +106,21 @@ function overviewSentence(src) { return clip(s); } -// Parse the public barrel for exported component names (Uppercase-initial value -// exports). Used only by the completeness guard. -function parseExportedComponents(src) { - const out = []; - for (const line of src.split(/\r?\n/)) { - let m = line.match(/^export \* from '\.\/([^/]+)\/([^'/]+)'/); - if (m) { - out.push({ name: m[2], cat: m[1] }); - continue; - } - // `export { A, B } from './cat/Mod'` — value exports only (not `export type`). - m = line.match(/^export \{ ([^}]+) \} from '\.\/([^/]+)\/([^'/]+)'/); - if (m) { - for (const raw of m[1].split(',')) { - const name = raw - .trim() - .split(/\s+as\s+/) - .pop(); - if (name) out.push({ name, cat: m[2] }); - } - } - } - return out; -} - async function main() { - // Discover category dirs; order the known ones first, append any unknown. - const present = new Set(await subdirs(API_DIR)); - const known = CATEGORY_ORDER.filter(([dir]) => present.has(dir)); - const knownDirs = new Set(known.map(([dir]) => dir)); - const extra = [...present] - .filter(dir => !knownDirs.has(dir)) - .sort(byCodePoint) - .map(dir => [dir, dir.charAt(0).toUpperCase() + dir.slice(1)]); - const categories = [...known, ...extra]; + // Category dirs in display order, each with its titled pages. The + // completeness guard below reads the same list, so a page it counts as + // documented is a page this catalog lists. + const categories = await readApiPages(API_DIR); const sections = []; - const documentedByCat = new Map(); // cat -> Set of page basenames (lowercased) let total = 0; - for (const [dir, label] of categories) { - const files = (await mdFiles(join(API_DIR, dir))).sort(byCodePoint); - const pages = new Set(); - const rows = []; - for (const file of files) { - pages.add(basename(file, '.md').toLowerCase()); - const src = await readFile(file, 'utf8'); - const title = frontmatterTitle(src); - if (!title) continue; - const slug = relative(API_DIR, file) - .replace(/\.md$/, '') - .split('\\') - .join('/'); - rows.push({ title, purpose: overviewSentence(src), slug }); - } - documentedByCat.set(dir, pages); + for (const { label, pages } of categories) { + const rows = pages.map(({ src, fm, slug }) => ({ + title: fm.title, + purpose: overviewSentence(src), + slug, + })); rows.sort((a, b) => byCodePoint(a.title, b.title)); if (!rows.length) continue; total += rows.length; @@ -271,27 +181,9 @@ ${sections.join('\n\n')} process.stdout.write(`Wrote ${relative(REPO, OUT)} (${total} components)\n`); // Completeness guard: every exported component must have an API page. - const exports = parseExportedComponents(await readFile(INDEX_TS, 'utf8')); - const missing = exports - .filter( - e => - /^[A-Z]/.test(e.name) && // components are PascalCase (skip hooks/utils) - !e.name.endsWith('Base') && // raw escape-hatch variants: documented w/ wrapper - !UNDOCUMENTED_EXPORTS.has(e.name) && - !documentedByCat.get(e.cat)?.has(e.name.toLowerCase()) - ) - .map(e => `${e.cat}/${e.name}`) - .sort(byCodePoint); - + const missing = missingApiPages(await readFile(INDEX_TS, 'utf8'), categories); if (missing.length) { - console.error( - `\nERROR: ${missing.length} exported component(s) have no API page under docs/docs/api/ ` + - `and are missing from the catalog:\n ` + - missing.join('\n ') + - `\n\nAdd an API page (docs/docs/api//.md), or if the export ` + - `is intentionally undocumented, add it to UNDOCUMENTED_EXPORTS in ` + - `scripts/gen-component-catalog.mjs.\n` - ); + console.error(missingApiPagesMessage(missing, 'the catalog')); process.exit(1); } } diff --git a/scripts/gen-mcp-index.mjs b/scripts/gen-mcp-index.mjs index d8acdc33a..4e203b3ae 100644 --- a/scripts/gen-mcp-index.mjs +++ b/scripts/gen-mcp-index.mjs @@ -39,7 +39,7 @@ */ import { readFile, writeFile, readdir, mkdir, rm } from 'node:fs/promises'; import { existsSync } from 'node:fs'; -import { join, relative, dirname, basename, extname } from 'node:path'; +import { join, relative, dirname, extname } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { createRequire } from 'node:module'; @@ -50,6 +50,12 @@ import { firstSentence, } from './lib/api-page.mjs'; import { readSkillNames } from './lib/skills.mjs'; +import { + mdFiles, + missingApiPages, + missingApiPagesMessage, + readApiPages, +} from './lib/api-catalog.mjs'; import { docsRoute } from './lib/docs-url.mjs'; import { extractComponent, varRootCandidates } from './lib/props-extract.mjs'; import { componentVars } from './lib/scss-vars.mjs'; @@ -90,31 +96,6 @@ const SCHEMA_VERSION = 1; // tool call away on the component's own file. Same budget the skill catalog uses. const MAX_PURPOSE = 160; -// Display order + human labels for the category dirs, mirroring -// gen-component-catalog.mjs. Categories NOT listed are appended alphabetically -// with a title-cased label, so a new api category is never silently dropped. -const CATEGORY_ORDER = [ - ['elements', 'Elements'], - ['components', 'Components'], - ['form', 'Form'], - ['columns', 'Columns'], - ['grid', 'Grid'], - ['layout', 'Layout'], - ['helpers', 'Helpers'], -]; - -// Exported names that intentionally have NO standalone API page (documented on -// a parent page). A NEW component missing its page will not be here, so the -// completeness guard flags it. -const UNDOCUMENTED_EXPORTS = new Set([ - 'Tbody', - 'Td', - 'Tfoot', - 'Th', - 'Thead', - 'Tr', // documented on the Table page -]); - // Deterministic, locale-independent comparator. localeCompare varies with the // runtime's ICU version and would make CI's regenerate-and-diff flake. const byCodePoint = (a, b) => (a < b ? -1 : a > b ? 1 : 0); @@ -124,24 +105,6 @@ const collapse = s => .replace(/\s+/g, ' ') .trim(); -async function subdirs(dir) { - const entries = await readdir(dir, { withFileTypes: true }); - return entries - .filter(e => e.isDirectory()) - .map(e => e.name) - .sort(byCodePoint); -} - -async function mdFiles(dir) { - const out = []; - for (const entry of await readdir(dir, { withFileTypes: true })) { - const full = join(dir, entry.name); - if (entry.isDirectory()) out.push(...(await mdFiles(full))); - else if (entry.name.endsWith('.md')) out.push(full); - } - return out.sort(byCodePoint); -} - /** Body of the `## Overview` section, first sentence, clipped for the catalog. */ function purposeOf(fm, sections, lines) { const explicit = collapse(fm.description); @@ -429,28 +392,6 @@ async function readSkills() { return out.sort((a, b) => byCodePoint(a.name, b.name)); } -function parseExportedComponents(src) { - const out = []; - for (const line of src.split(/\r?\n/)) { - let m = line.match(/^export \* from '\.\/([^/]+)\/([^'/]+)'/); - if (m) { - out.push({ name: m[2], cat: m[1] }); - continue; - } - m = line.match(/^export \{ ([^}]+) \} from '\.\/([^/]+)\/([^'/]+)'/); - if (m) { - for (const raw of m[1].split(',')) { - const name = raw - .trim() - .split(/\s+as\s+/) - .pop(); - if (name) out.push({ name, cat: m[2] }); - } - } - } - return out; -} - /** * Serialise through prettier, exactly as gen-api-docs.mjs does for markdown. * @@ -555,16 +496,10 @@ export async function bulmaClassTable() { } export async function build() { - const present = new Set(await subdirs(API_DIR)); - const known = CATEGORY_ORDER.filter(([dir]) => present.has(dir)); - const knownDirs = new Set(known.map(([dir]) => dir)); - const categories = [ - ...known, - ...[...present] - .filter(dir => !knownDirs.has(dir)) - .sort(byCodePoint) - .map(dir => [dir, dir.charAt(0).toUpperCase() + dir.slice(1)]), - ]; + // Category dirs in display order, each with its titled pages. The + // completeness guard reads the same list, so a page it counts as + // documented is a page this index serves. + const categories = await readApiPages(API_DIR); // Page path -> frontmatter title, for resolving Related Components links. const pageByPath = new Map(); @@ -579,28 +514,15 @@ export async function build() { const components = new Map(); const cssVarIndex = {}; const categoryList = []; - // category -> page basenames, for the completeness guard. Deliberately keyed - // on the FILE rather than the frontmatter title, matching - // gen-component-catalog.mjs: `export * from './helpers/Config'` names a - // module, and the page that documents it is `config.md` titled - // `ConfigProvider`. Matching on title would flag that as missing. - const pagesByCat = new Map(); - - for (const [dir, label] of categories) { + + for (const { dir, label, pages } of categories) { const members = []; - for (const file of await mdFiles(join(API_DIR, dir))) { - const src = await readFile(file, 'utf8'); - const relPath = relative(API_DIR, file).split('\\').join('/'); - const fm = frontmatter(src); - if (!fm.title) continue; + for (const { src, fm, relPath, slug } of pages) { const name = fm.title; - if (!pagesByCat.has(dir)) pagesByCat.set(dir, new Set()); - pagesByCat.get(dir).add(basename(file, '.md').toLowerCase()); const { lines, sections } = sectionSpans(src); const find = re => sections.find(s => re.test(s.heading)); const purpose = purposeOf(fm, sections, lines); - const slug = relPath.replace(/\.md$/, ''); // `slug` stays the file-path identity; the URL takes the route // Docusaurus actually serves, which collapses `grid/grid` (#597). const docsUrl = `${DOCS_BASE}/api/${docsRoute(slug)}`; @@ -719,15 +641,14 @@ export async function build() { return { catalog, components, - pagesByCat, + missing: missingApiPages(await readFile(INDEX_TS, 'utf8'), categories), skills: { skills: await readSkills() }, bulmaClasses: await bulmaClassTable(), }; } export async function main() { - const { catalog, components, pagesByCat, skills, bulmaClasses } = - await build(); + const { catalog, components, missing, skills, bulmaClasses } = await build(); // Rewrite the component directory rather than overwriting in place: a // component that was removed must lose its file, or the staleness gate @@ -750,26 +671,8 @@ export async function main() { // Completeness guard. Runs after writing so the failure names what to fix // rather than leaving a half-written index behind. - const exports = parseExportedComponents(await readFile(INDEX_TS, 'utf8')); - const missing = exports - .filter( - e => - /^[A-Z]/.test(e.name) && // components are PascalCase (skip hooks/utils) - !e.name.endsWith('Base') && // escape-hatch variants: documented w/ wrapper - !UNDOCUMENTED_EXPORTS.has(e.name) && - !pagesByCat.get(e.cat)?.has(e.name.toLowerCase()) - ) - .map(e => `${e.cat}/${e.name}`) - .sort(byCodePoint); - if (missing.length) { - console.error( - `\nERROR: ${missing.length} exported component(s) are missing from the MCP ` + - `index:\n ${missing.join('\n ')}\n\n` + - `Add an API page (docs/docs/api//.md) whose frontmatter ` + - `title: matches the export, or add the name to UNDOCUMENTED_EXPORTS in ` + - `scripts/gen-mcp-index.mjs if it is documented on a parent page.\n` - ); + console.error(missingApiPagesMessage(missing, 'the MCP index')); process.exit(1); } diff --git a/scripts/gen-mcp-index.test.mjs b/scripts/gen-mcp-index.test.mjs index 2dd18c80a..60690d5e1 100644 --- a/scripts/gen-mcp-index.test.mjs +++ b/scripts/gen-mcp-index.test.mjs @@ -28,11 +28,12 @@ const REPO = join(dirname(fileURLToPath(import.meta.url)), '..'); let catalog; let components; +let missing; let skills; let bulmaClasses; before(async () => { - ({ catalog, components, skills, bulmaClasses } = await build()); + ({ catalog, components, missing, skills, bulmaClasses } = await build()); }); test('the catalog pins the library version it was generated from', async () => { @@ -47,6 +48,7 @@ test('the catalog pins the library version it was generated from', async () => { test('every exported component reaches the index', () => { // The completeness guard in main() fails the build on this; assert it here // too so the reason is visible without reading a process exit code. + assert.deepEqual(missing, []); assert.ok(components.size >= 80, `only ${components.size} components`); for (const name of ['Button', 'Navbar', 'Field', 'Columns', 'Hero']) { assert.ok(components.has(name), `${name} missing from the index`); diff --git a/scripts/lib/api-catalog.mjs b/scripts/lib/api-catalog.mjs new file mode 100644 index 000000000..d0c7a8b67 --- /dev/null +++ b/scripts/lib/api-catalog.mjs @@ -0,0 +1,186 @@ +/** + * The API reference pages read as a catalog: which category directories + * exist and in what order, which pages each one lists, and whether every + * exported component has one. + * + * Two generators list every documented component, gen-component-catalog.mjs + * (the skill's component-catalog.md) and gen-mcp-index.mjs (the MCP server's + * catalog.json). Each used to carry its own copy of everything here, and the + * copies had drifted on which pages count as documenting an export. A catalog + * rule changes here or not at all. + * + * Output must be deterministic across machines and Node versions, so every + * listing sorts by code point, never with localeCompare. + */ +import { readFile, readdir } from 'node:fs/promises'; +import { basename, join, relative } from 'node:path'; + +import { frontmatter } from './api-page.mjs'; +import { byCodePoint } from './skills.mjs'; + +/** + * Preferred display order and human labels for the category dirs under + * docs/docs/api. Categories NOT listed here are still included (appended, + * alphabetically, with a title-cased label) so a new api category is never + * silently dropped. + */ +export const CATEGORY_ORDER = [ + ['elements', 'Elements'], + ['components', 'Components'], + ['form', 'Form'], + ['columns', 'Columns'], + ['grid', 'Grid'], + ['layout', 'Layout'], + ['helpers', 'Helpers'], +]; + +/** + * Exported names that intentionally have NO standalone API page (they're + * documented on a parent page). A NEW component missing its page will NOT be + * here, so it gets flagged by the completeness guard. (`*Base` escape-hatch + * variants are excluded by rule, not listed here.) + */ +export const UNDOCUMENTED_EXPORTS = new Set([ + 'Tbody', + 'Td', + 'Tfoot', + 'Th', + 'Thead', + 'Tr', // documented on the Table page +]); + +/** The names of the directories directly inside `dir`, sorted. */ +export async function subdirs(dir) { + const entries = await readdir(dir, { withFileTypes: true }); + return entries + .filter(e => e.isDirectory()) + .map(e => e.name) + .sort(byCodePoint); +} + +/** Every file under `dir`, at any depth, whose name matches `pattern`, sorted. */ +export async function mdFiles(dir, pattern = /\.md$/) { + const out = []; + for (const entry of await readdir(dir, { withFileTypes: true })) { + const full = join(dir, entry.name); + if (entry.isDirectory()) out.push(...(await mdFiles(full, pattern))); + else if (pattern.test(entry.name)) out.push(full); + } + return out.sort(byCodePoint); +} + +/** + * The category dirs in `present` as [dir, label] pairs: the ones + * CATEGORY_ORDER knows first, in its order, then the rest alphabetically with + * a title-cased label. + */ +export function orderCategories(present) { + const have = new Set(present); + const known = CATEGORY_ORDER.filter(([dir]) => have.has(dir)); + const knownDirs = new Set(known.map(([dir]) => dir)); + const extra = [...have] + .filter(dir => !knownDirs.has(dir)) + .sort(byCodePoint) + .map(dir => [dir, dir.charAt(0).toUpperCase() + dir.slice(1)]); + return [...known, ...extra]; +} + +/** + * Every page a catalog lists, by category in display order: + * [{ dir, label, pages: [{ file, relPath, slug, src, fm }] }], each category's + * pages sorted by path. A page with no frontmatter `title:` is left out, and + * missingApiPages reads this same list, so a page can never count as + * documenting an export while the catalog drops it. + */ +export async function readApiPages(apiDir) { + const categories = []; + for (const [dir, label] of orderCategories(await subdirs(apiDir))) { + const pages = []; + for (const file of await mdFiles(join(apiDir, dir))) { + const src = await readFile(file, 'utf8'); + const fm = frontmatter(src); + if (!fm.title) continue; + const relPath = relative(apiDir, file).split('\\').join('/'); + pages.push({ + file, + relPath, + slug: relPath.replace(/\.md$/, ''), + src, + fm, + }); + } + categories.push({ dir, label, pages }); + } + return categories; +} + +/** + * The barrel's exports as [{ name, cat }]: each `export * from './cat/Mod'` + * as the module name, and each name in `export { A, B } from './cat/Mod'` + * (value exports only, not `export type`). + */ +export function parseExportedComponents(src) { + const out = []; + for (const line of src.split(/\r?\n/)) { + let m = line.match(/^export \* from '\.\/([^/]+)\/([^'/]+)'/); + if (m) { + out.push({ name: m[2], cat: m[1] }); + continue; + } + m = line.match(/^export \{ ([^}]+) \} from '\.\/([^/]+)\/([^'/]+)'/); + if (m) { + for (const raw of m[1].split(',')) { + const name = raw + .trim() + .split(/\s+as\s+/) + .pop(); + if (name) out.push({ name, cat: m[2] }); + } + } + } + return out; +} + +/** + * The completeness guard: the components exported by the barrel source + * `indexSrc` that no page in `categories` (from readApiPages) documents, as + * sorted `category/Name` strings. Hooks and utilities (not PascalCase), + * `*Base` escape hatches and UNDOCUMENTED_EXPORTS are exempt. + * + * Keyed on the page's FILE name rather than its title, because + * `export * from './helpers/Config'` names a module, and the page that + * documents it is `config.md` titled `ConfigProvider`. Matching on the title + * would flag that as missing. + */ +export function missingApiPages(indexSrc, categories) { + const pagesByCat = new Map( + categories.map(({ dir, pages }) => [ + dir, + new Set(pages.map(p => basename(p.file, '.md').toLowerCase())), + ]) + ); + return parseExportedComponents(indexSrc) + .filter( + e => + /^[A-Z]/.test(e.name) && + !e.name.endsWith('Base') && + !UNDOCUMENTED_EXPORTS.has(e.name) && + !pagesByCat.get(e.cat)?.has(e.name.toLowerCase()) + ) + .map(e => `${e.cat}/${e.name}`) + .sort(byCodePoint); +} + +/** What a generator prints when `missing` is not empty. `from` names its output. */ +export function missingApiPagesMessage(missing, from) { + return ( + `\nERROR: ${missing.length} exported component(s) have no API page ` + + `under docs/docs/api/ and are missing from ${from}:\n ` + + missing.join('\n ') + + `\n\nA page counts when it sits under docs/docs/api//, its ` + + `file name is the export's name in any letter case (button.md for ` + + `Button), and it has a frontmatter title:. Add that page, or, if a ` + + `parent page documents the export, add the name to UNDOCUMENTED_EXPORTS ` + + `in scripts/lib/api-catalog.mjs.\n` + ); +} From 00af8fa32395c2f0b6d0be54d776ef8de352f27f Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sat, 3 Oct 2026 16:35:48 -0400 Subject: [PATCH 3/9] build: share the catalog one-liner pieces Both catalog generators kept a copy of the prose-line filter and the 160-character budget, and gen-component-catalog kept its own first-sentence rule while lib/api-page.mjs claimed firstSentence mirrored it. The copy had drifted: its lookahead after the period was `(?=\s)` where firstSentence has `(?=\s|$)`. Both run on a collapsed, trimmed line, so `$` only adds a match when the line ends in its only period, and that match is the whole line, which the old code returned anyway. No adjustment was needed. lib/api-catalog.mjs now holds MAX_PURPOSE, firstProseLine and both clips, named for the rule each applies. The generators still pick different clips and the MCP one still prefers the description, so both artifacts stay byte-identical. --- scripts/api-catalog.test.mjs | 72 ++++++++++++++++++++++++++++++ scripts/gen-component-catalog.mjs | 60 ++++--------------------- scripts/gen-mcp-index.mjs | 33 +++++--------- scripts/lib/api-catalog.mjs | 74 ++++++++++++++++++++++++++++++- scripts/lib/api-page.mjs | 14 +++--- 5 files changed, 171 insertions(+), 82 deletions(-) diff --git a/scripts/api-catalog.test.mjs b/scripts/api-catalog.test.mjs index 1ef438c04..9040c4c39 100644 --- a/scripts/api-catalog.test.mjs +++ b/scripts/api-catalog.test.mjs @@ -12,8 +12,13 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { firstSentence } from './lib/api-page.mjs'; import { CATEGORY_ORDER, + MAX_PURPOSE, + clipAtClause, + clipAtWord, + firstProseLine, mdFiles, missingApiPages, missingApiPagesMessage, @@ -159,3 +164,70 @@ test('every exported component of the real library has a page', async () => { [] ); }); + +test('the purpose line skips everything that is not a description', () => { + const body = [ + '', + ':::tip', + '', + "import X from './x';", + '### Sub', + '![shot](a.png)', + '- item', + '* item', + '| a | b |', + '> quote', + ' The `Box` component wraps content. ', + 'Second line.', + ].join('\r\n'); + assert.equal(firstProseLine(body), 'The `Box` component wraps content.'); + assert.equal(firstProseLine(':::note\n- only a list\n'), ''); +}); + +test('firstSentence keeps only a sentence long enough to stand alone', () => { + const s = 'The `Box` wraps content in a padded panel with a border. More.'; + assert.equal( + firstSentence(s), + 'The `Box` wraps content in a padded panel with a border.' + ); + // A period too early to end a real sentence, an abbreviation included, + // keeps the whole line. + assert.equal( + firstSentence('Use e.g. a card. Then more.'), + 'Use e.g. a card. Then more.' + ); + // A line ending in its only period comes back whole either way. + const one = 'A single sentence that runs past the forty character mark.'; + assert.equal(firstSentence(one), one); +}); + +test('the two catalog clips keep short text and cut long text their own way', () => { + const short = 'A short purpose.'; + assert.equal(clipAtClause(short), short); + assert.equal(clipAtWord(short), short); + + const words = 'word '.repeat(60).trim(); + for (const clip of [clipAtClause, clipAtWord]) { + const out = clip(words); + assert.ok(out.length <= MAX_PURPOSE + 1, out); + assert.ok(out.endsWith('word…'), out); + } + + // The skill catalog prefers the last clause boundary and drops the comma. + const clauses = `${'a'.repeat(60)}, ${'b'.repeat(60)}, ${'c '.repeat(40)}`; + assert.equal(clipAtClause(clauses), `${'a'.repeat(60)}, ${'b'.repeat(60)}…`); + // The MCP catalog cuts at the last word inside the budget instead. + assert.ok(clipAtWord(clauses).endsWith('c c…')); + + // A cut inside a code span drops the span rather than leave it open. + const code = `${'x'.repeat(30)} uses ${'y '.repeat(55)}\`zz, zz zz zz zz\` end`; + assert.equal( + clipAtClause(code), + `${'x'.repeat(30)} uses ${'y '.repeat(54)}y…` + ); + + // No boundary at all: a hard cut at the budget. + const solid = 'z'.repeat(200); + assert.equal(clipAtClause(solid), `${'z'.repeat(MAX_PURPOSE)}…`); + assert.equal(clipAtWord(solid), `${'z'.repeat(MAX_PURPOSE - 40)}…`); +}); diff --git a/scripts/gen-component-catalog.mjs b/scripts/gen-component-catalog.mjs index 44705a321..a498b215b 100644 --- a/scripts/gen-component-catalog.mjs +++ b/scripts/gen-component-catalog.mjs @@ -31,7 +31,10 @@ import { join, relative, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; import { docsRoute } from './lib/docs-url.mjs'; +import { firstSentence } from './lib/api-page.mjs'; import { + clipAtClause, + firstProseLine, missingApiPages, missingApiPagesMessage, readApiPages, @@ -50,60 +53,15 @@ const OUT = join( 'component-catalog.md' ); const DOCS_BASE = 'https://bestax.io/docs/api'; -const MAX_PURPOSE = 160; - -// Drop a trailing unbalanced inline-code backtick left by truncation. -function balanceBackticks(s) { - if ((s.match(/`/g) || []).length % 2 === 0) return s; - return s.slice(0, s.lastIndexOf('`')).trimEnd(); -} - -// Shorten a long purpose to the last natural boundary within the limit: prefer -// the last sentence/clause punctuation, fall back to a word boundary, then -// repair any split inline-code span and append an ellipsis. -function clip(s) { - if (s.length <= MAX_PURPOSE) return s; - let cut = s.slice(0, MAX_PURPOSE); - const lastPunct = Math.max( - cut.lastIndexOf('.'), - cut.lastIndexOf(','), - cut.lastIndexOf(';'), - cut.lastIndexOf(':'), - cut.lastIndexOf(')'), - cut.lastIndexOf(']') - ); - if (lastPunct >= 40) { - cut = cut.slice(0, lastPunct + 1); - } else { - const sp = cut.lastIndexOf(' '); - if (sp >= 40) cut = cut.slice(0, sp); - } - // Balance code spans, then drop a dangling opener or clause separator. - cut = balanceBackticks(cut) - .replace(/[ ([]+$/, '') - .replace(/[,;:]$/, ''); - return cut + '…'; -} +/** + * A page's catalog one-liner: the first sentence of the first prose line + * after its `## Overview` (or `### Overview`) heading, clipped at a clause. + */ function overviewSentence(src) { - // First prose line after the `## Overview` (or `### Overview`) heading — - // skip admonitions / HTML / import lines that aren't a description. const after = src.split(/^#{2,3}[ \t]+Overview[ \t]*$/m)[1]; - if (!after) return ''; - let line = ''; - for (const raw of after.split(/\r?\n/)) { - const t = raw.trim(); - if (!t) continue; - if (/^(:::|<|import\b|#|!\[|[-*|>])/.test(t)) continue; // not a prose sentence - line = t; - break; - } - if (!line) return ''; - let s = line.replace(/\s+/g, ' ').trim(); - // First sentence: a period ending a word of >1 char (skips "e.g." / "i.e."). - const sentence = s.match(/^.*?[A-Za-z0-9)"'`][.](?=\s)/); - if (sentence && sentence[0].length >= 40) s = sentence[0]; - return clip(s); + const line = after ? firstProseLine(after) : ''; + return line ? clipAtClause(firstSentence(line)) : ''; } async function main() { diff --git a/scripts/gen-mcp-index.mjs b/scripts/gen-mcp-index.mjs index 4e203b3ae..b1e2d6201 100644 --- a/scripts/gen-mcp-index.mjs +++ b/scripts/gen-mcp-index.mjs @@ -51,6 +51,8 @@ import { } from './lib/api-page.mjs'; import { readSkillNames } from './lib/skills.mjs'; import { + clipAtWord, + firstProseLine, mdFiles, missingApiPages, missingApiPagesMessage, @@ -92,10 +94,6 @@ const DOCS_BASE = 'https://bestax.io/docs'; */ const SCHEMA_VERSION = 1; -// Catalog one-liners are for scanning, not reading; the full summary is one -// tool call away on the component's own file. Same budget the skill catalog uses. -const MAX_PURPOSE = 160; - // Deterministic, locale-independent comparator. localeCompare varies with the // runtime's ICU version and would make CI's regenerate-and-diff flake. const byCodePoint = (a, b) => (a < b ? -1 : a > b ? 1 : 0); @@ -105,28 +103,17 @@ const collapse = s => .replace(/\s+/g, ' ') .trim(); -/** Body of the `## Overview` section, first sentence, clipped for the catalog. */ +/** + * A page's catalog one-liner: the frontmatter description, or else the first + * prose line of its `## Overview` section, cut to its first sentence and + * clipped at a word. + */ function purposeOf(fm, sections, lines) { - const explicit = collapse(fm.description); const overview = sections.find(s => /^Overview$/i.test(s.heading)); const text = - explicit || - collapse( - (overview ? sectionBody(lines, overview) : '') - .split(/\r?\n/) - // Skip admonitions, JSX, imports, headings, images, lists and quotes — - // the same filter gen-component-catalog.mjs applies. - .find(l => l.trim() && !/^(:::|<|import\b|#|!\[|[-*|>])/.test(l.trim())) - ); - if (!text) return ''; - const sentence = firstSentence(text); - if (sentence.length <= MAX_PURPOSE) return sentence; - const cut = sentence.slice(0, MAX_PURPOSE); - const at = Math.max(cut.lastIndexOf(' '), MAX_PURPOSE - 40); - return `${cut - .slice(0, at) - .replace(/[,;:([]$/, '') - .trim()}…`; + collapse(fm.description) || + firstProseLine(overview ? sectionBody(lines, overview) : ''); + return text ? clipAtWord(firstSentence(text)) : ''; } /** diff --git a/scripts/lib/api-catalog.mjs b/scripts/lib/api-catalog.mjs index d0c7a8b67..1ee10c4ee 100644 --- a/scripts/lib/api-catalog.mjs +++ b/scripts/lib/api-catalog.mjs @@ -1,7 +1,7 @@ /** * The API reference pages read as a catalog: which category directories - * exist and in what order, which pages each one lists, and whether every - * exported component has one. + * exist and in what order, which pages each one lists, whether every exported + * component has one, and the pieces each page's one-line purpose is cut from. * * Two generators list every documented component, gen-component-catalog.mjs * (the skill's component-catalog.md) and gen-mcp-index.mjs (the MCP server's @@ -49,6 +49,12 @@ export const UNDOCUMENTED_EXPORTS = new Set([ 'Tr', // documented on the Table page ]); +/** + * Catalog one-liners are for scanning, not reading. The full summary is one + * link or tool call away, so both catalogs clip to this budget. + */ +export const MAX_PURPOSE = 160; + /** The names of the directories directly inside `dir`, sorted. */ export async function subdirs(dir) { const entries = await readdir(dir, { withFileTypes: true }); @@ -114,6 +120,70 @@ export async function readApiPages(apiDir) { return categories; } +/** + * The first line of `text` that reads as prose, trimmed, or ''. Admonitions, + * JSX and HTML, imports, headings, images, lists, tables and quotes are not a + * description, so they are skipped. + */ +export function firstProseLine(text) { + for (const raw of String(text).split(/\r?\n/)) { + const t = raw.trim(); + if (t && !/^(:::|<|import\b|#|!\[|[-*|>])/.test(t)) return t; + } + return ''; +} + +/** Drop a trailing unbalanced inline-code backtick left by truncation. */ +function balanceBackticks(s) { + if ((s.match(/`/g) || []).length % 2 === 0) return s; + return s.slice(0, s.lastIndexOf('`')).trimEnd(); +} + +/** + * The skill catalog's clip. Shorten `s` to the last natural boundary within + * MAX_PURPOSE: prefer the last sentence or clause punctuation, fall back to a + * word boundary, then repair any split inline-code span and append an + * ellipsis. + */ +export function clipAtClause(s) { + if (s.length <= MAX_PURPOSE) return s; + let cut = s.slice(0, MAX_PURPOSE); + const lastPunct = Math.max( + cut.lastIndexOf('.'), + cut.lastIndexOf(','), + cut.lastIndexOf(';'), + cut.lastIndexOf(':'), + cut.lastIndexOf(')'), + cut.lastIndexOf(']') + ); + if (lastPunct >= 40) { + cut = cut.slice(0, lastPunct + 1); + } else { + const sp = cut.lastIndexOf(' '); + if (sp >= 40) cut = cut.slice(0, sp); + } + // Balance code spans, then drop a dangling opener or clause separator. + cut = balanceBackticks(cut) + .replace(/[ ([]+$/, '') + .replace(/[,;:]$/, ''); + return cut + '…'; +} + +/** + * The MCP catalog's clip. Shorten `s` at the last word boundary within + * MAX_PURPOSE (no earlier than 40 characters before it), drop a dangling + * opener or clause separator, and append an ellipsis. + */ +export function clipAtWord(s) { + if (s.length <= MAX_PURPOSE) return s; + const cut = s.slice(0, MAX_PURPOSE); + const at = Math.max(cut.lastIndexOf(' '), MAX_PURPOSE - 40); + return `${cut + .slice(0, at) + .replace(/[,;:([]$/, '') + .trim()}…`; +} + /** * The barrel's exports as [{ name, cat }]: each `export * from './cat/Mod'` * as the module name, and each name in `export { A, B } from './cat/Mod'` diff --git a/scripts/lib/api-page.mjs b/scripts/lib/api-page.mjs index e74437def..6ec0fb8ba 100644 --- a/scripts/lib/api-page.mjs +++ b/scripts/lib/api-page.mjs @@ -292,13 +292,15 @@ export function upsertFrontmatter(src, key, value, after = null) { } /** - * First sentence of a prose line. + * First sentence of a prose line: up to the first period that ends a word, + * before whitespace or the end of the line, and only when that is long + * enough to be a real sentence, so an early abbreviation like "e.g." keeps + * the whole line instead of cutting it short. * - * Deliberately the SAME rule as `overviewSentence()` in - * gen-component-catalog.mjs — a period ending a word of more than one character - * (so "e.g." doesn't split), and only when the result is long enough to be a - * real sentence. The generated Overview must round-trip through that function - * unchanged, or the skill catalog churns on every run. + * gen-api-docs writes the generated Overview with this, and both catalogs cut + * their one-liner with it, so the generated sentence round-trips through the + * catalogs unchanged. The skill catalog once kept its own copy, which had + * already drifted on what may follow the period. */ export function firstSentence(text) { const s = String(text).replace(/\s+/g, ' ').trim(); From 3d979621d8052139beee8a1046f4ccbff1626a16 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sat, 3 Oct 2026 16:37:56 -0400 Subject: [PATCH 4/9] build: import the pasted helpers from scripts/lib byCodePoint, frontmatterTitle and mdFiles were pasted into several scripts. Each now has one home: byCodePoint in lib/skills.mjs, frontmatterTitle in lib/api-page.mjs on top of the shared frontmatter reader, and mdFiles in lib/api-catalog.mjs. mdFiles takes a pattern, so check-conformance keeps reading .mdx too. gen-api-sources's function floor drops from 30 to 28. Its covered byCodePoint moved to lib/skills.mjs, which leaves 2 of 7 functions covered instead of 3 of 10. Nothing that ran before stopped running. Every artifact is byte-identical and conformance output is unchanged. --- scripts/check-conformance.mjs | 35 ++++++++++++----------------------- scripts/coverage-floors.json | 2 +- scripts/gen-api-docs.mjs | 23 +++-------------------- scripts/gen-api-sources.mjs | 22 +++------------------- scripts/gen-mcp-index.mjs | 6 +----- scripts/lib/api-page.mjs | 5 +++++ 6 files changed, 25 insertions(+), 68 deletions(-) diff --git a/scripts/check-conformance.mjs b/scripts/check-conformance.mjs index 989fee523..0bd031c9a 100644 --- a/scripts/check-conformance.mjs +++ b/scripts/check-conformance.mjs @@ -96,10 +96,12 @@ import { import { fenceMask, frontmatter, + frontmatterTitle, readRegions, sectionSpans, splitLines, } from './lib/api-page.mjs'; +import { mdFiles } from './lib/api-catalog.mjs'; import { SKILL_DIR_NAME, readSkillDirs as libReadSkillDirs, @@ -198,18 +200,12 @@ const STORY_EXEMPT = new Set([ ]); // --------------------------------------------------------------------------- -// Shared helpers (same parsing rules as gen-component-catalog.mjs). +// Shared helpers. The page walk and frontmatter reader are the generators' +// own (scripts/lib), so a page this checks is a page they read. // --------------------------------------------------------------------------- -async function mdFiles(dir) { - const out = []; - for (const entry of await readdir(dir, { withFileTypes: true })) { - const full = join(dir, entry.name); - if (entry.isDirectory()) out.push(...(await mdFiles(full))); - else if (/\.mdx?$/.test(entry.name)) out.push(full); - } - return out; -} +/** Every .md and .mdx file under `dir`, at any depth, sorted. */ +const docFiles = dir => mdFiles(dir, /\.mdx?$/); async function walk(dir, ext) { const out = []; @@ -221,13 +217,6 @@ async function walk(dir, ext) { return out; } -function frontmatterTitle(src) { - const m = src.match(/^---\r?\n([\s\S]*?)\r?\n---/); - if (!m) return null; - const t = m[1].match(/^title:[ \t]*(.+?)[ \t]*$/m); - return t ? t[1].replace(/^['"]|['"]$/g, '') : null; -} - function parseExportedModules(src) { const out = []; for (const line of src.split(/\r?\n/)) { @@ -242,7 +231,7 @@ function parseExportedModules(src) { // API pages by category: cat -> [{ title, relPath }] async function apiComponents() { const byCat = new Map(); - for (const file of await mdFiles(API_DIR)) { + for (const file of await docFiles(API_DIR)) { const title = frontmatterTitle(await readFile(file, 'utf8')); if (!title) continue; const rel = relative(API_DIR, file).split('\\').join('/'); @@ -322,7 +311,7 @@ async function checkListingsSync() { async function checkDocsSections() { const violations = []; - for (const file of await mdFiles(API_DIR)) { + for (const file of await docFiles(API_DIR)) { const rel = relative(API_DIR, file).split('\\').join('/'); const src = await readFile(file, 'utf8'); const isHelper = rel.startsWith('helpers/'); @@ -415,7 +404,7 @@ function managedPage(rel) { // pair is the documented per-region opt-out, so it must not be silent. async function checkDocsSectionOrder() { const violations = []; - for (const file of await mdFiles(API_DIR)) { + for (const file of await docFiles(API_DIR)) { const rel = relative(API_DIR, file).split('\\').join('/'); if (!orderedPage(rel)) continue; const src = await readFile(file, 'utf8'); @@ -470,7 +459,7 @@ async function checkDocsSectionOrder() { // .github/**. async function checkDocsGenerated() { const violations = []; - for (const file of await mdFiles(API_DIR)) { + for (const file of await docFiles(API_DIR)) { const rel = relative(API_DIR, file).split('\\').join('/'); if (!managedPage(rel)) continue; const src = await readFile(file, 'utf8'); @@ -1911,7 +1900,7 @@ async function checkAutodocsTag() { async function countInlineStyles() { const counts = {}; const stories = await walk(join(REPO, 'bulma-ui', 'src'), '.stories.tsx'); - const docs = await mdFiles(join(REPO, 'docs', 'docs')); + const docs = await docFiles(join(REPO, 'docs', 'docs')); for (const file of [...stories, ...docs]) { const n = ((await readFile(file, 'utf8')).match(/style=\{\{/g) || []) .length; @@ -3976,7 +3965,7 @@ export async function checkFragileProse(root = REPO) { // Migration guides are exempt: their counts describe a frozen upstream // (a vendored version of another library), and the ones that matter are // asserted by the mapping-coverage tests in bestax-migrate. - ...(await ifPresent(join(root, 'docs', 'docs', 'guides'), mdFiles)) + ...(await ifPresent(join(root, 'docs', 'docs', 'guides'), docFiles)) .filter(f => !f.includes(`${sep}migration${sep}`)) .map(f => [f, 'guide']), ]; diff --git a/scripts/coverage-floors.json b/scripts/coverage-floors.json index b48d105a9..85df3eea4 100644 --- a/scripts/coverage-floors.json +++ b/scripts/coverage-floors.json @@ -8,7 +8,7 @@ "scripts/consumer-sbom-meta.mjs": { "lines": 98, "branches": 89, "functions": 100 }, "scripts/coverage-floors.mjs": { "lines": 99, "branches": 100, "functions": 93 }, "scripts/gen-api-docs.mjs": { "lines": 17, "branches": 1, "functions": 0 }, - "scripts/gen-api-sources.mjs": { "lines": 30, "branches": 56, "functions": 30 }, + "scripts/gen-api-sources.mjs": { "lines": 30, "branches": 56, "functions": 28 }, "scripts/gen-eslint-meta.mjs": { "lines": 97, "branches": 82, "functions": 96 }, "scripts/gen-mcp-index.mjs": { "lines": 86, "branches": 85, "functions": 88 }, "scripts/gen-skills-rosters.mjs": { "lines": 72, "branches": 20, "functions": 50 }, diff --git a/scripts/gen-api-docs.mjs b/scripts/gen-api-docs.mjs index 4e833b46a..756df8765 100644 --- a/scripts/gen-api-docs.mjs +++ b/scripts/gen-api-docs.mjs @@ -23,7 +23,7 @@ * Regenerate with `pnpm gen` (which also refreshes the skill catalog, since * generated Overview sentences feed its one-liners). */ -import { readFile, writeFile, readdir } from 'node:fs/promises'; +import { readFile, writeFile } from 'node:fs/promises'; import { existsSync } from 'node:fs'; import { join, relative, dirname } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; @@ -37,7 +37,9 @@ import { upsertFrontmatter, renderTable, firstSentence, + frontmatterTitle, } from './lib/api-page.mjs'; +import { mdFiles } from './lib/api-catalog.mjs'; import { extractComponent, varRootCandidates } from './lib/props-extract.mjs'; import { componentVars } from './lib/scss-vars.mjs'; import { @@ -53,25 +55,6 @@ const REPO = join(HERE, '..'); const API_DIR = join(REPO, 'docs', 'docs', 'api'); const PACKAGE = '@allxsmith/bestax-bulma'; -const byCodePoint = (a, b) => (a < b ? -1 : a > b ? 1 : 0); - -async function mdFiles(dir) { - const out = []; - for (const entry of await readdir(dir, { withFileTypes: true })) { - const full = join(dir, entry.name); - if (entry.isDirectory()) out.push(...(await mdFiles(full))); - else if (entry.name.endsWith('.md')) out.push(full); - } - return out.sort(byCodePoint); -} - -function frontmatterTitle(src) { - const m = src.match(/^---\r?\n([\s\S]*?)\r?\n---/); - if (!m) return null; - const t = m[1].match(/^title:[ \t]*(.+?)[ \t]*$/m); - return t ? t[1].replace(/^['"]|['"]$/g, '') : null; -} - /** * Component name -> the API page that documents it, e.g. `Column` -> * `columns/column.md`. Used to link a compound sub-component to its own page diff --git a/scripts/gen-api-sources.mjs b/scripts/gen-api-sources.mjs index b0f0a00e3..6b7646425 100644 --- a/scripts/gen-api-sources.mjs +++ b/scripts/gen-api-sources.mjs @@ -38,6 +38,9 @@ import { varRootCandidates, } from './lib/props-extract.mjs'; import { GENERATED_EXEMPT } from './lib/api-sources.mjs'; +import { frontmatterTitle } from './lib/api-page.mjs'; +import { mdFiles } from './lib/api-catalog.mjs'; +import { byCodePoint } from './lib/skills.mjs'; const require = createRequire(import.meta.url); const HERE = dirname(fileURLToPath(import.meta.url)); @@ -45,25 +48,6 @@ const REPO = join(HERE, '..'); const API_DIR = join(REPO, 'docs', 'docs', 'api'); const OUT = join(HERE, 'lib', 'api-sources.mjs'); -const byCodePoint = (a, b) => (a < b ? -1 : a > b ? 1 : 0); - -async function mdFiles(dir) { - const out = []; - for (const e of await readdir(dir, { withFileTypes: true })) { - const f = join(dir, e.name); - if (e.isDirectory()) out.push(...(await mdFiles(f))); - else if (e.name.endsWith('.md')) out.push(f); - } - return out.sort(byCodePoint); -} - -function frontmatterTitle(src) { - const m = src.match(/^---\r?\n([\s\S]*?)\r?\n---/); - if (!m) return null; - const t = m[1].match(/^title:[ \t]*(.+?)[ \t]*$/m); - return t ? t[1].replace(/^['"]|['"]$/g, '') : null; -} - /** Every SCSS partial that could register component variables. */ async function candidatePartials() { const out = []; diff --git a/scripts/gen-mcp-index.mjs b/scripts/gen-mcp-index.mjs index b1e2d6201..adb792faf 100644 --- a/scripts/gen-mcp-index.mjs +++ b/scripts/gen-mcp-index.mjs @@ -49,7 +49,7 @@ import { sectionBody, firstSentence, } from './lib/api-page.mjs'; -import { readSkillNames } from './lib/skills.mjs'; +import { byCodePoint, readSkillNames } from './lib/skills.mjs'; import { clipAtWord, firstProseLine, @@ -94,10 +94,6 @@ const DOCS_BASE = 'https://bestax.io/docs'; */ const SCHEMA_VERSION = 1; -// Deterministic, locale-independent comparator. localeCompare varies with the -// runtime's ICU version and would make CI's regenerate-and-diff flake. -const byCodePoint = (a, b) => (a < b ? -1 : a > b ? 1 : 0); - const collapse = s => String(s ?? '') .replace(/\s+/g, ' ') diff --git a/scripts/lib/api-page.mjs b/scripts/lib/api-page.mjs index 6ec0fb8ba..2b48df937 100644 --- a/scripts/lib/api-page.mjs +++ b/scripts/lib/api-page.mjs @@ -240,6 +240,11 @@ export function frontmatter(src) { return out; } +/** A page's frontmatter `title:`, or null when it has none. */ +export function frontmatterTitle(src) { + return frontmatter(src).title || null; +} + /** * Insert or update a frontmatter key, preserving key order and the rest of the * block. Used for `description:` — `docusaurus-plugin-llms` picks each page's From 4c0d1729496794ac85453806e22c5aded2fe4954 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sat, 3 Oct 2026 16:41:05 -0400 Subject: [PATCH 5/9] build: make the catalog generator testable and prettier-formatted gen-component-catalog ran main() on import, exported nothing and had no test or coverage row. It now has the import.meta guard the other generators use, exports overviewSentence, renderCatalog and build, and gets a node:test sibling and a floor. It also formats its output through prettier before writing, like the other generators. Prettier leaves the catalog unchanged, so the .prettierignore entry that kept format:check off it goes. The generated header, the script's doc comment and skills/README.md said to run `npm run gen:catalog`. They now say `pnpm gen:catalog`. That shortens the catalog by three bytes, which moves its byte count in bestax-mcp/data/skills.json from 18545 to 18542. --- .prettierignore | 3 - bestax-mcp/data/skills.json | 2 +- scripts/coverage-floors.json | 1 + scripts/gen-component-catalog.mjs | 66 ++++++++---- scripts/gen-component-catalog.test.mjs | 100 ++++++++++++++++++ skills/README.md | 2 +- .../references/component-catalog.md | 2 +- 7 files changed, 152 insertions(+), 24 deletions(-) create mode 100644 scripts/gen-component-catalog.test.mjs diff --git a/.prettierignore b/.prettierignore index 03b22fd2e..ce7d179cc 100644 --- a/.prettierignore +++ b/.prettierignore @@ -14,9 +14,6 @@ pnpm-lock.yaml # so two editions of The State of React diff row by row. docs/src/data/state-of-react/ -# Generated by scripts/gen-component-catalog.mjs -skills/bestax-custom-component/references/component-catalog.md - # Codemod test fixtures: byte-exact expected transform output — reformatting # them would change what the tests assert. The kitchen-sink fixture app is # migration *input* (react-bulma-components source), also left untouched. diff --git a/bestax-mcp/data/skills.json b/bestax-mcp/data/skills.json index b89f391aa..ac2889265 100644 --- a/bestax-mcp/data/skills.json +++ b/bestax-mcp/data/skills.json @@ -10,7 +10,7 @@ { "id": "component-catalog", "file": "references/component-catalog.md", - "bytes": 18545 + "bytes": 18542 }, { "id": "library-contributor", diff --git a/scripts/coverage-floors.json b/scripts/coverage-floors.json index 85df3eea4..a62bf984c 100644 --- a/scripts/coverage-floors.json +++ b/scripts/coverage-floors.json @@ -9,6 +9,7 @@ "scripts/coverage-floors.mjs": { "lines": 99, "branches": 100, "functions": 93 }, "scripts/gen-api-docs.mjs": { "lines": 17, "branches": 1, "functions": 0 }, "scripts/gen-api-sources.mjs": { "lines": 30, "branches": 56, "functions": 28 }, + "scripts/gen-component-catalog.mjs": { "lines": 91, "branches": 77, "functions": 75 }, "scripts/gen-eslint-meta.mjs": { "lines": 97, "branches": 82, "functions": 96 }, "scripts/gen-mcp-index.mjs": { "lines": 86, "branches": 85, "functions": 88 }, "scripts/gen-skills-rosters.mjs": { "lines": 72, "branches": 20, "functions": 50 }, diff --git a/scripts/gen-component-catalog.mjs b/scripts/gen-component-catalog.mjs index a498b215b..143a9025d 100644 --- a/scripts/gen-component-catalog.mjs +++ b/scripts/gen-component-catalog.mjs @@ -15,20 +15,25 @@ * Completeness guard: after writing, cross-check `bulma-ui/src/index.ts` and FAIL * if any exported component lacks an API page — so a new component can't be * silently missing from the catalog (which would let an agent reinvent it). + * The page walk and the guard live in scripts/lib/api-catalog.mjs, shared with + * gen-mcp-index.mjs, so the two catalogs agree on what counts as documented. * * Output (committed, names + one-line purpose only — full props stay in the API * pages, loaded on demand): * skills/bestax-custom-component/references/component-catalog.md * * The catalog is intentionally lean (~140 lines) so it can load into an agent's - * context cheaply. Run via `npm run gen:catalog`; CI fails if the committed file + * context cheaply. Run via `pnpm gen:catalog`; CI fails if the committed file * is stale (regenerate + `git diff --exit-code`). The output MUST be * deterministic across machines/Node versions, so sorting is by code point (not - * locale collation) and parsing tolerates CRLF. + * locale collation) and parsing tolerates CRLF. Like the other generators, it + * formats its output with prettier before writing, so the committed bytes are + * what `format:check` wants. */ import { readFile, writeFile } from 'node:fs/promises'; import { join, relative, dirname } from 'node:path'; -import { fileURLToPath } from 'node:url'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import { createRequire } from 'node:module'; import { docsRoute } from './lib/docs-url.mjs'; import { firstSentence } from './lib/api-page.mjs'; @@ -41,6 +46,7 @@ import { } from './lib/api-catalog.mjs'; import { byCodePoint } from './lib/skills.mjs'; +const require = createRequire(import.meta.url); const HERE = dirname(fileURLToPath(import.meta.url)); const REPO = join(HERE, '..'); const API_DIR = join(REPO, 'docs', 'docs', 'api'); @@ -58,18 +64,18 @@ const DOCS_BASE = 'https://bestax.io/docs/api'; * A page's catalog one-liner: the first sentence of the first prose line * after its `## Overview` (or `### Overview`) heading, clipped at a clause. */ -function overviewSentence(src) { +export function overviewSentence(src) { const after = src.split(/^#{2,3}[ \t]+Overview[ \t]*$/m)[1]; const line = after ? firstProseLine(after) : ''; return line ? clipAtClause(firstSentence(line)) : ''; } -async function main() { - // Category dirs in display order, each with its titled pages. The - // completeness guard below reads the same list, so a page it counts as - // documented is a page this catalog lists. - const categories = await readApiPages(API_DIR); - +/** + * The catalog markdown for `categories` (from readApiPages), before + * formatting, and how many components it lists. Pure, so the tests can drive + * it with fixtures. + */ +export function renderCatalog(categories) { const sections = []; let total = 0; @@ -90,8 +96,8 @@ async function main() { sections.push(`## ${label}\n\n${lines.join('\n')}`); } - const header = ` - + const markdown = ` + # Component catalog @@ -134,19 +140,43 @@ component is guaranteed to appear (the generator fails if one lacks an API page) ${sections.join('\n\n')} `; + return { markdown, total }; +} + +/** + * The catalog as it is committed, formatted through prettier, with the + * exported components that have no API page. Category dirs come in display + * order, each with its titled pages, and the guard reads the same list, so a + * page it counts as documented is a page the catalog lists. + */ +export async function build() { + const categories = await readApiPages(API_DIR); + const { markdown, total } = renderCatalog(categories); + const prettier = require('prettier'); + const text = await prettier.format(markdown, { + ...(await prettier.resolveConfig(OUT)), + filepath: OUT, + }); + const missing = missingApiPages(await readFile(INDEX_TS, 'utf8'), categories); + return { text, total, missing }; +} - await writeFile(OUT, header); +export async function main() { + const { text, total, missing } = await build(); + await writeFile(OUT, text); process.stdout.write(`Wrote ${relative(REPO, OUT)} (${total} components)\n`); // Completeness guard: every exported component must have an API page. - const missing = missingApiPages(await readFile(INDEX_TS, 'utf8'), categories); + // Runs after writing so the failure names what to fix. if (missing.length) { console.error(missingApiPagesMessage(missing, 'the catalog')); process.exit(1); } } -main().catch(err => { - console.error(err); - process.exit(1); -}); +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + main().catch(err => { + console.error(err); + process.exit(1); + }); +} diff --git a/scripts/gen-component-catalog.test.mjs b/scripts/gen-component-catalog.test.mjs new file mode 100644 index 000000000..63e440866 --- /dev/null +++ b/scripts/gen-component-catalog.test.mjs @@ -0,0 +1,100 @@ +/** + * Guards on the skill component catalog generator. + * + * The catalog is what an agent scans before writing markup, so its failures + * are quiet: a dropped component gets reinvented by hand, and output that is + * not a prettier fixpoint turns the staleness gate into noise the first time + * someone formats the repo. The completeness guard and the page walk are + * tested with fixtures in api-catalog.test.mjs. This file holds the + * rendering and the real output. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { createRequire } from 'node:module'; +import { fileURLToPath } from 'node:url'; + +import { + build, + overviewSentence, + renderCatalog, +} from './gen-component-catalog.mjs'; + +const require = createRequire(import.meta.url); +const OUT = fileURLToPath( + new URL( + '../skills/bestax-custom-component/references/component-catalog.md', + import.meta.url + ) +); + +const page = (relPath, title, overview) => ({ + relPath, + slug: relPath.replace(/\.md$/, ''), + fm: { title }, + src: `---\ntitle: ${title}\n---\n\n## Overview\n\n${overview}\n`, +}); + +test('the one-liner is the first prose sentence under Overview', () => { + const src = [ + '---', + 'title: Box', + '---', + '', + '### Overview', + '', + "import Box from './box';", + '', + '![Box](box.png)', + '', + 'The `Box` component wraps content in a padded, bordered white panel. It is simple.', + ].join('\r\n'); + assert.equal( + overviewSentence(src), + 'The `Box` component wraps content in a padded, bordered white panel.' + ); + assert.equal(overviewSentence('---\ntitle: X\n---\n\n## Usage\n'), ''); + assert.equal(overviewSentence('## Overview\n\n- only a list\n'), ''); +}); + +test('the catalog lists titled pages by title, linking the served route', () => { + const { markdown, total } = renderCatalog([ + { + dir: 'grid', + label: 'Grid', + pages: [ + page( + 'grid/grid.md', + 'Grid', + 'The `Grid` lays out cells in a CSS grid.' + ), + page('grid/cell.md', 'Cell', ''), + ], + }, + { dir: 'empty', label: 'Empty', pages: [] }, + ]); + assert.equal(total, 2); + assert.match( + markdown, + /## Grid\n\n- \[Cell\]\(https:\/\/bestax\.io\/docs\/api\/grid\/cell\)\n- \[Grid\]\(https:\/\/bestax\.io\/docs\/api\/grid\) — The `Grid`/ + ); + assert.ok(!markdown.includes('## Empty'), 'an empty category is skipped'); + assert.match(markdown, /^2 documented components\./m); + assert.match(markdown, /Regenerate with `pnpm gen:catalog`/); +}); + +test('the real catalog covers every export and is a prettier fixpoint', async () => { + const { text, total, missing } = await build(); + assert.deepEqual(missing, []); + assert.ok(total >= 80, `only ${total} components`); + assert.equal( + (text.match(/^- \[/gm) || []).length, + total, + 'one row per component' + ); + const prettier = require('prettier'); + const formatted = await prettier.format(text, { + ...(await prettier.resolveConfig(OUT)), + filepath: OUT, + }); + assert.equal(formatted, text, 'formatting the catalog must not change it'); +}); diff --git a/skills/README.md b/skills/README.md index 42fb4f39c..2ee4db56b 100644 --- a/skills/README.md +++ b/skills/README.md @@ -101,7 +101,7 @@ skills/ ``` > `component-catalog.md` is generated from the API docs by `scripts/gen-component-catalog.mjs` -> (`npm run gen:catalog`). Don't edit it by hand. +> (`pnpm gen:catalog`). Don't edit it by hand. ## See also diff --git a/skills/bestax-custom-component/references/component-catalog.md b/skills/bestax-custom-component/references/component-catalog.md index b2eb873d1..0f8132008 100644 --- a/skills/bestax-custom-component/references/component-catalog.md +++ b/skills/bestax-custom-component/references/component-catalog.md @@ -1,5 +1,5 @@ - + # Component catalog From 10f422fa34b30be58f6499894edd24a8a8909b21 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sat, 3 Oct 2026 16:42:45 -0400 Subject: [PATCH 6/9] build: index only the skill files the sync scripts ship readSkills in gen-mcp-index walked references/ and examples/ and listed every regular file, including .DS_Store and untracked or gitignored files. Both sync scripts leave .DS_Store out of the copy and refuse to bundle untracked files, so a local run could write a skills.json naming a file the server never ships. lib/skills.mjs gains isDsStore, skillFiles (the walk that skips it) and assertSkillsVetted (the untracked-file refusal), and readSkills uses all three. skills.json is byte-identical on a clean tree. --- scripts/gen-mcp-index.mjs | 50 ++++++++++++++++++---------------- scripts/gen-mcp-index.test.mjs | 50 +++++++++++++++++++++++++++++++++- scripts/lib/skills.mjs | 45 ++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 25 deletions(-) diff --git a/scripts/gen-mcp-index.mjs b/scripts/gen-mcp-index.mjs index adb792faf..fd2290bee 100644 --- a/scripts/gen-mcp-index.mjs +++ b/scripts/gen-mcp-index.mjs @@ -37,7 +37,7 @@ * * Regenerate with `pnpm gen:mcp` (or `pnpm gen`, which runs all three). */ -import { readFile, writeFile, readdir, mkdir, rm } from 'node:fs/promises'; +import { readFile, writeFile, mkdir, rm } from 'node:fs/promises'; import { existsSync } from 'node:fs'; import { join, relative, dirname, extname } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; @@ -49,7 +49,12 @@ import { sectionBody, firstSentence, } from './lib/api-page.mjs'; -import { byCodePoint, readSkillNames } from './lib/skills.mjs'; +import { + assertSkillsVetted, + byCodePoint, + readSkillNames, + skillFiles, +} from './lib/skills.mjs'; import { clipAtWord, firstProseLine, @@ -295,11 +300,18 @@ function propRow(r) { * The skills roster, READ from the directory — never a hardcoded list. The * predicate (a directory holding a SKILL.md) lives in scripts/lib/skills.mjs, * shared with both sync scripts and check-conformance. + * + * The manifest lists what the sync scripts ship, so it reads the skills the + * way they do: it refuses untracked files exactly as they refuse to bundle + * them, and lists files through the same walk that leaves `.DS_Store` out. + * Indexing every file on disk let a local run list a file that never ships. */ -async function readSkills() { +export async function readSkills(skillsDir = SKILLS_DIR) { + const names = await readSkillNames(skillsDir); + assertSkillsVetted(skillsDir, names, 'index'); const out = []; - for (const name of await readSkillNames(SKILLS_DIR)) { - const skillFile = join(SKILLS_DIR, name, 'SKILL.md'); + for (const name of names) { + const skillFile = join(skillsDir, name, 'SKILL.md'); const src = await readFile(skillFile, 'utf8'); const fm = frontmatter(src); // Walks nested directories, not just the top level. A skill that serves @@ -322,27 +334,17 @@ async function readSkills() { // one, and agents discover ids through `get_skill` rather than hardcoding // them, so the rename is accepted rather than aliased. const listing = async sub => { - const root = join(SKILLS_DIR, name, sub); + const root = join(skillsDir, name, sub); if (!existsSync(root)) return []; const files = []; - const walk = async rel => { - const entries = await readdir(join(root, rel), { withFileTypes: true }); - for (const e of entries.sort((a, b) => byCodePoint(a.name, b.name))) { - const next = rel ? `${rel}/${e.name}` : e.name; - if (e.isDirectory()) { - await walk(next); - continue; - } - if (!e.isFile()) continue; - const body = await readFile(join(root, next), 'utf8'); - files.push({ - id: next.replace(extname(e.name), '').replace(/\//g, '-'), - file: `${sub}/${next}`, - bytes: Buffer.byteLength(body), - }); - } - }; - await walk(''); + for (const rel of await skillFiles(root)) { + const body = await readFile(join(root, rel), 'utf8'); + files.push({ + id: rel.replace(extname(rel), '').replace(/\//g, '-'), + file: `${sub}/${rel}`, + bytes: Buffer.byteLength(body), + }); + } // `a-b/x.md` and `a/b-x.md` both flatten to `a-b-x`, and `id` is the // MCP resource lookup key — two files answering to one key would make // the served content depend on ordering. No such pair exists today; diff --git a/scripts/gen-mcp-index.test.mjs b/scripts/gen-mcp-index.test.mjs index 60690d5e1..51784f516 100644 --- a/scripts/gen-mcp-index.test.mjs +++ b/scripts/gen-mcp-index.test.mjs @@ -22,7 +22,7 @@ import { readFile } from 'node:fs/promises'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { build } from './gen-mcp-index.mjs'; +import { build, readSkills } from './gen-mcp-index.mjs'; const REPO = join(dirname(fileURLToPath(import.meta.url)), '..'); @@ -201,6 +201,54 @@ test('references nested one level per subject still reach the index', () => { } }); +test('the skills manifest lists exactly what the sync scripts ship', async t => { + // The sync scripts refuse untracked files and leave `.DS_Store` out of the + // copy. The index used to list every regular file on disk, so a local run + // could commit a manifest naming a file the server never ships. + const { mkdtempSync, mkdirSync, rmSync, writeFileSync } = + await import('node:fs'); + const { execFileSync } = await import('node:child_process'); + const { tmpdir } = await import('node:os'); + + const root = mkdtempSync(join(tmpdir(), 'bestax-mcp-skills-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const skillsDir = join(root, 'skills'); + const skill = join(skillsDir, 'bestax-form'); + mkdirSync(join(skill, 'references', 'sub'), { recursive: true }); + writeFileSync( + join(skill, 'SKILL.md'), + '---\nname: bestax-form\ndescription: Build forms.\n---\n' + ); + writeFileSync(join(skill, 'references', 'api.md'), '# api\n'); + writeFileSync(join(skill, 'references', 'sub', 'map.md'), '# map\n'); + writeFileSync(join(skill, 'references', '.DS_Store'), 'finder'); + writeFileSync(join(skill, 'references', 'sub', '.DS_Store'), 'finder'); + writeFileSync(join(root, '.gitignore'), '*.log\n'); + + // No repository yet, like an exported tarball: the disk minus .DS_Store. + const listed = async () => + (await readSkills(skillsDir))[0].references.map(r => r.file); + const shipped = ['references/api.md', 'references/sub/map.md']; + assert.deepEqual(await listed(), shipped); + + const git = (...args) => + execFileSync('git', ['-C', root, ...args], { stdio: 'ignore' }); + git('init', '-q'); + git('add', 'skills', '.gitignore'); + assert.deepEqual(await listed(), shipped, '.DS_Store stays exempt'); + + writeFileSync(join(skill, 'references', 'scratch.md'), 'draft\n'); + await assert.rejects( + readSkills(skillsDir), + /refusing to index untracked file\(s\) under skills\/: bestax-form\/references\/scratch\.md/ + ); + rmSync(join(skill, 'references', 'scratch.md')); + + // Gitignored is still unvetted: the sync scripts would refuse it too. + writeFileSync(join(skill, 'references', 'debug.log'), 'noise\n'); + await assert.rejects(readSkills(skillsDir), /debug\.log/); +}); + test('every skill reference id is unique within its skill', () => { for (const s of skills.skills) { const ids = [...s.references, ...s.examples].map(r => r.id); diff --git a/scripts/lib/skills.mjs b/scripts/lib/skills.mjs index 9e06893bb..950072869 100644 --- a/scripts/lib/skills.mjs +++ b/scripts/lib/skills.mjs @@ -92,6 +92,51 @@ export function pathsInsideSkills(candidatePaths, names) { return candidatePaths.filter(p => p && bundled.has(p.split('/')[0])); } +/** + * Finder's `.DS_Store`, the one path the vetting gate exempts. Every bundler + * filters it out of its copy, and the MCP index out of its listing, so the + * exemption never becomes shipped content. + */ +export function isDsStore(path) { + return path.endsWith('.DS_Store'); +} + +/** + * The files under `dir` a bundler ships, as paths relative to it with + * forward slashes: depth first, each directory's entries in code-point + * order, and nothing isDsStore matches. What the MCP index lists, so it + * cannot list a file the sync scripts leave out. + */ +export async function skillFiles(dir) { + const out = []; + const walk = async rel => { + const entries = await readdir(join(dir, rel), { withFileTypes: true }); + for (const e of entries.sort((a, b) => byCodePoint(a.name, b.name))) { + const next = rel ? `${rel}/${e.name}` : e.name; + if (isDsStore(next)) continue; + if (e.isDirectory()) await walk(next); + else if (e.isFile()) out.push(next); + } + }; + await walk(''); + return out; +} + +/** + * Throws when untrackedSkillPaths finds anything in the skills `names`: the + * refusal both sync scripts and the MCP index give, so none of them can + * carry a file that the others refuse. `verb` names what was refused. + */ +export function assertSkillsVetted(skillsDir, names, verb) { + const untracked = untrackedSkillPaths(skillsDir, names); + if (untracked.length) { + throw new Error( + `refusing to ${verb} untracked file(s) under skills/: ` + + `${untracked.join(', ')}. \`git add\` them to vet them, or remove them.` + ); + } +} + function git(cwd, args) { return execFileSync('git', ['-C', cwd, ...args], { encoding: 'utf8', From 066a14cc9108a963e83f7772e19a9910bf543a10 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sat, 3 Oct 2026 16:48:07 -0400 Subject: [PATCH 7/9] build: share one skills sync between create-bestax and bestax-mcp The two sync-skills.mjs scripts were near-twins: the source check, the roster read, the empty-roster check, the untracked gate, the copy loop and the .DS_Store filter, plus a lock and fingerprint in bestax-mcp's. These move to scripts/lib/sync-skills.mjs as syncSkills, and both scripts become thin callers. bestax-mcp passes a stateDir, so it keeps its lock, freshness stamp, signal release and data/.sync-skills location. create-bestax passes none and still copies afresh on every run. Adopting the lock there would put state under templates/, which ships, for callers that never overlap. It now copies with node:fs cp instead of fs-extra. The fingerprint now walks only the files a copy ships, so a .DS_Store change no longer forces a recopy. Output, messages and the copied trees are identical, and npm pack lists the same files for both packages. --- bestax-mcp/CLAUDE.md | 8 +- bestax-mcp/scripts/sync-skills.mjs | 261 +++----------------------- create-bestax/scripts/sync-skills.mjs | 88 ++------- scripts/coverage-floors.json | 1 + scripts/lib/sync-skills.mjs | 241 ++++++++++++++++++++++++ scripts/sync-skills.test.mjs | 201 ++++++++++++++++++++ 6 files changed, 492 insertions(+), 308 deletions(-) create mode 100644 scripts/lib/sync-skills.mjs create mode 100644 scripts/sync-skills.test.mjs diff --git a/bestax-mcp/CLAUDE.md b/bestax-mcp/CLAUDE.md index f4ade6a73..19b784baf 100644 --- a/bestax-mcp/CLAUDE.md +++ b/bestax-mcp/CLAUDE.md @@ -28,9 +28,11 @@ hand-written `## Usage` / `## Accessibility` sections of the API pages. committed — enough for the staleness gate to catch a new or renamed skill, without putting a second copy of ~390 KB of markdown in every skill diff. Several Turbo tasks can invoke that script at the same time, so it takes a lock - and no-ops when the tree already matches the source; keep both properties if - you touch it, and see its trailing "Concurrency" note for why the freshness - check is required rather than merely an optimisation. + and no-ops when the tree already matches the source. The copy itself is + `scripts/lib/sync-skills.mjs` at the repo root, shared with create-bestax. + Keep both properties if you touch either file, and see the shared file's + trailing "Concurrency" note for why the freshness check is required rather + than merely an optimisation. - `gen:mcp:check` runs `git add --intent-to-add` before diffing. The output is a directory, and `git diff --exit-code` cannot see an untracked file — a newly added component would otherwise pass the gate. diff --git a/bestax-mcp/scripts/sync-skills.mjs b/bestax-mcp/scripts/sync-skills.mjs index 105c50bb0..cfdaaeccb 100644 --- a/bestax-mcp/scripts/sync-skills.mjs +++ b/bestax-mcp/scripts/sync-skills.mjs @@ -4,248 +4,35 @@ // // `/skills` is the single source of truth; `data/skills` is generated and // gitignored. Runs on `build`, `test`, `test:coverage`, `test:watch` and -// `prepack` — five entry points, and turbo runs the first three concurrently, -// which is what the locking below exists for. See "Concurrency" at the bottom. -// -// Two deliberate differences from create-bestax/scripts/sync-skills.mjs, -// which solves the same problem for the scaffolder: -// -// 1. No fs-extra. This package has one runtime dependency and no reason for a -// second; node:fs/promises `cp` covers it. -// 2. This one is concurrency-safe. The scaffolder's copy is only reachable -// from `build` and `prepack`, which never overlap, so it does not need to -// be — but it has the same destructive shape, so if a third caller is ever -// added there, port this. -// -// There used to be a third, and it is worth recording that it closed rather -// than leaving a reader to wonder. That file carried a hardcoded `SKILLS` -// array while this one read the directory, so a new skill could ship here and -// silently not there. #540 made it read the directory too, and added the -// `skills-roster` conformance check for the prose copies that cannot be -// derived, so the claim "nothing warns when a directory is absent" no longer -// holds anywhere. +// `prepack`, and turbo runs the first three concurrently. That is why this +// caller passes a `stateDir`: scripts/lib/sync-skills.mjs, which does the copy +// for both packages, then takes a lock and skips a copy that is already +// current. See its "Concurrency" note for why both are needed. // // The MANIFEST (names, descriptions, reference lists) is committed instead, in // data/skills.json — generated by scripts/gen-mcp-index.mjs. That is what the // CI staleness gate watches, so a new or renamed skill still fails the build // without a second committed copy of 390 KB of markdown bloating every diff. -import { - cp, - mkdir, - readdir, - readFile, - rm, - stat, - writeFile, -} from 'node:fs/promises'; -import { existsSync, rmSync } from 'node:fs'; -import { createHash } from 'node:crypto'; -import { setTimeout as sleep } from 'node:timers/promises'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { - readSkillNames, - untrackedSkillPaths, -} from '../../scripts/lib/skills.mjs'; - -const here = path.dirname(fileURLToPath(import.meta.url)); -const pkgRoot = path.resolve(here, '..'); -const skillsSrc = path.resolve(pkgRoot, '..', 'skills'); -const skillsDest = path.join(pkgRoot, 'data', 'skills'); - -// Lock and freshness stamp live in their own directory, NOT loose in `data/`. -// `gen:mcp:check` runs `git add --intent-to-add --all bestax-mcp/data` and -// diffs the result, so an untracked file anywhere else under data/ would fail -// that gate. This path is gitignored alongside data/skills itself. -const stateDir = path.join(pkgRoot, 'data', '.sync-skills'); -const lockDir = path.join(stateDir, 'lock'); -const stampFile = path.join(stateDir, 'fingerprint'); - -// A sync is ~390 KB of file copies and takes tens of milliseconds. A lock held -// longer than this is a crashed run, not a slow one. -const LOCK_STALE_MS = 60_000; -const LOCK_TIMEOUT_MS = 120_000; -const LOCK_POLL_MS = 50; - -if (!existsSync(skillsSrc)) { - console.error(`[sync-skills] source not found: ${skillsSrc}`); +import { syncSkills } from '../../scripts/lib/sync-skills.mjs'; + +const pkgRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '..' +); + +await syncSkills({ + src: path.resolve(pkgRoot, '..', 'skills'), + dest: path.join(pkgRoot, 'data', 'skills'), + label: 'data/skills', + // Lock and freshness stamp live in their own directory, NOT loose in + // `data/`. `gen:mcp:check` runs `git add --intent-to-add --all + // bestax-mcp/data` and diffs the result, so an untracked file anywhere else + // under data/ would fail that gate. This path is gitignored alongside + // data/skills itself, and `files` negates it so it never ships. + stateDir: path.join(pkgRoot, 'data', '.sync-skills'), +}).catch(err => { + console.error(err.message); process.exit(1); -} - -// The shared predicate (scripts/lib/skills.mjs): a directory holding a -// SKILL.md, sorted deterministically — one definition for all four consumers. -const names = await readSkillNames(skillsSrc); - -if (!names.length) { - console.error(`[sync-skills] no skills found in ${skillsSrc}`); - process.exit(1); -} - -// Same vetting gate as create-bestax's sync: data/skills ships in the npm -// tarball, so an untracked scratch skill must not reach a manual publish. -const untrackedHere = untrackedSkillPaths(skillsSrc, names); -if (untrackedHere.length) { - console.error( - `[sync-skills] refusing to bundle untracked file(s) under skills/: ` + - `${untrackedHere.join(', ')}. \`git add\` them to vet them, or remove them.` - ); - process.exit(1); -} - -// Hash of every source path and its bytes. Content rather than mtime on -// purpose: a fresh clone or a branch switch rewrites mtimes without changing -// what should be copied, and re-copying is the cheap half — it is the -// destructive `rm` that has to be avoided while another process is reading. -async function fingerprintSource() { - const hash = createHash('sha256'); - const walk = async (dir, rel) => { - const entries = await readdir(dir, { withFileTypes: true }); - entries.sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0)); - for (const entry of entries) { - const abs = path.join(dir, entry.name); - const key = `${rel}/${entry.name}`; - if (entry.isDirectory()) { - await walk(abs, key); - } else { - hash.update(key); - hash.update(await readFile(abs)); - } - } - }; - for (const name of names) await walk(path.join(skillsSrc, name), name); - return hash.digest('hex'); -} - -// Release on interrupt. Ctrl-C during `pnpm all` is the realistic way a lock -// gets abandoned, and node does not run `finally` blocks on a signal, so -// without this the next run would meet a lock nobody holds. Sync calls only: -// the process is on its way out and async cleanup would not finish. -let holdingLock = false; -function releaseLockSync() { - if (!holdingLock) return; - holdingLock = false; - try { - rmSync(lockDir, { recursive: true, force: true }); - } catch { - // Best effort — we are already exiting. - } -} -for (const signal of ['SIGINT', 'SIGTERM', 'SIGHUP']) { - process.once(signal, () => { - releaseLockSync(); - process.exit(130); - }); -} - -// `mkdir` without `recursive` is atomic and fails with EEXIST if the directory -// exists, which makes it a usable mutex with no dependency. -// -// There is deliberately NO automatic stale-lock reclaim, and that is the -// subtle part. The obvious version — stat the lock, and if it looks old enough -// delete it and retry — has a time-of-check/time-of-use hole: nothing ties the -// delete to the *specific* lock that was observed. Two waiters can both see -// the same abandoned lock, the first deletes it and takes ownership, and the -// second then deletes the *fresh* lock the first is holding. Both proceed into -// the destructive branch below, which is precisely the concurrent -// rm-while-copying bug this file exists to prevent, now reachable only after a -// crash and therefore much harder to reproduce. -// -// The freshness stamp does not rescue that case either: a killed run leaves a -// lock but no stamp (the stamp is written last, on purpose), so both waiters -// see a mismatch and both rebuild. -// -// Every cheap repair keeps a window — owner tokens, rename-to-unique, put-it- -// back-if-it-was-not-mine all still race on the recreate. Crash-safe mutual -// exclusion on a POSIX filesystem is genuinely hard, and a build script is the -// wrong place to hand-roll it. So an abandoned lock is reported with the exact -// command to fix it rather than guessed at. The signal handlers above mean the -// common interrupt never gets here in the first place. -async function acquireLock() { - const deadline = Date.now() + LOCK_TIMEOUT_MS; - for (;;) { - try { - await mkdir(lockDir); - holdingLock = true; - return; - } catch (err) { - if (err.code !== 'EEXIST') throw err; - - let ageMs = 0; - try { - ageMs = Date.now() - (await stat(lockDir)).mtimeMs; - } catch { - // Released between our mkdir and our stat. Retry immediately. - continue; - } - - // Held far longer than a sync can legitimately take, so the holder is - // gone. Report it now instead of burning the full timeout first. - if (ageMs > LOCK_STALE_MS) { - throw new Error( - `[sync-skills] the lock at ${lockDir} has been held for ` + - `${Math.round(ageMs / 1000)}s, which means an earlier run was killed ` + - `before it could clean up. Remove that directory and re-run:\n` + - ` rm -rf ${lockDir}` - ); - } - - if (Date.now() > deadline) { - throw new Error(`[sync-skills] timed out waiting for lock: ${lockDir}`); - } - await sleep(LOCK_POLL_MS); - } - } -} - -await mkdir(stateDir, { recursive: true }); -const want = await fingerprintSource(); - -await acquireLock(); -try { - const have = await readFile(stampFile, 'utf8').catch(() => ''); - - // The freshness check is load-bearing, not an optimisation. Without it the - // second and third callers would re-run the destructive copy below while the - // FIRST caller's jest is already reading data/skills, and `rm` would delete - // the tree out from under it. Skipping the work when it is already done is - // what actually makes concurrent callers safe; the lock alone would not. - if (have.trim() === want && existsSync(skillsDest)) { - console.log(`[sync-skills] up to date (${names.length} skills)`); - } else { - await rm(skillsDest, { recursive: true, force: true }); - await mkdir(skillsDest, { recursive: true }); - for (const name of names) { - await cp(path.join(skillsSrc, name), path.join(skillsDest, name), { - recursive: true, - // .DS_Store is the one path the vetting gate exempts; keep it out of - // the bundle too so the exemption never becomes shipped content. - filter: src => !src.endsWith('.DS_Store'), - }); - } - // Stamped last: a run killed mid-copy leaves no stamp, so the next caller - // redoes the work rather than trusting a half-populated tree. - await writeFile(stampFile, `${want}\n`); - console.log(`[sync-skills] copied ${names.length} skills -> data/skills`); - } -} finally { - holdingLock = false; - await rm(lockDir, { recursive: true, force: true }); -} - -// Concurrency -// ----------- -// `pnpm all` runs `turbo run build typecheck test test:coverage`, and -// bestax-mcp's build, test and test:coverage scripts each invoke this file. On -// a cold turbo cache all three start within milliseconds of each other, and the -// original implementation went straight to `rm -rf data/skills` followed by a -// per-skill copy. Interleaving those produced a different error every run — -// ENOENT on copyfile, EEXIST on mkdir, ENOTEMPTY on rmdir — which read as three -// unrelated flakes rather than one race. -// -// It stayed hidden because a warm cache means only one of the three actually -// executes; it surfaces reliably right after a merge, on a fresh clone, and in -// CI, which is the worst possible time for it. -// -// Fixed here rather than in turbo.json because `pnpm --filter bestax-mcp test` -// has to keep working on its own — bestax-mcp/CLAUDE.md documents it — so the -// script cannot rely on a task graph to serialise its callers. +}); diff --git a/create-bestax/scripts/sync-skills.mjs b/create-bestax/scripts/sync-skills.mjs index 771860e02..5dfe6c546 100644 --- a/create-bestax/scripts/sync-skills.mjs +++ b/create-bestax/scripts/sync-skills.mjs @@ -21,74 +21,26 @@ // so the omission is a decision in this file rather than an absence nobody // sees. // -// This was the last of the three roster consumers to be listed rather than -// read; `bestax-mcp/scripts/sync-skills.mjs` and `scripts/gen-mcp-index.mjs` -// already read the directory. The copies that CANNOT be derived are prose -// (README, docs, the scaffolded CLAUDE.md), and those are held by -// `pnpm check:conformance --only=skills-roster`. -// -// Two deliberate differences from bestax-mcp's copy survive, and its header -// lists the same two: fs-extra stays (this package already depends on it, so -// avoiding it buys nothing), and this is still not concurrency-safe (the only -// callers are `build` and `prepack`, which never overlap — port the locking if -// a third is ever added). +// The copy itself is scripts/lib/sync-skills.mjs, shared with bestax-mcp's +// sync, so the checks and the `.DS_Store` filter cannot drift between them. +// This caller passes no `stateDir`, so it takes no lock and keeps no +// fingerprint: its only callers are `build` and `prepack`, which never +// overlap, and a state directory under templates/ would ship in the tarball. +// Pass one if a third caller is ever added, and keep it out of `files`. import { fileURLToPath } from 'node:url'; import path from 'node:path'; -import fs from 'fs-extra'; -import { - readSkillNames, - untrackedSkillPaths, -} from '../../scripts/lib/skills.mjs'; - -const here = path.dirname(fileURLToPath(import.meta.url)); -const pkgRoot = path.resolve(here, '..'); -const skillsSrc = path.resolve(pkgRoot, '..', 'skills'); -const skillsDest = path.join(pkgRoot, 'templates', 'skills'); - -if (!fs.existsSync(skillsSrc)) { - console.error(`[sync-skills] source not found: ${skillsSrc}`); - process.exit(1); -} - -// A directory with a SKILL.md is a skill — the predicate lives once in -// scripts/lib/skills.mjs, shared with bestax-mcp's sync, gen-mcp-index and -// check-conformance, so the four consumers cannot drift. It is what excludes -// `skills/README.md` and `skills/CLAUDE.md` without naming them, and the list -// comes back sorted so the copy order — and the count in the success line — -// is deterministic. -const skills = await readSkillNames(skillsSrc); - -// Checked BEFORE emptying the destination, which the hardcoded version did not -// have to think about. Reading a roster off disk means a wrong path or a -// renamed directory now yields zero skills instead of an error, and emptying -// first would ship an empty bundle rather than failing. -if (!skills.length) { - console.error(`[sync-skills] no skills found in ${skillsSrc}`); +import { syncSkills } from '../../scripts/lib/sync-skills.mjs'; + +const pkgRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '..' +); + +await syncSkills({ + src: path.resolve(pkgRoot, '..', 'skills'), + dest: path.join(pkgRoot, 'templates', 'skills'), + label: 'templates/skills', +}).catch(err => { + console.error(err.message); process.exit(1); -} - -// The vetting gate the deleted allowlist used to be: discovery bundles -// whatever is on disk, and CI's skills-roster check only sees committed -// state — so an untracked scratch directory would ship in a local build or a -// manual publish with no gate anywhere in the path. `git add` is the act of -// vetting; a tree without git (an exported tarball) skips the gate. -const untracked = untrackedSkillPaths(skillsSrc, skills); -if (untracked.length) { - console.error( - `[sync-skills] refusing to bundle untracked file(s) under skills/: ` + - `${untracked.join(', ')}. \`git add\` them to vet them, or remove them.` - ); - process.exit(1); -} - -await fs.emptyDir(skillsDest); - -for (const name of skills) { - await fs.copy(path.join(skillsSrc, name), path.join(skillsDest, name), { - // .DS_Store is the one path the vetting gate exempts; keep it out of the - // bundle too so the exemption never becomes shipped content. - filter: src => !src.endsWith('.DS_Store'), - }); -} - -console.log(`[sync-skills] copied ${skills.length} skills -> templates/skills`); +}); diff --git a/scripts/coverage-floors.json b/scripts/coverage-floors.json index a62bf984c..e60e5afae 100644 --- a/scripts/coverage-floors.json +++ b/scripts/coverage-floors.json @@ -29,6 +29,7 @@ "scripts/lib/scss-vars.mjs": { "lines": 97, "branches": 89, "functions": 96 }, "scripts/lib/shell-words.mjs": { "lines": 100, "branches": 100, "functions": 100 }, "scripts/lib/skills.mjs": { "lines": 98, "branches": 78, "functions": 100 }, + "scripts/lib/sync-skills.mjs": { "lines": 97, "branches": 87, "functions": 100 }, "scripts/lib/version-regression.mjs": { "lines": 99, "branches": 95, "functions": 100 }, "scripts/mcp-registry-publish.mjs": { "lines": 100, "branches": 95, "functions": 100 }, "scripts/npm-install-retry.mjs": { "lines": 99, "branches": 91, "functions": 100 }, diff --git a/scripts/lib/sync-skills.mjs b/scripts/lib/sync-skills.mjs new file mode 100644 index 000000000..f7d1d0448 --- /dev/null +++ b/scripts/lib/sync-skills.mjs @@ -0,0 +1,241 @@ +// Copy the canonical Agent Skills from the monorepo's top-level `skills/` into +// a package. create-bestax/scripts/sync-skills.mjs (into templates/skills, so +// scaffolded apps get them) and bestax-mcp/scripts/sync-skills.mjs (into +// data/skills, so the server can serve their bodies offline) each used to +// carry a near-identical copy of this. They are thin callers now, and each +// says why it does or does not take the lock below. +// +// `/skills` is the single source of truth, and every destination is generated +// and gitignored. The roster is READ, not listed (#540): every directory +// holding a SKILL.md is copied, through the shared predicate in skills.mjs. +import { cp, mkdir, readFile, rm, stat, writeFile } from 'node:fs/promises'; +import { existsSync, rmSync } from 'node:fs'; +import { createHash } from 'node:crypto'; +import { setTimeout as sleep } from 'node:timers/promises'; +import { join } from 'node:path'; + +import { + assertSkillsVetted, + isDsStore, + readSkillNames, + skillFiles, +} from './skills.mjs'; + +const TAG = '[sync-skills]'; + +// A sync is ~390 KB of file copies and takes tens of milliseconds. A lock held +// longer than this is a crashed run, not a slow one. +const LOCK = { staleMs: 60_000, timeoutMs: 120_000, pollMs: 50 }; + +const SIGNALS = ['SIGINT', 'SIGTERM', 'SIGHUP']; + +/** + * Copy every skill under `src` into `dest`, after the checks every bundler + * makes, and log one line saying what happened. Throws an Error whose message + * starts with `[sync-skills]` on any expected failure, for the caller to print. + * + * - `label` is how the success line names `dest` (`templates/skills`). + * - `stateDir`, when given, holds a lock and a fingerprint of the source. + * Concurrent callers then take turns, and a run whose source matches the + * last copy changes nothing. Without it every run empties `dest` and copies + * afresh, which is only safe when no two callers ever overlap. + * - `lock` overrides the lock timings, for tests. + */ +export async function syncSkills({ + src, + dest, + label, + stateDir, + lock = {}, + log = console.log, +}) { + if (!existsSync(src)) throw new Error(`${TAG} source not found: ${src}`); + + const names = await readSkillNames(src); + + // Checked BEFORE emptying the destination. Reading a roster off disk means + // a wrong path or a renamed directory yields zero skills instead of an + // error, and emptying first would ship an empty bundle rather than failing. + if (!names.length) throw new Error(`${TAG} no skills found in ${src}`); + + // The vetting gate the deleted allowlist used to be: discovery bundles + // whatever is on disk, and CI's skills-roster check only sees committed + // state, so an untracked scratch file would ship in a local build or a + // manual publish with no gate anywhere in the path. `git add` is the act + // of vetting, and a tree without git (an exported tarball) skips the gate. + try { + assertSkillsVetted(src, names, 'bundle'); + } catch (err) { + throw new Error(`${TAG} ${err.message}`, { cause: err }); + } + + if (!stateDir) { + await copySkills(src, dest, names); + log(`${TAG} copied ${names.length} skills -> ${label}`); + return; + } + + const timing = { ...LOCK, ...lock }; + const stampFile = join(stateDir, 'fingerprint'); + const held = { dir: join(stateDir, 'lock'), held: false }; + + await mkdir(stateDir, { recursive: true }); + const want = await fingerprint(src, names); + + // Release on interrupt. Ctrl-C during `pnpm all` is the realistic way a lock + // gets abandoned, and node does not run `finally` blocks on a signal, so + // without this the next run would meet a lock nobody holds. The handlers go + // again once the run ends, so a caller that syncs twice does not stack them. + const onSignal = () => { + release(held); + process.exit(130); + }; + for (const signal of SIGNALS) process.once(signal, onSignal); + + try { + await acquireLock(held, timing); + const have = await readFile(stampFile, 'utf8').catch(() => ''); + + // The freshness check is load-bearing, not an optimisation. Without it a + // second and third caller would re-run the destructive copy below while + // the FIRST caller's jest is already reading the copy, and `rm` would + // delete the tree out from under it. Skipping the work when it is already + // done is what actually makes concurrent callers safe. The lock alone + // would not. + if (have.trim() === want && existsSync(dest)) { + log(`${TAG} up to date (${names.length} skills)`); + } else { + await copySkills(src, dest, names); + // Stamped last: a run killed mid-copy leaves no stamp, so the next + // caller redoes the work rather than trusting a half-populated tree. + await writeFile(stampFile, `${want}\n`); + log(`${TAG} copied ${names.length} skills -> ${label}`); + } + } finally { + release(held); + for (const signal of SIGNALS) process.off(signal, onSignal); + } +} + +/** Empty `dest`, then copy each skill into it, leaving `.DS_Store` out. */ +async function copySkills(src, dest, names) { + await rm(dest, { recursive: true, force: true }); + await mkdir(dest, { recursive: true }); + for (const name of names) { + await cp(join(src, name), join(dest, name), { + recursive: true, + // .DS_Store is the one path the vetting gate exempts. Keep it out of + // the bundle too so the exemption never becomes shipped content. + filter: path => !isDsStore(path), + }); + } +} + +// Hash of every source path and its bytes, over the files a copy ships. +// Content rather than mtime on purpose: a fresh clone or a branch switch +// rewrites mtimes without changing what should be copied, and re-copying is +// the cheap half. It is the destructive `rm` that has to be avoided while +// another process is reading. +async function fingerprint(src, names) { + const hash = createHash('sha256'); + for (const name of names) { + for (const rel of await skillFiles(join(src, name))) { + hash.update(`${name}/${rel}`); + hash.update(await readFile(join(src, name, rel))); + } + } + return hash.digest('hex'); +} + +// Sync calls only: on a signal the process is on its way out and async +// cleanup would not finish. +function release(lock) { + if (!lock.held) return; + lock.held = false; + try { + rmSync(lock.dir, { recursive: true, force: true }); + } catch { + // Best effort, since we are already exiting. + } +} + +// `mkdir` without `recursive` is atomic and fails with EEXIST if the directory +// exists, which makes it a usable mutex with no dependency. +// +// There is deliberately NO automatic stale-lock reclaim, and that is the +// subtle part. The obvious version (stat the lock, and if it looks old enough +// delete it and retry) has a time-of-check/time-of-use hole: nothing ties the +// delete to the *specific* lock that was observed. Two waiters can both see +// the same abandoned lock, the first deletes it and takes ownership, and the +// second then deletes the *fresh* lock the first is holding. Both proceed into +// the destructive copy, which is precisely the concurrent rm-while-copying +// bug this lock exists to prevent, now reachable only after a crash and +// therefore much harder to reproduce. +// +// The freshness stamp does not rescue that case either: a killed run leaves a +// lock but no stamp (the stamp is written last, on purpose), so both waiters +// see a mismatch and both rebuild. +// +// Every cheap repair keeps a window. Owner tokens, rename-to-unique, put-it- +// back-if-it-was-not-mine all still race on the recreate. Crash-safe mutual +// exclusion on a POSIX filesystem is genuinely hard, and a build script is the +// wrong place to hand-roll it. So an abandoned lock is reported with the exact +// command to fix it rather than guessed at. The signal handlers above mean the +// common interrupt never gets here in the first place. +async function acquireLock(lock, { staleMs, timeoutMs, pollMs }) { + const deadline = Date.now() + timeoutMs; + for (;;) { + try { + await mkdir(lock.dir); + lock.held = true; + return; + } catch (err) { + if (err.code !== 'EEXIST') throw err; + + let ageMs; + try { + ageMs = Date.now() - (await stat(lock.dir)).mtimeMs; + } catch { + // Released between our mkdir and our stat. Retry immediately. + continue; + } + + // Held far longer than a sync can legitimately take, so the holder is + // gone. Report it now instead of burning the full timeout first. + if (ageMs > staleMs) { + throw new Error( + `${TAG} the lock at ${lock.dir} has been held for ` + + `${Math.round(ageMs / 1000)}s, which means an earlier run was killed ` + + `before it could clean up. Remove that directory and re-run:\n` + + ` rm -rf ${lock.dir}`, + { cause: err } + ); + } + + if (Date.now() > deadline) { + throw new Error(`${TAG} timed out waiting for lock: ${lock.dir}`, { + cause: err, + }); + } + await sleep(pollMs); + } + } +} + +// Concurrency +// ----------- +// `pnpm all` runs `turbo run build typecheck test test:coverage`, and +// bestax-mcp's build, test and test:coverage scripts each invoke its sync. On +// a cold turbo cache all three start within milliseconds of each other, and the +// original implementation went straight to `rm -rf data/skills` followed by a +// per-skill copy. Interleaving those produced a different error every run +// (ENOENT on copyfile, EEXIST on mkdir, ENOTEMPTY on rmdir), which read as three +// unrelated flakes rather than one race. +// +// It stayed hidden because a warm cache means only one of the three actually +// executes. It surfaces reliably right after a merge, on a fresh clone, and in +// CI, which is the worst possible time for it. +// +// Fixed here rather than in turbo.json because `pnpm --filter bestax-mcp test` +// has to keep working on its own (bestax-mcp/CLAUDE.md documents it), so the +// script cannot rely on a task graph to serialise its callers. diff --git a/scripts/sync-skills.test.mjs b/scripts/sync-skills.test.mjs new file mode 100644 index 000000000..de31c4368 --- /dev/null +++ b/scripts/sync-skills.test.mjs @@ -0,0 +1,201 @@ +/** + * Holds scripts/lib/sync-skills.mjs, the copy both packages' sync-skills.mjs + * scripts make. They are thin callers, so this is where the behaviour they + * promise is tested: the checks before anything is emptied, the `.DS_Store` + * filter, and for bestax-mcp the lock and the freshness stamp. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + utimesSync, + writeFileSync, +} from 'node:fs'; +import { execFileSync, spawn } from 'node:child_process'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { pathToFileURL } from 'node:url'; + +import { syncSkills } from './lib/sync-skills.mjs'; + +/** A repo-shaped temp dir holding skills/ with two skills and some noise. */ +function fixture(t) { + const root = mkdtempSync(join(tmpdir(), 'bestax-sync-skills-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const src = join(root, 'skills'); + const files = { + 'bestax-a/SKILL.md': '---\nname: bestax-a\n---\n', + 'bestax-a/references/x.md': '# x\n', + 'bestax-a/references/.DS_Store': 'finder', + 'bestax-b/SKILL.md': '---\nname: bestax-b\n---\n', + 'README.md': '# not a skill\n', + 'notes/todo.md': 'no SKILL.md, so not a skill\n', + }; + for (const [rel, body] of Object.entries(files)) { + mkdirSync(join(src, rel, '..'), { recursive: true }); + writeFileSync(join(src, rel), body); + } + const lines = []; + return { + root, + src, + dest: join(root, 'pkg', 'out'), + stateDir: join(root, 'pkg', 'state'), + lines, + log: line => lines.push(line), + }; +} + +/** The options create-bestax passes: no state dir, so no lock. */ +const unlocked = f => ({ ...f, stateDir: undefined, label: 'out' }); + +test('copies every skill, leaves .DS_Store out, and replaces a stale copy', async t => { + const f = fixture(t); + mkdirSync(join(f.dest, 'bestax-gone'), { recursive: true }); + await syncSkills(unlocked(f)); + assert.deepEqual(f.lines, ['[sync-skills] copied 2 skills -> out']); + assert.equal( + readFileSync(join(f.dest, 'bestax-a/references/x.md'), 'utf8'), + '# x\n' + ); + assert.ok(existsSync(join(f.dest, 'bestax-b/SKILL.md'))); + assert.ok(!existsSync(join(f.dest, 'bestax-a/references/.DS_Store'))); + assert.ok(!existsSync(join(f.dest, 'README.md'))); + assert.ok(!existsSync(join(f.dest, 'notes'))); + assert.ok(!existsSync(join(f.dest, 'bestax-gone')), 'a stale skill stays'); + assert.ok(!existsSync(f.stateDir), 'no state without a state dir'); + await syncSkills(unlocked(f)); + assert.equal(f.lines.length, 2, 'without a stamp, every run copies'); +}); + +test('fails before touching the destination', async t => { + const f = fixture(t); + await assert.rejects( + syncSkills({ ...unlocked(f), src: join(f.root, 'nope') }), + /^Error: \[sync-skills\] source not found: / + ); + const empty = join(f.root, 'empty'); + mkdirSync(empty); + mkdirSync(join(f.dest, 'bestax-kept'), { recursive: true }); + await assert.rejects( + syncSkills({ ...unlocked(f), src: empty }), + /^Error: \[sync-skills\] no skills found in / + ); + assert.ok(existsSync(join(f.dest, 'bestax-kept')), 'emptied anyway'); +}); + +test('refuses untracked files in its own repository', async t => { + const f = fixture(t); + const git = (...args) => + execFileSync('git', ['-C', f.root, ...args], { stdio: 'ignore' }); + git('init', '-q'); + git('add', 'skills'); + writeFileSync(join(f.src, 'bestax-b', 'scratch.md'), 'draft\n'); + await assert.rejects( + syncSkills(unlocked(f)), + /^Error: \[sync-skills\] refusing to bundle untracked file\(s\) under skills\/: bestax-b\/scratch\.md\. `git add` them/ + ); + assert.ok(!existsSync(f.dest)); +}); + +test('with a state dir, a current copy is left alone', async t => { + const f = fixture(t); + const sync = () => syncSkills({ ...f, label: 'out' }); + await sync(); + const stamp = readFileSync(join(f.stateDir, 'fingerprint'), 'utf8'); + assert.match(stamp, /^[0-9a-f]{64}\n$/); + assert.ok(!existsSync(join(f.stateDir, 'lock')), 'the lock is released'); + + writeFileSync(join(f.dest, 'marker'), 'kept'); + await sync(); + assert.ok(existsSync(join(f.dest, 'marker')), 'a current copy was redone'); + + // .DS_Store never ships, so it does not make the copy stale either. + writeFileSync(join(f.src, 'bestax-a', '.DS_Store'), 'finder'); + await sync(); + assert.ok(existsSync(join(f.dest, 'marker'))); + + writeFileSync(join(f.src, 'bestax-b', 'SKILL.md'), 'changed\n'); + await sync(); + assert.ok(!existsSync(join(f.dest, 'marker')), 'a changed source was kept'); + assert.notEqual(readFileSync(join(f.stateDir, 'fingerprint'), 'utf8'), stamp); + + // A matching stamp with the copy gone still copies. + rmSync(f.dest, { recursive: true }); + await sync(); + assert.ok(existsSync(join(f.dest, 'bestax-b', 'SKILL.md'))); + assert.deepEqual(f.lines, [ + '[sync-skills] copied 2 skills -> out', + '[sync-skills] up to date (2 skills)', + '[sync-skills] up to date (2 skills)', + '[sync-skills] copied 2 skills -> out', + '[sync-skills] copied 2 skills -> out', + ]); + for (const signal of ['SIGINT', 'SIGTERM', 'SIGHUP']) { + assert.equal(process.listenerCount(signal), 0, `${signal} handler left`); + } +}); + +test('waits for a held lock, and reports an abandoned one', async t => { + const f = fixture(t); + const lockDir = join(f.stateDir, 'lock'); + mkdirSync(lockDir, { recursive: true }); + const lock = { pollMs: 5, timeoutMs: 50 }; + + // Held by someone else for the whole wait. + await assert.rejects( + syncSkills({ ...f, label: 'out', lock }), + /\[sync-skills\] timed out waiting for lock: / + ); + + // Released while we wait. + setTimeout(() => rmSync(lockDir, { recursive: true }), 20); + await syncSkills({ ...f, label: 'out', lock: { ...lock, timeoutMs: 5000 } }); + assert.deepEqual(f.lines, ['[sync-skills] copied 2 skills -> out']); + + // Older than any real run: reported with the fix, never reclaimed. + mkdirSync(lockDir); + const old = new Date(Date.now() - 120_000); + utimesSync(lockDir, old, old); + await assert.rejects( + syncSkills({ ...f, label: 'out', lock }), + /has been held for 1[0-9]{2}s, which means an earlier run was killed[\s\S]*rm -rf / + ); + assert.ok(existsSync(lockDir), 'an abandoned lock was reclaimed'); +}); + +test('a signal while waiting exits 130 and leaves the lock it never held', async t => { + const f = fixture(t); + const lockDir = join(f.stateDir, 'lock'); + mkdirSync(lockDir, { recursive: true }); + const lib = pathToFileURL(join(import.meta.dirname, 'lib/sync-skills.mjs')); + const options = { ...f, lines: undefined, log: undefined, label: 'out' }; + // The child says when its signal handlers are in, which is just before it + // starts waiting on the lock this test holds. + const child = spawn( + process.execPath, + [ + '--input-type=module', + '-e', + `import { syncSkills } from ${JSON.stringify(lib.href)}; + const run = syncSkills(${JSON.stringify({ ...options, lock: { pollMs: 5 } })}); + const tick = setInterval(() => { + if (process.listenerCount('SIGTERM')) { + clearInterval(tick); + process.stdout.write('waiting\\n'); + } + }, 5); + await run;`, + ], + { stdio: ['ignore', 'pipe', 'inherit'] } + ); + await new Promise(resolve => child.stdout.once('data', resolve)); + child.kill('SIGTERM'); + const code = await new Promise(resolve => child.on('exit', resolve)); + assert.equal(code, 130); + assert.ok(existsSync(lockDir), 'a lock this run never held was removed'); +}); From 759276e1dbd2f9351336513818edcb16fe5049a0 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sat, 3 Oct 2026 16:49:36 -0400 Subject: [PATCH 8/9] build: drop the skill roster wrappers in check-conformance check-conformance re-exported SKILL_DIR_NAME and rosterSkillNames, and its readSkillDirs and readSkillNames only forwarded to lib/skills.mjs. The check now imports from lib directly, and skills-roster.test.mjs takes each function from lib rather than from both places. checkSkillsRoster rebuilt readSkillNames inline from the dirs it had already read. lib/skills.mjs gains bundledSkillNames for that, and readSkillNames uses it too. The stale "local copy" comment in gen-skills-rosters.mjs goes. gen-skills-rosters's line floor drops from 72 to 71. The deleted comment counted as four covered lines, so the file moves from 88 of 121 lines to 84 of 117. Nothing that ran before stopped running. --- scripts/check-conformance.mjs | 21 ++++++--------------- scripts/coverage-floors.json | 2 +- scripts/gen-skills-rosters.mjs | 4 ---- scripts/lib/skills.mjs | 12 +++++++++--- scripts/skills-roster.test.mjs | 17 ++++++++--------- 5 files changed, 24 insertions(+), 32 deletions(-) diff --git a/scripts/check-conformance.mjs b/scripts/check-conformance.mjs index 0bd031c9a..60dfb47e9 100644 --- a/scripts/check-conformance.mjs +++ b/scripts/check-conformance.mjs @@ -104,8 +104,8 @@ import { import { mdFiles } from './lib/api-catalog.mjs'; import { SKILL_DIR_NAME, - readSkillDirs as libReadSkillDirs, - readSkillNames as libReadSkillNames, + bundledSkillNames, + readSkillDirs, rosterSkillNames, } from './lib/skills.mjs'; import { renderPage } from './gen-api-docs.mjs'; @@ -3282,18 +3282,9 @@ function section(heading) { }; } -// The roster definition itself lives in scripts/lib/skills.mjs, shared with -// both sync scripts and gen-mcp-index.mjs so the four consumers cannot drift. -// Re-exported here because this check and its tests are the historical home. -export { SKILL_DIR_NAME, rosterSkillNames }; - -export async function readSkillDirs(dir = join(REPO, 'skills')) { - return libReadSkillDirs(dir); -} - -export async function readSkillNames(dir = join(REPO, 'skills')) { - return libReadSkillNames(dir); -} +// The roster definition itself (readSkillDirs, rosterSkillNames and the rest) +// lives in scripts/lib/skills.mjs, shared with both sync scripts and +// gen-mcp-index.mjs so the consumers cannot drift. /** * What is wrong with the directories themselves, before any roster is read. @@ -3522,7 +3513,7 @@ async function checkSkillsRoster() { // region fails conformance without a separate gen:check step. Compared on // the BUNDLED name set (every dir with a SKILL.md), matching the generator, // not the prose-expressible subset the copies above are held to. - const bundled = dirs.filter(d => d.hasSkillFile).map(d => d.name); + const bundled = bundledSkillNames(dirs); for (const { file, fence } of SKILLS_INSTALL_TARGETS) { let text; try { diff --git a/scripts/coverage-floors.json b/scripts/coverage-floors.json index e60e5afae..d9c0870a6 100644 --- a/scripts/coverage-floors.json +++ b/scripts/coverage-floors.json @@ -12,7 +12,7 @@ "scripts/gen-component-catalog.mjs": { "lines": 91, "branches": 77, "functions": 75 }, "scripts/gen-eslint-meta.mjs": { "lines": 97, "branches": 82, "functions": 96 }, "scripts/gen-mcp-index.mjs": { "lines": 86, "branches": 85, "functions": 88 }, - "scripts/gen-skills-rosters.mjs": { "lines": 72, "branches": 20, "functions": 50 }, + "scripts/gen-skills-rosters.mjs": { "lines": 71, "branches": 20, "functions": 50 }, "scripts/handoff-message.mjs": { "lines": 100, "branches": 100, "functions": 100 }, "scripts/lib/api-catalog.mjs": { "lines": 100, "branches": 100, "functions": 100 }, "scripts/lib/api-page.mjs": { "lines": 69, "branches": 76, "functions": 44 }, diff --git a/scripts/gen-skills-rosters.mjs b/scripts/gen-skills-rosters.mjs index 88e35e917..e81242e80 100644 --- a/scripts/gen-skills-rosters.mjs +++ b/scripts/gen-skills-rosters.mjs @@ -59,10 +59,6 @@ export const TARGETS = [ { file: 'docs/docs/guides/llms/index.md', fence: 'bash' }, ]; -// The roster reader lives in scripts/lib/skills.mjs — the one predicate all -// consumers share (the local-copy-to-avoid-a-cycle rationale predates the -// lib; a lib import cannot cycle with check-conformance). - /** * The region body: the fenced block, one install line per skill, * alphabetical. Pure, so the conformance check can compare and the tests can diff --git a/scripts/lib/skills.mjs b/scripts/lib/skills.mjs index 950072869..c50dae158 100644 --- a/scripts/lib/skills.mjs +++ b/scripts/lib/skills.mjs @@ -65,9 +65,15 @@ export async function readSkillDirs(dir) { * READ from the directory — never a hardcoded list (#540). */ export async function readSkillNames(dir) { - return (await readSkillDirs(dir)) - .filter(d => d.hasSkillFile) - .map(d => d.name); + return bundledSkillNames(await readSkillDirs(dir)); +} + +/** + * readSkillNames for `dirs` already read with readSkillDirs, so a caller + * holding both views does not walk the directory twice. + */ +export function bundledSkillNames(dirs) { + return dirs.filter(d => d.hasSkillFile).map(d => d.name); } /** diff --git a/scripts/skills-roster.test.mjs b/scripts/skills-roster.test.mjs index 37e5f982f..ca5f43f9a 100644 --- a/scripts/skills-roster.test.mjs +++ b/scripts/skills-roster.test.mjs @@ -19,15 +19,18 @@ import assert from 'node:assert/strict'; import { frontmatter } from './lib/api-page.mjs'; import { SKILL_ROSTERS, - readSkillDirs, - readSkillNames, - rosterSkillNames, rosterViolations, skillDirViolations, skillsPageViolations, frontmatterNameViolations, } from './check-conformance.mjs'; -import { pathsInsideSkills, untrackedSkillPaths } from './lib/skills.mjs'; +import { + pathsInsideSkills, + readSkillDirs, + readSkillNames, + rosterSkillNames, + untrackedSkillPaths, +} from './lib/skills.mjs'; const repoFile = rel => readFileSync(fileURLToPath(new URL(`../${rel}`, import.meta.url)), 'utf8'); @@ -591,16 +594,12 @@ test('the generated install regions are fresh on the real tree', async () => { const { TARGETS, REGION_ID, renderInstallBlock } = await import('./gen-skills-rosters.mjs'); const { readRegions } = await import('./lib/api-page.mjs'); - const { readSkillNames } = await import('./lib/skills.mjs'); - const skills = await readSkillNames( - fileURLToPath(new URL('../skills', import.meta.url)) - ); for (const { file, fence } of TARGETS) { const region = readRegions(repoFile(file), file).get(REGION_ID); assert.ok(region, `${file} lost its ${REGION_ID} marker pair`); assert.equal( region.body, - renderInstallBlock(skills, fence), + renderInstallBlock(SKILLS, fence), `${file} is stale — run pnpm gen:skills` ); } From 402af3c7ac95d0b2538a76b1b34d86052ef3247e Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sat, 3 Oct 2026 16:51:01 -0400 Subject: [PATCH 9/9] build: key the MCP skill manifest off the directory name The MCP manifest took name and promptName from SKILL.md frontmatter (`fm.name || name`) while every roster used the directory name, and the `replace(/^bestax-/, '')` slug rule was copied five times. lib/skills.mjs gains skillSlug, and gen-mcp-index and the skills-roster check use it. The manifest now uses the directory name, like the rosters. frontmatterNameViolations stays, because the Agent Skills spec still requires `name` to match the directory, and agents load a skill by that name. Its rationale and message now say so. skills.json is byte-identical, since every name already equals its directory. --- scripts/check-conformance.mjs | 34 ++++++++++++++++++---------------- scripts/gen-mcp-index.mjs | 13 +++++++++---- scripts/gen-mcp-index.test.mjs | 5 ++++- scripts/lib/skills.mjs | 9 +++++++++ scripts/skills-roster.test.mjs | 7 ++++--- 5 files changed, 44 insertions(+), 24 deletions(-) diff --git a/scripts/check-conformance.mjs b/scripts/check-conformance.mjs index 60dfb47e9..f9c471350 100644 --- a/scripts/check-conformance.mjs +++ b/scripts/check-conformance.mjs @@ -107,6 +107,7 @@ import { bundledSkillNames, readSkillDirs, rosterSkillNames, + skillSlug, } from './lib/skills.mjs'; import { renderPage } from './gen-api-docs.mjs'; import { @@ -3057,7 +3058,7 @@ export const expiryRemediation = label => * directions, so a roster still advertising a deleted skill fails too. That * half has no other guard — sync-skills.mjs just silently stops copying it. * - * The docs-site surfaces are held through the slug transform (directory name + * The docs-site surfaces are held through the slug (`skillSlug`, directory name * minus `bestax-`, exactly what gen-mcp-index.mjs ships as `promptName`): the * sidebar entries and the intro bullet roster here, and the per-skill page * files in `skillsPageViolations`. A new skill fails conformance until its @@ -3108,7 +3109,7 @@ export const SKILL_ROSTERS = [ ), fromToken: slug => `bestax-${slug}`, example: n => - `- **[…](./${n.replace(/^bestax-/, '')})** — one line on when to reach for it.`, + `- **[…](./${skillSlug(n)})** — one line on when to reach for it.`, }, ], }, @@ -3172,7 +3173,7 @@ export const SKILL_ROSTERS = [ .map(m => m[1]) .filter(slug => slug !== 'intro'), fromToken: slug => `bestax-${slug}`, - example: n => `'skills/${n.replace(/^bestax-/, '')}',`, + example: n => `'skills/${skillSlug(n)}',`, }, ], }, @@ -3384,8 +3385,8 @@ export function rosterViolations(skills, sources) { } /** - * The per-skill docs pages, keyed by slug (directory name minus `bestax-`) — - * the same transform gen-mcp-index.mjs ships as `promptName`. Pure, like + * The per-skill docs pages, keyed by skillSlug (directory name minus + * `bestax-`), which gen-mcp-index.mjs also ships as `promptName`. Pure, like * rosterViolations, so the branches can be driven with fixtures. */ export function skillsPageViolations(skills, pageFiles) { @@ -3402,7 +3403,7 @@ export function skillsPageViolations(skills, pageFiles) { .map(f => f.replace(/\.(md|mdx)$/, '')) ); for (const name of skills) { - const slug = name.replace(/^bestax-/, ''); + const slug = skillSlug(name); if (!slugs.has(slug)) { violations.push( `docs/docs/skills/${slug}.mdx: missing — every skill has a docs page ` + @@ -3411,7 +3412,7 @@ export function skillsPageViolations(skills, pageFiles) { ); } } - const knownSlugs = new Set(skills.map(n => n.replace(/^bestax-/, ''))); + const knownSlugs = new Set(skills.map(skillSlug)); for (const slug of slugs) { if (slug === 'intro' || knownSlugs.has(slug)) continue; violations.push( @@ -3424,12 +3425,12 @@ export function skillsPageViolations(skills, pageFiles) { } /** - * Frontmatter `name:` must be present and equal the directory name. Every - * prose roster and install line is held to the DIRECTORY name, while - * gen-mcp-index.mjs keys the shipped MCP manifest off the FRONTMATTER - * (`fm.name || name`, and promptName derives from it) — with no gate, one - * edit ships two disagreeing rosters while everything stays green. The Agent - * Skills spec also requires the field, so a missing or empty one fails too. + * Frontmatter `name:` must be present and equal the directory name, as the + * Agent Skills spec requires. Every roster here, the install lines and the + * MCP manifest follow the DIRECTORY, while `npx skills add` and every agent + * that loads a skill read the FRONTMATTER. Without this gate one edit would + * install a skill under a name no roster or MCP prompt uses, with everything + * green. */ export function frontmatterNameViolations(entries) { const violations = []; @@ -3442,9 +3443,10 @@ export function frontmatterNameViolations(entries) { } else if (fmName !== name) { violations.push( `skills/${name}/SKILL.md: frontmatter says "name: ${fmName}" but the ` + - `directory is ${name}. The rosters follow the directory and the MCP ` + - `manifest follows the frontmatter, so a mismatch ships two ` + - `disagreeing rosters. Rename one to match the other.` + `directory is ${name}. The Agent Skills spec requires them to ` + + `match: the rosters and the MCP manifest follow the directory, and ` + + `agents load the skill by its frontmatter name. Rename one to match ` + + `the other.` ); } } diff --git a/scripts/gen-mcp-index.mjs b/scripts/gen-mcp-index.mjs index fd2290bee..992ca5704 100644 --- a/scripts/gen-mcp-index.mjs +++ b/scripts/gen-mcp-index.mjs @@ -54,6 +54,7 @@ import { byCodePoint, readSkillNames, skillFiles, + skillSlug, } from './lib/skills.mjs'; import { clipAtWord, @@ -362,19 +363,23 @@ export async function readSkills(skillsDir = SKILLS_DIR) { return files.sort((a, b) => byCodePoint(a.id, b.id)); }; out.push({ - name: fm.name || name, + // Keyed off the directory, like every roster and install line. The + // skills-roster check holds the frontmatter name to it, as the Agent + // Skills spec requires. + name, // The frontmatter description is already written as a trigger surface — // keyword-dense, ending in a "Use when…" clause. It is exactly what an // MCP tool/prompt description needs, so it ships verbatim. description: collapse(fm.description), - // `bestax-theming` -> `theming`. The prompt name an MCP client shows. - promptName: (fm.name || name).replace(/^bestax-/, ''), + // The prompt name an MCP client shows. + promptName: skillSlug(name), dir: name, references: await listing('references'), examples: await listing('examples'), }); } - return out.sort((a, b) => byCodePoint(a.name, b.name)); + // In readSkillNames' order, which is code-point order. + return out; } /** diff --git a/scripts/gen-mcp-index.test.mjs b/scripts/gen-mcp-index.test.mjs index 51784f516..96ee7ddba 100644 --- a/scripts/gen-mcp-index.test.mjs +++ b/scripts/gen-mcp-index.test.mjs @@ -23,6 +23,7 @@ import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; import { build, readSkills } from './gen-mcp-index.mjs'; +import { skillSlug } from './lib/skills.mjs'; const REPO = join(dirname(fileURLToPath(import.meta.url)), '..'); @@ -158,7 +159,9 @@ test('the skills roster is read from the directory, not a hardcoded list', () => s.description.length > 40, `${s.name} has no trigger description` ); - assert.equal(s.promptName, s.name.replace(/^bestax-/, '')); + // Keyed off the directory, with the shared slug rule. + assert.equal(s.name, s.dir); + assert.equal(s.promptName, skillSlug(s.name)); assert.ok(Array.isArray(s.references)); } const theming = skills.skills.find(s => s.name === 'bestax-theming'); diff --git a/scripts/lib/skills.mjs b/scripts/lib/skills.mjs index c50dae158..74646c624 100644 --- a/scripts/lib/skills.mjs +++ b/scripts/lib/skills.mjs @@ -76,6 +76,15 @@ export function bundledSkillNames(dirs) { return dirs.filter(d => d.hasSkillFile).map(d => d.name); } +/** + * A skill's slug, its directory name minus the `bestax-` prefix + * (`bestax-theming` -> `theming`): the MCP prompt name, and the name of its + * page under docs/docs/skills/ and its docs/sidebars.js entry. + */ +export function skillSlug(name) { + return name.replace(/^bestax-/, ''); +} + /** * The comparison view the skills-roster check holds prose to: bundled AND * expressible. An unexpressible name gets its own violation instead — asking diff --git a/scripts/skills-roster.test.mjs b/scripts/skills-roster.test.mjs index ca5f43f9a..b2e3e6fec 100644 --- a/scripts/skills-roster.test.mjs +++ b/scripts/skills-roster.test.mjs @@ -448,9 +448,10 @@ test('per-skill docs pages: missing and orphaned pages are both caught', () => { }); test('a frontmatter name that disagrees with the directory is caught', () => { - // Every prose roster follows the DIRECTORY name while gen-mcp-index keys the - // shipped MCP manifest off the FRONTMATTER — without this gate a rename in - // one place ships two disagreeing rosters with everything green. + // Every roster and the MCP manifest follow the DIRECTORY name while agents + // load a skill by its FRONTMATTER name. Without this gate a rename in one + // place installs a skill under a name nothing else uses, with everything + // green. const v = frontmatterNameViolations([ { name: 'bestax-optimize', fmName: 'bestax-css-optimize' }, { name: 'bestax-form', fmName: 'bestax-form' },