feat(skill): add /create-skill — skill factory with quality gates#587
feat(skill): add /create-skill — skill factory with quality gates#587carlos-alm wants to merge 89 commits intomainfrom
Conversation
Encodes lessons from 200+ Greptile review comments across the titan skill PRs into a reusable skill-creation workflow. Includes 10 mandatory patterns (no shell vars across fences, no silent failures, stable cross-references, etc.) and a 17-item self-review checklist that must pass before any new skill can be committed.
|
Claude finished @carlos-alm's task —— View job PR Review Complete ⭐⭐⭐⭐☆Todo List
Root Cause AnalysisProblem Being Solved: The "titan skill PRs" (presumably large skills like Why This Approach: Rather than writing better documentation or linting rules, the author chose to encode institutional knowledge into an interactive skill factory. This is justified — the problems are systematic (200+ comments) and structural (violate Claude Code's execution model), making them good candidates for programmatic prevention. Risk Assessment: Low implementation risk. This is pure markdown documentation with no code paths, but it represents significant process/quality impact for skill authoring workflows. Configuration Changes✅ No configuration changes detected — this adds a new skill file without modifying linting rules, build configs, or test thresholds. Test Integrity✅ No test changes — this PR is purely additive (new skill documentation) with no impact on existing test suite. Code Quality AnalysisStructural Quality: ExcellentThe skill follows codegraph's established skill patterns:
Content Quality: Very StrongThe 10 mandatory patterns (lines 97-204) directly address real issues:
All patterns include wrong/correct examples, which is excellent for skill authors. Self-Review Framework: ComprehensiveThe Phase 4 checklist (lines 238-270) covers:
This is more thorough than most code review templates. Minor Issues
Backlog Compliance Assessment
Critical Strengths
Minor Recommendations
Final AssessmentThis is excellent work that will meaningfully improve skill quality across the codebase. The systematic approach to preventing known failure modes is exactly the right solution. |
Greptile SummaryThis PR adds Key findings:
Confidence Score: 3/5Safe to merge the SKILL.md and smoke-test-skill.sh; the lint-skill.sh Check 4 logic gap means the linter has a false-negative path that degrades trust in the primary quality gate, and the settings.json change is undocumented. The SKILL.md and smoke-test-skill.sh are in good shape after extensive prior iteration. The P1 bug in lint-skill.sh (elif + inline-fi leaves in_detect stuck) is a false-negative that undermines the primary Check 4 quality gate — skill authors relying on the linter could ship a skill with hardcoded npm/yarn/pnpm commands without any warning. The undocumented settings.json change is a separate concern that changes hook firing behavior project-wide.
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["/create-skill skill-name"] --> B["Phase 0: Discovery & Pre-flight\n(git repo check, tool validation, 7 questions)"]
B --> C["Phase 1: Scaffold\n(idempotency guard, mkdir, write SKILL.md template)"]
C --> D["Phase 2: Write Skill Body\n(17 mandatory patterns enforced)"]
D --> E["Phase 3: Dangerous Op Guards\n(git, file deletion, API, code-mod guards)"]
E --> F["Phase 4: Self-Review Checklist\n(28-item checklist)"]
F --> G["Phase 5: Smoke Test\n(lint-skill.sh + smoke-test-skill.sh + Phase 0 dry-run)"]
G --> H["Phase 6: Finalize\n(user review, commit)"]
G --> G1["lint-skill.sh\n(11 static checks)"]
G --> G2["smoke-test-skill.sh\n(bash -n per block)"]
G1 --> C4["Check 4: Hardcoded pkg cmds\n⚠️ elif+inline-fi gap"]
Reviews (46): Last reviewed commit: "fix(skill): align scaffold template orde..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| **Correct:** | ||
| ````markdown | ||
| Detect the test runner: | ||
| ```bash | ||
| if [ -f "pnpm-lock.yaml" ]; then TEST_CMD="pnpm test" | ||
| elif [ -f "yarn.lock" ]; then TEST_CMD="yarn test" | ||
| else TEST_CMD="npm test"; fi | ||
| ``` | ||
| Then run: `$TEST_CMD` | ||
| ```` |
There was a problem hiding this comment.
The "Correct" example for Pattern 6 sets TEST_CMD inside a code fence (lines 179–181) and then references $TEST_CMD in prose outside the fence (Then run: \$TEST_CMD`). This is exactly the cross-fence variable usage that Pattern 1 prohibits — any skill author following this example would produce a skill where $TEST_CMD` is set in one block and used in a separate block, which is the bug Pattern 1 is designed to prevent.
The example should either:
- Keep detection and execution in the same block, or
- Show persisting
TEST_CMDto a file (consistent with Pattern 1's "Correct" guidance)
| **Correct:** | |
| ````markdown | |
| Detect the test runner: | |
| ```bash | |
| if [ -f "pnpm-lock.yaml" ]; then TEST_CMD="pnpm test" | |
| elif [ -f "yarn.lock" ]; then TEST_CMD="yarn test" | |
| else TEST_CMD="npm test"; fi | |
| ``` | |
| Then run: `$TEST_CMD` | |
| ```` | |
| **Correct:** | |
| ````markdown | |
| Detect the test runner and run in a single block: | |
| ```bash | |
| if [ -f "pnpm-lock.yaml" ]; then TEST_CMD="pnpm test" | |
| elif [ -f "yarn.lock" ]; then TEST_CMD="yarn test" | |
| else TEST_CMD="npm test"; fi | |
| $TEST_CMD |
```
There was a problem hiding this comment.
Fixed in 923b51b. Pattern 6's "Correct" example now keeps \ assignment and \ execution in the same bash block, so it no longer violates Pattern 1 (no cross-fence variable usage).
.claude/skills/create-skill/SKILL.md
Outdated
| **Exit condition:** Every dangerous operation identified in Phase: Discovery has a corresponding guard in the SKILL.md. | ||
|
|
||
| ### For code modifications: | ||
| - Run tests after changes: detect test runner per Phase: Write the Skill Body, Pattern 6 | ||
| - Run lint after changes: detect lint runner the same way (check for `biome.json` → `npx biome check`, `eslint.config.*` → `npx eslint`, fallback → `npm run lint`) |
There was a problem hiding this comment.
The Phase 3 exit condition ("Every dangerous operation identified in Phase: Discovery has a corresponding guard") appears on line 226, but the "For code modifications" section (lines 228–230) comes after it. This means an AI following these instructions could declare Phase 3 complete after passing the exit condition, never reading the code-modification guards (test/lint detection).
The exit condition should be the last item in a phase, not an intermediate gate. Move the ### For code modifications section above the exit condition so the gate covers all Phase 3 content:
| **Exit condition:** Every dangerous operation identified in Phase: Discovery has a corresponding guard in the SKILL.md. | |
| ### For code modifications: | |
| - Run tests after changes: detect test runner per Phase: Write the Skill Body, Pattern 6 | |
| - Run lint after changes: detect lint runner the same way (check for `biome.json` → `npx biome check`, `eslint.config.*` → `npx eslint`, fallback → `npm run lint`) | |
| ### For code modifications: | |
| - Run tests after changes: detect test runner per Phase: Write the Skill Body, Pattern 6 | |
| - Run lint after changes: detect lint runner the same way (check for `biome.json` → `npx biome check`, `eslint.config.*` → `npx eslint`, fallback → `npm run lint`) | |
| **Exit condition:** Every dangerous operation identified in Phase: Discovery has a corresponding guard in the SKILL.md. |
There was a problem hiding this comment.
Fixed in 923b51b. Moved the "For code modifications" section (test/lint detection) above the exit condition, so the gate now covers all Phase 3 content. An AI following the instructions will process all guard sections before reaching the exit check.
.claude/skills/create-skill/SKILL.md
Outdated
|
|
||
| ### For code modifications: | ||
| - Run tests after changes: detect test runner per Phase: Write the Skill Body, Pattern 6 | ||
| - Run lint after changes: detect lint runner the same way (check for `biome.json` → `npx biome check`, `eslint.config.*` → `npx eslint`, fallback → `npm run lint`) |
There was a problem hiding this comment.
Pattern 6 (line 172) requires: "write the explicit detection script — do not use <detected-value> placeholders." However, the lint runner detection here is only described in prose (check for biome.json → npx biome check ...) with no actual bash block. This violates the same pattern the skill teaches, and the detection criteria (config files) are also different from the test runner example (lock files), making "the same way" misleading.
Provide an explicit detection block so skill authors have a concrete template:
| - Run lint after changes: detect lint runner the same way (check for `biome.json` → `npx biome check`, `eslint.config.*` → `npx eslint`, fallback → `npm run lint`) | |
| - Run lint after changes: detect lint runner: | |
| ```bash | |
| if [ -f "biome.json" ]; then LINT_CMD="npx biome check" | |
| elif ls eslint.config.* 2>/dev/null | grep -q .; then LINT_CMD="npx eslint ." | |
| else LINT_CMD="npm run lint"; fi | |
| $LINT_CMD |
There was a problem hiding this comment.
Fixed in 923b51b. Replaced the prose-only lint detection with an explicit bash block that detects biome.json, eslint.config.*, or falls back to npm run lint. This now follows Pattern 6 properly (no placeholders, no "the same way" hand-waving).
- Pattern 6 example: keep TEST_CMD assignment and usage in same block to avoid violating Pattern 1 (no cross-fence variables) - Phase 3: move "For code modifications" section above exit condition so the gate covers all Phase 3 content - Phase 3: replace prose lint detection with explicit bash script to follow Pattern 6's own guidance (no placeholders)
|
Addressed Claude review feedback:
The three minor recommendations (troubleshooting section, pattern reference doc, Phase 5 testing instructions) are good ideas for a follow-up iteration but not blocking for the initial version. |
|
@greptileai What else is missing from this skill creation for it to be more robust and resilient and the skills it creates avoid future code reviews? |
| ```bash | ||
| if [ -f "biome.json" ]; then LINT_CMD="npx biome check" | ||
| elif ls eslint.config.* 2>/dev/null | grep -q .; then LINT_CMD="npx eslint ." | ||
| else LINT_CMD="npm run lint"; fi | ||
| $LINT_CMD | ||
| ``` |
There was a problem hiding this comment.
The lint detection template block uses 2>/dev/null on line 231 without any justification comment, which directly violates Pattern 2 ("Never use 2>/dev/null without documenting the skip path") and the explicit Rule at line 301 ("Every 2>/dev/null needs a justification comment in the generated skill").
When the AI generates a skill with code modifications and copies this Phase 3 template verbatim, the resulting skill will contain an unjustified 2>/dev/null. A skill author (or a generated skill's self-review in Phase 4) checking against Pattern 2 would correctly flag this as a violation — but the template they were given already contains the violation. This is a self-contradiction that undermines the quality gate it's supposed to enforce.
The 2>/dev/null here intentionally suppresses ls's error when no eslint.config.* glob matches (shell glob expansion failure). That rationale should be documented inline:
| ```bash | |
| if [ -f "biome.json" ]; then LINT_CMD="npx biome check" | |
| elif ls eslint.config.* 2>/dev/null | grep -q .; then LINT_CMD="npx eslint ." | |
| else LINT_CMD="npm run lint"; fi | |
| $LINT_CMD | |
| ``` | |
| - Run lint after changes: detect lint runner: | |
| ```bash | |
| if [ -f "biome.json" ]; then LINT_CMD="npx biome check" | |
| elif ls eslint.config.* 2>/dev/null | grep -q .; then LINT_CMD="npx eslint ." # 2>/dev/null: ls exits non-zero when glob matches nothing — intentionally tolerant | |
| else LINT_CMD="npm run lint"; fi | |
| $LINT_CMD |
There was a problem hiding this comment.
Fixed in f356b68. Added the justification comment to the 2>/dev/null in the lint detection template: # 2>/dev/null: ls exits non-zero when glob matches nothing — intentionally tolerant. The template now follows Pattern 2 properly.
| ````markdown | ||
| ```bash | ||
| mktemp -d > .codegraph/$SKILL_NAME/.tmpdir | ||
| ``` | ||
| Later: | ||
| ```bash | ||
| rm -rf "$(cat .codegraph/$SKILL_NAME/.tmpdir)" | ||
| ``` | ||
| ```` |
There was a problem hiding this comment.
$SKILL_NAME across separate fences
The Pattern 1 example is specifically teaching that shell variables do not survive across code fence boundaries — yet the "Correct" example itself uses $SKILL_NAME in two separate code blocks without redefining it:
```bash
mktemp -d > .codegraph/$SKILL_NAME/.tmpdir # $SKILL_NAME set... where?
```
Later:
```bash
rm -rf "$(cat .codegraph/$SKILL_NAME/.tmpdir)" # $SKILL_NAME undefined
```The intent is for the AI generating a new skill to substitute the literal skill name (e.g., deploy-check) in place of $SKILL_NAME, treating it as a meta-placeholder rather than a runtime shell variable. But that intent is nowhere explained, and an AI following the example could interpret $SKILL_NAME as a shell variable — producing generated skills where both blocks use an undefined variable, causing the tmpdir path to silently degrade to .codegraph//.tmpdir.
Consider adding a brief clarifying note, or replacing $SKILL_NAME with a concrete example name to make it unambiguous:
```bash
# Replace SKILL_NAME with your actual skill name (e.g. deploy-check)
mktemp -d > .codegraph/deploy-check/.tmpdir
```
Later:
```bash
rm -rf "$(cat .codegraph/deploy-check/.tmpdir)"
```There was a problem hiding this comment.
Fixed in f356b68. Replaced $SKILL_NAME with the concrete example deploy-check in Pattern 1's "Correct" example. Both fences now use .codegraph/deploy-check/.tmpdir — no ambiguity about whether it's a runtime variable or a substitution placeholder.
| ```bash | ||
| PREV_FILE=$(mktemp --suffix=.js) | ||
| if git show HEAD:$FILE > "$PREV_FILE" 2>&1; then | ||
| codegraph where --file "$PREV_FILE" | ||
| else | ||
| echo "WARN: $FILE is new (not in HEAD) — skipping before/after comparison" | ||
| fi | ||
| rm -f "$PREV_FILE" | ||
| ``` |
There was a problem hiding this comment.
.js extension
The Pattern 2 example creates a temp file with mktemp --suffix=.js, and this hardcoded .js extension will be copied by skill authors into new skills regardless of the language they're actually working with. Pattern 3 (directly below) correctly teaches that "temp files passed to codegraph must have the correct extension" — but the Pattern 2 example models the wrong behaviour for non-JS skills.
A skill authoring a Python or Go workflow would blindly copy --suffix=.js from this example, creating a codegraph language detection mismatch.
A small clarifying note would prevent this:
| ```bash | |
| PREV_FILE=$(mktemp --suffix=.js) | |
| if git show HEAD:$FILE > "$PREV_FILE" 2>&1; then | |
| codegraph where --file "$PREV_FILE" | |
| else | |
| echo "WARN: $FILE is new (not in HEAD) — skipping before/after comparison" | |
| fi | |
| rm -f "$PREV_FILE" | |
| ``` | |
| ```bash | |
| PREV_FILE=$(mktemp --suffix=.js) # adjust extension to match the language of $FILE | |
| if git show HEAD:$FILE > "$PREV_FILE" 2>&1; then |
There was a problem hiding this comment.
Fixed in f356b68. Added # adjust extension to match the language of $FILE comment to the mktemp --suffix=.js line. This makes it clear that .js is an example, not a universal default — consistent with Pattern 3's guidance on correct extensions.
|
Here are the critical gaps that would make generated skills more robust: Add execution testingPhase 4.5 — Smoke Test (between self-review and finalize):
The self-review is purely theoretical. Most dogfooding issues (wrong paths, shell syntax, missing tools) only surface when you actually try to run the code. Missing from checklistAdd to Phase 4 checks: Idempotency: Re-running the skill on the same state must be safe. Check: does it handle existing output files? What happens on second run? Dependency validation: Phase 0 must verify all Exit codes: Every error path uses explicit State cleanup: If the skill creates Examples section: Every skill needs 2-3 realistic usage examples showing common invocations Missing patternsPattern 11 — Progress indicators: For phases >10s, emit progress: Pattern 12 — Artifact reuse: Before running expensive operations, check if output exists: Pattern 13 — Platform portability: Test glob patterns on Linux and macOS (e.g., The smoke test phase would have the biggest impact — it turns "did I follow the checklist?" into "does this actually run?" |
…st items to /create-skill
- Add 2>/dev/null justification comment to lint detection template (Pattern 2 compliance) - Replace $SKILL_NAME with concrete example in Pattern 1 to avoid cross-fence ambiguity - Add extension-adjustment comment to Pattern 2 temp file example
|
Addressed round-2 Greptile feedback in f356b68:
|
| - [ ] **Artifact schema**: If the skill produces files, path/format/schema are documented | ||
| - [ ] **Exit conditions**: Each phase states what must be true before the next phase starts | ||
| - [ ] **Scope boundary**: The skill's purpose is clear — it does one thing, not five | ||
| - [ ] **Examples section**: At least 2-3 realistic usage examples showing common invocations are included |
There was a problem hiding this comment.
The Phase 4 Self-Review Checklist (line 308) explicitly requires: "Examples section: At least 2-3 realistic usage examples showing common invocations are included." The PR description states Phase 4 was run against this skill itself, yet the SKILL.md has no ## Examples section anywhere in its 390 lines.
This is a self-referential violation — the skill fails the very quality gate it's supposed to enforce. An Examples section might look like:
## Examples
- `/create-skill deploy-check` — scaffold a deployment validation skill
- `/create-skill review-pr` — scaffold a PR review skill with API calls
- `/create-skill db-migrate` — scaffold a database migration skill with dangerous-op guardsThere was a problem hiding this comment.
Fixed in fb08cbd. Added an ## Examples section with 3 realistic usage examples (deploy-check, review-pr, db-migrate) between Phase 6 and Rules. The skill now passes its own Phase 4 completeness checklist item for examples.
| ````markdown | ||
| ```bash | ||
| if [ -f ".codegraph/$SKILL_NAME/results.json" ]; then | ||
| echo "Using cached results from previous run" | ||
| else | ||
| # run expensive operation | ||
| fi | ||
| ``` | ||
| ```` |
There was a problem hiding this comment.
$SKILL_NAME meta-placeholder
The previous thread (line 113) identified that Pattern 1's "Correct" example used $SKILL_NAME ambiguously — an AI could read it as a runtime shell variable rather than a substitution placeholder — and it was fixed in commit f356b68 by replacing it with the concrete example deploy-check. The same problem still exists in Pattern 12's example:
if [ -f ".codegraph/$SKILL_NAME/results.json" ]; thenAn AI following this example when writing a new skill would face the same ambiguity: should it substitute the actual skill name (e.g., deploy-check), or define $SKILL_NAME as a runtime variable? The fix applied to Pattern 1 (using a concrete example name) was not applied here.
| ````markdown | |
| ```bash | |
| if [ -f ".codegraph/$SKILL_NAME/results.json" ]; then | |
| echo "Using cached results from previous run" | |
| else | |
| # run expensive operation | |
| fi | |
| ``` | |
| ```` | |
| Before running expensive operations (codegraph build, embedding generation, batch analysis), check if usable output already exists (replace `deploy-check` with your actual skill name): | |
| ````markdown | |
| ```bash | |
| if [ -f ".codegraph/deploy-check/results.json" ]; then | |
| echo "Using cached results from previous run" | |
| else | |
| # run expensive operation | |
| fi |
.claude/skills/create-skill/SKILL.md
Outdated
|
|
||
| ## Phase 2 — Write the Skill Body | ||
|
|
||
| Write each phase following these **mandatory patterns** (derived from the top 10 Greptile review findings): |
There was a problem hiding this comment.
The Phase 2 introduction says the patterns are "derived from the top 10 Greptile review findings" — but 13 patterns are defined (Patterns 1–13). The exit condition at the bottom of Phase 2 correctly states "follows all 13 patterns," but the leading description undermines this by implying only 10 are core findings.
An AI reading this skill might reasonably treat Patterns 11–13 as supplementary or optional based on the "top 10" framing, rather than as equally mandatory. The heading and exit condition are internally contradictory.
| Write each phase following these **mandatory patterns** (derived from the top 10 Greptile review findings): | |
| Write each phase following these **mandatory patterns** (derived from Greptile review findings across 200+ comments): |
There was a problem hiding this comment.
Fixed in fb08cbd. Changed "top 10" to "Greptile review findings across 200+ comments" in Phase 2 intro (line 95) and updated the checklist header from "the top 10" to "all 13 patterns" (line 286). Both now accurately reflect the 13 defined patterns.
.claude/skills/create-skill/SKILL.md
Outdated
| ### Anti-pattern checks (the top 10): | ||
| - [ ] **Shell variables**: No variable is set in one code fence and used in another. State that must persist is written to a file | ||
| - [ ] **Silent failures**: No `2>/dev/null` without a documented skip rationale. No commands that swallow errors | ||
| - [ ] **Temp file extensions**: Every temp file passed to codegraph has the correct language extension | ||
| - [ ] **Temp file uniqueness**: Every temp path uses `mktemp`, never hardcoded paths | ||
| - [ ] **Cross-references**: All step references use phase names, not bare numbers | ||
| - [ ] **Placeholders**: Every `<placeholder>` has a preceding detection/assignment script | ||
| - [ ] **Contradictions**: No two sections describe contradictory behavior for the same condition | ||
| - [ ] **Rules sync**: Every command/tool in the procedure is covered by Rules. Every Rules exception maps to a real step | ||
| - [ ] **Redundancy**: No codegraph command is run twice with the same arguments. Later phases reference earlier results | ||
| - [ ] **Skip validation**: If `--start-from`/`--skip-*` is supported, every skip path validates required artifacts |
There was a problem hiding this comment.
Every other mandatory pattern (1–11, 13) has a corresponding checklist item in Phase 4. Pattern 12 — "Before running expensive operations, check if usable output already exists" — has no corresponding verification step. The anti-pattern section header still says "the top 10" which explains why it was omitted, but since Pattern 12 is a mandatory pattern, an author running Phase 4 will never be prompted to verify they implemented it.
Consider adding to the "Robustness checks" or "Safety checks" section:
- [ ] **Artifact reuse**: Expensive operations (codegraph build, embedding generation, batch analysis) check for existing output before re-runningThere was a problem hiding this comment.
Fixed in fb08cbd. Added Pattern 12 (artifact reuse) to the anti-pattern checklist, along with Patterns 11 and 13 which were previously only in the Safety checks section. Moved all three to the "Anti-pattern checks (all 13 patterns)" section and removed the duplicates from Safety checks, so each pattern has exactly one checklist item.
Adds a top-level callout instructing Claude to always verify factual claims (licenses, versions, features) before stating them.
- Replace "top 10" with accurate "all 13 patterns" in Phase 2 intro and checklist header since 13 patterns are defined - Replace ambiguous $SKILL_NAME with concrete deploy-check in Pattern 12 example (same fix as Pattern 1 in previous round) - Add Patterns 11-13 (progress, artifact reuse, portability) to Phase 4 anti-pattern checklist and remove duplicates from Safety checks - Add missing Examples section with 3 realistic usage examples (required by own Phase 4 completeness checklist)
| This supports both idempotent re-runs and resume-after-failure. | ||
|
|
||
| ### Pattern 13: Platform portability |
There was a problem hiding this comment.
The lint detection template uses ls eslint.config.*, which is the exact glob-expansion pattern that Pattern 13 prohibits:
"Use
find ... -name "*.ext"instead of glob expansion (ls *.ext) which differs between bash versions"
Any skill generated with code-modification guards will copy this template verbatim and inherit a portability violation — the very kind of issue this skill is designed to prevent. A skill author running Phase 4 self-review would correctly flag this under the "Platform portability" checklist item, but then have no compliant template to use.
| This supports both idempotent re-runs and resume-after-failure. | |
| ### Pattern 13: Platform portability | |
| if [ -f "biome.json" ]; then LINT_CMD="npx biome check" | |
| elif find . -maxdepth 1 -name "eslint.config.*" -print -quit 2>/dev/null | grep -q .; then LINT_CMD="npx eslint ." # 2>/dev/null: find exits non-zero when path is unreadable — intentionally tolerant | |
| else LINT_CMD="npm run lint"; fi | |
| $LINT_CMD |
There was a problem hiding this comment.
Fixed in 98abfb2. Replaced ls eslint.config.* with find . -maxdepth 1 -name "eslint.config.*" -print -quit in the Phase 3 lint detection template. The template now follows Pattern 13 (platform portability) — no glob expansion.
| - Environment validation produces clear error messages on failure | ||
|
|
||
| ```bash | ||
| TEST_DIR=$(mktemp -d) | ||
| cd "$TEST_DIR" | ||
| git init | ||
| # Simulate the Phase 0 checks from the skill here | ||
| cd - | ||
| rm -rf "$TEST_DIR" | ||
| ``` | ||
|
|
There was a problem hiding this comment.
The smoke test block uses cd "$TEST_DIR" and defers cleanup to cd - and rm -rf "$TEST_DIR" at the end. If any of the "Simulate Phase 0 checks" commands fail partway through (e.g. a tool is missing and exit 1 fires), the shell exits the block mid-way, leaving:
- The process's working directory set to
$TEST_DIR(for the AI's shell session) $TEST_DIRundeleted on disk
The fix is to add a trap guard so cleanup always runs, consistent with Pattern 2's guidance that every failure path must be handled explicitly:
| - Environment validation produces clear error messages on failure | |
| ```bash | |
| TEST_DIR=$(mktemp -d) | |
| cd "$TEST_DIR" | |
| git init | |
| # Simulate the Phase 0 checks from the skill here | |
| cd - | |
| rm -rf "$TEST_DIR" | |
| ``` | |
| TEST_DIR=$(mktemp -d) | |
| trap 'cd - > /dev/null 2>&1; rm -rf "$TEST_DIR"' EXIT | |
| cd "$TEST_DIR" | |
| git init | |
| # Simulate the Phase 0 checks from the skill here | |
| cd - | |
| rm -rf "$TEST_DIR" | |
| trap - EXIT |
There was a problem hiding this comment.
Fixed in 98abfb2. Added a \ guard to the Phase 5 smoke test block. If any command fails mid-block, the trap ensures cleanup runs (restore working directory + delete temp dir). The trap is cleared after normal cleanup with .
- Phase 3 lint template: replace `ls eslint.config.*` with `find . -maxdepth 1 -name "eslint.config.*"` to comply with Pattern 13 (platform portability) - Phase 5 smoke test: add `trap` guard for cleanup so early exits don't leave the shell in a foreign directory or temp files on disk
.claude/skills/create-skill/SKILL.md
Outdated
| ## Phase 0 — Discovery | ||
|
|
||
| Before writing anything, gather requirements interactively. Ask the user these questions (all at once, not one-by-one): | ||
|
|
||
| 1. **Purpose** — What does this skill do? (one paragraph) | ||
| 2. **Arguments** — What CLI arguments should it accept? (e.g. `--fix`, `--dry-run`, `<path>`) | ||
| 3. **Phases** — What are the major steps? (bullet list of 3-8 phases) | ||
| 4. **Tools needed** — Which tools does it need? (Bash, Read, Write, Edit, Glob, Grep, Agent) | ||
| 5. **Artifacts** — Does it produce output files? If so, where and what format? | ||
| 6. **Dangerous operations** — Does it modify code, push to git, call external APIs, or delete files? | ||
| 7. **Resume/skip support** — Should it support `--start-from` or `--skip-*` flags for long-running pipelines? | ||
|
|
||
| **Wait for the user's answers before proceeding.** Do not guess or assume. | ||
|
|
||
| **Exit condition:** All 7 questions have answers. Purpose, arguments, phases, tools, artifacts, dangerous ops, and resume support are defined. |
There was a problem hiding this comment.
The structural requirements in Phase 1 (line 83) state: "Phase 0 always exists — pre-flight checks, argument parsing, environment validation." The Phase 4 self-review checklist (line 282) also checks: "Phase 0 exists and validates the environment."
However, this skill's own Phase 0 is purely discovery/requirements gathering — it has no pre-flight checks and no environment validation. Specifically:
- There's no check that the current directory is a git repo (or any other environment precondition)
- No verification that required tools (
Bash,Write, etc.) are accessible - Argument parsing (
$ARGUMENTS→SKILL_NAME+ regex validation) happens in the## Argumentssection before Phase 0, not in Phase 0 itself
A skill author who faithfully runs Phase 4 self-review against this SKILL.md would check "Phase 0 exists and validates the environment" and find a gap.
Consider renaming Phase 0 to "Phase 0 — Discovery & Pre-flight" and prepending a brief environment check, e.g.:
## Phase 0 — Discovery & Pre-flight
**Pre-flight:** Confirm you are in a git repository root (`git rev-parse --show-toplevel` should succeed). Validate `$ARGUMENTS` is set and matches kebab-case (`^[a-z][a-z0-9]*(-[a-z0-9]+)*$`). If not, abort with a clear error.
**Discovery:** …This brings the skill into alignment with the structural requirement it teaches and the checklist item it enforces.
There was a problem hiding this comment.
Fixed in f741925. Renamed Phase 0 to "Discovery & Pre-flight" and added pre-flight checks at the top: git repo root validation (git rev-parse --show-toplevel) and $ARGUMENTS kebab-case validation. The exit condition now includes "Pre-flight passed (git repo confirmed, skill name validated)." This brings the skill into alignment with its own structural requirement (line 83) and Phase 4 checklist item (line 282).
.claude/skills/create-skill/SKILL.md
Outdated
|
|
||
| ### Pattern 9: No command redundancy | ||
|
|
||
| If a phase runs a codegraph command and stores the result, later phases must reference that result — not re-run the command. Add a note: "Using <result> from Phase: <Name>". |
There was a problem hiding this comment.
<result> / <Name> placeholders — ambiguous guidance
Pattern 6 (line 170) explicitly prohibits undefined placeholders: "Every variable or placeholder in pseudocode must have a preceding assignment." Yet Pattern 9's instruction reads:
Add a note: "Using <result> from Phase: <Name>".
The <result> and <Name> tokens are placeholders with no preceding definition. A skill author following this instruction might write the note literally with angle brackets rather than substituting the actual artifact variable name and phase name. The fix from the previous thread (replacing $SKILL_NAME with deploy-check in Pattern 12's example) should be applied here too — use a concrete example:
| If a phase runs a codegraph command and stores the result, later phases must reference that result — not re-run the command. Add a note: "Using <result> from Phase: <Name>". | |
| If a phase runs a codegraph command and stores the result, later phases must reference that result — not re-run the command. Add a note like: "Using `impact_report` from Phase: Impact Analysis". |
There was a problem hiding this comment.
.claude/skills/create-skill/SKILL.md
Outdated
| - Run lint after changes: detect lint runner: | ||
| ```bash | ||
| if [ -f "biome.json" ]; then LINT_CMD="npx biome check" | ||
| elif find . -maxdepth 1 -name "eslint.config.*" -print -quit 2>/dev/null | grep -q .; then LINT_CMD="npx eslint ." # 2>/dev/null: find exits non-zero when path is unreadable — intentionally tolerant |
There was a problem hiding this comment.
2>/dev/null justification is inaccurate after ls→find migration
The previous version used ls eslint.config.* which does produce stderr when no glob matches, legitimately justifying 2>/dev/null. Since that was replaced with find . -maxdepth 1 -name "eslint.config.*" -print -quit, find no longer produces any stderr output when no files match the pattern — it simply outputs nothing with exit code 0. The current comment ("find exits non-zero when path is unreadable") describes an edge case (unreadable child entries under -maxdepth 1) that is both uncommon and not the original intent.
This is a self-referential Pattern 2 issue: the skill teaches "never use 2>/dev/null without documenting the skip path," but the documentation it provides is now misleading about when the suppression actually fires.
Either remove the 2>/dev/null (since find doesn't need it for the no-match case) or update the comment to accurately reflect the actual scenario it guards:
| elif find . -maxdepth 1 -name "eslint.config.*" -print -quit 2>/dev/null | grep -q .; then LINT_CMD="npx eslint ." # 2>/dev/null: find exits non-zero when path is unreadable — intentionally tolerant | |
| elif find . -maxdepth 1 -name "eslint.config.*" -print -quit | grep -q .; then LINT_CMD="npx eslint ." |
If you prefer to keep it as a defensive guard against unexpected find errors (e.g. permission issues on .'s children), the comment should say so explicitly:
# 2>/dev/null: suppresses find permission errors on unreadable directory entries — defensive only, unlikely in practiceThere was a problem hiding this comment.
Fixed in 88955d0. Removed the \ entirely since \ doesn't produce stderr on no-match (it outputs nothing with exit 0). The inaccurate justification comment is gone too — no suppression needed means no comment needed.
.claude/skills/create-skill/SKILL.md
Outdated
| - `$ARGUMENTS` must contain the skill name in kebab-case (e.g. `deploy-check`) | ||
| - If `$ARGUMENTS` is empty, ask the user for a skill name before proceeding | ||
|
|
||
| Set `SKILL_NAME` to the provided name. Validate it is kebab-case (`^[a-z][a-z0-9]*(-[a-z0-9]+)*$`). Reject otherwise. | ||
|
|
||
| --- | ||
|
|
||
| ## Phase 0 — Discovery & Pre-flight | ||
|
|
||
| **Pre-flight:** Confirm you are in a git repository root (`git rev-parse --show-toplevel` should succeed). Validate `$ARGUMENTS` is set and matches kebab-case (`^[a-z][a-z0-9]*(-[a-z0-9]+)*$`). If not, abort with a clear error. |
There was a problem hiding this comment.
The same validation decision (validate $ARGUMENTS against ^[a-z][a-z0-9]*(-[a-z0-9]+)*$) is specified in two separate places:
- Lines 14–17 (Arguments section): "Set
SKILL_NAMEto the provided name. Validate it is kebab-case. Reject otherwise." - Line 23 (Phase 0): "Validate
$ARGUMENTSis set and matches kebab-case… If not, abort with a clear error."
Pattern 7 (line 188) says: "Each decision… must be defined in exactly one place. If two sections describe the same decision path, consolidate them and reference the single source."
The Arguments section defining the validation once and Phase 0 simply invoking it (e.g. "Parse $ARGUMENTS per the Arguments section above") would satisfy Pattern 7 and remove the ambiguity about which location has the authoritative rejection logic.
There was a problem hiding this comment.
Fixed in 88955d0. Phase 0 now says "Parse $ARGUMENTS per the Arguments section above" instead of re-specifying the kebab-case regex. The Arguments section (line 17) is the single source of truth for validation, satisfying Pattern 7.
.claude/skills/create-skill/SKILL.md
Outdated
| cd "$TEST_DIR" | ||
| git init | ||
| # Simulate the Phase 0 checks from the skill here | ||
| cd - |
There was a problem hiding this comment.
cd - prints the previous directory to stdout
cd - outputs the target directory path to stdout by default on most shells. In the smoke test context this pollutes the output and is inconsistent with the trap line above (which correctly suppresses it with > /dev/null 2>&1).
| cd - | |
| cd - > /dev/null |
There was a problem hiding this comment.
Fixed in 88955d0. Changed cd - to cd - > /dev/null in the smoke test cleanup (line 355), consistent with the trap line above which already suppresses stdout.
- Remove unnecessary 2>/dev/null from find command in lint template - Deduplicate kebab-case validation (Phase 0 now references Arguments section) - Suppress cd - stdout in smoke test template
| ```bash | ||
| mktemp -d > .codegraph/deploy-check/.tmpdir | ||
| ``` | ||
| Later: | ||
| ```bash | ||
| rm -rf "$(cat .codegraph/deploy-check/.tmpdir)" | ||
| ``` |
There was a problem hiding this comment.
.codegraph/deploy-check/ doesn't exist
The redirect mktemp -d > .codegraph/deploy-check/.tmpdir requires the parent directory .codegraph/deploy-check/ to already exist. If it doesn't, bash sets up the redirect first, fails to open the file, and never invokes mktemp -d — so the temp directory is never created and the .tmpdir file is never written. The later block's $(cat .codegraph/deploy-check/.tmpdir) then expands to an empty string, and rm -rf "" is executed.
This is the exact kind of silent failure Pattern 2 is designed to prevent, yet the Pattern 1 "Correct" example contains it. Skill authors who copy this pattern without pre-creating the directory will get a non-obvious failure.
The example should either show directory creation as a prerequisite step, or add a note that the directory must already exist:
**Correct:** Persist state to a file (use your actual skill name, not a variable).
First ensure the directory exists:
```bash
mkdir -p .codegraph/deploy-check
mktemp -d > .codegraph/deploy-check/.tmpdir
```
Later:
```bash
rm -rf "$(cat .codegraph/deploy-check/.tmpdir)"
```There was a problem hiding this comment.
Fixed in 659e60d. Added mkdir -p .codegraph/deploy-check before the mktemp -d redirect in Pattern 1's Correct example. The parent directory is now explicitly created, preventing the silent failure where bash can't open the redirect target.
| ```bash | ||
| TEST_DIR=$(mktemp -d) | ||
| trap 'cd - > /dev/null 2>&1; rm -rf "$TEST_DIR"' EXIT | ||
| cd "$TEST_DIR" | ||
| git init | ||
| # Simulate the Phase 0 checks from the skill here | ||
| cd - > /dev/null | ||
| rm -rf "$TEST_DIR" | ||
| trap - EXIT | ||
| ``` |
There was a problem hiding this comment.
trap and explicit cleanup
The trap on line 351 uses cd - > /dev/null 2>&1 (suppresses both stdout and stderr), but the explicit cleanup on line 355 uses only cd - > /dev/null (suppresses stdout only). If $OLDPWD is unset or the previous directory was removed, cd - will print an error to stderr on the explicit cleanup path. The previous review thread fixed the missing > /dev/null but didn't bring the stderr suppression into parity with the trap line.
| ```bash | |
| TEST_DIR=$(mktemp -d) | |
| trap 'cd - > /dev/null 2>&1; rm -rf "$TEST_DIR"' EXIT | |
| cd "$TEST_DIR" | |
| git init | |
| # Simulate the Phase 0 checks from the skill here | |
| cd - > /dev/null | |
| rm -rf "$TEST_DIR" | |
| trap - EXIT | |
| ``` | |
| cd - > /dev/null 2>&1 |
There was a problem hiding this comment.
Fixed in 659e60d. Changed explicit cleanup path from cd - > /dev/null to cd - > /dev/null 2>&1, matching the trap line's stderr suppression. Both paths are now consistent.
| ## Phase 1 — Scaffold | ||
|
|
||
| Create the skill directory and SKILL.md with frontmatter: | ||
|
|
||
| ```bash | ||
| mkdir -p .claude/skills/$SKILL_NAME | ||
| ``` | ||
|
|
||
| Write the SKILL.md file starting with this structure: | ||
|
|
||
| ```markdown | ||
| --- | ||
| name: $SKILL_NAME | ||
| description: <one-line from user's purpose> | ||
| argument-hint: "<from user's argument design>" | ||
| allowed-tools: <from user's tool list> | ||
| --- | ||
|
|
||
| # /$SKILL_NAME — <Title> | ||
|
|
||
| <Purpose paragraph from Phase 0> | ||
|
|
||
| ## Arguments | ||
|
|
||
| - `$ARGUMENTS` parsing rules here | ||
| - Set state variables: `DRY_RUN`, `AUTO_FIX`, etc. | ||
|
|
||
| ## Phase 0 — Pre-flight | ||
|
|
||
| 1. Confirm environment (repo root, node version, required tools) | ||
| 2. Parse `$ARGUMENTS` into state variables | ||
| 3. Validate preconditions | ||
|
|
||
| ## Phase N — <Name> | ||
|
|
||
| <Steps> | ||
|
|
||
| ## Rules | ||
|
|
||
| - <Hard constraints> | ||
| ``` | ||
|
|
||
| ### Structural requirements to include in every skill: | ||
|
|
||
| 1. **Phase 0 always exists** — pre-flight checks, argument parsing, environment validation | ||
| 2. **Every phase has a clear exit condition** — what must be true before moving to the next phase | ||
| 3. **Arguments section** — explicit parsing of `$ARGUMENTS` into named state variables | ||
| 4. **Rules section** — hard constraints at the bottom, kept in sync with the procedure | ||
| 5. **Artifact definitions** — if the skill produces files, specify path, format, and schema | ||
|
|
||
| **Exit condition:** `.claude/skills/$SKILL_NAME/SKILL.md` exists with valid frontmatter, Phase 0, Arguments section, and Rules section. | ||
|
|
There was a problem hiding this comment.
The Phase 4 Safety checks mandate: "Idempotency: Re-running the skill on the same state is safe. Existing output files are handled (skip, overwrite with warning, or merge)."
However, Phase 1 never checks whether .claude/skills/$SKILL_NAME/SKILL.md already exists before writing it. A second invocation of /create-skill deploy-check silently overwrites the user's existing, possibly partially-written skill without any warning. This is a self-referential violation of the idempotency requirement the skill itself defines.
A guard should be added at the start of Phase 1:
if [ -f ".claude/skills/$SKILL_NAME/SKILL.md" ]; then
echo "WARN: .claude/skills/$SKILL_NAME/SKILL.md already exists."
echo "Proceeding will overwrite it. Confirm or abort."
# Prompt user for confirmation before continuing
fiThere was a problem hiding this comment.
Fixed in 659e60d. Added an idempotency guard at the top of Phase 1 that checks for an existing SKILL.md and warns before overwriting. This brings the skill into compliance with its own Phase 4 Idempotency checklist item.
| if echo "$stripped" | grep -qE '"/tmp/[a-zA-Z]|/tmp/[a-zA-Z][a-zA-Z]'; then | ||
| # Allow ${TMPDIR:-/tmp} pattern | ||
| if ! echo "$stripped" | grep -qE '\$\{TMPDIR:-/tmp\}'; then |
There was a problem hiding this comment.
/tmp/ paths not caught
The two alternatives in the regex are:
"/tmp/[a-zA-Z]— requires a leading"character before/tmp//tmp/[a-zA-Z][a-zA-Z]— requires at least two letters in the path name (no leading quote required)
This creates a blind spot: a bare (unquoted) hardcoded path with a single-letter tail — e.g. /tmp/f.json — matches neither alternative.
The second alternative should use [a-zA-Z] (one letter) to be consistent with the quoting-aware first alternative:
| if echo "$stripped" | grep -qE '"/tmp/[a-zA-Z]|/tmp/[a-zA-Z][a-zA-Z]'; then | |
| # Allow ${TMPDIR:-/tmp} pattern | |
| if ! echo "$stripped" | grep -qE '\$\{TMPDIR:-/tmp\}'; then | |
| if echo "$stripped" | grep -qE '"/tmp/[a-zA-Z]|/tmp/[a-zA-Z]'; then |
This also subsumes the existing two-letter form, so no coverage is lost.
There was a problem hiding this comment.
Fixed in de8e6c5. Changed /tmp/[a-zA-Z][a-zA-Z] to /tmp/[a-zA-Z] so single-letter unquoted paths like /tmp/f.json are now caught by Check 11.
There was a problem hiding this comment.
Already fixed in de8e6c5 — Check 11 regex uses single [a-zA-Z] in both alternatives, catching unquoted single-letter paths like /tmp/f.json.
| '```bash'*) in_block=true; prev_line="$line"; continue ;; | ||
| '```'*) in_block=false; prev_line="$line"; continue ;; | ||
| esac | ||
| if $in_block && echo "$line" | grep -qE '2>/dev/null|>[ ]?/dev/null 2>&1|&>/dev/null'; then |
There was a problem hiding this comment.
> followed by 2+ spaces before /dev/null
The pattern >[ ]?/dev/null 2>&1 uses [ ]? which matches zero or one space. The form > /dev/null 2>&1 (two spaces, which some auto-formatters or editors emit) would not match either the 2>/dev/null alternative nor the >[ ]? alternative, and would silently bypass the justification requirement.
Extending to [ ]* (zero-or-more) closes the gap at no cost:
| if $in_block && echo "$line" | grep -qE '2>/dev/null|>[ ]?/dev/null 2>&1|&>/dev/null'; then | |
| if $in_block && echo "$line" | grep -qE '2>/dev/null|>[ ]*/dev/null 2>&1|&>/dev/null'; then |
There was a problem hiding this comment.
Fixed in de8e6c5. Changed >[ ]?/dev/null to >[ ]*/dev/null so multi-space variants (e.g. from auto-formatters) are now caught by Check 2.
There was a problem hiding this comment.
Fixed in de8e6c5. Changed >[ ]?/dev/null to >[ ]*/dev/null so multi-space variants (e.g. from auto-formatters) are now caught by Check 2.
There was a problem hiding this comment.
Already fixed in de8e6c5 — Check 2 regex now uses >[ ]*/dev/null (zero-or-more) instead of >[ ]?, closing the multi-space gap.
| ### Phase 0 dry-run | ||
|
|
||
| Run the skill's Phase 0 (pre-flight) logic in a temporary test directory to verify: | ||
| - Argument parsing works for valid inputs and rejects invalid ones | ||
| - Tool availability checks actually detect missing tools (temporarily rename one to confirm) | ||
| - Environment validation produces clear error messages on failure | ||
|
|
||
| ```bash | ||
| TEST_DIR=$(mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX") | ||
| # > /dev/null 2>&1: suppress cd's directory-path output — cleanup should be silent | ||
| trap 'cd - > /dev/null 2>&1; rm -rf "$TEST_DIR"' EXIT | ||
| cd "$TEST_DIR" | ||
| git init --quiet | ||
| # Simulate the Phase 0 checks from the skill here | ||
| # > /dev/null 2>&1: suppress cd's directory-path output — returning to original directory | ||
| cd - > /dev/null 2>&1 | ||
| rm -rf "$TEST_DIR" | ||
| trap - EXIT | ||
| ``` |
There was a problem hiding this comment.
The Phase 5 description (line 508) explicitly requires testing that the skill rejects invalid inputs and produces clear error messages when required tools are absent. But the template bash block only sets up a valid environment (git init --quiet) and leaves a generic # Simulate the Phase 0 checks from the skill here comment. An AI following this template literally will only exercise the success path.
Consider adding stub comments for the failure-mode cases so the template is self-documenting:
# --- Happy path: valid skill name and all tools present ---
# Simulate: ARGUMENTS="deploy-check"; run the Phase 0 pre-flight checks
# --- Failure path 1: not in a git repo ---
# (run checks from a non-git directory — expect ERROR message + non-zero exit)
# --- Failure path 2: invalid skill name ---
# ARGUMENTS="BadName!"; run argument validation — expect rejectionThere was a problem hiding this comment.
Fixed in fc54517. Added failure-path stub comments to the Phase 5 smoke-test template: non-git directory, invalid skill name, and missing required tool. The template now guides skill authors to exercise both success and error paths.
There was a problem hiding this comment.
Already fixed in fc54517 — Phase 5 smoke-test template now includes failure-path stubs for non-git directory, invalid skill name, and missing required tool.
Check 2: change `>[ ]?/dev/null` to `>[ ]*/dev/null` so multi-space variants emitted by auto-formatters are caught. Check 11: change `/tmp/[a-zA-Z][a-zA-Z]` to `/tmp/[a-zA-Z]` so single-letter unquoted paths like `/tmp/f.json` are caught.
The template previously only scaffolded the happy path. Now includes stub comments for three failure scenarios (non-git directory, invalid skill name, missing required tool) so generated skills exercise both success and error paths.
| in_block=false | ||
| # Run syntax check on the collected block | ||
| if [ -s "$TMPBLOCK" ]; then | ||
| if output=$(bash -n "$TMPBLOCK" 2>&1); then |
There was a problem hiding this comment.
bash -n uses PATH bash, not the version-guarded bash
The script guards against bash < 4 at startup (lines 8–11) using ${BASH_VERSINFO[0]}, which reflects the bash that's actually executing this script. But line 68 invokes bash -n "$TMPBLOCK" — this resolves bash from PATH, which on macOS is /bin/bash (3.2) even after brew install bash.
On macOS, a user who runs smoke-test-skill.sh via /usr/local/bin/bash smoke-test-skill.sh (bash 4+) will pass the version guard, but bash -n will call /bin/bash (3.2). Any generated skill using bash 4+ features (declare -A, mapfile, readarray) would be falsely failed with a cryptic "bash: line X: declare: -A: invalid option" error rather than a clean "syntax OK".
This is exactly the portability concern Pattern 13 and the bash 4+ guard in both scripts are designed to prevent. The fix is to use "$BASH" (the special variable that holds the path to the currently-running bash):
| if output=$(bash -n "$TMPBLOCK" 2>&1); then | |
| if output=$("$BASH" -n "$TMPBLOCK" 2>&1); then |
$BASH is set automatically by bash to the full path of the current interpreter, so it will always be the 4+ binary that passed the version guard.
There was a problem hiding this comment.
Fixed in ba179cd. Replaced bash -n with "$BASH" -n so the syntax check uses the same bash interpreter that passed the version guard, not the PATH-resolved one. On macOS where /bin/bash is 3.2, this prevents false failures for bash 4+ syntax like declare -A or mapfile.
There was a problem hiding this comment.
Already fixed in ba179cd — smoke-test-skill.sh now uses \"\/usr/bin/bash\" -n instead of bash -n, ensuring the version-guarded interpreter is used.
|
|
||
| # ── Check 3: git add . or git add -A (inside bash blocks only) ─────── | ||
| while IFS=$'\t' read -r bnum line; do | ||
| if echo "$line" | grep -qE '^\s*git add (--\s+\.([ \t;#]|$)|\.([ \t;#]|$)|(-A|--all))'; then |
There was a problem hiding this comment.
--all alternative lacks trailing word boundary
The other two git add alternatives correctly anchor with ([ \t;#]|$) to prevent partial matches, but (-A|--all) has no trailing terminator. Any future git option prefixed with --all would match and emit a false-positive error.
| if echo "$line" | grep -qE '^\s*git add (--\s+\.([ \t;#]|$)|\.([ \t;#]|$)|(-A|--all))'; then | |
| if echo "$line" | grep -qE '^\s*git add (--\s+\.([ \t;#]|$)|\.([ \t;#]|$)|(-A|--all)([ \t;#]|$))'; then |
There was a problem hiding this comment.
Fixed in e63d742. Added ([ \t;#]|$) trailing anchor to the (-A|--all) alternative in Check 3, matching the pattern used by the other two alternatives. Hypothetical future options like --all-staged will no longer false-positive.
There was a problem hiding this comment.
Already fixed in e63d742 — Check 3 --all alternative now has trailing ([ \t;#]|$) boundary, consistent with the other alternatives.
| elif $in_detect && echo "$line" | grep -qE '\bfi\b'; then | ||
| # fi appears inline (e.g. "else ...; fi") — still closes the outermost detection block | ||
| if [ "$detect_depth" -gt 0 ]; then | ||
| detect_depth=$((detect_depth - 1)) | ||
| [ "$detect_depth" -eq 0 ] && in_detect=false | ||
| else | ||
| in_detect=false | ||
| fi | ||
| fi | ||
| if ! $in_detect; then | ||
| if echo "$line" | grep -qE '^\s*((npm|yarn|pnpm) test|(npm|yarn|pnpm) run (test|lint))([^:A-Za-z0-9_]|$)'; then | ||
| warn "Line $line_num: Hardcoded '$(echo "$line" | sed 's/^[[:space:]]*//')' — detect package manager first (Pattern 6)" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
fi resets in_detect before the hardcoded-command check runs on the same line
Branch 5 handles compact one-liners like else npm test; fi. When processed: branch 5 fires → in_detect=false, then the hardcoded-command check (line 161) runs on the same line and flags npm test with "detect package manager first" — a misleading message since detection already happened; the real issue is invoking the command directly rather than via TEST_CMD/LINT_CMD. Worth documenting in the comment even if the full fix is deferred.
There was a problem hiding this comment.
Fixed in e63d742. Introduced a \ variable that captures the detection context before fi-processing runs. The hardcoded-command check now evaluates against \ instead of , so commands on the same line as an inline \ (e.g. ) are correctly recognized as inside the detection block and not falsely flagged.
There was a problem hiding this comment.
Already fixed in e63d742 — introduced was_in_detect variable that captures detection context before fi-processing, so inline commands like else npm test; fi are evaluated correctly. Explanatory comments added at lines 136 and 166-168.
On macOS, `bash -n` resolves to /bin/bash (3.2) even when the script is running under bash 4+. Using "$BASH" ensures the same interpreter that passed the version guard is used for syntax checking.
…fi detection context (#587) - Check 3: added ([ \t;#]|$) after (-A|--all) to prevent false positives on hypothetical --all-prefixed options. - Check 4: use was_in_detect to evaluate hardcoded-command check in the detection context that was active at the start of the line, so compact forms like "else npm test; fi" are not falsely flagged after the inline fi resets in_detect.
| ## Phase 4 — Self-Review Checklist | ||
|
|
||
| Before finalizing, audit the SKILL.md against every item below. **Do not skip any item.** Fix violations before proceeding. | ||
|
|
||
| ### Structure checks: | ||
| - [ ] Frontmatter has all four fields: `name`, `description`, `argument-hint`, `allowed-tools` | ||
| - [ ] `name` matches the directory name | ||
| - [ ] Phase 0 exists and validates the environment | ||
| - [ ] Arguments section explicitly parses `$ARGUMENTS` into named variables | ||
| - [ ] Rules section exists at the bottom | ||
| - [ ] Every phase has a clear name (not just a number) | ||
|
|
||
| ### Anti-pattern checks (all 17 patterns): | ||
| - [ ] **Shell variables**: No variable is set in one code fence and used in another. State that must persist is written to a file | ||
| - [ ] **Silent failures**: No `2>/dev/null` without a documented skip rationale. No commands that swallow errors | ||
| - [ ] **Temp file extensions**: Every temp file passed to codegraph has the correct language extension | ||
| - [ ] **Temp file uniqueness**: Every temp path uses `mktemp`, never hardcoded paths | ||
| - [ ] **Cross-references**: All step references use phase names, not bare numbers | ||
| - [ ] **Placeholders**: Every `<placeholder>` has a preceding detection/assignment script | ||
| - [ ] **Contradictions**: No two sections describe contradictory behavior for the same condition | ||
| - [ ] **Rules sync**: Every command/tool in the procedure is covered by Rules. Every Rules exception maps to a real step | ||
| - [ ] **Redundancy**: No codegraph command is run twice with the same arguments. Later phases reference earlier results | ||
| - [ ] **Skip validation**: If `--start-from`/`--skip-*` is supported, every skip path validates required artifacts | ||
| - [ ] **Progress indicators**: Phases that iterate over files or run batch operations emit progress (`Processing $i/$total`) | ||
| - [ ] **Artifact reuse**: Expensive operations (codegraph build, embedding generation, batch analysis) check for existing output before re-running | ||
| - [ ] **Platform portability**: No `sed -i ''`, no unquoted globs, no GNU-only flags without fallback or documentation | ||
| - [ ] **Cleanup traps**: Phases that create temp files or modify repo state use `trap ... EXIT` for cleanup on error paths | ||
| - [ ] **Git stash safety**: Every `git stash push` has a named STASH_REF lookup; every `pop`/`drop` is guarded by `[ -n "$STASH_REF" ]` | ||
| - [ ] **Division-by-zero**: Every arithmetic division or percentage computation guards against zero denominators | ||
| - [ ] **DRY_RUN consistency**: If `--dry-run` is supported, every destructive operation is gated on the flag at the point of action, not just at phase entry | ||
|
|
||
| ### Robustness checks: | ||
| - [ ] **Rollback paths**: Every destructive operation has documented undo instructions | ||
| - [ ] **Error messages**: Every failure path produces a specific, actionable error message (not just "failed") | ||
| - [ ] **Concurrency safety**: No shared global state that would break under parallel invocation | ||
| - [ ] **Determinism**: No non-deterministic algorithm output used for before/after comparisons (e.g., Louvain community IDs) | ||
|
|
||
| ### Completeness checks: | ||
| - [ ] **Artifact schema**: If the skill produces files, path/format/schema are documented | ||
| - [ ] **Exit conditions**: Each phase states what must be true before the next phase starts | ||
| - [ ] **Scope boundary**: The skill's purpose is clear — it does one thing, not five | ||
| - [ ] **Examples section**: At least 2-3 realistic usage examples showing common invocations are included | ||
|
|
||
| ### Safety checks: | ||
| - [ ] **Idempotency**: Re-running the skill on the same state is safe. Existing output files are handled (skip, overwrite with warning, or merge) | ||
| - [ ] **Dependency validation**: Phase 0 verifies all shell commands used in bash blocks are available before starting work (e.g. `command -v git mktemp jq`). "Command not found" is caught before Phase 2, not during Phase 3 | ||
| - [ ] **Exit codes**: Every error path uses explicit `exit 1`. No silent early returns that leave the pipeline in an ambiguous state | ||
| - [ ] **State cleanup**: If the skill creates `.codegraph/$SKILL_NAME/*` files, the skill documents when they're cleaned up or how users remove them (e.g., `rm -rf .codegraph/$SKILL_NAME` in a cleanup section) | ||
| - [ ] **Git commit safety**: All `git add` calls use explicit file paths (never `.` or `-A`); `git diff --cached --quiet` is checked before committing to avoid empty commits | ||
|
|
||
| Read through the entire SKILL.md one more time after checking all items. Fix anything found. |
There was a problem hiding this comment.
Phase 4 (Self-Review Checklist) has no **Exit condition:** line. Every other phase (0–3, 5, 6) has one. This creates two self-referential failures:
-
lint-skill.sh Check 9 will emit a WARN when run against this SKILL.md itself:
WARN: Phase '## Phase 4 — Self-Review Checklist' has no 'Exit condition' before the next phaseThis is confirmed by tracing the logic: when Check 9 encounters
## Phase 5 — Smoke Test(line 483),prev_phaseis## Phase 4 — Self-Review Checklistandphase_has_exitisfalse. -
Phase 4's own checklist item (line 468) requires: "Exit conditions: Each phase states what must be true before the next phase starts." Phase 4 itself violates the rule it enforces.
The PR description states "Ran the skill's Phase 4 self-review checklist against itself" — but a skill that produces a Check 9 WARN when linted cannot have fully passed that validation.
| ## Phase 4 — Self-Review Checklist | |
| Before finalizing, audit the SKILL.md against every item below. **Do not skip any item.** Fix violations before proceeding. | |
| ### Structure checks: | |
| - [ ] Frontmatter has all four fields: `name`, `description`, `argument-hint`, `allowed-tools` | |
| - [ ] `name` matches the directory name | |
| - [ ] Phase 0 exists and validates the environment | |
| - [ ] Arguments section explicitly parses `$ARGUMENTS` into named variables | |
| - [ ] Rules section exists at the bottom | |
| - [ ] Every phase has a clear name (not just a number) | |
| ### Anti-pattern checks (all 17 patterns): | |
| - [ ] **Shell variables**: No variable is set in one code fence and used in another. State that must persist is written to a file | |
| - [ ] **Silent failures**: No `2>/dev/null` without a documented skip rationale. No commands that swallow errors | |
| - [ ] **Temp file extensions**: Every temp file passed to codegraph has the correct language extension | |
| - [ ] **Temp file uniqueness**: Every temp path uses `mktemp`, never hardcoded paths | |
| - [ ] **Cross-references**: All step references use phase names, not bare numbers | |
| - [ ] **Placeholders**: Every `<placeholder>` has a preceding detection/assignment script | |
| - [ ] **Contradictions**: No two sections describe contradictory behavior for the same condition | |
| - [ ] **Rules sync**: Every command/tool in the procedure is covered by Rules. Every Rules exception maps to a real step | |
| - [ ] **Redundancy**: No codegraph command is run twice with the same arguments. Later phases reference earlier results | |
| - [ ] **Skip validation**: If `--start-from`/`--skip-*` is supported, every skip path validates required artifacts | |
| - [ ] **Progress indicators**: Phases that iterate over files or run batch operations emit progress (`Processing $i/$total`) | |
| - [ ] **Artifact reuse**: Expensive operations (codegraph build, embedding generation, batch analysis) check for existing output before re-running | |
| - [ ] **Platform portability**: No `sed -i ''`, no unquoted globs, no GNU-only flags without fallback or documentation | |
| - [ ] **Cleanup traps**: Phases that create temp files or modify repo state use `trap ... EXIT` for cleanup on error paths | |
| - [ ] **Git stash safety**: Every `git stash push` has a named STASH_REF lookup; every `pop`/`drop` is guarded by `[ -n "$STASH_REF" ]` | |
| - [ ] **Division-by-zero**: Every arithmetic division or percentage computation guards against zero denominators | |
| - [ ] **DRY_RUN consistency**: If `--dry-run` is supported, every destructive operation is gated on the flag at the point of action, not just at phase entry | |
| ### Robustness checks: | |
| - [ ] **Rollback paths**: Every destructive operation has documented undo instructions | |
| - [ ] **Error messages**: Every failure path produces a specific, actionable error message (not just "failed") | |
| - [ ] **Concurrency safety**: No shared global state that would break under parallel invocation | |
| - [ ] **Determinism**: No non-deterministic algorithm output used for before/after comparisons (e.g., Louvain community IDs) | |
| ### Completeness checks: | |
| - [ ] **Artifact schema**: If the skill produces files, path/format/schema are documented | |
| - [ ] **Exit conditions**: Each phase states what must be true before the next phase starts | |
| - [ ] **Scope boundary**: The skill's purpose is clear — it does one thing, not five | |
| - [ ] **Examples section**: At least 2-3 realistic usage examples showing common invocations are included | |
| ### Safety checks: | |
| - [ ] **Idempotency**: Re-running the skill on the same state is safe. Existing output files are handled (skip, overwrite with warning, or merge) | |
| - [ ] **Dependency validation**: Phase 0 verifies all shell commands used in bash blocks are available before starting work (e.g. `command -v git mktemp jq`). "Command not found" is caught before Phase 2, not during Phase 3 | |
| - [ ] **Exit codes**: Every error path uses explicit `exit 1`. No silent early returns that leave the pipeline in an ambiguous state | |
| - [ ] **State cleanup**: If the skill creates `.codegraph/$SKILL_NAME/*` files, the skill documents when they're cleaned up or how users remove them (e.g., `rm -rf .codegraph/$SKILL_NAME` in a cleanup section) | |
| - [ ] **Git commit safety**: All `git add` calls use explicit file paths (never `.` or `-A`); `git diff --cached --quiet` is checked before committing to avoid empty commits | |
| Read through the entire SKILL.md one more time after checking all items. Fix anything found. | |
| Read through the entire SKILL.md one more time after checking all items. Fix anything found. | |
| **Exit condition:** All 17 checklist items pass. The SKILL.md has no unresolved structure, anti-pattern, robustness, completeness, or safety violations. | |
| --- |
There was a problem hiding this comment.
Fixed in e04545d — added the missing **Exit condition:** line to Phase 4. The skill now passes its own Check 9 with zero warnings and zero errors.
| if echo "$line" | grep -qE '^\s*if\s.*(-f\s|-d\s|lock|package|command -v|which\s)'; then | ||
| in_detect=true | ||
| was_in_detect=true | ||
| detect_depth=$((detect_depth + 1)) | ||
| elif echo "$line" | grep -qE '^\s*elif\s.*(-f\s|-d\s|lock|package|command -v|which\s)'; then | ||
| # elif is a sibling branch — set in_detect but do NOT increment depth | ||
| in_detect=true | ||
| was_in_detect=true | ||
| elif echo "$line" | grep -qE '^\s*if\b'; then | ||
| # nested if (not a detection block) — track depth only when inside detection | ||
| [ "$in_detect" = true ] && detect_depth=$((detect_depth + 1)) | ||
| elif echo "$line" | grep -qE '^\s*fi\b'; then | ||
| if [ "$detect_depth" -gt 0 ]; then | ||
| detect_depth=$((detect_depth - 1)) | ||
| [ "$detect_depth" -eq 0 ] && in_detect=false | ||
| else | ||
| # Safety reset: in_detect was set by an elif without a preceding detection if | ||
| in_detect=false | ||
| fi | ||
| elif $in_detect && echo "$line" | grep -qE '\bfi\b'; then | ||
| # fi appears inline (e.g. "else ...; fi") — still closes the outermost detection block | ||
| if [ "$detect_depth" -gt 0 ]; then | ||
| detect_depth=$((detect_depth - 1)) | ||
| [ "$detect_depth" -eq 0 ] && in_detect=false | ||
| else | ||
| in_detect=false | ||
| fi | ||
| fi |
There was a problem hiding this comment.
if ...; fi leaves detect_depth stuck at 1
When a detection if and its closing fi appear on the same line (e.g. if [ -f "package.json" ]; then echo "found"; fi), the first if-detection branch (line 138) fires and increments detect_depth to 1. Because bash if/elif short-circuits, the later branches — including the inline-fi handler at line 157 — are never evaluated for the same line. The result is that detect_depth stays at 1 and in_detect stays true for every subsequent line in the same code fence.
Consequence: any npm test / yarn test line that follows the one-liner in the same fence is silently treated as "inside a detection block" and never warned about — a false negative.
Concrete failing case:
if [ -f "package.json" ]; then echo "found package.json"; fi
npm test # ← not flagged, because in_detect is stuck trueA fix is to check for the closing fi on the same line as the opening detection if, before committing the depth increment:
if echo "$line" | grep -qE '^\s*if\s.*(-f\s|-d\s|lock|package|command -v|which\s)'; then
in_detect=true
was_in_detect=true
# Only increment depth if fi does NOT also close on this line
if ! echo "$line" | grep -qE '\bfi\b'; then
detect_depth=$((detect_depth + 1))
fiThere was a problem hiding this comment.
Fixed in 5b5ff9e — the detection if branch now checks for inline \bfi\b on the same line and skips the detect_depth increment when found. One-liner if [ -f ... ]; then ...; fi patterns no longer leave detect_depth stuck at 1.
| elif echo "$line" | grep -qE '^\s*elif\s.*(-f\s|-d\s|lock|package|command -v|which\s)'; then | ||
| # elif is a sibling branch — set in_detect but do NOT increment depth | ||
| in_detect=true | ||
| was_in_detect=true |
There was a problem hiding this comment.
find-based conditions
The elif detection pattern (line 142) recognises -f, -d, lock, package, command -v, and which — but not find-based conditions. The Phase 3 lint-runner template in SKILL.md (line 419) uses:
elif find . -maxdepth 1 -name "eslint.config.*" | grep -q .; then LINT_CMD="npx eslint ."This elif branch is currently safe only because the preceding if [ -f "biome.json" ] already sets in_detect=true. However, a skill author who restructures the detection block to lead with a find condition — e.g.:
if find . -name "*.go" | grep -q .; then CMD="go test ./..."
elif [ -f "package.json" ]; then CMD="npm test"
else CMD="true"; fi…would have the outer if fall through to the non-detection if\b branch (line 146), which only increments detect_depth if already inside a detection block. in_detect would stay false, and npm test in the elif/else branch would be incorrectly flagged.
Consider adding find\s to both detection patterns:
if echo "$line" | grep -qE '^\s*if\s.*(-f\s|-d\s|lock|package|command -v|which\s|find\s)'; thenAnd the same for the elif branch (line 142).
There was a problem hiding this comment.
Fixed in 5b5ff9e — added find\s to both the if and elif detection keyword patterns. A skill author who leads a detection block with a find-based condition (e.g. if find . -name "*.go" | grep -q .) will now have in_detect correctly set to true.
#587) - Guard detect_depth increment when fi appears on same line as detection if - Add find\s to both if and elif detection keyword patterns
| ```markdown | ||
| --- | ||
| name: $SKILL_NAME | ||
| description: <one-line from user's purpose> | ||
| argument-hint: "<from user's argument design>" | ||
| allowed-tools: <from user's tool list> | ||
| --- | ||
|
|
||
| # /$SKILL_NAME — <Title> | ||
|
|
||
| <Purpose paragraph from Phase 0> | ||
|
|
||
| ## Arguments | ||
|
|
||
| - `$ARGUMENTS` parsing rules here | ||
| - Set state variables: `DRY_RUN`, `AUTO_FIX`, etc. | ||
|
|
||
| ## Phase 0 — Pre-flight | ||
|
|
||
| 1. Confirm environment (repo root, required runtime/toolchain version, required tools) | ||
| 2. Parse `$ARGUMENTS` into state variables | ||
| 3. Validate preconditions | ||
|
|
||
| **Exit condition:** <What must be true before Phase 1 starts, e.g. "git repo confirmed, arguments validated, all required tools present"> | ||
|
|
||
| ## Phase N — <Name> | ||
|
|
||
| <Steps> | ||
|
|
||
| **Exit condition:** <What must be true before the next phase starts> | ||
|
|
||
| ## Rules | ||
|
|
||
| - <Hard constraints> | ||
| ``` |
There was a problem hiding this comment.
## Examples section placeholder
The scaffold template at lines 72–106 produces a SKILL.md with: frontmatter → title → Arguments → Phase 0 → Phase N → Rules. There is no ## Examples section.
The Phase 4 Self-Review Checklist (line 470) explicitly requires:
**Examples section**: At least 2-3 realistic usage examples showing common invocations are included
A skill author who faithfully follows this scaffold will produce a SKILL.md without an Examples section, then immediately fail the Phase 4 checklist item for the one thing the scaffold failed to guide them to include. The fix applied to SKILL.md itself (adding ## Examples in commit fb08cbd) was not applied to the scaffold template.
Consider adding a placeholder section between ## Rules and the end of the template:
## Examples
- `/<SKILL_NAME> <example-arg>` — <what this invocation does>
- `/<SKILL_NAME> --dry-run` — <what dry-run mode does>
- `/<SKILL_NAME> --fix <path>` — <what targeted invocation does>This makes the scaffold self-consistent with its own Phase 4 requirement from the first write.
There was a problem hiding this comment.
Fixed in f365f8b. Added an ## Examples section placeholder to the scaffold template between ## Rules and the end of the markdown block. Also added "Examples section" as item 5 in the structural requirements checklist to keep the two in sync.
Bash hooks (guard-git, pre-commit, lint-staged, check-readme, guard-pr-body, track-moves, post-git-ops) now use `Bash(git )` matcher instead of `Bash`, so they only fire on git commands rather than every Bash invocation — reduces token consumption.
| while IFS=$'\t' read -r bnum line; do | ||
| for var in "${!VAR_BLOCK[@]}"; do | ||
| assigned_in="${VAR_BLOCK[$var]}" | ||
| if [ "$bnum" -gt "$assigned_in" ]; then | ||
| # Check if this line references the variable ($VAR or ${VAR}) | ||
| if echo "$line" | grep -qE '\$'"${var}"'([^A-Za-z0-9_]|$)' \ | ||
| || echo "$line" | grep -qF "\${${var}}"; then | ||
| # Check if the same block also assigns it (re-assignment is fine) — O(1) lookup | ||
| if [ -z "${REASSIGNED[${var}:${bnum}]+x}" ]; then | ||
| # Check it's not read from a file (cat, $(...) with cat/read) | ||
| if ! echo "$line" | grep -qE 'cat |read |< |<"|\$\(<'; then | ||
| error "Cross-fence variable: \$$var assigned in bash block $assigned_in, referenced in block $bnum without file persistence (Pattern 1)" | ||
| fi | ||
| fi | ||
| fi | ||
| fi | ||
| done | ||
| done < "$BLOCKS_FILE" |
There was a problem hiding this comment.
The reference-checking loop applies the cross-fence variable check to every line from $BLOCKS_FILE, including # comment lines. If a bash block contains a context comment that mentions an uppercase variable defined in an earlier block — e.g.:
# $FILE is the path resolved in Phase: Setup above; uses Pattern 1 file persistence
diff <(something) <(something_else)Check 1 would incorrectly emit an ERROR because $FILE appears in the comment in a later block. The comment isn't actually using the variable at runtime, so this is a false positive that undermines trust in the linter.
The fix is to skip comment lines before the variable-reference checks:
while IFS=$'\t' read -r bnum line; do
# Skip comment lines — they document context but don't execute variables at runtime
echo "$line" | grep -qE '^\s*#' && continue
for var in "${!VAR_BLOCK[@]}"; do
...
done
done < "$BLOCKS_FILE"This is a latent bug in user-generated skills: SKILL.md itself avoids it because no uppercase variable assigned in an earlier bash block is mentioned in a comment in a later block, but any generated skill that follows Pattern 11's taught convention of adding # $VAR is set by the surrounding loop context comments in later blocks will get a false-positive ERROR on each comment.
There was a problem hiding this comment.
Fixed — added a guard to skip comment lines before the variable-reference checks in Check 1. Lines starting with # are now excluded so context comments like $FILE is set above no longer trigger false-positive cross-fence errors.
| if echo "$line" | grep -qE '^\s*if\s.*(-f\s|-d\s|lock|package|command -v|which\s|find\s)'; then | ||
| in_detect=true | ||
| was_in_detect=true | ||
| # Only increment depth if fi does NOT also close on this line (one-liner guard) | ||
| if ! echo "$line" | grep -qE '\bfi\b'; then | ||
| detect_depth=$((detect_depth + 1)) | ||
| fi | ||
| elif echo "$line" | grep -qE '^\s*elif\s.*(-f\s|-d\s|lock|package|command -v|which\s|find\s)'; then | ||
| # elif is a sibling branch — set in_detect but do NOT increment depth | ||
| in_detect=true | ||
| was_in_detect=true |
There was a problem hiding this comment.
lock and package are substring matches — no word boundary
The if and elif detection patterns use bare substrings lock and package without word boundaries:
'^\s*if\s.*(-f\s|-d\s|lock|package|command -v|which\s|find\s)'
'^\s*elif\s.*(-f\s|-d\s|lock|package|command -v|which\s|find\s)'Any if/elif condition that contains the word "lock" or "package" as a substring — even unrelated to package-manager detection — will incorrectly set in_detect=true, preventing Check 4 from flagging hardcoded npm test / yarn test lines in that block. Concrete false-negative cases:
# "lock" as a concept, not a lockfile
if [ "$status" = "lock-acquired" ]; then
npm test # ← NOT flagged, but should be (in_detect is true due to "lock" match)
fiUsing word-boundary anchors would prevent these false negatives:
'^\s*if\s.*(-f\s|-d\s|\block\b|\bpackage\b|command -v|which\s|find\s)'There was a problem hiding this comment.
Fixed — added \b word-boundary anchors around lock and package in both the if and elif detection patterns. Lines containing substrings like CLOCKWORK or repackage no longer falsely trigger detection mode.
| Write the SKILL.md file starting with this structure: | ||
|
|
||
| ```markdown | ||
| --- | ||
| name: $SKILL_NAME | ||
| description: <one-line from user's purpose> | ||
| argument-hint: "<from user's argument design>" | ||
| allowed-tools: <from user's tool list> | ||
| --- | ||
|
|
||
| # /$SKILL_NAME — <Title> | ||
|
|
||
| <Purpose paragraph from Phase 0> | ||
|
|
||
| ## Arguments | ||
|
|
||
| - `$ARGUMENTS` parsing rules here | ||
| - Set state variables: `DRY_RUN`, `AUTO_FIX`, etc. | ||
|
|
||
| ## Phase 0 — Pre-flight | ||
|
|
||
| 1. Confirm environment (repo root, required runtime/toolchain version, required tools) | ||
| 2. Parse `$ARGUMENTS` into state variables | ||
| 3. Validate preconditions | ||
|
|
||
| **Exit condition:** <What must be true before Phase 1 starts, e.g. "git repo confirmed, arguments validated, all required tools present"> | ||
|
|
||
| ## Phase N — <Name> | ||
|
|
||
| <Steps> | ||
|
|
||
| **Exit condition:** <What must be true before the next phase starts> | ||
|
|
||
| ## Rules | ||
|
|
||
| - <Hard constraints> | ||
|
|
||
| ## Examples | ||
|
|
||
| - <Usage examples showing common invocations and expected behavior> | ||
| ``` |
There was a problem hiding this comment.
The scaffold template produces a Phase 0 with three numbered prose steps but no concrete bash block. The actual create-skill SKILL.md's own Phase 0 teaches the concrete pattern:
for tool in git mktemp; do
command -v "$tool" > /dev/null 2>&1 || { echo "ERROR: required tool '$tool' not found"; exit 1; }
done
git rev-parse --show-toplevel > /dev/null 2>&1 || { echo "ERROR: not in a git repository"; exit 1; }An AI following the template will produce a Phase 0 with prose-only instructions that passes Phase 4's structural check and Check 7 in lint-skill.sh, but the actual generated pre-flight will be weaker than the pattern this skill itself demonstrates. Adding a commented-out bash stub to the template would guide AI authors to follow Pattern 2 and the dependency-validation checklist item from the start.
There was a problem hiding this comment.
Fixed — added a concrete bash pre-flight block to the scaffold template's Phase 0, mirroring the actual SKILL.md's own Phase 0 pattern (tool availability check via \ + git repo validation). Includes justification comments on \ lines per Pattern 2.
.claude/skills/create-skill/SKILL.md
Outdated
| ## Rules | ||
|
|
||
| - <Hard constraints> | ||
|
|
||
| ## Examples | ||
|
|
||
| - <Usage examples showing common invocations and expected behavior> |
There was a problem hiding this comment.
## Rules before ## Examples, but SKILL.md itself reverses this order
The scaffold template (lines 103–109) produces Rules → Examples ordering, but the actual SKILL.md has ## Examples (line 572) before ## Rules (line 580). An AI author following this template will produce a skill with the opposite ordering from the exemplar. The structural requirements don't mandate a specific ordering, but the inconsistency between the template and the exemplar is the kind of silent contradiction Pattern 7 is designed to prevent.
Aligning the template to match the actual SKILL.md ordering (Examples before Rules) would make the scaffold self-consistent.
There was a problem hiding this comment.
Fixed — swapped the scaffold template to produce Examples before Rules, matching the actual SKILL.md's ordering. The template and exemplar are now consistent.
Skip comment lines in Check 1 cross-fence variable detection to avoid flagging context comments like `# $VAR is set above`. Add word-boundary anchors to `lock` and `package` keywords in Check 4 detection heuristic so substrings like `CLOCKWORK` or `repackage` do not falsely trigger detection mode.
…le (#587) Add a concrete bash pre-flight block to the scaffold template Phase 0 so generated skills follow the same Pattern 2 convention the actual SKILL.md demonstrates. Swap Rules/Examples ordering in the template to match the exemplar (Examples before Rules).
| "PreToolUse": [ | ||
| { | ||
| "matcher": "Bash", | ||
| "matcher": "Bash(git )", |
There was a problem hiding this comment.
Both .claude/settings.json and docs/examples/claude-code-hooks/settings.json change the hook matcher from "Bash" to "Bash(git )" in both PreToolUse and PostToolUse. This is a meaningful narrowing: previously guard-git.sh, guard-pr-body.sh, pre-commit.sh, lint-staged.sh, and check-readme.sh fired before every Bash invocation; now they only fire when the command begins with git (the trailing space means bare git without a subcommand would also be excluded).
This change is not mentioned anywhere in the PR description, and CLAUDE.md states: "One PR = one concern. Each pull request should address a single feature, fix, or refactor." The hook-scope change is a separate behavioral change from the skill addition.
Could you document the rationale here (e.g., was this causing noise from non-git bash commands the skill runs, or is it a general cleanup)? If it's a general fix, it would be cleaner in its own PR so the hook behavior change is independently reviewable.
Context Used: CLAUDE.md (source)
| elif echo "$line" | grep -qE '^\s*elif\s.*(-f\s|-d\s|\block\b|\bpackage\b|command -v|which\s|find\s)'; then | ||
| # elif is a sibling branch — set in_detect but do NOT increment depth | ||
| in_detect=true | ||
| was_in_detect=true | ||
| elif echo "$line" | grep -qE '^\s*if\b'; then | ||
| # nested if (not a detection block) — track depth only when inside detection | ||
| [ "$in_detect" = true ] && detect_depth=$((detect_depth + 1)) | ||
| elif echo "$line" | grep -qE '^\s*fi\b'; then | ||
| if [ "$detect_depth" -gt 0 ]; then | ||
| detect_depth=$((detect_depth - 1)) | ||
| [ "$detect_depth" -eq 0 ] && in_detect=false | ||
| else | ||
| # Safety reset: in_detect was set by an elif without a preceding detection if | ||
| in_detect=false | ||
| fi | ||
| elif $in_detect && echo "$line" | grep -qE '\bfi\b'; then | ||
| # fi appears inline (e.g. "else ...; fi") — still closes the outermost detection block | ||
| if [ "$detect_depth" -gt 0 ]; then | ||
| detect_depth=$((detect_depth - 1)) | ||
| [ "$detect_depth" -eq 0 ] && in_detect=false | ||
| else | ||
| in_detect=false | ||
| fi | ||
| fi |
There was a problem hiding this comment.
elif detection branch with inline fi leaves in_detect permanently true
When an elif detection branch (line 147) matches, the entire if/elif chain exits immediately — the inline-fi handler at line 162 (elif $in_detect && echo "$line" | grep -qE '\bfi\b') is a sibling elif in the same chain and is never evaluated for the same line. This means for a two-branch detection block where fi is inline on the final elif:
if [ -f "pnpm-lock.yaml" ]; then TEST_CMD="pnpm test"
elif [ -f "yarn.lock" ]; then TEST_CMD="yarn test"; fi
npm test # ← NOT flagged — in_detect is stuck true, detect_depth stays 1Step-by-step:
if [ -f "pnpm-lock.yaml" ]...→ branch at line 140 fires,detect_depth=1,in_detect=trueelif [ -f "yarn.lock" ]; then ...; fi→ branch at line 147 fires, setsin_detect=true. The^\s*fi\bcheck (line 154) and inline-fi check (line 162) are both skipped because the elif at line 147 already matched.detect_depthstays 1,in_detectstays true — all subsequent lines in the fence are treated as inside a detection block.
The fix requires handling the fi within the elif detection branch itself before exiting:
elif echo "$line" | grep -qE '^\s*elif\s.*(-f\s|-d\s|\block\b|\bpackage\b|command -v|which\s|find\s)'; then
# elif is a sibling branch — set in_detect but do NOT increment depth
in_detect=true
was_in_detect=true
# Also handle inline fi on this same elif line
if echo "$line" | grep -qE '\bfi\b'; then
if [ "$detect_depth" -gt 0 ]; then
detect_depth=$((detect_depth - 1))
[ "$detect_depth" -eq 0 ] && in_detect=false
else
in_detect=false
fi
fi1 participant
Summary
/create-skillskill that scaffolds, writes, and validates new Claude Code skillsMotivation
The titan skill PRs accumulated 210 review comments (103 from Greptile alone) across 10 recurring issue categories. Most were structural consistency problems — shell vars across fences, stale step references, silent failures, internal contradictions. This skill prevents those issues upfront by making them part of the authoring process rather than catching them in review.
What the skill does
Test plan