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

src/skillspector/allowed_tools.py

@@ -0,0 +1,124 @@
+# 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)``).
+As in Claude Code's permission rule syntax, parentheses inside the specifier
+are literal characters, not nested structure: ``Bash(echo '(')`` is a complete
+grant. 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
+
+_TOKENS = re.compile(r"[(),]|\s+")
+_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``).
+# Unlike that check, the specifier's parentheses do not have to balance.
+_TOOL_NAME = re.compile(r"[A-Za-z0-9_*.-]+")
+
+
+def _ends_specifier(value: str, end: int) -> bool:
+    """Return whether a ``)`` ending at ``end`` is followed by a separator or the end."""
+    return end == len(value) or value[end] == "," or value[end].isspace()
+
+
+def _separators(value: str) -> Iterator[re.Match[str]]:
+    """Yield the commas and whitespace runs of ``value`` that lie outside a specifier.
+
+    A ``(`` outside a specifier opens one. Inside it, ``(`` and ``)`` are
+    literal, except that a ``)`` followed by a comma, whitespace or the end of
+    the value closes it.
+    """
+    in_specifier = False
+    for match in _TOKENS.finditer(value):
+        token = match.group()
+        if in_specifier:
+            if token == ")" and _ends_specifier(value, match.end()):
+                in_specifier = False
+        elif token == "(":
+            in_specifier = True
+        elif token != ")":
+            yield match
+
+
+def iter_allowed_tools_entries(value: str) -> Iterator[str]:
+    """Yield the stripped entries of a string ``allowed-tools`` value.
+
+    A comma outside any specifier selects the comma-separated form; otherwise
+    entries are separated by whitespace. Only separators outside a specifier
+    split. A specifier runs from a ``(`` to the first ``)`` that is followed by
+    a comma, whitespace or the end of the value, and any ``(`` or ``)`` in
+    between is literal. So ``"Bash(git status:*) Read"`` and
+    ``"Bash(echo '(') Read"`` each yield two entries, and
+    ``"Read(./docs Bash(notes).md)"`` and ``"Read(./a, b)"`` each stay one
+    entry. A specifier whose own text has a ``)`` followed by a separator
+    cannot be written in a string value; the list form keeps it whole. An
+    unclosed ``(`` keeps the rest of the value in its entry, which
+    :func:`allowed_tool_grant_name` rejects.
+
+    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 = any(match.group() == "," for match in _separators(value))
+    start = 0
+    for match in _separators(value):
+        if comma_form and match.group() != ",":
+            continue
+        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 complete ``allowed-tools`` grant, else ``None``.
+
+    A complete grant is either a bare tool name or ``Tool(specifier)``, where
+    the specifier is everything between the first ``(`` and the final ``)``,
+    which must be the last character, and is not blank. Parentheses inside the
+    specifier are literal, so ``Bash(echo '(')`` and ``Bash(echo ')')`` are
+    complete. Incomplete grants such as ``Bash(``, ``Bash(notes`` or
+    ``Bash(echo '('`` and blank ones such as ``Bash()`` 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
+    return name

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"`` and ``"Bash(echo '(') Read"`` each
+    yield 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,23 @@ 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. Parentheses inside the
+    specifier are literal, so ``Bash(echo '(')`` maps to ``shell``. Incomplete
+    or blank grants such as ``Bash(``, ``Bash(notes`` or ``Bash()`` 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,65 @@ 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",
+        ),
+        pytest.param(
+            "Bash(echo '(') Read", ["Bash(echo '(')", "Read"], id="literal_open_paren_in_scope"
+        ),
+        pytest.param(
+            "Bash(echo ')') Read", ["Bash(echo ')')", "Read"], id="literal_close_paren_in_scope"
+        ),
+        pytest.param(
+            "Bash(echo '('), Read",
+            ["Bash(echo '(')", "Read"],
+            id="comma_string_literal_open_paren_in_scope",
+        ),
+        pytest.param(
+            "Bash(echo ')'), Read",
+            ["Bash(echo ')')", "Read"],
+            id="comma_string_literal_close_paren_in_scope",
+        ),
+    ],
+)
+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")