fix(skills): lint-skill.sh no longer masks cross-fence leaks fed via read here-strings - #2574
Merged
Conversation
…read here-strings
Two blanket substring exemptions in the Pattern-1 cross-fence check ('read '
and '< ') let a stale variable slip through undetected when it appeared as
the INPUT to a `read` here-string (e.g. `read -r BAR <<< "$FOO"`) rather than
as its destination — the 'read ' exemption didn't check which variable was
actually being bound, and the '< ' exemption's blanket match also matched
`<<<`'s own trailing '< ' substring.
Replaces both with is_read_dest_of_line (checks the line's actual read
destination via a new shared extract_read_dest_vars helper) and
has_file_redirect_in (a regex that excludes << and <<< from genuine
single-< file redirection).
Closes #2491
Contributor
Contributor
Greptile SummaryThe PR refines Pattern-1 parsing of
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (5): Last reviewed commit: "fix(skills): lint-skill.sh comment strip..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… read Greptile round 1 on PR #2574: is_read_dest_of_line only checked whether $var was *a* destination somewhere on the line, so `read -r FOO <<< "$FOO"` (destination and stale here-string input sharing the same name) still slipped through — both via the deeper per-reference exemption and via the block-wide REASSIGNED short-circuit, which correctly protects later lines referencing the freshly self-bound value but doesn't distinguish those from this exact line's own pre-rebind reference. Adds is_read_input_of_line/is_self_referential_read and forces the deeper check even when REASSIGNED is set, for this one self-referential line only.
Contributor
Author
…its redirection Greptile round 2 on PR #2574: extract_read_dest_vars truncated read_args at the FIRST '<', discarding any destination that came after the redirection. `read -r <<< "hello" FOO` is valid bash (FOO really is bound), so a later, legitimate reference to the freshly-read value was falsely reported as a stale cross-fence leak, since REASSIGNED never got set for that block. Replaces the truncation with a shared READ_REDIRECT_RE that removes the redirection clause from anywhere in the args (not just a trailing cut), used by both extract_read_dest_vars (destination extraction) and extract_read_input (redirect-target capture, previously "everything after the first '<'", now precisely just the clause's own target).
Contributor
Author
… redirects on read Greptile round 3 on PR #2574 found three further gaps in the generalized redirect handling, each confirmed against real bash before fixing: - A trailing unquoted `# comment` after a redirect's bare-word target was left dangling for the destination grep, so an uppercase comment word (`read -r NUM < f # MAX_LIMIT`) got mistaken for a real destination. - A multiword single-quoted redirect target (`<<< 'FOO BAZ'`) only had its first word consumed by the bare-word match, leaving the rest to leak into destination extraction. - Only the first of multiple redirects on one line participated in self-reference detection, so a genuine file redirect could paper over an unrelated `<<<` leak later on the same line (`read -r FOO < "$CONFIG" <<< "$FOO"`). Adds strip_trailing_comment (quote-aware, so a `#` inside a still-open double-quoted string isn't mistaken for a comment) and rewrites split_read_redirects as a proper tokenizer that consumes every redirect clause on the line (quoted targets in full, regardless of embedded whitespace) instead of a single regex substitution, tracking here-string targets separately from file targets so a genuine file redirect no longer blanket-exempts a different, unrelated here-string leak on the same line.
Contributor
Author
…es too Greptile round 4 on PR #2574: strip_trailing_comment only alternated on double-quoted spans, so a literal '#' inside a single-quoted redirect target (read -r <<< '#tag value' FOO EXTRA is valid bash - EXTRA really is a destination) was mistaken for a genuine comment marker, truncating away any destination that followed it and causing a false cross-fence error on a later, legitimate reference. Generalizes the walk to alternate on whichever quote character opens first, matching how split_read_redirects already handles both quote kinds for target extraction.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lint-skill.sh's Pattern-1 cross-fence variable check exempted any line containing the substringreadfrom being flagged, without checking whether the variable in question was actually the destinationreadbound — so a stale variable fed as the input to areadhere-string (read -r BAR <<< "$FOO") slipped through undetected.<substring match, meant to detect genuine file redirection) made this worse:<<<'s own text contains a trailing<, so even narrowing thereadexemption alone wouldn't have caught the exact repro.extract_read_dest_vars/is_read_dest_of_line(checks whether$varis genuinely the destination areadline binds) andhas_file_redirect_in(a regex distinguishing a real single-<file redirect from<</<<<), replacing both blanket substring checks.Test plan
tests/unit/lint-skill-here-string-2491.test.ts: exact issue repro + here-string-no-read case fail before the fix and pass after (revert-verified against the pre-fix script).read-binding exemption (follow-up: lint-skill.sh flags a cross-fence $COUNT violation in fixer/SKILL.md (possible false positive) #2344) and genuine file-redirection exemption still pass — no regressions.SKILL.mdfiles in the repo — output identical to before the fix (zero new false positives).npm test(full suite, 92 files / 1929 tests) andnpm run lintboth pass.Closes #2491