diff --git a/.claude/skills/create-skill/scripts/lint-skill.sh b/.claude/skills/create-skill/scripts/lint-skill.sh index 8bc6847db..9474b6e8c 100644 --- a/.claude/skills/create-skill/scripts/lint-skill.sh +++ b/.claude/skills/create-skill/scripts/lint-skill.sh @@ -52,20 +52,68 @@ awk ' # Collect variable assignments per block and build reassignment lookup (O(1) per check) declare -A VAR_BLOCK declare -A REASSIGNED +register_var() { + local var="$1" bnum="$2" + if [ -z "${VAR_BLOCK[$var]+x}" ]; then + VAR_BLOCK["$var"]="$bnum" + else + # Track re-assignments in later blocks for O(1) lookup + REASSIGNED["${var}:${bnum}"]=1 + fi +} +# Finds every UPPER_CASE_VAR=/VAR+= assignment on a line using pure bash +# builtins ([[ =~ ]] + parameter expansion) rather than forking echo|grep|sed +# per line. This loop runs once per non-comment bash-block line — for a large +# SKILL.md (fixer/SKILL.md is ~850 such lines) a per-line subprocess pipeline +# is a few seconds on macOS/Linux but can blow well past CI's default test +# timeout on Windows, where process creation is ~100x slower (see this +# file's own performance note above; discovered via PR #2490/#2344 once a +# test exercised lint-skill.sh against this specific large file directly). +collect_assignments() { + local s="$1" bnum="$2" + while [[ "$s" =~ (^|[^A-Za-z0-9_])([A-Z][A-Z0-9_]+)\+?= ]]; do + register_var "${BASH_REMATCH[2]}" "$bnum" + s="${s#*"${BASH_REMATCH[0]}"}" + done +} while IFS=$'\t' read -r bnum line; do # Skip comment lines — they document context but don't register variable assignments [[ "$line" =~ ^[[:space:]]*# ]] && continue # Match UPPER_CASE_VAR= assignments (skip lowercase/mixed to reduce false positives) - # Use while-read instead of for-in-$() to avoid empty-string iteration when grep matches nothing - while IFS= read -r var; do - [ -z "$var" ] && continue - if [ -z "${VAR_BLOCK[$var]+x}" ]; then - VAR_BLOCK["$var"]="$bnum" - else - # Track re-assignments in later blocks for O(1) lookup - REASSIGNED["${var}:${bnum}"]=1 - fi - done < <(echo "$line" | grep -oE '\b[A-Z][A-Z0-9_]+\+?=' | sed -E 's/\+?=$//') + collect_assignments "$line" "$bnum" + # `read`/`read -r VAR1 VAR2` binds variables just like VAR=, but with no '=' + # after the name — e.g. `while IFS=$'\t' read -r F COUNT LINE; do`. Without + # this, a var re-derived via `read` in a later block (a legitimate, fresh, + # block-local binding) looks identical to a stale reference to an + # earlier block's same-named variable, producing a false Pattern-1 error + # (issue #2344 — fixer/SKILL.md's own I4 integrity check does exactly this + # with a loop-local $COUNT that collides in name only with the unrelated + # batch-size $COUNT set up in Phase 0). + if [[ "$line" =~ (^|[^A-Za-z0-9_])read([[:space:]].*)?$ ]]; then + read_args="${BASH_REMATCH[2]}" + # Only the destination variable *names* on a `read` line are real + # bindings — strip everything else first so none of it is misread as + # one (Greptile review on PR #2490/#2344): + # - a trailing command on the same line (`; do`) + # - the input source (`< file`, `<<< "$X"` here-strings) + # - quoted option arguments (`-p "prompt text"`, which may contain + # arbitrary uppercase words that aren't destinations at all) + # - a value-taking flag's own argument (`-t 5`, `-u FD`, `-d ':'`, + # `-i "initial text"`, and combined short forms like `-ei FOO`, + # where `-e` takes no argument but the trailing `-i` does — every + # read flag EXCEPT `-a` (whose argument is itself a genuine + # destination: the array read into), so the strip only fires when + # the cluster's LAST letter is one of the other value-taking flags + # - a `$`-prefixed token, which REFERENCES a var rather than binding it + read_args="${read_args%%;*}" + read_args="${read_args%%<*}" + read_args=$(printf '%s' "$read_args" | sed -E 's/"[^"]*"//g' | sed -E "s/'[^']*'//g") + read_args=$(printf '%s' "$read_args" | sed -E 's/-[a-zA-Z]*[ptnNdui][[:space:]]+[^[:space:]]+//g') + while IFS= read -r var; do + [ -z "$var" ] && continue + register_var "$var" "$bnum" + done < <(printf '%s' "$read_args" | grep -oE '(^|[^A-Za-z0-9_$])[A-Z][A-Z0-9_]+' | sed -E 's/^[^A-Za-z0-9_]//') + fi done < "$BLOCKS_FILE" # Check for references in later blocks without file persistence diff --git a/tests/unit/lint-skill-read-binding-2344.test.ts b/tests/unit/lint-skill-read-binding-2344.test.ts new file mode 100644 index 000000000..744eb8faa --- /dev/null +++ b/tests/unit/lint-skill-read-binding-2344.test.ts @@ -0,0 +1,204 @@ +/** + * Regression test for issue #2344: `lint-skill.sh`'s cross-fence variable + * check (Pattern 1) only recognised `VAR=value` as a variable assignment. + * `read`/`read -r VAR1 VAR2 ...` binds variables just as validly, but with + * no `=` after the name — e.g. `while IFS=$'\t' read -r F COUNT LINE; do`. + * + * `fixer/SKILL.md`'s own I4 integrity check does exactly this: a loop-local + * `$COUNT` (the per-line expected occurrence count, bound via `read`) that + * collides in name only with the unrelated batch-size `$COUNT` set up in + * Phase 0. Since each ```bash fence is a separate bash invocation, there is + * no real collision at runtime — but the linter didn't know the later block + * had its own fresh `$COUNT` via `read`, so it flagged the block's own local + * variable as if it were a stale reference leaking from Phase 0. + */ +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = path.resolve(__dirname, '..', '..'); +const LINT_SCRIPT = path.join( + REPO_ROOT, + '.claude', + 'skills', + 'create-skill', + 'scripts', + 'lint-skill.sh', +); + +// lint-skill.sh requires bash 4+ (associative arrays). macOS ships bash 3.2 +// as the `bash` on PATH, including on GitHub Actions' macos-latest runner — +// resolve a real bash 4+ explicitly rather than assuming plain "bash" works. +function resolveBash4(): string | null { + const candidates = ['/opt/homebrew/bin/bash', '/usr/local/bin/bash', 'bash']; + for (const candidate of candidates) { + try { + const version = execFileSync(candidate, ['--version'], { encoding: 'utf8' }); + const match = version.match(/version (\d+)\./); + if (match && Number(match[1]) >= 4) return candidate; + } catch { + // candidate not on PATH — try the next one + } + } + return null; +} + +const BASH4 = resolveBash4(); + +const FRONTMATTER = `--- +name: lint-test-skill +description: test +argument-hint: none +allowed-tools: Bash +--- + +## Phase 0 + +pre-flight +`; + +// Unlike the mktemp test's single-block helper, this bug is inherently +// cross-block: it only reproduces across two or more separate \`\`\`bash +// fences (each its own bash invocation), so the fixture builder takes a +// list of blocks rather than one. +function runLint(bashBlocks: string[]): { stdout: string; ranSuccessfully: boolean } { + const blocksMarkdown = bashBlocks.map((b) => `\`\`\`bash\n${b}\n\`\`\`\n`).join('\n'); + const content = `${FRONTMATTER}\n${blocksMarkdown}\n## Rules\n\nrules here\n\n## Examples\n\nexample here\n\n**Exit condition:** done\n`; + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'lint-skill-read-')); + const skillPath = path.join(dir, 'SKILL.md'); + fs.writeFileSync(skillPath, content); + try { + const result = execFileSync(BASH4!, [LINT_SCRIPT, skillPath], { encoding: 'utf8' }); + return { stdout: result, ranSuccessfully: true }; + } catch (err) { + const e = err as { stdout?: string }; + const stdout = e.stdout ?? ''; + return { stdout, ranSuccessfully: /lint-skill: \d+ error/.test(stdout) }; + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +describe.skipIf(BASH4 === null)( + 'lint-skill.sh recognises `read` as a variable binding, not just VAR= (#2344)', + () => { + it('does not flag a var re-bound via `read` and referenced in the same later block', () => { + const { stdout, ranSuccessfully } = runLint([ + 'COUNT=5\nprintf \'%s\\n\' "$COUNT" > .codegraph/count', + [ + "while IFS=$'\\t' read -r F COUNT LINE; do", + ' if [ "$LINE" != "" ] && [ 0 -lt "$COUNT" ]; then', + ' echo "seen $COUNT for $F"', + ' fi', + 'done < .codegraph/data.tsv', + ].join('\n'), + ]); + expect(ranSuccessfully).toBe(true); + expect(stdout).not.toContain('Cross-fence variable: $COUNT'); + }); + + it('still flags a genuine cross-fence leak (no read-rebinding in the referencing block)', () => { + const { stdout, ranSuccessfully } = runLint(['FOO=hello', 'echo "leak: $FOO"']); + expect(ranSuccessfully).toBe(true); + expect(stdout).toContain( + 'Cross-fence variable: $FOO assigned in bash block 1, referenced in block 2', + ); + }); + + it('still flags a reference in a block whose own `read` binds a different variable', () => { + const { stdout, ranSuccessfully } = runLint([ + 'FOO=hello', + 'read -r OTHER < .codegraph/other\necho "leak: $FOO"', + ]); + expect(ranSuccessfully).toBe(true); + expect(stdout).toContain( + 'Cross-fence variable: $FOO assigned in bash block 1, referenced in block 2', + ); + }); + + // Greptile review on PR #2490: the read-binding scan must not treat every + // uppercase token after `read` as a destination — a prompt string, or a + // value-taking flag's own argument, can contain an unrelated uppercase + // word that is not actually being bound. + it('still flags a leak past a `read -p` prompt string containing an unrelated uppercase word', () => { + const { stdout, ranSuccessfully } = runLint([ + 'FOO=hello', + 'read -p "Enter FOO value: " BAR\necho "leak: $FOO"', + ]); + expect(ranSuccessfully).toBe(true); + expect(stdout).toContain( + 'Cross-fence variable: $FOO assigned in bash block 1, referenced in block 2', + ); + }); + + it('still flags a leak past a value-taking flag argument that is an uppercase word', () => { + const { stdout, ranSuccessfully } = runLint([ + 'FOO=hello', + 'read -u FOO BAR\necho "leak: $FOO"', + ]); + expect(ranSuccessfully).toBe(true); + expect(stdout).toContain( + 'Cross-fence variable: $FOO assigned in bash block 1, referenced in block 2', + ); + }); + + // Greptile round 2 on PR #2490: `-i` (readline initial text) is a + // value-taking flag too, and was missing from the first fix's flag list. + it('still flags a leak past a `read -i` initial-text argument that is an uppercase word', () => { + const { stdout, ranSuccessfully } = runLint([ + 'FOO=hello', + 'read -e -i FOO BAR\necho "leak: $FOO"', + ]); + expect(ranSuccessfully).toBe(true); + expect(stdout).toContain( + 'Cross-fence variable: $FOO assigned in bash block 1, referenced in block 2', + ); + }); + + it('does not flag `-a` (its argument is a genuine array destination, not a value)', () => { + const { stdout, ranSuccessfully } = runLint(['read -a ARR <<< "one two three"']); + expect(ranSuccessfully).toBe(true); + expect(stdout).not.toContain('Cross-fence variable: $ARR'); + }); + + // Greptile round 3 on PR #2490: a value-taking flag combined into a + // multi-letter cluster (e.g. `-ei`, where `-e` takes no argument but + // the trailing `-i` does) wasn't recognized — only a standalone + // single-letter flag was. + it('still flags a leak past a combined short-flag cluster ending in a value-taking flag', () => { + const { stdout, ranSuccessfully } = runLint([ + 'FOO=hello', + 'read -ei FOO BAR\necho "leak: $FOO"', + ]); + expect(ranSuccessfully).toBe(true); + expect(stdout).toContain( + 'Cross-fence variable: $FOO assigned in bash block 1, referenced in block 2', + ); + }); + + it('does not flag a combined cluster ending in `-a` (still a genuine array destination)', () => { + const { stdout, ranSuccessfully } = runLint(['read -ra ARR <<< "one two three"']); + expect(ranSuccessfully).toBe(true); + expect(stdout).not.toContain('Cross-fence variable: $ARR'); + }); + }, +); + +describe.skipIf(BASH4 === null)('.claude/skills/fixer/SKILL.md itself (#2344 repro)', () => { + // fixer/SKILL.md is by far the largest SKILL.md in the repo (~1600 lines, + // ~1000 non-comment bash-block lines), and Check 1 forks a handful of + // subprocesses per line. That's a few seconds on macOS/Linux but can blow + // past the 30s default testTimeout on Windows CI, where process creation + // is ~100x slower (see this script's own header comment) — this is the + // first test to run lint-skill.sh against this specific file directly, so + // it's the first to hit that pre-existing characteristic head-on. + it('passes lint-skill.sh with no Cross-fence $COUNT error', { timeout: 120_000 }, () => { + const realSkillPath = path.join(REPO_ROOT, '.claude', 'skills', 'fixer', 'SKILL.md'); + const stdout = execFileSync(BASH4!, [LINT_SCRIPT, realSkillPath], { encoding: 'utf8' }); + expect(stdout).not.toContain('Cross-fence variable: $COUNT'); + }); +});