Skip to content
31 changes: 23 additions & 8 deletions src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,16 @@
LINE_BREAK_CHARS,
MARKDOWN_FENCE_CLOSE,
MARKDOWN_FENCE_OPEN,
get_context,
get_line_number,
SourceLocationIndex,
get_context_from_lines,
is_reference_material,
)
from .pattern_defaults import PatternCategory

logger = get_logger(__name__)

ANALYZER_ID = "static_patterns_tool_misuse"
USES_RUNTIME_CHECK = True
ANALYZE_USES_POSTPROCESS = True
POSTPROCESS_USES_PYTHON_AST = True
_VARIABLE_SHELL_FLAG_EVIDENCE = "_tm1_variable_shell_flag"
Expand Down Expand Up @@ -4715,16 +4716,22 @@ def analyze(
file_path: str,
file_type: str,
*,
check_runtime: Callable[[], None] | None = None,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This signature conflicts with #578 (dd95368) on main, which added _direct_shell_only here. That is one of 7 conflict hunks in this file, and GitHub reports CONFLICTING/DIRTY. Taking this side as-is fails ruff: F821 _direct_shell_only and classification_context, and F821 get_context, which postprocess still calls at main:6673. It also makes _DirectShellReplayLexical.analyze (main:6181-6186) raise TypeError, which ruff does not catch. Please merge main and keep check_runtime, _direct_shell_only and defer_variable_reconciliation, keep main's per-line context_by_line cache (filled with get_context_from_lines), and keep main's TM1 hunk. See finding 1.

defer_variable_reconciliation: bool = False,
) -> list[AnalyzerFinding]:
"""Analyze content for tool misuse patterns (TM1–TM3)."""
runtime_check = check_runtime or (lambda: None)
runtime_check()
findings: list[AnalyzerFinding] = []
locations = SourceLocationIndex(content, file_path)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the maintainer merge f4432cc, the diff no longer adds a scope index. What it does is this line index plus window-sized context lookups (9.28 s to 0.33 s on the dense 256K window against current main), correct context anchoring for CR, U+2028 and VT line breaks, and the check_runtime plumbing. The title and body still describe "one scope index per matching source window" and a 120-to-1 parse reduction. Please retitle and rewrite the description to match the diff after the conflict resolution. See finding 3.

lines = content.splitlines()

def loc(ln: int) -> Location:
return Location(file=file_path, start_line=ln)

def ctx(start: int) -> str:
return get_context(content, start)
line, column = locations.line_and_column(start)
return get_context_from_lines(lines, line, column=column)

tag = [PatternCategory.TOOL_MISUSE.value]
tm1_findings_by_key: dict[tuple[int, str, int], AnalyzerFinding] = {}
Expand All @@ -4735,10 +4742,14 @@ def ctx(start: int) -> str:
}
invisible_variable_matches: set[tuple[int, int]] = set()
if file_type == "python" and variable_matches and not defer_variable_reconciliation:
runtime_check()
parsed = parse_python_source(content, file_path)
runtime_check()
ast_index = _build_variable_shell_ast_index(parsed)
runtime_check()
if ast_index is not None:
for span, (_, match) in variable_matches.items():
runtime_check()
assignment_line = bisect_right(ast_index.line_character_starts, match.start(1))
candidate = _resolve_variable_shell_candidate(ast_index, match, assignment_line)
if candidate is not None and (
Expand All @@ -4750,19 +4761,20 @@ def ctx(start: int) -> str:
invisible_variable_matches.add(span)

shell_content = (
_perl_literal_print_shell_text(content, lambda: None) if file_type == "perl" else None
_perl_literal_print_shell_text(content, runtime_check) if file_type == "perl" else None
)
for match_start, match_end, matched_text, confidence in _tm1_candidates(
content, shell_content=shell_content
):
runtime_check()
variable_match = variable_matches.get((match_start, match_end))
if variable_match is not None and variable_match[0].casefold().startswith("true"):
# The case-insensitive direct ``shell=True`` pattern already owns
# true-prefixed names at the exact call location.
continue
if variable_match is not None and (match_start, match_end) in invisible_variable_matches:
continue
line_num = get_line_number(content, match_start)
line_num = locations.line_and_column(match_start)[0]
context_text = ctx(match_start)
matched = matched_text[:200]
matched_line = _line_containing(content, match_start, match_end)
Expand Down Expand Up @@ -4807,7 +4819,8 @@ def ctx(start: int) -> str:
else re.finditer
)
for match in matches(pattern, content, re.IGNORECASE | re.MULTILINE):
line_num = get_line_number(content, match.start())
runtime_check()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open PR #741 makes the same SourceLocationIndex/get_context_from_lines change in this function, but enforces deadlines through static_runner.iter_pattern_matches instead of USES_RUNTIME_CHECK. git merge-tree of the two heads conflicts in exactly these TM2-TM4 loop heads, and both PRs also conflict with #578 here. No change is needed in this PR now, but whichever PR lands second has to reconcile these lines. This PR's distinct piece is the Perl helper deadline at line 4764. See finding 4.

line_num = locations.line_and_column(match.start())[0]
context_text = ctx(match.start())
matched = match.group(0)[:200]

Expand Down Expand Up @@ -4837,7 +4850,8 @@ def ctx(start: int) -> str:
else re.finditer
)
for match in matches(pattern, content, re.IGNORECASE | re.MULTILINE):
line_num = get_line_number(content, match.start())
runtime_check()
line_num = locations.line_and_column(match.start())[0]
findings.append(
AnalyzerFinding(
rule_id="TM3",
Expand All @@ -4859,7 +4873,8 @@ def ctx(start: int) -> str:
tm4_tags = [*tag, "contextual-triage", "likely-benign-context"] if reference_material else tag
for pattern, confidence in TM4_PATTERNS:
for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE):
line_num = get_line_number(content, match.start())
runtime_check()
line_num = locations.line_and_column(match.start())[0]
findings.append(
AnalyzerFinding(
rule_id="TM4",
Expand Down
61 changes: 61 additions & 0 deletions tests/unit/test_patterns_new.py
Original file line number Diff line number Diff line change
Expand Up @@ -1527,6 +1527,67 @@ def test_tm1_keeps_global_shell_variable(self) -> None:
findings = tm_mod.analyze(content, "runner.py", "python")
assert any(finding.rule_id == "TM1" for finding in findings)

def test_tm1_parses_dense_shell_flags_once(self) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run against current main (transcribed), this test passes without the PR's change. It takes about 9.2 s there and about 1 s with the change, but it only asserts one parse and 4,000 TM1 findings, and main already parses once since #577. Five of the six new tests pass on main, and the sixth fails only because the check_runtime keyword is missing. So reverting the get_context_from_lines change would not fail any of them. Consider patching tm_mod.get_context with side_effect=AssertionError and calling analyze(..., defer_variable_reconciliation=True) on this input, and adding a deadline test on the deferred path. See finding 2.

content = "use_shell = True\nsubprocess.run(cmd, shell=use_shell)\n" * 4000
content += "#" + " ordinary" * ((256_000 - len(content)) // 9)
content = content.ljust(256_000)
assert len(content) == 256_000
with patch.object(tm_mod, "parse_python_source", wraps=tm_mod.parse_python_source) as parse:
findings = tm_mod.analyze(content, "runner.py", "python")
assert parse.call_count == 1
assert len([finding for finding in findings if finding.rule_id == "TM1"]) == 4000

def test_tm1_caches_unparseable_window_conservatively(self) -> None:
content = "use_shell = True\nsubprocess.run(cmd, shell=use_shell)\n" * 20 + "\nif :"
with patch.object(tm_mod, "parse_python_source", wraps=tm_mod.parse_python_source) as parse:
findings = tm_mod.analyze(content, "runner.py", "python")
assert parse.call_count == 1
assert len([finding for finding in findings if finding.rule_id == "TM1"]) == 20

def test_tm1_remains_lexical_for_large_and_normalized_views(self) -> None:
from skillspector.nodes.analyzers import static_runner

for content in (
"# ordinary source\n" * 16_000 + "subprocess.run(cmd, shell=True)",
"subprocess.run(cmd, shell=Tru\u200be)",
):
result = static_runner.run_static_patterns_with_ledger(
{"components": ["runner.py"], "file_cache": {"runner.py": content}}, [tm_mod]
)
assert any(finding.rule_id == "TM1" for finding in result["findings"])

def test_tm1_scope_index_honors_runtime_check(self) -> None:
content = "use_shell = True\nsubprocess.run(cmd, shell=use_shell)\n" * 300
calls = 0

def check_runtime() -> None:
nonlocal calls
calls += 1
if calls == 20:
raise RuntimeError("deadline")

with pytest.raises(RuntimeError, match="deadline"):
tm_mod.analyze(content, "runner.py", "python", check_runtime=check_runtime)
assert calls == 20

def test_tm1_skips_parse_without_variable_shell_flags(self) -> None:
with patch.object(tm_mod, "parse_python_source", side_effect=AssertionError("parsed")):
findings = tm_mod.analyze("subprocess.run(cmd, shell=True)", "runner.py", "python")
assert any(finding.rule_id == "TM1" for finding in findings)

def test_tm1_preserves_nonlocal_binding_scope(self) -> None:
content = (
"def outer():\n"
" use_shell = False\n"
" def assign():\n"
" use_shell = True\n"
" def run():\n"
" nonlocal use_shell\n"
" subprocess.run(cmd, shell=use_shell)\n"
)
findings = tm_mod.analyze(content, "runner.py", "python")
assert not any(finding.rule_id == "TM1" for finding in findings)

def test_application_specific_no_verify_flag_is_not_tool_misuse(self) -> None:
content = """\
print("verification: skipped (--no-verify)")
Expand Down