feat: centralize skill definitions with build-time sync#214
Conversation
🟢 GitNexus Blast Radius: LOW
Changed: None detected View full blast radius graph → Generated by GitNexus — code intelligence powered by knowledge graphs |
|
@L1nusB is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
xkonjin
left a comment
There was a problem hiding this comment.
Quick review pass:
- Main risk area here is input validation, path handling, and malformed payload behavior.
- Good to see test coverage move with the code; I’d still make sure it exercises the unhappy path around input validation, path handling, and malformed payload behavior rather than only the happy path.
- Before merge, I’d smoke-test the behavior touched by SKILL.md, SKILL.md, SKILL.md (+33 more) with malformed input / retry / rollback cases, since that’s where this class of change usually breaks.
reversTeam
left a comment
There was a problem hiding this comment.
Excellent work, @L1nusB. This is a well-thought-out solution to the skill drift problem. Key observations:
Architecture: Single source of truth in gitnexus/skills/*.md with build-time sync to 3 targets is the right call. The skills.manifest.json per target makes subset decisions explicit and reviewable, and the CI enforcement via git diff --exit-code closes the loop.
Code quality:
planSync()as a pure function with injectedreadFileis very testable — good design.discoverSkillNames()in setup.ts replacing the hardcodedSKILL_NAMESarray fixes thegitnexus-pr-reviewomission and prevents future drift.- The lazy initialization pattern for
SKILL_NAMESis clean. - Error handling improvements (propagating real I/O errors vs. silently swallowing ENOENT) are a nice touch.
Testing: 35 tests across 10 groups (T1-T10) is thorough. The analyze-skills-notice.test.ts tests for the migration path are a good addition — testing both the early-return and full-analyze code paths.
Migration path: The checkStaleProjectSkills() deprecation notice in analyze + cleanupProjectLocalSkills() in setup is a thoughtful migration strategy — warn first, clean up when the user explicitly runs setup.
Minor notes:
- The 439-line
docs/skill-sync.mddesign doc is comprehensive but quite large for a doc that ships with the repo. Consider whether this could live in a PR description or wiki instead, since most of it documents the decision-making process rather than ongoing usage. - The Cursor integration now gets all 7 skills (previously 5) — worth calling out in release notes.
LGTM — this eliminates a real maintenance burden.
d5900f9 to
5c4348e
Compare
|
Rebase cleanup and squashing into a single commit. |
CI Report❌ Some checks failed Pipeline Status
Test Results
❌ 2 failed / 4659 passed 46 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
…tegration targets Add a build-time sync system that keeps skill files in parity across .claude/skills/gitnexus, gitnexus-claude-plugin/skills, and gitnexus-cursor-integration/skills from a single canonical source. - Canonical source: gitnexus/src/skills/ with per-target manifests - sync-skills script: reads manifests, copies files, validates parity - CI enforcement: ci-skill-sync.yml reusable workflow fails if files drift - Wired into CI gate and PR metadata alongside quality, unit, integration
5c4348e to
56b8e33
Compare
|
Please submit a new PR if this is still relevant |
Problem
GitNexus agent skills exist as 26 files across 4 locations in the monorepo (
gitnexus/skills/,.claude/skills/gitnexus/,gitnexus-claude-plugin/skills/,gitnexus-cursor-integration/skills/). There is no mechanism to keep them in sync, and drift has already occurred:.claude/gitnexus-cliwas missing a content sentence about PostToolUse hooks present in the source.claudecopies had prettified table formatting diverging from the sourcegitnexus-pr-reviewwas missing from the runtime installer'sSKILL_NAMESarray insetup.ts, meaning it was never installed to user machinesgitnexus-cli,gitnexus-guide) with no documented rationaleSolution
Single source of truth
gitnexus/skills/*.mdis the canonical location. All other copies are derived.Build-time sync script
gitnexus/scripts/sync-skills.tsreads canonical skills and generates derivedSKILL.mdfiles into each integration directory:Per-target manifests
Each target directory has a
skills.manifest.jsondeclaring which skills to include, making subsets explicit and reviewable.CI enforcement
A
skill-sync-checkjob in.github/workflows/ci.ymlruns the sync script thengit diff --exit-code— PRs with stale derived copies fail CI.Content transformations
setup.tsat runtime)<!-- AUTO-GENERATED ... DO NOT EDIT -->header is prependedmcp.json) are never touchedAdditional fixes
SKILL_NAMESinsetup.tsupdated to include all 7 skills (was missinggitnexus-pr-review)gitnexus-cliandgitnexus-guide)Test coverage
35 tests across 10 groups (T1–T10) in
gitnexus/test/unit/sync-skills.test.ts:.md→{name}/SKILL.md, multi-target independencemcp.jsonand other files are never in operation listgitnexus/skills/directoryHow to use
Checklist
npm run sync:skillsis idempotent (0 writes on second run)skill-sync-checkjob)SKILL_NAMESupdated to include all 7 skillsskills.manifest.jsonwith explicit allowlistmcp.jsoncompanion files untouchedSKILL.mddiffers from what the sync script generates