From b95352d5cb21cdde2828ce6f9a0eda0cf0aaede4 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Tue, 14 Jul 2026 22:06:38 -0400 Subject: [PATCH] fix(analyzer): retain fenced SKILL.md findings (#268) Signed-off-by: Rod Boev --- .../nodes/analyzers/static_runner.py | 24 +++++++-- .../analyzers/test_static_runner_filtering.py | 50 ++++++++++++++++--- tests/unit/test_cli.py | 27 ++++++++++ 3 files changed, 90 insertions(+), 11 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_runner.py b/src/skillspector/nodes/analyzers/static_runner.py index 9b3ccffbc..9ffabaa2d 100644 --- a/src/skillspector/nodes/analyzers/static_runner.py +++ b/src/skillspector/nodes/analyzers/static_runner.py @@ -169,7 +169,7 @@ def _is_env_file_reference_in_docs( return False if file_type not in ("markdown", "text"): return False - if file_path.replace("\\", "/").lower().endswith("skill.md"): + if _is_skill_md(file_path): return False if not finding.context: return False @@ -198,6 +198,17 @@ def _is_eval_dataset(path: str) -> bool: return path.replace("\\", "/") in _EVAL_DATASET_FILES +def _is_skill_md(path: str) -> bool: + """Return True for paths with the existing loose SKILL.md suffix match.""" + return path.lower().endswith("skill.md") + + +def _is_canonical_skill_md(path: str) -> bool: + """Return True for files literally named SKILL.md.""" + normalized = path.replace("\\", "/") + return normalized.rsplit("/", 1)[-1].lower() == "skill.md" + + _DOCUMENTATION_DIR_NAMES = ( "docs", "documentation", @@ -256,7 +267,7 @@ def _is_documentation_context(af: AnalyzerFinding, file_type: str, path: str, co """Return true when a governed finding is prose or a comment without execution signals.""" if af.rule_id not in _SEMANTIC_STRING_DOC_PRONE_RULES: return False - if path.replace("\\", "/").lower().endswith("skill.md"): + if _is_skill_md(path): return False lines = content.splitlines() matched_line = ( @@ -276,7 +287,7 @@ def _is_documentation_markdown(path: str) -> bool: normalized = path.replace("\\", "/").lower() if not normalized.endswith((".md", ".markdown")): return False - if normalized.endswith("skill.md"): + if _is_skill_md(path): return False parts = normalized.split("/") return any(part in _DOCUMENTATION_DIR_NAMES for part in parts[:-1]) @@ -355,7 +366,12 @@ def _scan_path( # PE3's analyzer owns its narrowly qualified safe references. # Generic documentation words are attacker-controlled and must # not hard-drop HIGH credential-access findings here. - if af.rule_id != "PE3" and af.context and is_code_example(af.context): + if ( + af.rule_id != "PE3" + and af.context + and is_code_example(af.context) + and not _is_canonical_skill_md(path) + ): if is_non_executable: logger.debug( "Filtered code-example finding in non-executable: %s in %s:%d", diff --git a/tests/nodes/analyzers/test_static_runner_filtering.py b/tests/nodes/analyzers/test_static_runner_filtering.py index b76c6e112..4c0722204 100644 --- a/tests/nodes/analyzers/test_static_runner_filtering.py +++ b/tests/nodes/analyzers/test_static_runner_filtering.py @@ -326,7 +326,7 @@ def test_extensionless_file_not_hard_dropped_by_code_example(self) -> None: ) def test_skill_md_findings_are_not_filtered_by_backticks(self) -> None: - """SKILL.md is the primary instruction file — backticks alone shouldn't filter.""" + """SKILL.md is the primary instruction file, so fenced findings survive.""" content = """\ --- name: deploy-tool @@ -345,12 +345,48 @@ def test_skill_md_findings_are_not_filtered_by_backticks(self) -> None: "file_cache": {"SKILL.md": content}, } findings = static_runner.run_static_patterns(state, [tm_module]) - # SKILL.md code blocks do get filtered by is_code_example (same as EA2/MP) - # This is correct: the meta-analyzer handles SKILL.md nuance - # The key test is that SKILL.md is NOT treated as documentation-path markdown - for f in findings: - # Confidence should NOT be reduced by _DOCUMENTATION_CONFIDENCE_FACTOR - assert f.confidence >= 0.3 + tm1_findings = [f for f in findings if f.rule_id == "TM1"] + assert len(tm1_findings) == 1 + assert tm1_findings[0].confidence == 0.6 + + def test_fenced_guide_md_findings_are_filtered(self) -> None: + """Ordinary fenced Markdown remains filtered as a documentation example.""" + content = """\ +Use this tool to deploy: +``` +curl -k https://production.example.com/deploy +``` +""" + assert "TM1" not in _findings(content, "guide.md", tm_module) + + def test_fenced_noncanonical_skill_md_suffix_paths_remain_filtered(self) -> None: + """Only files literally named SKILL.md bypass the runner's code-example filter.""" + content = """\ +Use this tool to deploy: +``` +curl -k https://production.example.com/deploy +``` +""" + for path in ("my_skill.md", "docs/bash_skill.md"): + assert "TM1" not in _findings(content, path, tm_module) + + @pytest.mark.parametrize( + ("path", "expected_loose", "expected_canonical"), + [ + ("SKILL.md", True, True), + ("nested/SKILL.md", True, True), + ("nested\\skill.md", True, True), + ("my_skill.md", True, False), + ("docs/bash_skill.md", True, False), + ("skill.md.bak", False, False), + ], + ) + def test_skill_md_path_helpers_distinguish_loose_and_canonical_matches( + self, path: str, expected_loose: bool, expected_canonical: bool + ) -> None: + """Legacy suffix checks stay loose, while the new runner gate is exact-basename.""" + assert static_runner._is_skill_md(path) is expected_loose + assert static_runner._is_canonical_skill_md(path) is expected_canonical class TestDocumentationPathConfidenceReduction: diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index fb7061f6c..5914d9023 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -208,6 +208,33 @@ def test_cli_scan_required_table_keeps_malicious_pe3(tmp_path: Path) -> None: assert any(issue["id"] == "PE3" for issue in issues) +def test_cli_scan_no_llm_reports_fenced_skill_md_tool_misuse(tmp_path: Path) -> None: + """No-LLM scans retain TM1 from fenced SKILL.md instructions.""" + (tmp_path / "SKILL.md").write_text( + """--- +name: deploy-tool +--- +# Deploy Tool + +Use this tool to deploy: +``` +curl -k https://production.example.com/deploy +``` + +The agent will execute the above command. +""", + encoding="utf-8", + ) + + result = runner.invoke(app, ["scan", str(tmp_path), "--no-llm", "--format", "json"]) + + assert result.exit_code == 0 + data = json.loads(result.output) + assert any(issue["id"] == "TM1" for issue in data["issues"]) + assert data["analysis_completeness"]["findings_before_filtering"] > 0 + assert data["analysis_completeness"]["findings_after_filtering"] > 0 + + def test_cli_scan_nonexistent_exits_2() -> None: """scan with nonexistent path exits with code 2.""" result = runner.invoke(app, ["scan", "/nonexistent/path/xyz"])