feat(skills): add Systematic skill authoring guardrails

[marcusrbrown]

Summary

• Add the writing-systematic-skills bundled skill and foundation reference for Systematic-specific authoring conventions.
• Extend content-integrity with enforced skill frontmatter and bundled-agent model: inherit checks.
• Normalize existing catalog drift surfaced by the new checks and keep registry/docs counts aligned.

Validation

• bun run build
• bun run typecheck
• bun test
• bun scripts/content-integrity.ts
• bun run registry:build
• bun run registry:validate
• bun run docs:build
• node --input-type=module -e "const m = await import('./dist/index.js'); if (typeof m.default !== 'function') throw new Error('default export is not function'); const keys = Object.keys(m); if (keys.length !== 1 || keys[0] !== 'default') throw new Error('unexpected exports: ' + keys.join(','));"
• grep -rn -E "\\bR[0-9]+|Unit [0-9]+|\\(v[0-9]+\\)" "skills/writing-systematic-skills/" "scripts/content-integrity.ts" "tests/unit/content-integrity.test.ts"

Notes

• bun run lint still reports pre-existing unrelated Biome warnings in skills/claude-permissions-optimizer/scripts, skills/onboarding/scripts, and manual probe files; touched files pass biome check.

Post-Deploy Monitoring & Validation

No additional operational monitoring required. This changes bundled content, validation scripts, tests, and generated registry metadata only; there is no long-running service path to monitor.

[fro-bot]

Verdict: PASS

This is a well-structured, high-quality PR that adds enforcement tooling for Systematic skill authoring conventions. The implementation is thorough, the tests are comprehensive, and all verification passes cleanly.

Blocking issues

None.

Non-blocking concerns

1. checkAgentModel uses parsed.data.model directly without type guard — In content-integrity.ts L712, parsed.data.model is accessed without checking whether data is a record or whether model exists. The rest of the function works correctly because !== 'inherit' is falsy for undefined, null, and empty-string cases — all of which correctly flag as violations. However, the pattern is inconsistent with how checkFrontmatter carefully validates fields via checkRequiredSkillField. This is a minor style concern, not a bug, since the behavior is correct for all cases (missing, null, empty, hardcoded model → all flagged).

2. SUBFILE_PATH_REGEX doesn't match .yaml/.yml in sub-directory paths — The regex at L535-539 matches (?:md|json|ya?ml|sh|ts|js|mjs|txt|py) file extensions, but .yaml would match ya?ml only when preceded by a dot. Since ya?ml already covers both .yaml and .yml, this is fine. The regex is intentionally conservative to avoid false positives, which is the right call. Just noting this for awareness.

3. docs/scripts/transform-content.ts has its own parseFrontmatter (L27-44) that duplicates the logic in src/lib/frontmatter.ts. The PR adds writing-systematic-skills as a registry entry but doesn't address this pre-existing duplication. Not a problem introduced by this PR, just flagged for future cleanup.

Missing tests

None. The test additions are thorough:

• Frontmatter checks: Tests cover minimal valid frontmatter, all 13 runtime-recognized fields, banned preconditions field, unknown fields, missing/null/empty required fields, malformed YAML, missing frontmatter, and scan-scope tests (only SKILL.md, not nested reference files or agent files).
• Agent model checks: Tests cover model: inherit (pass), missing model, hardcoded model, empty model, null model, and scope (skill files not scanned).
• Integration tests: Real-repo smoke test, CLI exit codes, structured result output.
• Existing tests: All 447 tests pass, typecheck passes, lint passes, content-integrity gate passes clean.

The test coverage for the new checks is excellent. The checkContentIntegrity integration test (L974-992) exercises both frontmatter and agent-model violations in a single pass, mirroring real usage.

Risk assessment: LOW

Rationale:

• Regression risk is minimal — The new checks in content-integrity.ts are additive (checks 4 and 5 alongside existing checks 1-3). The existing three checks continue to work identically.
• No plugin API changes — SKILL_FRONTMATTER_FIELDS is a newly exported const from skills.ts, but it's not a breaking change to the plugin's hooks (config, tool, system.transform). The agent normalization changes (model: inherit additions and trailing whitespace removal) are content-only and don't affect runtime behavior.
• Security risk is negligible — The frontmatter and model checks are purely static content validation with no execution paths. The SKILL_FRONTMATTER_FIELDS list is hardcoded and corresponds exactly to the runtime loader's SkillFrontmatter interface, so there's no injection surface.
• Blast radius is contained — If a skill author writes frontmatter with an unrecognized field, the gate catches it at CI time with a clear remediation message pointing to systematic:writing-systematic-skills. No runtime breakage possible.
• All CI checks pass: typecheck, 447 unit/integration tests, content-integrity gate, build.

The compound-docs preconditions removal and body-migration is clean — it correctly replaces YAML frontmatter that had no runtime consumer with a prose "Prerequisites" section in the skill body, which is exactly what the writing-systematic-skills skill prescribes.

---

Run Summary

Field	Value
Event	pull_request
Repository	marcusrbrown/systematic
Run ID	25177784099
Cache	hit
Session	ses_220b7bc30ffeMB8JoFOVMf1dSh

[fro-bot]

Verdict: PASS

Re-reviewing after commit 9f33e2c (fix: default-export smoke test and harden checkAgentModel). This commit addresses both non-blocking concerns from my original review and introduces additional improvements.

Changes in fix commit

1. Hardened checkAgentModel — Now uses an explicit isRecord() type guard before accessing parsed.data.model, resolving my original concern about unchecked access on potentially non-object frontmatter data. A new test covers the case where frontmatter parses as a YAML list (not a record), confirming it's correctly flagged as a violation.

2. Removed parseFrontmatter duplication in docs/scripts/transform-content.ts — My second non-blocking concern flagged this. The docs script now imports parseFrontmatter from src/lib/frontmatter.js instead of maintaining its own regex+yaml implementation. It also uses the parse error field rather than try/catch, consistent with the canonical implementation.

3. Fixed CI workflow — The main.yaml smoke test now uses m.default (the actual plugin export) instead of m.SystematicPlugin (which was never exported), and a plugin test asserts the workflow file contains the correct pattern.

Blocking issues

None.

Non-blocking concerns

None remaining. Both original concerns have been addressed:

• checkAgentModel uses parsed.data.model directly without type guard → Fixed with isRecord() guard
• docs/scripts/transform-content.ts has its own parseFrontmatter → Fixed by importing from src/lib/frontmatter.js

Missing tests

None. The new test for non-object frontmatter in checkAgentModel covers the edge case the type guard now explicitly handles. All 102 tests across the three most-relevant test files pass, all 447 total tests pass, typecheck passes, and content-integrity gate runs clean.

Risk assessment: LOW

Rationale:

• Fix commit is narrowly scoped — type guard addition, duplication removal, CI assertion improvement.
• The isRecord() guard is the canonical pattern already used in loadAllowlist in the same file, so this is consistent.
• Importing parseFrontmatter from src/lib/frontmatter.js in the docs script is correct (.js extension for ESM interop, node: protocol for builtins already correct in that module).
• The CI workflow change corrects a bug (calling a non-existent named export) — the plugin only ever exported default, so the old code was broken at runtime.
• All verification green: typecheck, 447 tests, content-integrity gate, build.

---

Run Summary

Field	Value
Event	pull_request
Repository	marcusrbrown/systematic
Run ID	25178420085
Cache	hit
Session	ses_220b7bc30ffeMB8JoFOVMf1dSh

[fro-bot]

Verdict: PASS

Re-reviewing after merge commit 4862b32 (merge branch 'main'). The merge brings in only an unrelated dependency update (bfra-me/.github v4.16.11, touching two workflow YAML files). No new feature code since the fix commit 9f33e2c that I reviewed in my previous pass.

Verification

• 449 tests pass (2 new from the merge's dependency update, previously 447)
• Content-integrity gate: clean — 0 violations, 0 warnings
• TypeScript: strict mode passes
• No new feature changes since last review

Blocking issues

None.

Non-blocking concerns

None. Both original concerns (type guard in checkAgentModel, parseFrontmatter duplication) were resolved in the fix commit 9f33e2c.

Missing tests

None.

Risk assessment: LOW

No change from previous assessment. The merge is a clean integration of an upstream dependency bump with no conflicts in the PR's feature files.

---

Run Summary

Field	Value
Event	pull_request
Repository	marcusrbrown/systematic
Run ID	25179347519
Cache	hit
Session	ses_220b7bc30ffeMB8JoFOVMf1dSh