From 8dc4acbb044988a3dbb5c322d08973494ec6bf0c Mon Sep 17 00:00:00 2001 From: Adebimpe Babsalaam Date: Wed, 26 Aug 2026 18:52:04 -0500 Subject: [PATCH] fix: parse space-separated allowed-tools strings (fixes #327) Signed-off-by: Adebimpe Babsalaam --- .../nodes/analyzers/mcp_least_privilege.py | 14 +++++--- src/skillspector/nodes/build_context.py | 34 +++++++++++-------- tests/nodes/test_build_context.py | 33 ++++++++++++++++++ tests/test_mcp_least_privilege.py | 12 +++++++ 4 files changed, 74 insertions(+), 19 deletions(-) diff --git a/src/skillspector/nodes/analyzers/mcp_least_privilege.py b/src/skillspector/nodes/analyzers/mcp_least_privilege.py index f48b3438..1033bbfe 100644 --- a/src/skillspector/nodes/analyzers/mcp_least_privilege.py +++ b/src/skillspector/nodes/analyzers/mcp_least_privilege.py @@ -246,16 +246,20 @@ def _normalize_allowed_tools( ) -> 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. """ tools: list[str] = [] if isinstance(value, list): candidates = iter(value) elif isinstance(value, str): - # A bounded split prevents a comma-dense declaration from creating an - # arbitrarily large temporary list before the analyzer can stop it. - candidates = iter(value.split(",", _MAX_DECLARATION_VALUES)) + if "," in value: + # A bounded split prevents a comma-dense declaration from creating an + # arbitrarily large temporary list before the analyzer can stop it. + candidates = iter(value.split(",", _MAX_DECLARATION_VALUES)) + else: + candidates = iter(value.split(None, _MAX_DECLARATION_VALUES)) else: return tools diff --git a/src/skillspector/nodes/build_context.py b/src/skillspector/nodes/build_context.py index f9746051..fa8ae93c 100644 --- a/src/skillspector/nodes/build_context.py +++ b/src/skillspector/nodes/build_context.py @@ -1458,22 +1458,28 @@ def _string_list(value: object) -> list[str]: manifest["triggers"] = _string_list(data.get("triggers", [])) manifest["permissions"] = _string_list(data.get("permissions", [])) + # `allowed-tools` (Agent Skills standard) — accept list, comma string, or space-separated string. allowed_tools = data.get("allowed-tools", []) if isinstance(allowed_tools, str): - tools: list[str] = [] - cursor = 0 - while cursor <= len(allowed_tools): - _check() - separator = allowed_tools.find(",", cursor) - if separator < 0: - separator = len(allowed_tools) - item = allowed_tools[cursor:separator].strip() - if item: - tools.append(_scalar_text(item)) - if separator == len(allowed_tools): - break - cursor = separator + 1 - manifest["allowed-tools"] = tools + if "," in allowed_tools: + tools: list[str] = [] + cursor = 0 + while cursor <= len(allowed_tools): + _check() + separator = allowed_tools.find(",", cursor) + if separator < 0: + separator = len(allowed_tools) + item = allowed_tools[cursor:separator].strip() + if item: + tools.append(_scalar_text(item)) + if separator == len(allowed_tools): + break + cursor = separator + 1 + manifest["allowed-tools"] = tools + else: + manifest["allowed-tools"] = [ + _scalar_text(t) for t in allowed_tools.split() if t.strip() + ] elif isinstance(allowed_tools, list): manifest["allowed-tools"] = [ item.strip() for item in _string_list(allowed_tools) if item.strip() diff --git a/tests/nodes/test_build_context.py b/tests/nodes/test_build_context.py index 51a917de..6415e453 100644 --- a/tests/nodes/test_build_context.py +++ b/tests/nodes/test_build_context.py @@ -874,6 +874,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 2d4d3023..082ce8d4 100644 --- a/tests/test_mcp_least_privilege.py +++ b/tests/test_mcp_least_privilege.py @@ -286,6 +286,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")