Repository navigation
build: fix and consolidate the skill and catalog generators - #880
Merged
Merged
Conversation
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.
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.
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.
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.
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.
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.
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.
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.
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.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (4)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (23)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe changes add shared API catalog and skill utilities, update generators and conformance checks to use them, and centralize skill synchronization for the bestax-mcp and create-bestax scripts. ChangesAPI catalog pipeline
Skill roster and synchronization
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable issue is established; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The shared implementation preserves the inspected callers’ destinations and coordination choices, and extends source vetting to skill indexing. No introduced security concern was established. Existing synchronization assumptions and incomplete exposure coverage leave limited uncertainty. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 79.63% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 54 functions across 16 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
bestax-mcp/scripts/sync-skills.mjsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. create-bestax/scripts/sync-skills.mjsESLint skipped: the matched ESLint configuration already failed (missing-dependency). scripts/api-catalog.test.mjsESLint skipped: the matched ESLint configuration already failed (missing-dependency).
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Preview DeploymentPreview URL: https://3bb13a78.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | Both sync callers print err.message only, so an unexpected error (fs, or a programming error inside syncSkills) is now reported as a single untagged line with no stack — where the old bestax-mcp script let it surface as an unhandled rejection with a full trace. The exit code is still 1, so no gate goes green. |
bestax-mcp/scripts/sync-skills.mjs:33, create-bestax/scripts/sync-skills.mjs:43 |
| 2 | 🔵 Advisory | Correctness | skillFiles pushes only e.isFile() entries, so the bestax-mcp fingerprint no longer notices a change behind a symlinked skill file — which cp (default verbatimSymlinks: false) would still dereference and ship. The old fingerprintSource hashed any non-directory entry. The listing side is unchanged (the old walk already had if (!e.isFile()) continue). find skills -type l returns nothing today. |
scripts/lib/skills.mjs:125 |
| 3 | 🔵 Advisory | Robustness | The shared frontmatter reader is documented as flat ("Nested YAML is not parsed"), so a valid folded or block scalar name (name: >- on its own line) reads as empty and now fails conformance as "frontmatter has no name" rather than being read. All seven skills use plain scalars, so this is latent, not live. |
scripts/lib/api-page.mjs:232 |
| 4 | 🔵 Advisory | API | The module header now lists "frontmatter reads" under "Everything here is FENCE-AWARE … Every scan therefore runs through fenceMask()", but frontmatter() and frontmatterTitle() never call it — they anchor at the --- at byte 0, which is why they are correct anyway. Deleting "Every scan therefore runs through fenceMask()." leaves the rest true. |
scripts/lib/api-page.mjs:5 |
Overall: Sound, and unusually well evidenced for a refactor of this size — all three claimed bugs are real in the pre-PR code, and I confirmed the byte-identical-artefacts claim rather than taking it: gen:catalog:check and gen:mcp:check are both clean, check:conformance is green except the pre-existing shallow-clone version-regression rule, prettier --check passes on the now-unignored catalog, and the twelve new test cases all pass. The riskiest part is the widened blast radius of scripts/lib/sync-skills.mjs and scripts/lib/skills.mjs — four consumers now share one copy loop, one vetting gate and one frontmatter reader, so a future edit there lands everywhere at once; the compensating test files are the thing to read first. Second focus: pnpm gen:mcp is now a hard failure on any untracked or gitignored file under skills/ (new behaviour, deliberate, matching the sync gate) — worth knowing before it bites someone mid-authoring.
Residual risk:
- Coverage floors — refuted empirically. The suite is red locally only on the documented "build
bulma-ui/distfirst" assertions, but the spec reporter still prints the table:lib/api-catalog.mjs100.00/100.00/100.00,lib/sync-skills.mjs97.93/92.50/100.00,gen-component-catalog.mjs91.21/88.89/75.00,gen-api-sources.mjsfunctions 28.57,gen-skills-rosters.mjslines 71.79. Every new row and both lowered rows clear, and the two drops match their commit-message arithmetic exactly (2 of 7 functions; 84 of 117 lines). The 100 branch floor onapi-catalog.mjshas no slack by design (coverage-floors.mjs: a metric at 100% keeps a floor of 100) and matchesshell-words.mjsandapi-sources.mjs. firstSentencedrift — refuted. The deleted copy used(?=\s)where the lib uses(?=\s|$); because.*?is lazy and the input is a collapsed single line, the two can only differ when no period is followed by whitespace and the line ends in one, where the match is the whole line the old code returned anyway. The catalog regenerating byte-identical is the empirical half.- The guard still runs from a different page list than
check-conformancedoes.apiComponents()keys on the frontmatter title and includes.mdx;missingApiPageskeys on the file name and.mdonly. Refuted as drift: they answer different questions (which titles must appear on the listing surfaces, versus which exports have a page), andmissingApiPagesMessagenow states the file-name rule it actually applies. STORY_EXEMPTatscripts/check-conformance.mjs:193still hand-duplicates the six Table sub-parts thatUNDOCUMENTED_EXPORTSlists. Open, and the same shape of drift this PR exists to end: a new Table sub-part needs adding to both. Not consolidated here, and arguably should not be (a story exemption is not a page exemption), but nothing holds the two in step.bestax-mcp/src/server.ts:682keeps its ownbestax-prefix strip. Refuted as a sixth roster copy: it normalises a caller-suppliednameargument before matching againstpromptNameorname, so it derives no shipped roster.- git-less trees still generate.
untrackedSkillPathsreturns an empty list when git is missing, when the tree is outside any repository, or when the resolved toplevel does not ownskills/— so addingassertSkillsVettedtoreadSkillscannot hard-fail an exported tarball. The new gen-mcp-index test drives both the no-repo and the in-repo paths. - New
format:checkcoupling on the catalog. Refuted: the generator'sprettier.resolveConfigcall and the CLI agree here because the repo has no.editorconfigat all (the one divergence between the API and the CLI), the generator matches every other generator in this repo, andgen-component-catalog.test.mjsasserts the fixpoint directly. - Not yet observed:
Build and Testwas stillin_progresson402af3cwhen this review ran, so the green coverage-floor judgement andformat:checkare my local runs, not CI's.
🏄 Nine commits of pure gnarly cleanup and every one of them paddles out clean — three real bugs caught, the artefacts come back byte-for-byte identical, and the duplicate copies that were quietly drifting apart now ride one board. Nothing here to wipe out on. Ship it.
github-actions
Bot
added
the
review-converged
allxsmith
added a commit
that referenced
this pull request
Oct 4, 2026
* build: refuse symlinked skill files and keep sync crash traces Follow-ups from the deep review of #880. skillFiles now refuses a symbolic link instead of skipping it, and syncSkills runs that check for every caller before copying. cp would follow a link and ship its target while the bestax-mcp fingerprint never saw a change behind it. Both sync callers print syncFailureText, which keeps the message for a tagged refusal and the full stack for an unexpected error. The api-page header no longer says every scan goes through fenceMask, since the frontmatter readers anchor at the first line instead. * build: tag only skill refusals and refuse a symlinked skill directory A permission error during the vetting walk was wrapped with the [sync-skills] tag and printed without its stack. Untracked files and symbolic links now carry an ESKILLREFUSAL code, and syncSkills tags only those, so any other error keeps its own message and stack. readSkillDirs now lists a symlinked skill directory instead of skipping it, and skillFiles refuses it, so a linked skill fails loudly rather than going missing from every bundle. A symlink named .DS_Store is refused too, and the comments now say cp would recreate a link pointing at this checkout rather than follow it. * build: share the symlink refusal and copy only the vetted files readSkillDirs lists a symbolic link only when it points at a directory, and marks it with isSymlink, so a linked file such as AGENTS.md is no longer read as a skill. The conformance check reports a linked skill directory, and gen-mcp-index walks each skill once from its root, so it refuses a linked skill directory or SKILL.md just as the sync does. bestax-mcp/data is unchanged. syncSkills now checks each skill after taking the lock and copies exactly the files it checked, so a link added while a run waited can no longer ship. The fingerprint and the copy share that list, which also drops the second walk. On the real tree the copied files and the bestax-mcp fingerprint match what cp produced. A .DS_Store symlink is skipped again instead of refused, since the copy ships only listed files and the listing never includes .DS_Store. Every expected failure carries ESKILLREFUSAL, and failureText in skills.mjs prints its message, or the stack for anything else. Both sync scripts and gen-mcp-index print through it, which replaces syncFailureText and its prefix check.
|
🎉 This PR is included in version 5.26.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three bug fixes, each with a regression test:
name:was read from the next line.The sync scripts, catalog code, frontmatter reader and pasted helpers now live in scripts/lib. Generated files are byte-identical except the catalog header, which now says
pnpm gen:catalog. Two coverage floors drop slightly because covered lines moved or were deleted. Each commit says why.Summary by CodeRabbit
pnpm gen:catalog.