From e2080b2ff7438f56389bbb55bd58677e3d01df60 Mon Sep 17 00:00:00 2001 From: Deepak Jain Date: Thu, 24 Sep 2026 16:02:49 -0700 Subject: [PATCH 1/3] fix(analyzer): preserve parsed shell assignment quote ownership Signed-off-by: Deepak Jain --- .../analyzers/static_patterns_tool_misuse.py | 18 +++++++- .../analyzers/test_shell_assignment_quotes.py | 41 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 tests/nodes/analyzers/test_shell_assignment_quotes.py diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index eb7fc60d5..7ae3a331b 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -1421,6 +1421,7 @@ def _parse_shell_command_word( backtick_end_cache: dict[int, int | None] | None = None, *, check_runtime: Callable[[], None] | None = None, + owned_word_positions: set[int] | None = None, ) -> _ShellCommandWord | None: runtime_check = check_runtime or (lambda: None) output: list[str] = [] @@ -1437,6 +1438,8 @@ def _parse_shell_command_word( character = content[cursor] if quote is not None: if character == quote: + if owned_word_positions is not None: + owned_word_positions.add(cursor) quote = None ansi_c_quote = False elif character == "\\" and ansi_c_quote: @@ -1450,6 +1453,8 @@ def _parse_shell_command_word( output.append(decoded) continue elif quote == '"' and character == "$" and cursor + 1 < limit: + if owned_word_positions is not None: + owned_word_positions.add(cursor) inherited_quote_closed = [False] if content[cursor + 1] == "(": substitution_end = _skip_command_substitution( @@ -1726,6 +1731,9 @@ def _has_shell_command_word_exhaustion( ) -> bool: """Find candidate command words whose deterministic parse hit a safety bound.""" parsed_through = 0 + # Completed words own their closing quotes and quoted expansion starts. + # Inner commands remain independent candidates; never suppress their bodies. + owned_word_positions: set[int] = set() parameter_end_cache: dict[int, _ParameterExpansionEnd] = {} substitution_end_cache: dict[int, int | None] = {} backtick_end_cache: dict[int, int | None] = {} @@ -1734,17 +1742,23 @@ def _has_shell_command_word_exhaustion( for candidate in _SHELL_COMMAND_WORD_START_RE.finditer(content): check_runtime() start = candidate.start() + if start in owned_word_positions: + continue if structural_quote_closers is not None and start in structural_quote_closers: continue json_string_start = structural_quote_openers is not None and ( start in structural_quote_openers or start - 1 in structural_quote_openers ) + assignment_quote = start > 0 and content[start - 1] == "=" and content[start] in "'\"" if start < parsed_through or ( - not json_string_start and not _is_shell_command_word_start(content, start) + not json_string_start + and not assignment_quote + and not _is_shell_command_word_start(content, start) ): continue if _has_quoted_assignment_prefix(content, start): continue + candidate_word_positions: set[int] = set() parsed = _parse_shell_command_word( content, start, @@ -1752,6 +1766,7 @@ def _has_shell_command_word_exhaustion( substitution_end_cache, backtick_end_cache, check_runtime=check_runtime, + owned_word_positions=candidate_word_positions, ) if parsed is None: substitution_start = ( @@ -1789,6 +1804,7 @@ def _has_shell_command_word_exhaustion( if unresolved_end - start > _SHELL_COMMAND_WORD_CHARS: return True continue + owned_word_positions.update(candidate_word_positions) # Only executable nested substitutions retain independent command # positions. A plain dynamic data argument still owns its inner bytes; # revisiting those as commands would turn quoted printf data into code. diff --git a/tests/nodes/analyzers/test_shell_assignment_quotes.py b/tests/nodes/analyzers/test_shell_assignment_quotes.py new file mode 100644 index 000000000..fc3dce379 --- /dev/null +++ b/tests/nodes/analyzers/test_shell_assignment_quotes.py @@ -0,0 +1,41 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +import pytest + +from skillspector.nodes.analyzers.static_patterns_tool_misuse import has_bounded_parse_exhaustion + + +@pytest.mark.parametrize( + "source", + [ + 'out="$(date)"', + 'v="a (b)"', + 'v="|$a|"', + 'printf "%s" "$(f "$x")"', + '[ "$(f)" = true ]', + 'cat <<< "$(f)"', + 'v="a \\" (b)"', + 'v="a \\" |$a|"', + ], +) +def test_complete_shell_quotes_do_not_consume_following_padding(source: str) -> None: + assert not has_bounded_parse_exhaustion(source + "\n" + "#" * 6000 + "\n", lambda: None) + + +@pytest.mark.parametrize( + "source", ['x="$(unterminated', 'x="unclosed', '"$(printf "%s" "$x")" -rf /'] +) +def test_unresolved_shell_words_still_exhaust(source: str) -> None: + assert has_bounded_parse_exhaustion(source + "\n" + "#" * 6000 + "\n", lambda: None) + + +def test_assignment_ownership_retains_nested_command_budget() -> None: + nested_word = "r" * 4097 + source = f'out="$({nested_word} -rf /)"' + assert has_bounded_parse_exhaustion(source, lambda: None) + + +def test_failed_word_parse_does_not_claim_following_command() -> None: + source = 'Test-Path "$($_.FullName)\\cli-path"; $($CMD) -rf /' + assert has_bounded_parse_exhaustion(source, lambda: None) From 20bb62d7864d5b5dc44617b485a25a7fa8028cbf Mon Sep 17 00:00:00 2001 From: Deepak Jain Date: Mon, 28 Sep 2026 14:49:03 -0700 Subject: [PATCH 2/3] fix(parser): retain quoted runtime command checks Signed-off-by: Deepak Jain --- .../analyzers/static_patterns_tool_misuse.py | 11 +++++--- .../analyzers/test_shell_assignment_quotes.py | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index 7ae3a331b..9854d2271 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -1425,6 +1425,7 @@ def _parse_shell_command_word( ) -> _ShellCommandWord | None: runtime_check = check_runtime or (lambda: None) output: list[str] = [] + wrapper_quote = _command_wrapper_quote(content, start) if content[start] in "$`" else None quote: str | None = None ansi_c_quote = False dynamic = False @@ -1453,8 +1454,6 @@ def _parse_shell_command_word( output.append(decoded) continue elif quote == '"' and character == "$" and cursor + 1 < limit: - if owned_word_positions is not None: - owned_word_positions.add(cursor) inherited_quote_closed = [False] if content[cursor + 1] == "(": substitution_end = _skip_command_substitution( @@ -1635,6 +1634,8 @@ def _parse_shell_command_word( cursor = substitution_end continue elif character in "'\"": + if character == wrapper_quote: + break quote = character elif character == "\\" and cursor + 1 < limit: if content[cursor + 1] == "\n": @@ -1731,7 +1732,7 @@ def _has_shell_command_word_exhaustion( ) -> bool: """Find candidate command words whose deterministic parse hit a safety bound.""" parsed_through = 0 - # Completed words own their closing quotes and quoted expansion starts. + # Completed words own closing quotes, never executable expansion starts. # Inner commands remain independent candidates; never suppress their bodies. owned_word_positions: set[int] = set() parameter_end_cache: dict[int, _ParameterExpansionEnd] = {} @@ -1804,7 +1805,9 @@ def _has_shell_command_word_exhaustion( if unresolved_end - start > _SHELL_COMMAND_WORD_CHARS: return True continue - owned_word_positions.update(candidate_word_positions) + line_start = content.rfind("\n", 0, start) + 1 + if not content[line_start:start].lstrip().startswith("#"): + owned_word_positions.update(candidate_word_positions) # Only executable nested substitutions retain independent command # positions. A plain dynamic data argument still owns its inner bytes; # revisiting those as commands would turn quoted printf data into code. diff --git a/tests/nodes/analyzers/test_shell_assignment_quotes.py b/tests/nodes/analyzers/test_shell_assignment_quotes.py index fc3dce379..1847d00b8 100644 --- a/tests/nodes/analyzers/test_shell_assignment_quotes.py +++ b/tests/nodes/analyzers/test_shell_assignment_quotes.py @@ -39,3 +39,28 @@ def test_assignment_ownership_retains_nested_command_budget() -> None: def test_failed_word_parse_does_not_claim_following_command() -> None: source = 'Test-Path "$($_.FullName)\\cli-path"; $($CMD) -rf /' assert has_bounded_parse_exhaustion(source, lambda: None) + + +@pytest.mark.parametrize( + "source", ['Run "$(resolve_tool) -rf /"', '# see "notes\n$(resolve_tool) -rf /\n# "'] +) +def test_quoted_runtime_commands_keep_fail_closed_coverage(source: str) -> None: + from skillspector.inspection_ledger import LedgerOutcome, LedgerReason + from skillspector.nodes.analyzers import static_patterns_tool_misuse, static_runner + + assert has_bounded_parse_exhaustion(source, lambda: None) + path = "script.sh" if source.startswith("#") else "SKILL.md" + result = static_runner.run_static_patterns_with_ledger( + {"components": [path], "local_file_cache": {path: source}, "file_cache": {path: source}}, + [static_patterns_tool_misuse], + ) + assert any( + e["outcome"] == LedgerOutcome.PARTIAL + and e["reason_code"] == LedgerReason.STATIC_PARSE_LIMIT + for e in result["inspection_ledger"] + ) + + +def test_non_shell_unclosed_assignment_remains_conservative() -> None: + source = 'name="value' + "\n" + "text " * 1200 + assert has_bounded_parse_exhaustion(source, lambda: None) From 11d1629cc394c9fd67c108fdb3021e41047cb70b Mon Sep 17 00:00:00 2001 From: Narendran Raghavan Date: Tue, 29 Sep 2026 12:50:18 -0700 Subject: [PATCH 3/3] fix(parser): confine mis-paired quote ownership and assignment values Keep review finding 1 closed for trailing comments and prose: a word that starts in a comment, or at a quote after `=`, can claim quotes only on its own line, and a quote that opens a runtime word (`"$(...)`, `"${...}`, `` "`...` ``) is never claimed. Before this, `ls # see "notes` could hide a following `"$(resolve_tool)" -rf /` that main reports as partial. Let comment-origin words claim same-line closing quotes, so the #628 comment form `# use "$(cmd)" here` no longer consumes following padding. A quote after `=` names an assignment value, never a command, and main never parsed that position. It now only claims ownership, which restores main's result for `x="$(printf '%s' "$X")"`. When a backtick or quote directly encloses the assignment, as in a documented `` `X="${X:-default}"` ``, the word stops at that delimiter and owns the closing backtick instead of opening a new substitution that runs to the end of the file. Add the reviewer-requested coverage: non-shell file types, a Markdown fence, the #628 comment form and an end-to-end #628 bundle, plus regressions for the trailing-comment, prose and host-language ownership cases. Signed-off-by: Narendran Raghavan Co-Authored-By: Claude Opus 5.5 --- .../analyzers/static_patterns_tool_misuse.py | 53 +++++++++-- .../analyzers/test_shell_assignment_quotes.py | 88 ++++++++++++++++++- 2 files changed, 134 insertions(+), 7 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index 9854d2271..e5240a58e 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -66,6 +66,9 @@ _RUNTIME_SHELL_PARAMETER_SENTINEL = "\ue002" _SIMPLE_BRACED_PARAMETER_RE = re.compile(r"\$\{(?:[A-Za-z_][A-Za-z0-9_]*|[0-9]+|[@*#?$!-])\}") _SHELL_DELIMITER_WORD_RE = re.compile(r"[^$'\"`\\(){}<>#;|&!\s]++") +# A ``#`` that starts a word begins a shell comment. Quoting is not tracked, so +# ``echo "a # b"`` also matches; callers only use this to restrict ownership. +_SHELL_COMMENT_START_RE = re.compile(r"(?])#") _PERL_LITERAL_PRINT_RE = re.compile( r"^[ \t]*+print\b(?:[ \t]++(?:STDOUT|STDERR)\b)?[ \t]*+(?P\()?[ \t]*+" r"(?P\"(?:\\[\\\"'nrt]|[^\\\"$@`\r\n])*+\"" @@ -1422,10 +1425,13 @@ def _parse_shell_command_word( *, check_runtime: Callable[[], None] | None = None, owned_word_positions: set[int] | None = None, + enclosing_delimiter: str | None = None, ) -> _ShellCommandWord | None: runtime_check = check_runtime or (lambda: None) output: list[str] = [] - wrapper_quote = _command_wrapper_quote(content, start) if content[start] in "$`" else None + wrapper_quote = enclosing_delimiter or ( + _command_wrapper_quote(content, start) if content[start] in "$`" else None + ) quote: str | None = None ansi_c_quote = False dynamic = False @@ -1602,6 +1608,11 @@ def _parse_shell_command_word( cursor = parameter_end continue elif character == "`": + if enclosing_delimiter == "`": + # The enclosing substitution ends here; this is not an opener. + if owned_word_positions is not None: + owned_word_positions.add(cursor) + break substitution_end = _skip_backtick_substitution( content, cursor, @@ -1768,6 +1779,9 @@ def _has_shell_command_word_exhaustion( backtick_end_cache, check_runtime=check_runtime, owned_word_positions=candidate_word_positions, + enclosing_delimiter=( + _assignment_value_wrapper(content, start) if assignment_quote else None + ), ) if parsed is None: substitution_start = ( @@ -1805,9 +1819,21 @@ def _has_shell_command_word_exhaustion( if unresolved_end - start > _SHELL_COMMAND_WORD_CHARS: return True continue - line_start = content.rfind("\n", 0, start) + 1 - if not content[line_start:start].lstrip().startswith("#"): - owned_word_positions.update(candidate_word_positions) + # Comments end at a newline, and a quote after ``=`` in prose or host + # source may be mis-paired. Such a word cannot own a later line. + confined_word = "\n" in content[start : parsed.end] and ( + assignment_quote + or _SHELL_COMMENT_START_RE.search(content, content.rfind("\n", 0, start) + 1, start) + is not None + ) + if not confined_word: + # A quote that opens a runtime-selected word stays an independent + # candidate, so a mis-paired claim cannot hide its operands. + owned_word_positions.update( + position + for position in candidate_word_positions + if content[position + 1 : position + 2] not in ("$", "`") + ) # Only executable nested substitutions retain independent command # positions. A plain dynamic data argument still owns its inner bytes; # revisiting those as commands would turn quoted printf data into code. @@ -1824,8 +1850,12 @@ def _has_shell_command_word_exhaustion( or "$" not in raw_word or not any(marker in raw_word for marker in ("$(", "`")) or simple_backtick_parameter - ): + ) and not (assignment_quote and confined_word): parsed_through = max(parsed_through, parsed.end) + if assignment_quote: + # An assignment value never names the command, and main never + # parsed this position, so it only claims ownership. + continue if parsed.limited: return True if parsed.dynamic and may_have_destructive_outer_operands: @@ -2108,6 +2138,19 @@ def _command_wrapper_quote(content: str, command_start: int) -> str | None: return content[command_start - 1] if backslashes % 2 == 0 else None +def _assignment_value_wrapper(content: str, value_start: int) -> str | None: + """Return an unescaped quote or backtick directly enclosing an assignment.""" + name_start = value_start - 1 + if name_start > 0 and content[name_start - 1] == "+": + name_start -= 1 + while name_start > 0 and ( + content[name_start - 1] == "_" + or (content[name_start - 1].isascii() and content[name_start - 1].isalnum()) + ): + name_start -= 1 + return _command_wrapper_quote(content, name_start) + + def _perl_literal_print_shell_text( content: str, check_runtime: Callable[[], None], diff --git a/tests/nodes/analyzers/test_shell_assignment_quotes.py b/tests/nodes/analyzers/test_shell_assignment_quotes.py index 1847d00b8..ef664f9b8 100644 --- a/tests/nodes/analyzers/test_shell_assignment_quotes.py +++ b/tests/nodes/analyzers/test_shell_assignment_quotes.py @@ -1,10 +1,18 @@ # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 +import json +from pathlib import Path + import pytest +from typer.testing import CliRunner +from skillspector.cli import app from skillspector.nodes.analyzers.static_patterns_tool_misuse import has_bounded_parse_exhaustion +_COMMENT_PADDING = "\n" + "#" * 6000 + "\n" +_APOSTROPHE_PADDING = "\n" + "y = 'b'\n" * 800 + @pytest.mark.parametrize( "source", @@ -61,6 +69,82 @@ def test_quoted_runtime_commands_keep_fail_closed_coverage(source: str) -> None: ) -def test_non_shell_unclosed_assignment_remains_conservative() -> None: +@pytest.mark.parametrize("file_type", ["shell", "markdown", "python"]) +def test_non_shell_unclosed_assignment_remains_conservative(file_type: str) -> None: source = 'name="value' + "\n" + "text " * 1200 - assert has_bounded_parse_exhaustion(source, lambda: None) + assert has_bounded_parse_exhaustion(source, lambda: None, file_type=file_type) + + +@pytest.mark.parametrize( + ("source", "file_type"), + [ + ('# use "$(cmd)" here', "shell"), + ('```bash\nout="$(date)"\n```', "markdown"), + ('x="$(printf \'%s\' "$X")"', "shell"), + ("model_count=\"$(printf '%s\\n' \"$j\" | jq -r 'length')\"", "shell"), + ], + ids=["issue-comment-form", "markdown-fence", "printf-value", "printf-pipeline-value"], +) +def test_benign_quoted_values_do_not_consume_comment_padding(source: str, file_type: str) -> None: + assert not has_bounded_parse_exhaustion( + source + _COMMENT_PADDING, lambda: None, file_type=file_type + ) + + +@pytest.mark.parametrize( + ("source", "file_type"), + [ + ('# Keep `X="${X}"` so X\'s value wins.', "python"), + ("# Keep `X='${X}'`, so X's value wins.", "python"), + ('Use `X="${X}"`. That\'s the self-reference.', "shell"), + ], + ids=["double-quoted", "single-quoted", "prose"], +) +def test_backtick_enclosed_assignment_stops_at_its_closing_backtick( + source: str, file_type: str +) -> None: + assert not has_bounded_parse_exhaustion( + source + _APOSTROPHE_PADDING, lambda: None, file_type=file_type + ) + + +@pytest.mark.parametrize( + ("source", "file_type"), + [ + ('ls # see "notes\n"$(resolve_tool)" -rf /\n# "', "shell"), + ('Set name="value in prose.\n\n```bash\n"$(resolve_tool)" -rf /\n```\n"\n', "markdown"), + ('x = f(name="value)\n"$(resolve_tool)" -rf /\n"\n', "python"), + ('Set name="value\n$T -rf /\n"\n', "markdown"), + ('name="a "$(resolve_tool)" -rf /"', "shell"), + ], + ids=[ + "trailing-comment", + "prose-assignment-before-fence", + "host-language-assignment", + "prose-assignment-parameter", + "same-line-runtime-opener", + ], +) +def test_misaligned_quote_claims_keep_runtime_commands_partial(source: str, file_type: str) -> None: + assert has_bounded_parse_exhaustion(source, lambda: None, file_type=file_type) + + +def test_issue_628_bundle_reports_complete_coverage(tmp_path: Path) -> None: + (tmp_path / "SKILL.md").write_text( + "---\nname: repro\ndescription: Minimal repro.\n---\nRun `scripts/run.sh`.\n", + encoding="utf-8", + ) + padding = "".join(f"# padding line {index} " + "." * 50 + "\n" for index in range(90)) + (tmp_path / "scripts").mkdir() + (tmp_path / "scripts" / "run.sh").write_text( + '#!/bin/bash\nout="$(date)"\necho "$out"\n' + padding, encoding="utf-8" + ) + + result = CliRunner().invoke(app, ["scan", str(tmp_path), "--format", "json", "--no-llm"]) + + # Exit 1 is the documented high-risk verdict, not a scan execution failure. + assert result.exit_code in {0, 1}, result.output + report = json.loads(result.output) + assert report["analysis_completeness"]["status"] == "complete" + assert report["analysis_completeness"]["ledger_exceptions"] == [] + assert not any(issue["id"] == "AE1" for issue in report["issues"])