fix(lint): lint-skill.sh recognizes read as a variable binding, not just VAR=

.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

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');
+  });
+});