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 111 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 The PR went through an extensive iterative review cycle (30+ threads), and the vast majority of structural, self-referential, and portability issues have been resolved. The final code is well-engineered: the linter uses an O(1) associative-array approach for cross-fence variable tracking, a depth-counter for nested detection blocks, and POSIX-compatible patterns ( Findings:
Confidence Score: 4/5Safe to merge — no blocking issues; the two findings are minor linter gaps that don't affect the skill's core functionality. The skill and its quality-gate scripts are production-ready after an extensive multi-round review. The two remaining issues are low-severity: a one-character regex gap in Check 2 (spaced
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["/create-skill skill-name"] --> B["Phase 0 — Discovery & Pre-flight\n• Bash: tool + git repo validation\n• Interactive: 7 requirement questions"]
B --> C["Phase 1 — Scaffold\n• Idempotency guard\n• mkdir + Write SKILL.md from template"]
C --> D["Phase 2 — Write the Skill Body\n• Apply 17 mandatory patterns"]
D --> E["Phase 3 — Dangerous Operation Guards\n• git / file deletion / API / code-mod guards"]
E --> F["Phase 4 — Self-Review Checklist\n• 17-item anti-pattern check"]
F --> G["Phase 5 — Smoke Test\n• lint-skill.sh — 11 static checks\n• smoke-test-skill.sh — bash -n all blocks"]
G --> H["Phase 6 — Finalize\n• User review + commit"]
Reviews (56): Last reviewed commit: "fix(skill): check 11 catches unquoted an..." | 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.
.claude/skills/create-skill/SKILL.md
Outdated
|
|
||
| Write the SKILL.md file starting with this structure: | ||
|
|
||
| ```markdown |
There was a problem hiding this comment.
\``markdown```` instead of quadruple-backtick fences
Check 9 in lint-skill.sh itself warns: "Limitation: nested fences inside \``markdown```blocks … will togglein_block` incorrectly. Wrap such regions in quadruple-backtick ```` fences to avoid false positives."
The scaffold template here is exactly that case — a \``markdown```block that contains an inner```bash```block (the pre-flight example at line 91). Whenlint-skill.sh` Check 9 parses this file, the sequence is:
```markdown→in_block=true(skip content)- Inner
```bash(line 91) →in_block=false(content now processed as outside-a-block!) - The pre-flight bash code (lines 92–97) is examined for
## Phase/**Exit condition:**markers - Inner
```(line 98) →in_block=trueagain - Template prose including
## Phase 0 — Pre-flight,## Phase N, and**Exit condition:**placeholders is skipped - Closing
```(line 119) →in_block=false
Currently this doesn't produce false results because the bash code in the template (lines 92–97) happens to contain no phase markers. But it is fragile: if a future edit adds a comment containing ## Phase or **Exit condition:** inside the template's bash block, Check 9 would produce incorrect results when linting create-skill/SKILL.md itself.
The fix is to wrap the scaffold template in quadruple-backtick fences, consistent with the recommendation in Check 9's own code comment:
| ```markdown | |
| ````markdown |
And close with:
This would cause Check 9 to skip the entire template block (including the inner bash block), making the linter's behavior for this file robust regardless of future template edits.
There was a problem hiding this comment.
Fixed in c32e1d5 — changed the scaffold template fences from triple-backtick to quadruple-backtick on lines 72 and 119. Check 9 now correctly skips the entire template region (including the inner bash block at line 91), making the linter robust against future edits that add phase markers inside the template. Both lint-skill.sh (0 errors, 0 warnings) and smoke-test-skill.sh (10/10 passed) run clean.
The scaffold template at line 72 used triple-backtick fences which causes Check 9's in_block toggle to misfire when the template contains inner bash blocks. Switching to quadruple-backtick fences ensures Check 9 skips the entire template region, making the linter robust against future edits that add phase markers inside the template.
|
Addressed the remaining P2 Greptile finding in c32e1d5: changed the scaffold template fences from triple-backtick to quadruple-backtick (lines 72 and 119), so Check 9 correctly skips the entire template region. Both lint-skill.sh (0 errors, 0 warnings) and smoke-test-skill.sh (10/10 passed) run clean. |
) Every inner-loop check now uses [[ =~ ]] and [[ == *glob* ]] instead of spawning echo|grep subshells. On Windows, where process creation is ~100x slower than Linux, this reduces runtime from >120 s (timeout) to ~5 s. All 11 checks produce identical results — verified with violation, detection-block, and one-liner test cases.
|
Fixed a performance regression in lint-skill.sh (3b630ab): Problem: On Windows, lint-skill.sh timed out after 120+ seconds due to process fork overhead. Every inner-loop check used Fix: Replaced all
Result: Runtime dropped from >120s (timeout) to ~5s on Windows. All 11 checks produce identical results — verified with violation detection, detection-block, and one-liner test cases. Both lint-skill.sh (0 errors, 0 warnings) and smoke-test-skill.sh (10/10 passed) run clean against SKILL.md. |
| # One-liner: detection block is self-contained — reset so subsequent lines are checked normally | ||
| in_detect=false | ||
| else | ||
| detect_depth=$((detect_depth + 1)) |
There was a problem hiding this comment.
The terminal anchor ([^:A-Za-z0-9_]|$) correctly excludes : (to avoid flagging npm run test:unit) but does NOT exclude -. Because - is not a word character and not in the excluded set, any hyphenated npm script will be falsely flagged as a hardcoded command:
npm run lint-staged # ← FALSE POSITIVE: "lint" matches, "-" satisfies [^:A-Za-z0-9_]
npm run test-ci # ← FALSE POSITIVE: "test" matches, "-" satisfies [^:A-Za-z0-9_]
yarn run lint-fix # ← FALSE POSITIVE
pnpm run test-watch # ← FALSE POSITIVE
lint-staged in particular is an extremely common dependency — most Node.js projects that use it will trigger this false positive, eroding trust in the linter in the same way the previous grep -qF prefix-match fix (daa4c88) was motivated.
The fix mirrors the colon-exclusion approach: add - to the excluded character set.
| # One-liner: detection block is self-contained — reset so subsequent lines are checked normally | |
| in_detect=false | |
| else | |
| detect_depth=$((detect_depth + 1)) | |
| if [[ "$line" =~ ^[[:space:]]*((npm|yarn|pnpm)\ test|(npm|yarn|pnpm)\ run\ (test|lint))([^:\-A-Za-z0-9_]|$) ]]; then |
With this, npm run lint-staged and npm run test-ci are correctly allowed, while bare npm run lint, npm run test, etc. are still caught.
There was a problem hiding this comment.
Fixed in 0332d88. Placed - at the end of the negated character class [^:A-Za-z0-9_-] instead of using \-, which broke the entire regex in bash [[ =~ ]]. Verified: npm run lint-staged and npm run test-ci are now correctly allowed, while bare npm run lint, npm run test, and npm test are still caught.
| prev_phase="$line" | ||
| phase_has_exit=false | ||
| fi | ||
| if [[ "${line,,}" == *'**exit condition'* ]]; then | ||
| phase_has_exit=true | ||
| fi |
There was a problem hiding this comment.
/tmp/ paths
The regex [\"/]/tmp/[a-zA-Z] uses the character class [\"/] which matches ", \, or /. It catches double-quoted paths ("/tmp/output.json") and unquoted paths (/tmp/output.json), but NOT single-quoted paths:
FILE='/tmp/output.json' # ← NOT caught — single quote is not in [\"/]
rm -f '/tmp/tempfile.sh' # ← NOT caughtAdding ' to the character class closes this gap:
| prev_phase="$line" | |
| phase_has_exit=false | |
| fi | |
| if [[ "${line,,}" == *'**exit condition'* ]]; then | |
| phase_has_exit=true | |
| fi | |
| if [[ "$stripped" =~ [\"\'\/]/tmp/[a-zA-Z] ]]; then |
The ${TMPDIR:-/tmp} allowlist check on the next line still applies, so correct template-syntax patterns are not affected.
There was a problem hiding this comment.
Fixed in 0332d88. Added single-quote to the character class: ["'\/] → catches FILE='/tmp/output.json' alongside double-quoted and unquoted paths. The ${TMPDIR:-/tmp} allowlist is unaffected. Verified with a test file containing single-quoted, double-quoted, and unquoted /tmp/ paths — all three are now caught while the portable template syntax is still allowed.
) Check 4: Place `-` at end of negated character class `[^:A-Za-z0-9_-]` so hyphenated script names like `npm run lint-staged` and `npm run test-ci` are not falsely flagged. `\-` inside `[[ =~ ]]` broke the entire regex on bash. Check 11: Add single-quote to the `/tmp/` path detection character class `["'\/]` so `FILE='/tmp/output.json'` is caught alongside double-quoted and unquoted variants.
|
Addressed both findings from the latest Greptile review in 0332d88:
|
| '```'*) in_block=false; prev_line="$line"; continue ;; | ||
| esac | ||
| if $in_block; then | ||
| if [[ "$line" =~ 2\>/dev/null ]] || [[ "$line" =~ \>[[:space:]]*/dev/null\ 2\>\&1 ]] || [[ "$line" == *'&>/dev/null'* ]]; then |
There was a problem hiding this comment.
/dev/null and 2>&1
The regex \>[[:space:]]*/dev/null\ 2\>\&1 uses [[:space:]]* (zero-or-more whitespace) between > and /dev/null, correctly catching both >/dev/null and > /dev/null. However, the \ 2\>\&1 part matches exactly one space before 2>&1. A double-spaced form like >/dev/null 2>&1 would silently bypass Check 2 because neither the 2>/dev/null nor the \>[[:space:]]*/dev/null\ 2\>\&1 alternative catches it.
While uncommon in hand-written code, auto-formatters or editors that insert an extra space could produce this form. Replacing \ 2 with [[:space:]]+2 closes the gap consistently:
| if [[ "$line" =~ 2\>/dev/null ]] || [[ "$line" =~ \>[[:space:]]*/dev/null\ 2\>\&1 ]] || [[ "$line" == *'&>/dev/null'* ]]; then | |
| if [[ "$line" =~ 2\>/dev/null ]] || [[ "$line" =~ \>[[:space:]]*/dev/null[[:space:]]+2\>\&1 ]] || [[ "$line" == *'&>/dev/null'* ]]; then |
There was a problem hiding this comment.
Fixed in 83a3b1d — replaced the literal single-space anchor ( \ 2) with [[:space:]]+2 so double-spaced forms like >/dev/null 2>&1 are now also caught by Check 2. Both lint-skill.sh (0 errors, 0 warnings) and smoke-test-skill.sh (10/10 passed) run clean.
Replace literal single-space anchor (\ 2) with [[:space:]]+2 so >/dev/null 2>&1 (double-spaced) is also caught by the bare-redirect linter check.
Replace literal single-space anchor (\ 2) with [[:space:]]+2 so >/dev/null 2>&1 (double-spaced) is also caught by the bare-redirect linter check.
carlos-alm
force-pushed
the
feat/create-skill
branch
from
58890fc to
d275e12
Compare
| fi | ||
| prev_phase="$line" | ||
| phase_has_exit=false | ||
| fi | ||
| if [[ "${line,,}" == *'**exit condition'* ]]; then | ||
| phase_has_exit=true |
There was a problem hiding this comment.
/tmp/ assignments and arguments
The regex [\"\'\/]/tmp/[a-zA-Z] only triggers when /tmp/ is immediately preceded by ", ', or /. Two very common patterns are missed:
TMPFILE=/tmp/output.json # '=' before '/tmp/' — not in ["'\/]
rm /tmp/output.json # ' ' before '/tmp/' — not in ["'\/]These are among the most natural ways to write a hardcoded /tmp/ path, and both slip through without any warning. Pattern 4 specifically targets hardcoded paths like /tmp/skill-output.json — both examples above are exactly what it prohibits.
Alternatively, match anything NOT alphanumeric/underscore before /tmp/:
if [[ "$stripped" =~ (^|[^A-Za-z0-9_])/tmp/[a-zA-Z] ]]; thenThis catches =/tmp/..., /tmp/..., and /tmp/... at the start of a line, while the ${TMPDIR:-/tmp} allowlist exclusion on the next line still correctly suppresses the portable mktemp pattern.
There was a problem hiding this comment.
Fixed in c1fc463 — widened the Check 11 regex from ["'\/]/tmp/[a-zA-Z] to (^|[^A-Za-z0-9_])/tmp/[a-zA-Z] so TMPFILE=/tmp/output.json (preceded by =) and rm /tmp/output.json (preceded by space) are now caught. The ${TMPDIR:-/tmp} allowlist exclusion still correctly suppresses the portable mktemp pattern. Both lint-skill.sh (0 errors, 0 warnings) and smoke-test-skill.sh (10/10 passed) run clean.
Replace literal single-space anchor (\ 2) with [[:space:]]+2 so >/dev/null 2>&1 (double-spaced) is also caught by the bare-redirect linter check.
carlos-alm
force-pushed
the
feat/create-skill
branch
from
d275e12 to
d757c63
Compare
Widen the regex from ["'\/]/tmp/ to (^|[^A-Za-z0-9_])/tmp/ so
TMPFILE=/tmp/output.json and rm /tmp/output.json are also flagged.
The ${TMPDIR:-/tmp} allowlist exclusion still suppresses the portable
mktemp pattern.
| '```'*) in_block=false; prev_line="$line"; continue ;; | ||
| esac | ||
| if $in_block; then | ||
| if [[ "$line" =~ 2\>/dev/null ]] || [[ "$line" =~ \>[[:space:]]*/dev/null[[:space:]]+2\>\&1 ]] || [[ "$line" == *'&>/dev/null'* ]]; then |
There was a problem hiding this comment.
2> /dev/null (spaced redirect form)
The first condition [[ "$line" =~ 2\>/dev/null ]] only matches the no-space form. The valid bash redirect 2> /dev/null (with a space between 2> and /dev/null) satisfies neither the first condition nor the second (\>[[:space:]]*/dev/null[[:space:]]+2\>\&1). A skill author who writes:
git show HEAD:"$FILE" 2> /dev/nullwould have this line silently bypass Check 2 — Pattern 2's core quality gate. The fix extends the 2> alternative to allow optional whitespace:
| if [[ "$line" =~ 2\>/dev/null ]] || [[ "$line" =~ \>[[:space:]]*/dev/null[[:space:]]+2\>\&1 ]] || [[ "$line" == *'&>/dev/null'* ]]; then | |
| if [[ "$line" =~ 2[[:space:]]*/dev/null ]] || [[ "$line" =~ \>[[:space:]]*/dev/null[[:space:]]+2\>\&1 ]] || [[ "$line" == *'&>/dev/null'* ]]; then |
There was a problem hiding this comment.
Fixed in 3ea7a35 — changed 2\>/dev/null to 2\>[[:space:]]*/dev/null so the spaced redirect form 2> /dev/null is now caught by Check 2. Verified with a test skill containing the spaced form — correctly flags it.
.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` / `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. |
There was a problem hiding this comment.
The lint-skill.sh description lists all the checks authors should expect, but two checks added during this PR's review cycle are missing:
- Check 6b —
namefield must match the directory name (errors when they diverge) - Check 8b —
## Examplessection must be present (added alongside the structural requirement)
An author who runs lint-skill.sh and sees ERROR: Missing '## Examples' section or ERROR: Frontmatter 'name: ...' does not match directory name will have no description here to explain why those checks fired. They'd need to read the linter source directly.
| - **`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. | |
| - **`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 (including `name` matching directory name), missing Phase 0 / Rules / Examples, missing exit conditions, GNU-only `find -quit`, hardcoded `/tmp/` paths, and `sed -i` portability issues. |
There was a problem hiding this comment.
Fixed in 6e1f4fb — updated the Phase 5 lint-skill.sh description to include both missing checks: Check 6b (name field must match directory name) and Check 8b (## Examples section must be present). Description now reads: "missing frontmatter (including name matching directory name), missing Phase 0 / Rules / Examples".
|
Addressed both findings from the latest Greptile review:
Both lint-skill.sh (0 errors, 0 warnings) and smoke-test-skill.sh (10/10 passed) run clean. |
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