Skip to content

build: fix and consolidate the skill and catalog generators #880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
allxsmith merged 9 commits into from
Oct 3, 2026
Merged

build: fix and consolidate the skill and catalog generators #880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5be2a96
build: read frontmatter once and stop name: reading the next line
allxsmith Oct 3, 2026
ab03abe
build: share the API page walk and its completeness guard
allxsmith Oct 3, 2026
00af8fa
build: share the catalog one-liner pieces
allxsmith Oct 3, 2026
3d97962
build: import the pasted helpers from scripts/lib
allxsmith Oct 3, 2026
4c0d172
build: make the catalog generator testable and prettier-formatted
allxsmith Oct 3, 2026
10f422f
build: index only the skill files the sync scripts ship
allxsmith Oct 3, 2026
066a14c
build: share one skills sync between create-bestax and bestax-mcp
allxsmith Oct 3, 2026
759276e
build: drop the skill roster wrappers in check-conformance
allxsmith Oct 3, 2026
402af3c
build: key the MCP skill manifest off the directory name
allxsmith Oct 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 5 additions & 3 deletions bestax-mcp/CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion bestax-mcp/data/skills.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
{
"id": "component-catalog",
"file": "references/component-catalog.md",
"bytes": 18545
"bytes": 18542
},
{
"id": "library-contributor",
Expand Down
261 changes: 24 additions & 237 deletions bestax-mcp/scripts/sync-skills.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
});
Loading
Loading