fix(lp1): map scoped allowed-tools grants such as Bash(git:*) by tool name

src/skillspector/allowed_tools.py

@@ -0,0 +1,116 @@
+# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Split and validate Agent Skills ``allowed-tools`` grants.
+
+A string ``allowed-tools`` value lists grants separated by commas or by
+whitespace. Each grant is a bare tool name (``Read``) or a scoped grant in the
+``Tool(specifier)`` form (``Bash(git status:*)``, ``Read(./docs, notes.md)``).
+A separator inside a specifier belongs to the specifier, so a scoped grant is
+kept whole and its specifier text is never read as another grant.
+"""
+
+from __future__ import annotations
+
+import re
+from collections.abc import Iterator
+
+# Tool names longer than this are not looked up.
+MAX_ALLOWED_TOOL_NAME_CHARS = 64
+
+_COMMA_FORM_TOKENS = re.compile(r"[(),]")
+_WHITESPACE_FORM_TOKENS = re.compile(r"[()]|\s+")
+_PARENTHESES = re.compile(r"[()]")
+_NON_WHITESPACE = re.compile(r"\S")
+# Same tool-name alphabet as the settings permission-rule check in
+# ``nodes/analyzers/bundled_execution_surface.py`` (``_permission_rule_is_valid``).
+_TOOL_NAME = re.compile(r"[A-Za-z0-9_*.-]+")
+
+
+def _has_top_level_comma(value: str) -> bool:
+    depth = 0
+    for match in _COMMA_FORM_TOKENS.finditer(value):
+        token = match.group()
+        if token == "(":
+            depth += 1
+        elif token == ")":
+            depth = max(0, depth - 1)
+        elif depth == 0:
+            return True
+    return False
+
+
+def iter_allowed_tools_entries(value: str) -> Iterator[str]:
+    """Yield the stripped entries of a string ``allowed-tools`` value.
+
+    A comma outside any parentheses selects the comma-separated form;
+    otherwise entries are separated by whitespace. Only separators outside
+    parentheses split, so ``"Bash(git status:*) Read"`` yields
+    ``Bash(git status:*)`` and ``Read``, and ``"Read(./a, b)"`` stays one
+    entry. An unclosed ``(`` keeps the rest of the value in its entry, and a
+    stray ``)`` closes nothing; :func:`allowed_tool_grant_name` rejects both.
+
+    Entries are produced lazily. As with ``str.split``, the comma form yields
+    an empty entry between adjacent commas and the whitespace form yields no
+    empty entries; callers skip empty entries.
+    """
+    comma_form = _has_top_level_comma(value)
+    pattern = _COMMA_FORM_TOKENS if comma_form else _WHITESPACE_FORM_TOKENS
+    depth = 0
+    start = 0
+    for match in pattern.finditer(value):
+        token = match.group()
+        if token == "(":
+            depth += 1
+        elif token == ")":
+            depth = max(0, depth - 1)
+        elif depth == 0:
+            entry = value[start : match.start()].strip()
+            if entry or comma_form:
+                yield entry
+            start = match.end()
+    entry = value[start:].strip()
+    if entry or comma_form:
+        yield entry
+
+
+def allowed_tool_grant_name(entry: str) -> str | None:
+    """Return the tool name of a well-formed ``allowed-tools`` grant, else ``None``.
+
+    A well-formed grant is either a bare tool name or ``Tool(specifier)``
+    where the specifier is not blank, its parentheses balance, and the ``)``
+    that closes the first ``(`` is the last character. Incomplete or
+    malformed grants such as ``Bash(``, ``Bash(notes``, ``Bash()`` or
+    ``Bash(notes).md)`` return ``None``, so they name no tool. Names longer
+    than :data:`MAX_ALLOWED_TOOL_NAME_CHARS` also return ``None``.
+    """
+    entry = entry.strip()
+    # Bound the search so a long specifier is not scanned for the name.
+    paren = entry.find("(", 0, MAX_ALLOWED_TOOL_NAME_CHARS + 1)
+    if paren == -1 and len(entry) > MAX_ALLOWED_TOOL_NAME_CHARS:
+        return None
+    name = entry if paren == -1 else entry[:paren]
+    if _TOOL_NAME.fullmatch(name) is None:
+        return None
+    if paren == -1:
+        return name
+    if not entry.endswith(")") or _NON_WHITESPACE.search(entry, paren + 1, len(entry) - 1) is None:
+        return None
+    depth = 0
+    for match in _PARENTHESES.finditer(entry, paren + 1, len(entry) - 1):
+        depth += 1 if match.group() == "(" else -1
+        if depth < 0:
+            return None
+    return name if depth == 0 else None

src/skillspector/nodes/analyzers/mcp_least_privilege.py

@@ -22,6 +22,7 @@
 from dataclasses import dataclass, field
 from pathlib import Path
 
+from skillspector.allowed_tools import allowed_tool_grant_name, iter_allowed_tools_entries
 from skillspector.inspection_ledger import (
     InspectionLedgerEvent,
     LedgerOutcome,
@@ -250,18 +251,17 @@ def _normalize_allowed_tools(
 
     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.
+    (``"Bash Read"``). Separators inside a ``Tool(specifier)`` scope do not
+    split, so ``"Bash(git status:*) Read"`` yields two entries. Anything else
+    yields an empty list.
     """
     tools: list[str] = []
     if isinstance(value, list):
         candidates = iter(value)
     elif isinstance(value, str):
-        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))
+        # Entries are produced lazily, so a comma-dense declaration stops at the
+        # limit below without building an arbitrarily large temporary list.
+        candidates = iter_allowed_tools_entries(value)
     else:
         return tools
 
@@ -350,7 +350,8 @@ def _map_permissions_to_categories(
     return categories
 
 
-# Tool name → capability category (Claude / Agent Skills tool names, case-insensitive exact match)
+# Tool name → capability category (Claude / Agent Skills tool names, case-insensitive exact match
+# on the name of a well-formed bare or ``Tool(specifier)`` grant)
 _TOOL_TO_CAPABILITY: dict[str, str] = {
     "bash": "shell",
     "execute": "shell",
@@ -373,14 +374,21 @@ def _map_allowed_tools_to_categories(
     tools: list[str],
     budget: _LeastPrivilegeBudget | None = None,
 ) -> set[str]:
-    """Map Agent Skills ``allowed-tools`` tool names to capability category names."""
+    """Map Agent Skills ``allowed-tools`` tool names to capability category names.
+
+    Scoped grants in the ``Tool(specifier)`` form, such as ``Bash(git:*)`` or
+    ``WebFetch(domain:example.com)``, map by their tool name, so a narrower
+    grant covers the same category as the bare tool. Incomplete or malformed
+    grants such as ``Bash(`` or ``Bash(notes).md)`` map to no category.
+    """
     categories: set[str] = set()
     for tool in tools:
         if budget is not None:
             budget.check_runtime("SKILL.md")
-        if len(tool) > 64:
+        name = allowed_tool_grant_name(tool)
+        if name is None:
             continue
-        cat = _TOOL_TO_CAPABILITY.get(tool.lower().strip())
+        cat = _TOOL_TO_CAPABILITY.get(name.lower())
         if cat:
             categories.add(cat)
     return categories

src/skillspector/nodes/build_context.py

@@ -36,6 +36,7 @@
 
 import yaml
 
+from skillspector.allowed_tools import iter_allowed_tools_entries
 from skillspector.artifacts import (
     ArtifactDisposition,
     ArtifactRecord,
@@ -2296,27 +2297,16 @@ 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.
+    # Separators inside a `Tool(specifier)` scope belong to the scope, so
+    # `Bash(git status:*) Read` stays two grants.
     allowed_tools = data.get("allowed-tools", [])
     if isinstance(allowed_tools, str):
-        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()
-            ]
+        tools: list[str] = []
+        for item in iter_allowed_tools_entries(allowed_tools):
+            _check()
+            if item:
+                tools.append(_scalar_text(item))
+        manifest["allowed-tools"] = tools
     elif isinstance(allowed_tools, list):
         manifest["allowed-tools"] = [
             item.strip() for item in _string_list(allowed_tools) if item.strip()

tests/nodes/test_build_context.py

@@ -1067,6 +1067,49 @@ def test_build_context_parses_allowed_tools_single_space_string(tmp_path: Path)
     assert result["manifest"]["allowed-tools"] == ["Bash"]
 
 
+@pytest.mark.parametrize(
+    ("declaration", "expected"),
+    [
+        pytest.param(
+            "Bash(git status:*) Read",
+            ["Bash(git status:*)", "Read"],
+            id="space_inside_scope",
+        ),
+        pytest.param(
+            "Bash(git status:*), Read",
+            ["Bash(git status:*)", "Read"],
+            id="comma_string",
+        ),
+        pytest.param(
+            "Read(./docs Bash(notes).md)",
+            ["Read(./docs Bash(notes).md)"],
+            id="nested_parens_inside_scope",
+        ),
+        pytest.param(
+            "Read(./docs, Bash(notes).md)",
+            ["Read(./docs, Bash(notes).md)"],
+            id="comma_inside_scope",
+        ),
+        pytest.param(
+            "Bash(git status:*) Read(./a, b)",
+            ["Bash(git status:*)", "Read(./a, b)"],
+            id="comma_only_inside_scope_keeps_space_form",
+        ),
+    ],
+)
+def test_build_context_keeps_scoped_allowed_tools_whole(
+    tmp_path: Path, declaration: str, expected: list[str]
+) -> None:
+    """Separators inside a `Tool(specifier)` scope belong to the scope."""
+    (tmp_path / "SKILL.md").write_text(
+        f"---\nname: deployer\ndescription: deploys services\nallowed-tools: {declaration}\n---\n",
+        encoding="utf-8",
+    )
+    state: SkillspectorState = {"skill_path": str(tmp_path)}
+    result = build_context(state)
+    assert result["manifest"]["allowed-tools"] == expected
+
+
 def test_build_context_inventories_excluded_executable_descendants(tmp_path: Path) -> None:
     """Excluded trees retain bounded inventory and executable coverage evidence."""
     (tmp_path / "SKILL.md").write_text("# Skill\n", encoding="utf-8")