Skip to content

fix(skills): finish the skill-relative path contract below SKILL.md (#474 finding 1) - #482

Merged
clark-cant merged 1 commit into
nextlevelbuilder:mainfrom
notbucki:fix/skill-paths-references-and-contract
Sep 3, 2026
Merged

fix(skills): finish the skill-relative path contract below SKILL.md (#474 finding 1)#482
clark-cant merged 1 commit into
nextlevelbuilder:mainfrom
notbucki:fix/skill-paths-references-and-contract

Conversation

@notbucki

@notbucki notbucki commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What does this PR change?

Finishes the path contract started in #476: the same install-specific paths one level below SKILL.md (references/, two scripts), a "Script Paths" statement per sub-skill, a positive test that every shipped markdown invocation resolves, a regression test for the code path that silently skipped, and a CI contract that covers both skill trees and every rooted form.

Why?

Refs #474 — finding 1 follow-up; findings 2 and 3 stay open there. #476 rewrote design/SKILL.md and its contract grepped only */SKILL.md, so 27 home-rooted paths in design/references/ (29 after #470 merged), 19 project-rooted invocations across brand/, slides/ and design/ references and two code paths in brand/scripts/ kept the defect (see the review in #474).

Matches the 2026-09-03 re-triage in #474: one install-context-safe mechanism (skill-relative paths built from the skill directory), applied to SKILL.md and references in both shipped copies, a test covering every shipped markdown invocation, check:assets retained; workflow-reference and count fixes left out. The working directory stays at the project root rather than becoming the skill directory, because the scripts read and write project files (docs/brand-guidelines.md, assets/design-tokens.json, argument paths) from process.cwd() — the contract documents where the path comes from.

Details

  • 30 home-rooted paths (~/.claude/skills/design/scripts/...): 29 in design/references/{cip,icon,logo}-design.md (27 at review time plus the two --provider muapi lines feat(design): add opt-in MuAPI logo provider #470 added) → scripts/...; the printed hint in design/scripts/cip/generate.py now prints the absolute path derived from __file__, so it is correct from any cwd.
  • 19 project-rooted invocations (.claude/skills/<skill>/scripts/...) in brand/references/, slides/references/ and design/references/scripts/... for a skill's own scripts, ../<skill>/scripts/... for a sibling sub-skill (design → brand / design-system, slides → design-system). The sub-skills are installed side by side in every layout — plugin cache, and copySubSkills in cli/src/utils/template.ts for CLI installs — so the sibling form resolves wherever the skill itself does. design/references/slides-*.md are byte-identical duplicates of slides/references/*.md; both copies are patched in lockstep.
  • 2 code paths in brand/scripts/: sync-brand-to-tokens.cjs resolved generate-tokens.cjs from process.cwd(), so under a plugin or --global install the CSS regeneration was silently skipped (existsSync false); it now resolves from __dirname and warns when the sibling is missing instead of skipping silently. extract-colors.cjs printed a concrete gemini_batch_process.py invocation under a project-rooted path into ai-multimodal, a skill this plugin does not ship; replaced by a one-line hint that names ai-multimodal only as an optional example. That line borders finding 2 — drop it if you would rather handle it there.
  • "Script Paths" section in the five sub-skills that invoke scripts (brand, design, design-system, slides, ui-styling): build the script path from the directory containing the SKILL.md, keep the working directory at the project root. banner-design ships no scripts; the core skill already carries "Running the search tool".
  • Positive test src/ui-ux-pro-max/scripts/tests/test_skill_script_paths.py (mirrored to both scripts/tests copies by sync-assets.mjs, run by the tests.yml pytest job and by test:python): extracts every python|python3|node|bash <path> invocation from every markdown file under .claude/skills/ and cli/assets/skills/ (183 today) and asserts each is skill-relative (scripts/<file>, ../<skill>/scripts/<file>, or the core skill's ${CLAUDE_PLUGIN_ROOT}/... resolved against the repo root) and names a file that ships. On the base 08b2e54 it reports 96 problems (48 per copy; 92 on 58c220f); on this branch 0.
  • Regression test: test_sync_brand_to_tokens.py now asserts that the CSS regeneration ran — fails on the base (the step was skipped), passes on this branch.
  • Contract in check-asset-sync.yml (step renamed "Path contract - no install-specific skill paths"): every file under .claude/skills/ and cli/assets/skills/; home-rooted, project-rooted and variable-rooted forms ($HOME/..., ${PWD}/...); ${CLAUDE_PLUGIN_ROOT}/... allowed only in the plugin-only .claude/skills/ui-ux-pro-max/SKILL.md: a third check pins the token to that file (and to the checker that names it), and the positive test accepts the form only there — sub-skills also ship through the CLI, where the variable does not exist. Patterns require a path into a named skill, so prose that does not name a skill after skills/ (e.g. the install-context comments in design-system/scripts/fetch-background.py) is not a hit; prose that does is flagged like code. LC_ALL=C + -I skip ui-styling/scripts/.coverage, a tracked SQLite file whose recorded absolute paths would otherwise match, while an offending line with a stray non-UTF-8 byte is still printed rather than suppressed. A grep error (rc 2, e.g. an unreadable file) fails the step instead of passing it; a missing skill tree is caught by an explicit check before grep runs. The push filter now includes the workflow itself and sync-assets.mjs. Not covered, documented in the step: backslash-separated Windows spellings and the platform-root-relative skills/<skill>/... form — the positive test above catches any invocation of those shapes, since neither resolves.
  • CLI copy regenerated via cli/scripts/sync-assets.mjs; --check passes.

Verification

  • Contract on the base 08b2e54 (unpatched): 60 home-rooted + 42 project-rooted hits across both copies, exit 1 (56 + 42 on 58c220f, the state reviewed in design skill: 2 plugin-install defects (hard-coded ~/.claude paths, unbundled claudekit references) + style-count drift in manifests #474). On this branch: 0 + 0, exit 0. Decoys: cd ~/.claude/skills/design && ..., ${HOME}/.claude/skills/brand/..., ${CLAUDE_PLUGIN_ROOT}/.claude/skills/brand/..., $(pwd)/.claude/skills/design, /home/me/.claude/skills/design/... are flagged; "${CLAUDE_PLUGIN_ROOT}/.claude/skills/ui-ux-pro-max/scripts/search.py", the bare variable form, and prose such as ~/.claude/skills/, or as a plugin are not; a NUL-containing binary is skipped, a non-UTF-8 text file with a forbidden path is reported, a missing directory exits 1.
  • python3 -m pytest .claude/skills (the tests.yml job): 231 passed / 7936 subtests on this branch; the new positive test fails on the base with 96 problems, and test_sync_brand_to_tokens.py with the new assertion fails on the base's script on exactly that assertion.
  • node --check on both .cjs, py_compile on generate.py and the tests; the generate.py hint prints the absolute logo script path from an unrelated cwd; sync-brand-to-tokens.cjs regenerates assets/design-tokens.css from an unrelated cwd and warns when the sibling skill is absent.

Interaction with open PRs

Out of scope

Checklist

  • Changes were made in src/ui-ux-pro-max/ (source of truth), not directly in .claude/ or .factory/ — the new test lives in src/ui-ux-pro-max/scripts/tests/ and is mirrored by sync-assets.mjs; the sub-skill edits are in .claude/skills/<skill>/, which is their source of truth (mirrored to cli/assets/skills/)
  • Ran npm run sync:assets && npm run check:assets in cli/
  • Added or updated tests if behavior changed (src/ui-ux-pro-max/scripts/tests/test_skill_script_paths.py, .claude/skills/brand/scripts/tests/test_sync_brand_to_tokens.py)
  • Commit messages follow Conventional Commits
  • This PR targets a feature branch, not pushed directly to main

Researched and implemented with Claude Code (an AI coding agent); a human reviewed the changes and approved the submission.

🤖 Generated with Claude Code

Refs #474 (finding 1 follow-up to #476, which rewrote design/SKILL.md
and added a contract that grepped only */SKILL.md; the same defect one
level down survived).

- 29 home-rooted paths (~/.claude/skills/design/scripts/...) in
  design/references/{cip,icon,logo}-design.md -> scripts/... (27 at
  review time, two more added by #470); the printed
  hint in design/scripts/cip/generate.py now derives the absolute path
  from __file__
- 19 project-rooted invocations (.claude/skills/<skill>/scripts/...) in
  brand/, slides/ and design/ references -> scripts/... (own skill) or
  ../<skill>/scripts/... (sibling sub-skill; sub-skills are installed
  side by side in every layout)
- brand/scripts/sync-brand-to-tokens.cjs resolved its sibling script from
  process.cwd(), silently skipping CSS regeneration under plugin and
  --global installs; now resolved from __dirname, with a warning when
  the sibling skill is missing; regression test asserts the regeneration
- brand/scripts/extract-colors.cjs: tool-neutral hint instead of a
  project-rooted path into a skill this plugin does not ship
- "Script Paths" section in the five sub-skills that invoke scripts:
  script path from the skill directory, working directory at the
  project root
- new test_skill_script_paths.py (src, mirrored to both scripts/tests
  copies): every python/node/bash invocation in every shipped skill
  markdown must be skill-relative and name a file that ships
- check-asset-sync.yml: contract covers every file under both skill
  trees and home-/project-/variable-rooted forms; ${CLAUDE_PLUGIN_ROOT}
  allowed only in the plugin-only core SKILL.md; LC_ALL=C + -I for the tracked
  .coverage binary; grep errors fail instead of passing; push filter
  includes the workflow and sync-assets.mjs
- CLI copy regenerated via sync-assets.mjs

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@clark-cant clark-cant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved: focused follow-up for #474 finding 1. The change removes install-specific invocation anchors across shipped skill documentation, validates the shipped paths, preserves asset synchronization, and the required checks are green. Findings 2 and 3 remain tracked in #474.nnPosted by github-maintain cron

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants