fix(generate): make --rule usable across multiple skill manifests - #82
Conversation
The `--rule <slug>` not-found check ran inside the per-skill loop over SKILLS, so any single-rule run exited 1 on the first skill whose manifest lacked the slug. With two skills, every `--rule` invocation failed: the rule file was written, but `process.exit(1)` fired before `npm run format` and before the AGENTS.md reassembly, leaving the tree unformatted and AGENTS.md stale. Track whether the slug matched an entry in *any* manifest and do the not-found check after the loop, where nothing has been written yet. Also reassemble AGENTS.md and the SKILL.md index on single-rule runs instead of skipping them. The validator asserts AGENTS.md is an exact round-trip of the rule bodies, so skipping reassembly meant a `--rule` run could never produce a committable state. Reassembly is a pure function of the on-disk rule bodies plus the manifest, so it is cheap and idempotent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the rule generation script to allow the --rule flag to search across all manifests before failing, rather than exiting on the first manifest where the rule is missing. Additionally, it ensures that AGENTS.md and SKILL.md are reassembled and formatted during single-rule runs to maintain a committable state. The reviewer suggested simplifying the reassembly loop by removing redundant nullish coalescing fallbacks for skill.__manifest and skill.__rulesDir, as these properties are guaranteed to be populated by the preceding loop.
| const manifest = skill.__manifest ?? (await loadManifest(skill)); | ||
| const rulesDir = skill.__rulesDir ?? path.join(process.cwd(), skill.dir, skill.rulesDir); |
There was a problem hiding this comment.
Since the first loop over SKILLS always runs completely and populates skill.__manifest and skill.__rulesDir for every skill (or exits early via process.exit(1) on failure), these properties are guaranteed to be defined when reaching this reassembly loop. We can simplify this code by removing the redundant nullish coalescing fallbacks and loadManifest calls.
| const manifest = skill.__manifest ?? (await loadManifest(skill)); | |
| const rulesDir = skill.__rulesDir ?? path.join(process.cwd(), skill.dir, skill.rulesDir); | |
| const manifest = skill.__manifest; | |
| const rulesDir = skill.__rulesDir; |
References
- In teaching or example code, avoid adding redundant defensive checks (such as nullish coalescing operators) for fields that are guaranteed to be populated and where the absent-record case is already handled, to keep the code clean and focused on the core concept.
kriszyp
left a comment
There was a problem hiding this comment.
sounds good
🤖 Reviewed with Codex
| console.error(`Rule "${args.rule}" not found in ${skill.manifestFile}`); | ||
| process.exit(1); | ||
| } | ||
| if (args.rule && rules.length > 0) ruleMatched = true; |
There was a problem hiding this comment.
ruleMatched only records whether at least one match exists, but global slug uniqueness is not an enforced invariant: validation resets its seen set per manifest (scripts/generation/validate-generated.mjs:82-99), and the documented contract requires uniqueness only within each manifest (docs/plans-archive/docs-driven-skills.md:98). If two skills legally define the same slug, --rule <slug> regenerates both bodies—and may make two LLM calls—despite promising one rule. Please collect matches before writing and reject ambiguity, add a skill-qualified selector, or deliberately enforce global uniqueness in validation. A duplicate-slug regression test would lock in the chosen contract.
## [1.12.7](v1.12.6...v1.12.7) (2026-09-04) ### Bug Fixes * **generate:** make --rule usable across multiple skill manifests ([#82](#82)) ([6f928c5](6f928c5))
|
🎉 This PR is included in version 1.12.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Problem
scripts/generation/generate-rules.mjs --rule <slug>has been unusable since the repo gained a second skill.The not-found check lived inside the per-skill loop over
SKILLS:A slug lives in exactly one manifest, so every
--rulerun exits 1 on the first skill that doesn't have it. Worse, it exits after writing the rule file but beforenpm run formatand before the AGENTS.md reassembly — so the tree is left unformatted and AGENTS.md stale.Reproduced against
main, both directions:Second, related issue: the AGENTS.md reassembly block was guarded by
if (!args.rule)— "skip on single-rule runs, AGENTS.md is regenerated on the next full run." Butvalidate-generated.mjsasserts AGENTS.md is an exact round-trip of the rule bodies, so a--rulerun could never produce a committable state.Fix
Hoist the not-found check out of the per-skill loop. Track whether the slug matched an entry in any manifest; error and exit after the loop, where nothing has been written yet. The message now names both manifests.
Always reassemble AGENTS.md and the SKILL.md index, single-rule runs included. It's a pure function of the on-disk rule bodies plus the manifest — cheap and idempotent — so there's no reason to skip it, and skipping it is what made
--ruleruns uncommittable. Theif (!args.rule)wrapper is gone and the block is dedented (most of the diff is that reindent).The
--force/LLM generation path is untouched.Verification
--rule no-such-ruleexits 1 withRule "no-such-rule" not found in any manifest (harper-best-practices/rules.manifest.yaml, harper-mcp/rules.manifest.yaml).harper-mcp/rules/connecting-clients.md, ran--rule adding-tables-with-schemas(the other skill's slug), and the marker appeared inharper-mcp/AGENTS.md. Reverted and re-ran — tree clean, confirming idempotence.npm run validatepasses (format check, build, skill validation, generated round-trip).Both test slugs are
mode: synthesized, so these runs needed no docs build and noANTHROPIC_API_KEY.Context
Found while working on #81, which had to work around this by replaying the post-loop phase (format,
assembleAgentsMd, oxfmt, SKILL.md index splice) in a throwaway script — a fullnpm run generatewasn't an option there, since 7 unrelated rules with older baselines would have been regenerated and polluted the PR.🤖 Generated with Claude Code