[udsy19]

Fixes #857.

Impact: silent-wrong-result

Who reaches this: any skillspector scan (CLI or MCP scan_skill) of a skill that bundles a Python script, through the static_patterns_tool_misuse analyzer's TM1 rule.

Triggered when: a script passes shell=True to a subprocess.* or Popen call, and an earlier argument contains a ). For example, subprocess.run(' '.join(sys.argv[1:]), shell=True). The direct patterns match the arguments with [^)]*, so the match stops at the first ).

What the user observes: no TM1 finding. A skill whose only risky script is that line gets score: 16, recommendation: SAFE. The same call with the command in a variable first gets TM1 HIGH and CAUTION (score 45).

Fix

Swap [^)]* for a pattern that consumes balanced parentheses up to two levels deep. An unmatched ) still closes the call, so a shell=True passed to a later call is still not attributed to the subprocess call. Every alternative starts on a different character, so matching stays linear: 200k characters of pathological input take about 30 ms.

One side effect: subprocess.run(wrap(shell=True)) used to match, because the old pattern never stopped at (. It no longer does, since that shell=True is an argument of wrap.

Tests

New file tests/nodes/analyzers/test_tm1_nested_call_arguments.py. It covers six positive shapes: join, format, qualified and bare Popen, $(...), and a multi-line call. It also has two negatives: shell=True in a later call, and shell=False.

Negative control: reverted static_patterns_tool_misuse.py to main and kept the new test:

FAILED tests/nodes/analyzers/test_tm1_nested_call_arguments.py::test_shell_true_after_nested_call_argument_is_detected[join_argv]
FAILED tests/nodes/analyzers/test_tm1_nested_call_arguments.py::test_shell_true_after_nested_call_argument_is_detected[format]
FAILED tests/nodes/analyzers/test_tm1_nested_call_arguments.py::test_shell_true_after_nested_call_argument_is_detected[qualified_popen]
FAILED tests/nodes/analyzers/test_tm1_nested_call_arguments.py::test_shell_true_after_nested_call_argument_is_detected[bare_popen]
FAILED tests/nodes/analyzers/test_tm1_nested_call_arguments.py::test_shell_true_after_nested_call_argument_is_detected[command_substitution]
FAILED tests/nodes/analyzers/test_tm1_nested_call_arguments.py::test_shell_true_after_nested_call_argument_is_detected[multiline]
6 failed in 2.45s

With the fix: 6 passed. All existing test files that use shell=True or exercise TM1 still pass (3368 + 706 tests). ruff check and ruff format --check are clean.

Signed-off-by: Udaya Tejas udayatejas2004@gmail.com

[MohammedAlkindi]

Requiring balanced parentheses loses a case the old [^)]* caught: an unbalanced paren inside a string literal. On this branch subprocess.run("echo :(", shell=True) and subprocess.run("grep -E '(foo|bar' log", shell=True) give no TM1 finding, while main reports one HIGH each. Letting the prefix consume quoted strings as a single token kept both, plus the new parametrized cases, in a standalone regex check.

[rng1995]

[SkillSpector Review]

Value and readiness

The nested-call cases improve coverage, and the prior quoted-parenthesis finding has been addressed with quote tokens and tests. The replacement still changes previously detected valid Python into a miss.

Material findings

1. Required: consume Python comments before balancing parentheses. src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:256-258 recognizes single-line strings but treats an opening parenthesis in a comment as a real nested call. For subprocess.run(cmd, # ( shell=True), the comment parenthesis remains unclosed when the shell keyword is reached, so the new direct patterns cannot produce the previous TM1 match. The old [^)]* accepted that prefix. This is valid Python and can carry any runtime command. The companion AST analyzer does not rescue constant True: its _inspect_call only emits for ast.Name shell values, while direct-literal metadata only enriches retained lexical findings. Tokenize comments (and multiline Python string literals) without interpreting their parentheses, and add a production-node must-flag regression for this example.

PIC tradeoffs

The two-level nesting bound is an explicit scope choice; losing direct shell=True because of inert comment punctuation is a separate regression. Keep the existing outside-call negative control while extending token ownership.

Verification and gaps

Reviewed the complete diff, description, linked issue material, all previous reviews/comments/replies and pinned-head surrounding source at 6bbbf3442648a30661e6de92992a6e70401ec0b4. Verified the previous unbalanced single-line literal blocker is addressed in this head. Traced direct candidate construction, retained-finding enrichment and the companion AST constant/name split; reviewed every new positive and negative case. GitHub reports changes: SUCCESS, lint: SUCCESS, test-unit: SUCCESS, OpenCode TypeScript Tests: SUCCESS, DCO Check: SUCCESS, docker-smoke: SUCCESS. No contributor code, tests, imports, build steps or scanner invocations were executed during this review.

[rng1995]

[SkillSpector Review]

Value and readiness

The reported comment-parenthesis and ordinary triple-quoted-string cases are fixed. Escaped physical newlines inside valid quoted strings still cause the balancing replacement to lose the direct shell=True signal.

Material findings

Prior probes resolved: comments now consume their full physical line, and terminated ordinary triple-quoted literals consume internal parentheses. Required remaining boundary fix — src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:258: the quoted escape alternative uses \\., whose dot does not match LF under the actual matcher flags. This valid Python is therefore still missed:

subprocess.run("""echo \
:(""", shell=True)

The triple-string token fails at backslash-newline; the shorter token fallback then treats its internal ( as nesting. The old direct pattern matches this call, while the current replacement does not. Make quoted escape consumption handle line continuations (including in triple strings), and add this must-flag production-node regression. Keep the outside-call negative control.

PIC tradeoffs

The two-level nested-call bound remains the accepted scope. Literal escaped-newline syntax should not consume that bound or hide a direct shell=True keyword.

Verification and gaps

Reviewed exact head 89856370a3f8ca23b1e522adfff64f67af0d9ba5 against 6bbbf3442648a30661e6de92992a6e70401ec0b4: one author commit, 15 production-line changes and five test-line additions. Read the complete earlier assessment/diff, current token and candidate construction, all tests and current discussion. A reviewer-authored literal transcription using Python 3.12's standard regex engine confirms that the comment and plain-triple controls now match, while the escaped-LF input parses as Python and remains old=True/current=False. No contributor module, tests, builds or scanner code was executed. Hosted lint, changes, TypeScript, DCO and Docker succeeded; test-unit is still running.

[rng1995]

[SkillSpector Review]

Value and readiness

The remaining escaped-newline tokenization issue is fixed. The bounded balancing change now retains the intended direct shell=True signal and is ready for maintainer/PIC final review.

Material findings

All prior findings resolved: comments consume parentheses without opening nesting; terminated triple strings retain their contents as one token; quoted escapes now use \s/\S coverage instead of a dot, so escaped LF is retained inside both ordinary and triple strings. The production-node tests now require a HIGH TM1 finding for the exact prior escaped-LF probe and ordinary-string variant, alongside the nested-call/comment/triple-string controls. Later-call shell=True and shell=False negatives remain. No further required changes found within the accepted two-level balancing scope.

PIC tradeoffs

The existing two-level nested-call bound remains the deliberate scope. This change does not claim arbitrary-depth Python parsing.

Verification and gaps

Read the complete two-file current diff, pinned surrounding candidate/ownership paths and all tests, prior marked reviews and linked issue #857. All six hosted checks succeeded on the assessed head. Reviewed source and hosted CI without running contributor modules, tests, imports, builds or reproduction snippets.

Reviewed head: b1fc3169622e53a6d886517cbfea535b8631fdf5.

[rng1995]

[SkillSpector Review]

Value and readiness

This corrects my approval earlier in this review pass. The previous LF, comment and ordinary triple-string findings are resolved, but a final independently checked CRLF boundary still loses the direct shell=True signal. This head requires a further correction and must remain unmerged.

Material findings

Required — src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:261. The ordinary quoted-string escape \\[\s\S] consumes backslash plus CR but leaves the LF in a valid Windows backslash-CRLF continuation. The ordinary string body excludes LF and its closing quote is optional, so it publishes a truncated token there. Take the existing ordinary-string LF regression and change only its physical line ending to CRLF: the source has subprocess.run("echo followed by backslash+CRLF and then :(", shell=True). The literal ( after the continuation is then treated as nested-call syntax, and the following quote token consumes the shell=True tail. No direct candidate reaches the keyword, whereas the old direct pattern retained it.

This is not recovered by other paths: byte decoding preserves CRLF; the ASCII security-view fast path keeps the raw text; the AST truthiness analyzer emits only bound names, not literal True; metadata/replay require a retained owner. Consume CRLF as a complete escaped physical newline before the generic escaped-character alternative, and add the exact production-node CRLF variant alongside the now-passing LF control. Retain later-call and shell=False negatives.

PIC tradeoffs

No new product tradeoff. The accepted two-level nesting bound should not be consumed by characters inside an ordinary valid string.

Verification and gaps

The initial approval verified the requested LF correction but missed this distinct Windows line ending. A second reviewer independently traced the complete token, raw-text, AST and retained-owner paths at this exact head and reached the same conclusion; independent-crlf-proof.json records the source evidence. All six hosted checks are green, but the added escape regression uses LF only. This is a static candidate-loss proof, not a measured scan or complete-verdict claim. No contributor code, tests, imports, builds, supplied snippets or runtime probes were executed.

Reviewed head: b1fc3169622e53a6d886517cbfea535b8631fdf5.

[udsy19]

@rng1995 addressed the CRLF finding in 9667a80: the backslash-escape alternative in all four quoted forms is now \\(?:\r\n|[\s\S]), so a CRLF line continuation stays inside its string. Added LF and CRLF continuation cases to test_tm1_nested_call_arguments.py; the analyzer suite passes. Ready for another look.

[yashrajp22]

Source review of ad94833 found three actionable issues in the argument tokenizer. The inline comments explain the source proofs and suggested corrections. Runtime tests, scanner runs, and fresh-wheel verification have not been performed; this is not a completed behavioral verification or an approval.

[yashrajp22]

Could we make the CRLF escape choice mutually exclusive or commit it before repeating the triple-string body? In Markdown starting with subprocess.run(""", repeated backslash+CRLF sequences and no closing triple quote, each continuation has two parses: one CRLF escape, or an escaped CR followed by an ordinary LF. The missing terminator makes this branch explore exponentially many combinations before failing. The outer atomic group only commits after the token succeeds, and re.finditer cannot call the scanner's deadline callback while matching, so a small malformed skill can stall scanning. Both triple-quoted branches need this change and a bounded malformed-input regression. This is a source-level backtracking proof; I have not run the payload or measured timings.

[udsy19]

Confirmed: with an unterminated triple quote, time grew about 4x per two extra backslash+CRLF repeats. Fixed in 95e3a2d. The escape is now atomic, \\(?>\r\n|[\s\S]), and all four string branches share it, so each continuation has only one parse. 5000 repeats now take about 1 ms. Added test_unterminated_triple_quote_with_crlf_continuations_is_bounded, which runs both quote styles through a Markdown skill.

[yashrajp22]

Could we preserve f-string replacement-field boundaries here? subprocess.run(f"{"("}", shell=True) is valid on the supported Python 3.12+ versions. This token ends at the expression's opening quote, treats the inner string's ( as call nesting, and cannot reach the real outer shell=True; the previous [^)]* pattern did reach it. The AST companion does not recover the warning because it emits bound-name flags, while literal metadata only enriches existing lexical findings. Please handle this quoting shape and add it alongside a different-quote f-string control.

[udsy19]

Confirmed. Fixed in 95e3a2d. Single-line f-strings (f/fr prefixes) now consume {...} replacement fields as part of the string, including quoted strings inside the field and one nested {} level for format specs, so f"{"("}" no longer opens a nested call. {{ and an unclosed { stay ordinary characters, and f""" still uses the triple-quoted branch. Added same-quote and different-quote f-string cases to the must-flag parametrization.

[yashrajp22]

Could the comment token stop at CR as well as LF? In a fenced Python block in SKILL.md, subprocess.run(cmd, # note\rshell=True) with an actual carriage return in place of \r has a real shell keyword on the next physical line. Markdown loading preserves that CR, so #[^\n]* consumes the keyword and removes a TM1 warning the old pattern produced. Ordinary .py loading normalizes line endings, so a Markdown regression that preserves the raw bytes is needed to cover this path.

[udsy19]

Confirmed. Fixed in 95e3a2d: the comment token is now #[^\r\n]*. Added test_shell_true_after_cr_terminated_comment_in_markdown_is_detected, which puts a real CR before shell=True in a SKILL.md fenced block.