fix(skills): lint-skill.sh no longer masks cross-fence leaks fed via read here-strings

.claude/skills/create-skill/scripts/lint-skill.sh

@@ -76,6 +76,228 @@ collect_assignments() {
     s="${s#*"${BASH_REMATCH[0]}"}"
   done
 }
+# Strips a trailing, unquoted `# comment` from a read command's argument
+# text. A `#` is only a genuine shell comment when it isn't inside an open
+# quoted string (double OR single — a literal `#` inside a single-quoted
+# redirect target, e.g. `<<< '#tag value'`, is data, not a comment marker,
+# and must not truncate away a genuine destination that follows it (#2491
+# Greptile round 4)), so this walks the string quote-span by quote-span —
+# whichever quote character opens first — checking for `#` only in the
+# text BETWEEN quotes, rather than scanning for the first `#` outright
+# (#2491 Greptile round 3: `read -r NUM < f # MAX_LIMIT` left
+# `# MAX_LIMIT` for the destination grep to pick up `MAX_LIMIT` as a
+# spurious destination).
+strip_trailing_comment() {
+  local s="$1" out="" seg q before_dq before_sq
+  while [[ "$s" == *'"'* ]] || [[ "$s" == *"'"* ]]; do
+    before_dq="${s%%\"*}"
+    before_sq="${s%%\'*}"
+    if [[ "$s" == *'"'* ]] && { [[ "$s" != *"'"* ]] || [ "${#before_dq}" -le "${#before_sq}" ]; }; then
+      q='"'
+      seg="$before_dq"
+    else
+      q="'"
+      seg="$before_sq"
+    fi
+    if [[ "$seg" == *'#'* ]]; then
+      printf '%s' "${out}${seg%%#*}"
+      return
+    fi
+    out+="${seg}${q}"
+    s="${s#*"$q"}"
+    seg="${s%%"$q"*}"
+    out+="${seg}${q}"
+    s="${s#*"$q"}"
+  done
+  if [[ "$s" == *'#'* ]]; then
+    printf '%s' "${out}${s%%#*}"
+  else
+    printf '%s' "${out}${s}"
+  fi
+}
+# Consumes EVERY redirection clause (operator + target) from read_args,
+# regardless of how many there are or where they fall among read's other
+# arguments, populating two caller-local outputs via dynamic scoping (the
+# caller must `local` these two names before calling):
+#   READ_DEST_ARGS    — read_args with every redirection clause removed,
+#                        leaving only flags and genuine destinations
+#   READ_INPUT_TARGETS — every redirection's target, space-joined
+# A quoted target (double OR single) is consumed in full regardless of
+# embedded whitespace, so a multiword single-quoted operand like
+# `<<< 'FOO BAZ'` doesn't leave `BAZ'` dangling for the destination grep
+# (#2491 Greptile round 3), and EVERY redirect on the line participates —
+# not just the first — since `read` permits more than one input
+# redirection syntactically and each target could independently reference
+# a stale variable (#2491 Greptile round 3: `read -r FOO < "$CONFIG" <<<
+# "$FOO"` — the later here-string must still be checked for self-reference).
+split_read_redirects() {
+  local args="$1" op op_core after target inner remaining
+  READ_DEST_ARGS="$args"
+  READ_INPUT_TARGETS=""
+  READ_HERE_TARGETS=""
+  while [[ "$READ_DEST_ARGS" =~ (\<{1,3}[[:space:]]*) ]]; do
+    op="${BASH_REMATCH[1]}"
+    op_core="${op%%[[:space:]]*}"
+    after="${READ_DEST_ARGS#*"$op"}"
+    case "$after" in
+      \"*)
+        inner="${after#\"}"
+        inner="${inner%%\"*}"
+        target="\"${inner}\""
+        remaining="${after#*\"}"
+        remaining="${remaining#*\"}"
+        ;;
+      \'*)
+        inner="${after#\'}"
+        inner="${inner%%\'*}"
+        target="'${inner}'"
+        remaining="${after#*\'}"
+        remaining="${remaining#*\'}"
+        ;;
+      *)
+        target="${after%%[[:space:]]*}"
+        if [[ "$after" == *[[:space:]]* ]]; then
+          remaining="${after#*[[:space:]]}"
+        else
+          remaining=""
+        fi
+        ;;
+    esac
+    READ_INPUT_TARGETS+="${target} "
+    # `<<`/`<<<` (heredoc/here-string) feed an IN-MEMORY value, never a
+    # file — tracked separately so has_file_redirect_in's file-persistence
+    # exemption can require $var's reference to come from a genuine
+    # single-`<` target, not from a same-line `<<<` sitting alongside an
+    # unrelated file redirect (#2491 Greptile round 3: `read -r FOO <
+    # "$CONFIG" <<< "$FOO"` — the file redirect on $CONFIG must not exempt
+    # the separate, genuine here-string leak of $FOO).
+    if [ "${#op_core}" -ge 2 ]; then
+      READ_HERE_TARGETS+="${target} "
+    fi
+    READ_DEST_ARGS="${READ_DEST_ARGS%%"$op"*}${remaining}"
+  done
+}
+# Extracts the destination variable name(s) actually bound by a `read ...`
+# command on a line (e.g. `read -r VAR1 VAR2`) — one per output line via
+# stdout — stripping everything that ISN'T a real destination first:
+#   - a trailing command on the same line (`; do`)
+#   - a trailing unquoted comment (`# ...`)
+#   - every input source (`< file`, `<<< "$X"` here-strings), from
+#     anywhere in the argument list, not just a trailing truncation
+#   - 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
+# Shared by collect_assignments's registration pass and the cross-fence
+# reference check's read-exemption (#2491) so both agree on what a `read`
+# line actually binds.
+extract_read_dest_vars() {
+  local line="$1"
+  [[ "$line" =~ (^|[^A-Za-z0-9_])read([[:space:]].*)?$ ]] || return 0
+  local read_args="${BASH_REMATCH[2]}"
+  read_args="${read_args%%;*}"
+  read_args=$(strip_trailing_comment "$read_args")
+  local READ_DEST_ARGS READ_INPUT_TARGETS
+  split_read_redirects "$read_args"
+  read_args="$READ_DEST_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')
+  printf '%s' "$read_args" | grep -oE '(^|[^A-Za-z0-9_$])[A-Z][A-Z0-9_]+' | sed -E 's/^[^A-Za-z0-9_]//'
+}
+# Whether $var is one of the destination variables a `read` on this line
+# actually binds — as opposed to merely appearing somewhere else on the
+# line (e.g. as a here-string/redirection INPUT to that same read, which is
+# a genuine cross-fence reference, not a rebinding).
+is_read_dest_of_line() {
+  local var="$1" line="$2" dest
+  while IFS= read -r dest; do
+    [ "$dest" = "$var" ] && return 0
+  done < <(extract_read_dest_vars "$line")
+  return 1
+}
+# Extracts every TARGET of a `read` line's input redirections, space-joined
+# (e.g. `"$FOO"` for `read -r BAR <<< "$FOO"`), via the same
+# split_read_redirects used by extract_read_dest_vars so both agree on
+# where the redirections are. Empty output if the line has none at all
+# (e.g. `read -p "..." VAR`).
+extract_read_input() {
+  local line="$1"
+  [[ "$line" =~ (^|[^A-Za-z0-9_])read([[:space:]].*)?$ ]] || return 0
+  local read_args="${BASH_REMATCH[2]}"
+  read_args="${read_args%%;*}"
+  local READ_DEST_ARGS READ_INPUT_TARGETS READ_HERE_TARGETS
+  split_read_redirects "$read_args"
+  printf '%s' "$READ_INPUT_TARGETS"
+}
+# Extracts only the HERE-string/heredoc (`<<`, `<<<`) targets of a `read`
+# line, excluding genuine single-`<` file targets — the in-memory subset of
+# extract_read_input, used where a mix of a real file redirect and an
+# in-memory here-string on the SAME line must be told apart (#2491 Greptile
+# round 3).
+extract_read_here_input() {
+  local line="$1"
+  [[ "$line" =~ (^|[^A-Za-z0-9_])read([[:space:]].*)?$ ]] || return 0
+  local read_args="${BASH_REMATCH[2]}"
+  read_args="${read_args%%;*}"
+  local READ_DEST_ARGS READ_INPUT_TARGETS READ_HERE_TARGETS
+  split_read_redirects "$read_args"
+  printf '%s' "$READ_HERE_TARGETS"
+}
+# Whether $var (as $var, not the bare destination name) appears in this
+# line's own `read` input clause — the here-string/redirect operand, not
+# the destination it binds.
+is_read_input_of_line() {
+  local var="$1" line="$2" input
+  input=$(extract_read_input "$line")
+  [ -n "$input" ] && [[ "$input" =~ \$\{?${var}\}?([^A-Za-z0-9_]|$) ]]
+}
+# Whether $var appears specifically within a here-string/heredoc target on
+# this `read` line, as opposed to a genuine single-`<` file target — used
+# to tell apart `read -r FOO < "$CONFIG" <<< "$FOO"`'s two redirects: the
+# first is a real (if itself possibly stale) file path, the second is an
+# in-memory leak of $FOO that a blanket "this line has a file redirect"
+# check must not paper over.
+is_read_here_input_of_line() {
+  local var="$1" line="$2" input
+  input=$(extract_read_here_input "$line")
+  [ -n "$input" ] && [[ "$input" =~ \$\{?${var}\}?([^A-Za-z0-9_]|$) ]]
+}
+# Whether this line both binds $var as a `read` destination AND feeds $var
+# into that same read's own input — a same-line self-reference like
+# `read -r FOO <<< "$FOO"` genuinely leaks the stale $FOO into itself even
+# though FOO is also this line's destination, because the here-string's
+# input is evaluated before the destination is rebound. Every OTHER line in
+# the block that references $var after this one is still legitimately safe
+# (this line does bind a fresh, in-process value) — only this exact line's
+# own reference is a leak (Greptile round 1 on PR #2574 for #2491:
+# is_read_dest_of_line alone only checks whether $var is *a* destination
+# somewhere on the line, not whether the specific $var occurrence that
+# triggered the outer reference check is safe, and the block-wide
+# REASSIGNED exemption doesn't distinguish this line from later ones).
+is_self_referential_read() {
+  is_read_dest_of_line "$1" "$2" && is_read_input_of_line "$1" "$2"
+}
+# Whether $var is a destination this line's `read` binds, WITHOUT also
+# being a same-line self-reference (see is_self_referential_read above).
+is_read_rebind_exempt() {
+  is_read_dest_of_line "$1" "$2" && ! is_self_referential_read "$1" "$2"
+}
+# Whether $line has a genuine single-`<` file-redirection ("< file",
+# "<\"file\"") — as opposed to a `<<<` here-string, which feeds an
+# IN-MEMORY value as input (the opposite of reading from a file) but
+# contains the same `< `/`<"` substrings a blanket check would wrongly
+# treat as file persistence (#2491: `read -r BAR <<< "$FOO"` — $FOO is a
+# genuine cross-fence leak, not a file re-derivation, even though the `<<<`
+# operator's own text contains a trailing `< `).
+has_file_redirect_in() {
+  local line="$1"
+  [[ "$line" =~ (^|[^\<])\<[[:space:]] ]] || [[ "$line" =~ (^|[^\<])\<\" ]]
+}
 while IFS=$'\t' read -r bnum line; do
   # Skip comment lines — they document context but don't register variable assignments
   [[ "$line" =~ ^[[:space:]]*# ]] && continue
@@ -89,31 +311,10 @@ while IFS=$'\t' read -r bnum line; do
   # (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
+  while IFS= read -r var; do
+    [ -z "$var" ] && continue
+    register_var "$var" "$bnum"
+  done < <(extract_read_dest_vars "$line")
 done < "$BLOCKS_FILE"
 
 # Check for references in later blocks without file persistence
@@ -128,11 +329,31 @@ while IFS=$'\t' read -r bnum line; do
         # Narrow check: ensure the $VAR reference isn't followed by [A-Za-z0-9_]
         # (which would mean it's a different, longer variable name)
         if [[ "$line" =~ \$${var}([^A-Za-z0-9_]|$) ]] || [[ "$line" == *'${'"${var}"'}'* ]]; then
-          # Check if the same block also assigns it (re-assignment is fine) — O(1) lookup
-          if [ -z "${REASSIGNED[${var}:${bnum}]+x}" ]; then
-            # Check it's not read from a file (cat, $(...) with cat/read)
-            if [[ "$line" != *'cat '* ]] && [[ "$line" != *'read '* ]] && \
-               [[ "$line" != *'< '* ]] && [[ "$line" != *'<"'* ]] && [[ "$line" != *'$(<'* ]]; then
+          # Check if the same block also assigns it (re-assignment is fine) — O(1) lookup.
+          # A same-line self-referential read (`read -r FOO <<< "$FOO"`) still forces
+          # the deeper check below even though REASSIGNED is set for this block: that
+          # flag correctly protects LATER lines referencing the freshly-bound $var, but
+          # this exact line's own reference predates the rebind and is still a leak.
+          if [ -z "${REASSIGNED[${var}:${bnum}]+x}" ] || is_self_referential_read "$var" "$line"; then
+            # Check it's not read from a file (cat, $(...) with cat/read).
+            # The `read` case checks that $var is actually THIS line's read
+            # destination AND isn't also that same read's own input (a
+            # same-line self-reference like `read -r FOO <<< "$FOO"` still
+            # leaks the stale value into itself), and the redirection case
+            # excludes here-strings (`<<<`) UNLESS $var's own reference is
+            # itself the here-string's target — a line can have a genuine
+            # single-`<` file redirect ALONGSIDE an unrelated `<<<` leak
+            # (`read -r FOO < "$CONFIG" <<< "$FOO"`: the file redirect must
+            # not paper over the separate, genuine leak of $FOO) — all were
+            # blanket substring/whole-line matches that wrongly exempted a
+            # genuine cross-fence leak used as that same line's `read`
+            # INPUT (e.g. `read -r BAR <<< "$FOO"`: $FOO is a stale
+            # in-memory reference, not a file re-derivation, even though
+            # the line contains `read ` and the `<<<` operator's own text
+            # contains a trailing `< `) (#2491).
+            if [[ "$line" != *'cat '* ]] && ! is_read_rebind_exempt "$var" "$line" && \
+               { ! has_file_redirect_in "$line" || is_read_here_input_of_line "$var" "$line"; } && \
+               [[ "$line" != *'$(<'* ]]; then
               error "Cross-fence variable: \$$var assigned in bash block $assigned_in, referenced in block $bnum without file persistence (Pattern 1)"
             fi
           fi