Fix codegen/validator bugs and a standalone-install break across four skills#346
Conversation
`tailwind_config_gen.py` serialized the config by json.dumps()-ing
everything except `plugins`, stripping the outer braces, then appending
`plugins: [...]` in the template. The JSON block's last property (`theme`)
has no trailing comma, so the emitted config was:
"theme": { ... }
plugins: [],
which is a syntax error (`Unexpected identifier 'plugins'`) — every
generated tailwind.config.ts / tailwind.config.js failed to parse.
Add the missing comma after the serialized block in both
`_generate_typescript` and `_generate_javascript`.
The existing 54 tests passed because they only assert on the data
structure, never parsing the emitted string as JS. Add a
`TestGeneratedConfigIsValidJs` regression class that (1) asserts the
property before `plugins` is comma-terminated (pure Python) and (2)
parses the generated config with `node --check` when node is available.
Both new cases fail on the pre-fix code.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The scanner skipped any line containing `var(--` outright, so a hardcoded
value sharing a line with a token reference was never reported:
.btn { background: #FF6B6B; color: var(--color-primary); } // missed
This is a broad false-negative: real CSS routinely puts several properties
on one line, and minified CSS is a single line — one `var()` anywhere
suppressed every violation in the file. (The Python slide validator already
handles this correctly, so the two validators disagreed.)
Remove the line-level skip. Token-definition files are already excluded by
`skipPatterns` at the file level, and none of the detection regexes can
match inside a `var(--...)` reference, so this adds no false positives.
Add a pytest regression test (drives the CLI via node) asserting a hex
sharing a line with a token is flagged, and that a token-only line stays
clean. The first case fails on the pre-fix code.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The color parser required a parenthesized name in the Quick Reference row
(`#2563EB (name)`) and a bolded label in the color tables
(`**Primary Blue** | #hex`). The bundled `brand-guidelines-starter.md` uses
neither — its rows are `| Primary Color | #2563EB |` and
`| Primary Blue | #2563EB |`. So no base hex was extracted, and
`generateColorScale(undefined)` → `adjustBrightness(undefined)` threw:
TypeError: Cannot read properties of undefined (reading 'replace')
i.e. the documented "edit guidelines → sync" flow crashed on the skill's
own starter template.
- Rewrite the parser: Quick Reference matches hex without a parenthesized
name; color-table rows match with optional bold; an accent swatch living
in another table (the starter's "Accent Green" under Secondary Colors) is
picked up by a fallback scan.
- Skip any role with no base hex (with a warning) instead of crashing, guard
adjustBrightness against non-string input, and initialize tokens.primitive
so a tokens file without it doesn't throw.
Add a pytest regression test (drives the CLI via node) that syncs the bundled
starter template and asserts the expected base colors are written. It fails
on the pre-fix code.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`.claude/skills/ui-ux-pro-max/scripts` and `.../data` were symlinks into the
repo's top-level `src/ui-ux-pro-max/`:
scripts -> ../../../src/ui-ux-pro-max/scripts
data -> ../../../src/ui-ux-pro-max/data
That resolves in a full checkout, but installing a single skill by copying
`.claude/skills/<name>/` into `~/.claude/skills/` leaves the links pointing at
`~/src/ui-ux-pro-max/`, which does not exist. The links dangle and `search.py`
— the entire searchable engine behind --design-system / --domain / --persist —
fails with `No such file or directory`. (The embedded rule tables in SKILL.md
still work, masking the breakage.)
Replace the two symlinks with the real `scripts/` and `data/` files so the
skill is self-contained, matching how the sibling skills (design,
design-system, ui-styling, brand) already ship real files under their own
`scripts/`. Maintainers who prefer to keep the `src/` + symlink architecture
may instead document that this skill requires a full-repo checkout.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Summary: This PR is too large for a cron-safe maintainer review in one pass. Decision: deferred Evidence:
Next step: please split this into reviewable PRs by concern:
After splitting, each lane can be reviewed and merged without blocking unrelated fixes behind the large vendored data/script diff. |
mrgoonie
added
agent:github-maintain
|
Summary: Deferred from the cron maintainer lane due to size/scope. This PR fixes useful validator/codegen bugs, but it also vendors ~9k lines of |
|
Cron maintainer follow-up: this PR is unchanged since the previous defer (head `78343656aef2d1b7b24458449d50bae8cfeeae27`, 42 files, +9474/-79). Keeping it deferred for the same reason: it combines three useful focused validator/codegen fixes with a large standalone-install vendoring/symlink strategy that overlaps the packaging/asset-sync queue. Please split the three bugfix/test lanes from the vendored data/scripts packaging decision, or add an explicit sync/verification command and rationale showing why this supersedes the overlapping asset/package PRs. |
|
Cron maintainer follow-up: still deferring deep review of this PR in cron-safe mode. Current metadata is 42 changed files with +9474/-79, spanning codegen/validator fixes plus standalone-install packaging across four skills. That is too large and too mixed for the bounded 1-PR lane, and it overlaps the packaging/symlink/release queue (#336, #340, #355, #368, #353, #362). Please split into narrow PRs with one source-of-truth change per PR and include the sync/verification command for any generated or packaged asset mirrors. |
|
Cron maintainer follow-up: keeping this PR deferred in cron-safe mode. It is unchanged since the previous defer (head Next action remains: split the validator/codegen fixes into narrow PRs, and handle |
clark-cant
left a comment
clark-cant
left a comment
There was a problem hiding this comment.
Review Summary
PR: #346 — Fix codegen/validator bugs and a standalone-install break across four skills
Author: @Pirate252
Risk level: Medium (42 files, but bulk is vendored data; actual code changes are small and focused)
Verdict: Approve
Mandatory Gates
- Duplicate / prior implementation: Clear. No overlapping PRs found for these specific fixes. PR #368 also vendors symlinks but is a separate broader effort (+9300 lines, different scope). These are distinct bug fixes.
- Project standards: Docs found (CLAUDE.md). Changes follow existing patterns — regression tests in pytest, CJS scripts unchanged in style, CSV data files match existing format.
- Strategic necessity: Clear value. Three real bugs fixed (broken Tailwind config output, false-negative token validation, crash on bundled starter template) plus standalone install break. These directly affect user experience and issue #347.
Findings
No Critical or Important issues found.
Suggestion: The vendored data/scripts files (~9000+ lines) are exact copies from src/ui-ux-pro-max/. This is the correct approach for standalone install (fixes #347), but creates a sync maintenance burden. Consider adding a CI check or pre-commit hook that verifies .claude/skills/ui-ux-pro-max/data/ and scripts/ stay in sync with src/ui-ux-pro-max/ source of truth. This is non-blocking.
Code Quality Assessment
Each of the 4 commits fixes a specific, well-documented bug with a clear root cause explanation:
- Tailwind config missing comma —
json.dumps()serialization omitted trailing comma beforepluginsarray, producing unparseable JS. Fix: add comma. Regression test usesnode --check. - validate-tokens.cjs false negative — Line-level
var(--skip suppressed all violations on lines mixing hardcoded values with token references. Fix: remove the skip (file-levelskipPatternsalready handles token definitions). Regression test covers both cases. - sync-brand-to-tokens.cjs crash — Color parser required parenthesized names and bolded labels that the bundled starter template doesn't use, causing
adjustBrightness(undefined)TypeError. Fix: robust parser with optional bold, fallback scanning, andtypeofguard. Regression test runs sync against bundled starter. - Standalone install break — Symlinked
data/andscripts/dangled when skill was copied to~/.claude/skills/. Fix: vendor real files. This resolves issue #347.
All fixes include regression tests that fail on pre-fix code. Commit messages are exemplary — detailed root cause analysis with code examples.
Posted by github-maintain cron at 2026-06-26T13:23:00Z
4 participants
Three codegen/validator bugs plus a packaging fix. The first two bugs passed the existing test suites because those tests only checked in-memory data structures, never the actual emitted/scanned output; the third script had no tests at all and crashed on the skill's own starter template; the fourth is an install-packaging fix that makes
ui-ux-pro-maxwork as a standalone skill.1.
ui-styling: generated Tailwind config is invalid JStailwind_config_gen.pyserializes the config byjson.dumps()-ing everything exceptplugins, stripping the outer braces, then appendingplugins: [...]in the template. The JSON block's last property (theme) has no trailing comma, so every generatedtailwind.config.ts/.jscame out as:Fixed by adding the missing comma after the serialized block in both
_generate_typescriptand_generate_javascript.2.
design-system:validate-tokens.cjsmisses violations on token linesThe scanner skipped any line containing
var(--outright, so a hardcoded value sharing a line with a token reference was never reported:This is a broad false-negative — real CSS routinely puts several properties on one line, and minified CSS is a single line, so one
var()anywhere suppressed every violation in the file. (The Python slide validator already handled this correctly, so the two validators disagreed.) Fixed by removing the line-level skip; token-definition files remain excluded by the file-levelskipPatterns, and no detection regex can match inside avar(--...)reference, so this adds no false positives.3.
brand:sync-brand-to-tokens.cjscrashes on the bundled starter templateThe color parser required a parenthesized name in the Quick Reference row (
#2563EB (name)) and a bolded label in the color tables (**Primary Blue** | #hex). The bundledbrand-guidelines-starter.mduses neither — its rows are| Primary Color | #2563EB |and| Primary Blue | #2563EB |. So no base hex was extracted, andgenerateColorScale(undefined)→adjustBrightness(undefined)threw:i.e. the documented "edit guidelines → sync" flow crashed on the skill's own starter template. Fixed by rewriting the parser (Quick-Reference hex without a parenthesized name; table rows with optional bold; a fallback that picks up an accent swatch living in another table, as the starter lists "Accent Green" under Secondary Colors), skipping any role with no base hex instead of crashing, guarding
adjustBrightnessagainst non-string input, and initializingtokens.primitiveso a tokens file lacking it doesn't throw.Tests
ui-styling: newTestGeneratedConfigIsValidJs— asserts the property beforepluginsis comma-terminated (pure Python) and parses the output withnode --check. 54 → 58 tests.design-system: newtests/test_validate_tokens.py— drives the CLI vianodeand asserts a hex sharing a line with a token is flagged while a token-only line stays clean.brand: newtests/test_sync_brand_to_tokens.py— drives the CLI vianode, syncs the bundled starter template, and asserts the expected base colors are written.All three new failing cases were confirmed to fail on the pre-fix code; the test files are pytest-based so the existing CI runs them. Full suite: 61 passed.
Note (not addressed in this PR):
banner-designhas undeclared dependenciesNot a code change — flagging for maintainers. The
banner-designskill's workflow delegates to sibling skills and scripts that aren't in this repo, so its end-to-end pipeline can't run from a clean install:frontend-design,ai-artist,ai-multimodal,chrome-devtools,assets-organizing..claude/skills/.venv/bin/python3,.claude/skills/ai-artist/scripts/search.py,.claude/skills/ai-multimodal/scripts/gemini_batch_process.py,.claude/skills/chrome-devtools/scripts/screenshot.js.The skill degrades to manual guidance rather than failing loudly. Worth either bundling/declaring these as prerequisites in
banner-design/SKILL.mdor documenting that it requires the larger skill suite. (Thedesignskill's banner section references the sameai-artist/ai-multimodal/chrome-devtoolsset.)4.
ui-ux-pro-max: symlinks break a standalone skill installIn
.claude/skills/ui-ux-pro-max/, bothscriptsanddataare symlinks into the repo's separate top-level source dir:That resolves inside a full repo checkout, but the natural way to install a single skill is to copy the
.claude/skills/<name>/directory into~/.claude/skills/. After that copy the relative path resolves to~/src/ui-ux-pro-max/, which doesn't exist — the links dangle andsearch.py(the entire searchable engine) fails withNo such file or directory. The embedded rule tables inSKILL.mdstill work, masking the breakage, but every--design-system/--domain/--persistcommand is broken.Fixed by vendoring the real
scripts/+data/files into.claude/skills/ui-ux-pro-max/, making it self-contained like the sibling skills (design,design-system,ui-styling,brand) which already ship real files. Maintainers who prefer to keep thesrc/+ symlink architecture may instead revert this commit and document that the skill requires a full-repo checkout — the commit is isolated for exactly that reason.