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 83 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 introduces Confidence Score: 4/5Safe to merge; two minor edge cases in lint-skill.sh's Check 4 detection-block heuristic are false-negative/false-positive gaps with very low real-world impact After 40+ resolved review rounds the code is thoroughly hardened. The remaining issues are P2 edge cases: (1) one-liner if/fi leaves detect_depth at 1 causing a false negative, and (2) find-based elif branches aren't in the detection keyword set. Neither affects the quality gates for skills following the taught patterns (all examples use multi-line detection blocks).
Important Files Changed
Reviews (42): Last reviewed commit: "Merge branch 'main' into feat/create-ski..." | 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.
… into fix/review-587-agent
|
Addressed both findings from the latest Greptile review in 5717c21:
|
| if echo "$line" | grep -qE '^\s*git add (--\s+\.|\.|(-A|--all))'; then | ||
| error "bash block $bnum: 'git add .' or 'git add -A' — stage named files only" | ||
| fi |
There was a problem hiding this comment.
The \. alternative in the regex matches any literal dot, so git add .gitignore, git add .env, and git add ./src/file.ts are all flagged as violations — even though they are staging specific, named files (the correct behavior this check is designed to enforce).
Verified locally:
git add .gitignore → MATCH (false positive)
git add ./src/file.ts → MATCH (false positive)
git add .env → MATCH (false positive)
git add -- .gitignore → MATCH (false positive) ← the `--\s+\.` variant also lacks termination
A skill that stages .gitignore or .eslintrc.cjs would be incorrectly blocked by its own quality gate, eroding trust in the linter — the same concern raised in the earlier grep -qF prefix thread (fixed in daa4c88).
The fix is to require that the dot ends the token (followed by whitespace, end-of-line, or a comment #):
| if echo "$line" | grep -qE '^\s*git add (--\s+\.|\.|(-A|--all))'; then | |
| error "bash block $bnum: 'git add .' or 'git add -A' — stage named files only" | |
| fi | |
| if echo "$line" | grep -qE '^\s*git add (--\s+\.([[:space:]]|$)|\.([ \t;#]|$)|(-A|--all))'; then |
With this, git add . and git add -- . still fire, while git add .gitignore, git add ./src/file.ts, and git add -- .gitignore are correctly allowed.
There was a problem hiding this comment.
Fixed in 163b328 — the \. alternatives in the regex now require the dot to be a standalone token by anchoring with ([ \t;#]|$). This prevents false positives on dotfiles (.gitignore, .env) and relative paths (./src/file.ts).
There was a problem hiding this comment.
Already fixed in 163b328 — Check 3 regex now terminates the dot with ([ \t;#]|$) so git add .gitignore, git add ./src/file.ts, and git add -- .gitignore are correctly allowed.
There was a problem hiding this comment.
Already fixed in 163b328 — Check 3 regex now terminates the dot with ([ \t;#]|$) so git add .gitignore, git add ./src/file.ts, and git add -- .gitignore are correctly allowed.
| - **No Claude Code references** in commit messages or the skill body. | ||
| - **Persist temp state to files, not shell variables** when it must survive across code fences. | ||
| - **Test commands must be detected, not assumed.** Never hardcode `npm test` — detect the package manager first. | ||
| - **Every `2>/dev/null` needs a justification comment** in the generated skill. |
There was a problem hiding this comment.
The rule currently reads:
Test commands must be detected, not assumed. Never hardcode \npm test` — detect the package manager first.`
But lint-skill.sh Check 4 (line 162) now catches three forms:
'^\s*(npm test|npm run (test|lint))([^:A-Za-z0-9_]|$)'
A skill author who reads only this Rules section would believe npm run test and npm run lint are acceptable outside a detection block, then be surprised when Check 4 fires. Since the Rules section is the canonical quick-reference that authors consult, it should enumerate all three prohibited patterns:
| - **Every `2>/dev/null` needs a justification comment** in the generated skill. | |
| - **Test commands must be detected, not assumed.** Never hardcode `npm test`, `npm run test`, or `npm run lint` — detect the package manager first. |
There was a problem hiding this comment.
Fixed in 00fb02b — Rules section now lists all prohibited forms: npm test, npm run test, npm run lint, yarn test, and pnpm test, matching what lint-skill.sh Check 4 enforces.
| fi | ||
| fi | ||
| if ! $in_detect; then | ||
| if echo "$line" | grep -qE '^\s*(npm test|npm run (test|lint))([^:A-Za-z0-9_]|$)'; then |
There was a problem hiding this comment.
yarn test and pnpm test
The check catches npm test, npm run test, and npm run lint hardcoded outside a detection block, but not yarn test or pnpm test. A skill that unconditionally runs:
yarn test # outside any detection blockpasses Check 4 silently even though it violates Pattern 6 in exactly the same way. Pattern 6's "Correct" example includes pnpm-lock.yaml and yarn.lock detection precisely because those runners also need to be detected.
Adding yarn test and pnpm test to the regex closes the gap:
if echo "$line" | grep -qE '^\s*(npm test|npm run (test|lint)|yarn test|pnpm test)([^:A-Za-z0-9_]|$)'; then(Note: yarn run lint and pnpm run lint are less common than their test equivalents, but could be added alongside for completeness.)
There was a problem hiding this comment.
Fixed in 00fb02b — Check 4 regex now uses (npm|yarn|pnpm) alternation to catch all three package managers for both direct commands (e.g. yarn test) and run variants (e.g. pnpm run lint) outside detection blocks.
There was a problem hiding this comment.
Already fixed in 00fb02b — Check 4 regex now matches (npm|yarn|pnpm) test|(npm|yarn|pnpm) run (test|lint), covering all three package managers.
.claude/skills/create-skill/SKILL.md
Outdated
| fi | ||
| ``` | ||
|
|
||
| - **`lint-skill.sh`** checks for cross-fence variable bugs, bare `2>/dev/null`, hardcoded `npm test` / `npm run test` / `npm run lint`, `git add .` / `git add -- .`, missing frontmatter, missing Phase 0 / Rules, missing exit conditions, GNU-only `find -quit`, hardcoded `/tmp/` paths, and `sed -i` portability issues. |
There was a problem hiding this comment.
After commit 00fb02b, lint-skill.sh Check 4 was expanded to catch all three package managers — including yarn run test, yarn run lint, pnpm run test, and pnpm run lint — via the regex:
(npm|yarn|pnpm) test|(npm|yarn|pnpm) run (test|lint)
But this line still only lists npm variants:
"hardcoded
npm test/npm run test/npm run lint"
A skill author who writes yarn run lint or pnpm run test outside a detection block will see an unexpected WARN from the linter with no explanation in the documentation they were given to understand it.
The Rules section (line 570) has a related gap: it enumerates yarn test and pnpm test but not yarn run test, yarn run lint, pnpm run test, or pnpm run lint — so authors reading the Rules section as the authoritative prohibited list will also miss these forms.
| - **`lint-skill.sh`** checks for cross-fence variable bugs, bare `2>/dev/null`, hardcoded `npm test` / `npm run test` / `npm run lint`, `git add .` / `git add -- .`, missing frontmatter, missing Phase 0 / Rules, missing exit conditions, GNU-only `find -quit`, hardcoded `/tmp/` paths, and `sed -i` portability issues. | |
| - **`lint-skill.sh`** checks for cross-fence variable bugs, bare `2>/dev/null`, hardcoded `npm test` / `npm run test` / `npm run lint` / `yarn test` / `yarn run test` / `yarn run lint` / `pnpm test` / `pnpm run test` / `pnpm run lint`, `git add .` / `git add -- .`, missing frontmatter, missing Phase 0 / Rules, missing exit conditions, GNU-only `find -quit`, hardcoded `/tmp/` paths, and `sed -i` portability issues. |
The Rules section (line 570) should similarly be updated to include yarn run test, yarn run lint, pnpm run test, and pnpm run lint.
There was a problem hiding this comment.
Fixed in 3f43d8b — Phase 5 description (line 501) now lists all nine prohibited forms across npm/yarn/pnpm (including run variants). Rules section (line 570) similarly updated to include yarn run test, yarn run lint, pnpm run test, and pnpm run lint.
There was a problem hiding this comment.
Already fixed in 3f43d8b — Phase 5 description and Rules section now list all 9 prohibited forms including yarn/pnpm run variants.
| 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))
fi| 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).
1 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