From 1b77dc14e0575cc21c4960df1146ee9bcb91552e Mon Sep 17 00:00:00 2001 From: Adebimpe Babsalaam Date: Sat, 1 Aug 2026 15:17:31 -0500 Subject: [PATCH] fix: parse space-separated allowed-tools strings (fixes #327) --- .../nodes/analyzers/mcp_least_privilege.py | 9 +++-- src/skillspector/nodes/build_context.py | 7 ++-- tests/nodes/test_build_context.py | 33 +++++++++++++++++++ tests/test_mcp_least_privilege.py | 12 +++++++ 4 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/skillspector/nodes/analyzers/mcp_least_privilege.py b/src/skillspector/nodes/analyzers/mcp_least_privilege.py index 4a4106fe..7885067f 100644 --- a/src/skillspector/nodes/analyzers/mcp_least_privilege.py +++ b/src/skillspector/nodes/analyzers/mcp_least_privilege.py @@ -134,13 +134,16 @@ def _is_test_file(path: str) -> bool: def _normalize_allowed_tools(value: object) -> list[str]: """Coerce a manifest ``allowed-tools`` value into a list of tool names. - Accepts the list form (``[Bash, Read]``) and the comma-separated string - form (``"Bash, Read"``). Anything else yields an empty list. + Accepts the list form (``[Bash, Read]``), the comma-separated string + form (``"Bash, Read"``), and the space-separated string form + (``"Bash Read"``). Anything else yields an empty list. """ if isinstance(value, list): return [str(t).strip() for t in value if str(t).strip()] if isinstance(value, str): - return [t.strip() for t in value.split(",") if t.strip()] + if "," in value: + return [t.strip() for t in value.split(",") if t.strip()] + return [t.strip() for t in value.split() if t.strip()] return [] diff --git a/src/skillspector/nodes/build_context.py b/src/skillspector/nodes/build_context.py index 3c8e192e..8fd93866 100644 --- a/src/skillspector/nodes/build_context.py +++ b/src/skillspector/nodes/build_context.py @@ -289,12 +289,15 @@ def _parse_manifest(skill_dir: Path) -> dict[str, object]: manifest["permissions"] = ( [str(p) for p in permissions] if isinstance(permissions, list) else [] ) - # `allowed-tools` (Agent Skills standard) — accept list or comma string. + # `allowed-tools` (Agent Skills standard) — accept list, comma string, or space-separated string. allowed_tools = data.get("allowed-tools", []) if isinstance(allowed_tools, list): manifest["allowed-tools"] = [str(t).strip() for t in allowed_tools if str(t).strip()] elif isinstance(allowed_tools, str): - manifest["allowed-tools"] = [t.strip() for t in allowed_tools.split(",") if t.strip()] + if "," in allowed_tools: + manifest["allowed-tools"] = [t.strip() for t in allowed_tools.split(",") if t.strip()] + else: + manifest["allowed-tools"] = [t.strip() for t in allowed_tools.split() if t.strip()] else: manifest["allowed-tools"] = [] # Preserve parameter definitions as dicts so the MCP tool-poisoning diff --git a/tests/nodes/test_build_context.py b/tests/nodes/test_build_context.py index 1a267720..a88d4631 100644 --- a/tests/nodes/test_build_context.py +++ b/tests/nodes/test_build_context.py @@ -276,6 +276,39 @@ def test_build_context_parses_allowed_tools_comma_string(tmp_path: Path) -> None assert result["manifest"]["allowed-tools"] == ["Bash", "Read"] +def test_build_context_parses_allowed_tools_space_string(tmp_path: Path) -> None: + """`allowed-tools` space-separated string form is normalized to a list.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: deployer\ndescription: deploys services\nallowed-tools: Bash Read\n---\n", + encoding="utf-8", + ) + state: SkillspectorState = {"skill_path": str(tmp_path)} + result = build_context(state) + assert result["manifest"]["allowed-tools"] == ["Bash", "Read"] + + +def test_build_context_parses_allowed_tools_mixed_whitespace(tmp_path: Path) -> None: + """`allowed-tools` string with mixed whitespace (multiple spaces) is normalized.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: deployer\ndescription: deploys services\nallowed-tools: Bash Read Write\n---\n", + encoding="utf-8", + ) + state: SkillspectorState = {"skill_path": str(tmp_path)} + result = build_context(state) + assert result["manifest"]["allowed-tools"] == ["Bash", "Read", "Write"] + + +def test_build_context_parses_allowed_tools_single_space_string(tmp_path: Path) -> None: + """`allowed-tools` single tool as space-separated string yields one item.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: deployer\ndescription: deploys services\nallowed-tools: Bash\n---\n", + encoding="utf-8", + ) + state: SkillspectorState = {"skill_path": str(tmp_path)} + result = build_context(state) + assert result["manifest"]["allowed-tools"] == ["Bash"] + + def test_build_context_reports_exclusion_boundary_without_descendants(tmp_path: Path) -> None: """Excluded directory trees produce one boundary record, not child records.""" (tmp_path / "SKILL.md").write_text("# Skill\n", encoding="utf-8") diff --git a/tests/test_mcp_least_privilege.py b/tests/test_mcp_least_privilege.py index 2453e7a7..7d0b22d1 100644 --- a/tests/test_mcp_least_privilege.py +++ b/tests/test_mcp_least_privilege.py @@ -283,6 +283,18 @@ def test_allowed_tools_comma_string_no_lp3(self): f"allowed-tools should satisfy LP3, got: {[f.rule_id for f in findings]}" ) + def test_allowed_tools_space_string_no_lp3(self): + """allowed-tools space-string form ('Bash Read') is also a declaration → no LP3.""" + state = _make_state("mcp_underdeclared_skill") + state["manifest"]["permissions"] = None + state["manifest"]["allowed-tools"] = "Bash Read" + result = mcp_least_privilege.node(state) + findings = result["findings"] + lp3_findings = [f for f in findings if f.rule_id == "LP3"] + assert lp3_findings == [], ( + f"allowed-tools should satisfy LP3, got: {[f.rule_id for f in findings]}" + ) + def test_allowed_tools_underdeclared_fires_lp1(self): """allowed-tools: [Read] + Bash code → LP3 suppressed but LP1 fires HIGH for shell.""" state = _make_state("mcp_underdeclared_skill")