skill: gate embedded shell through the permission system

src/tool_system/tools/skill.py

@@ -231,38 +231,95 @@ def _run_markdown_skill(skill_name: str, args: str, context: ToolContext) -> Too
     )
 
 
+def _permission_context_with_skill_bash_rules(
+    base: Any, allowed_tools: list[str] | None
+) -> Any:
+    """Return ``base`` with the skill's ``Bash(...)`` allowed-tools added as
+    session allow rules.
+
+    Mirrors TS injecting a skill's ``allowed-tools`` as
+    ``alwaysAllowRules.command`` for the duration of the call: commands the skill
+    declares auto-allow, while everything else stays gated. Non-Bash entries are
+    irrelevant to embedded shell and ignored. A new context is returned; ``base``
+    is never mutated.
+    """
+    from dataclasses import replace
+
+    bash_rules = [
+        t for t in (allowed_tools or []) if t == "Bash" or t.startswith("Bash(")
+    ]
+    if not bash_rules:
+        return base
+    # Use the ``command`` source (TS injects allowed-tools as the slash
+    # command's own rules), so these never conflate with genuine session grants.
+    merged = {src: list(rules) for src, rules in base.always_allow_rules.items()}
+    merged["command"] = merged.get("command", []) + bash_rules
+    return replace(base, always_allow_rules=merged)
+
+
 def _make_shell_executor(
     context: ToolContext,
     allowed_tools: list[str] | None,
     *,
     slash_command_name: str,
 ):
-    """Return a callable that runs a shell command via BashTool.
-
-    The returned executor matches the
-    ``runtime_substitution.ShellExecutor`` signature: ``(command, inline)
-    -> rendered text``. Errors and non-zero exits are formatted via
-    ``format_shell_error`` / ``format_shell_output`` so the renderer can
-    splice the result back into the prompt without raising.
-
-    The skill's ``allowed_tools`` list is documented for parity with TS
-    (which injects them as ``alwaysAllowRules.command`` for the duration
-    of the call), but the Python BashTool's ``call()`` path bypasses the
-    registry-level permission gate and runs the command directly under
-    the active ``ToolContext`` permission mode. Wiring the
-    ``alwaysAllowRules.command`` injection precisely is tracked as a
-    follow-up; in bypass-permissions sessions (the default for the
-    in-process SkillTool path) commands run unprompted.
+    """Return a callable that runs a skill's embedded ``!`` shell command via
+    BashTool, **gated through the permission system**.
+
+    The returned executor matches the ``runtime_substitution.ShellExecutor``
+    signature ``(command, inline) -> rendered text``. Before running, each
+    command is permission-checked exactly like any Bash tool call (deny rules →
+    bash safety screen → the skill's declared ``allowed_tools`` Bash rules), with
+    ``allowed_tools`` injected as the command's allow rules so declared commands
+    run silently. Only a permission ``allow`` runs; an undeclared or
+    safety-screened command is hard-denied and rendered inline as an error
+    (matching TS ``promptShellExecution``, which fails rather than prompting the
+    user mid-expansion). ``bypassPermissions`` mode still runs everything. So
+    embedded shell can no longer bypass the gate.
     """
     from .bash import BashTool
     from src.skills.runtime_substitution import (
         format_shell_error,
         format_shell_output,
     )
 
-    _ = allowed_tools  # acknowledged; precise injection deferred (see docstring)
+    # Skill-scoped permission context: the skill's declared Bash commands
+    # auto-allow; everything else flows through the normal gate.
+    skill_perm_ctx = _permission_context_with_skill_bash_rules(
+        context.permission_context, allowed_tools
+    )
 
     def _exec(command: str, inline: bool) -> str:
+        # Gate the command before running it. Mirrors TS ``promptShellExecution``:
+        # only a permission ``allow`` runs the command; anything else (``ask`` or
+        # ``deny``) is treated as denied and rendered inline as an error instead
+        # of executing — TS hard-denies here rather than prompting the user
+        # mid-skill-expansion, and we match that. A skill grants its commands by
+        # DECLARING them in ``allowed-tools``; undeclared / safety-screened
+        # commands do not run. Fails CLOSED on any gate error.
+        try:
+            from src.permissions.check import has_permissions_to_use_tool
+
+            decision = has_permissions_to_use_tool(
+                BashTool,
+                {"command": command},
+                skill_perm_ctx,
+                tool_use_context=context,
+            )
+        except Exception as exc:  # noqa: BLE001 — fail closed, never crash render
+            return format_shell_error(exc, command, inline=inline)
+
+        if decision.behavior != "allow":
+            reason = getattr(decision, "decision_reason", None)
+            if reason is not None and getattr(reason, "type", None) == "safetyCheck":
+                msg = getattr(decision, "message", None) or "blocked by a safety check"
+            else:
+                msg = (
+                    "command not permitted — declare it in the skill's "
+                    "`allowed-tools` (e.g. `Bash(<cmd>:*)`)"
+                )
+            return format_shell_error(msg, command, inline=inline)
+
         try:
             tr = BashTool.call({"command": command}, context)
         except Exception as exc:  # noqa: BLE001 — surface every failure

tests/test_tool_permission_parity.py

@@ -194,6 +194,65 @@ def test_config_write_asks(self) -> None:
         )
 
 
+class TestSkillEmbeddedShellGated(_Base):
+    """A skill's embedded ``!`` shell must go through the permission gate.
+
+    Previously ``_make_shell_executor`` ran ``BashTool.call`` directly, so
+    embedded shell bypassed permissions entirely. Now it is gated, with the
+    skill's ``allowed_tools`` injected as Bash allow rules.
+    """
+
+    def _exec(self, allowed, command, mode="default"):
+        from src.tool_system.tools.skill import _make_shell_executor
+
+        ctx = _ctx(mode, self.ws)
+        return _make_shell_executor(ctx, allowed, slash_command_name="/t")(command, False)
+
+    def test_declared_command_runs(self) -> None:
+        marker = self.ws / "declared.marker"
+        self._exec(["Bash(touch:*)"], f"touch {marker}")
+        self.assertTrue(marker.exists(), "declared command should execute")
+
+    def test_undeclared_command_blocked(self) -> None:
+        # Hard-denied in default mode (matches TS — not prompted, not run).
+        marker = self.ws / "undeclared.marker"
+        out = self._exec([], f"touch {marker}")
+        self.assertFalse(marker.exists(), "undeclared command must NOT execute")
+        self.assertIn("Error", out)
+
+    def test_dangerous_command_blocked_even_when_declared(self) -> None:
+        # Safety screen wins over an allowed_tools grant: the marker survives
+        # because the declared-but-destructive rm never runs.
+        marker = self.ws / "danger.marker"
+        marker.write_text("keep")
+        out = self._exec(["Bash(rm:*)"], f"rm -rf {marker}")
+        self.assertTrue(marker.exists(), "destructive command must be blocked despite being declared")
+        self.assertIn("Error", out)
+
+    def test_chained_command_blocked(self) -> None:
+        # Chaining can't ride in on a single-command allow rule.
+        marker = self.ws / "chain.marker"
+        out = self._exec(["Bash(echo:*)"], f"echo hi && touch {marker}")
+        self.assertFalse(marker.exists(), "chained command must not run")
+        self.assertIn("Error", out)
+
+    def test_bare_bash_grant_runs_safe_blocks_dangerous(self) -> None:
+        # A bare `Bash` allowed-tool grants all *non-screened* shell, but the
+        # safety screen still fires first for destructive commands.
+        safe = self.ws / "bare_safe.marker"
+        self._exec(["Bash"], f"touch {safe}")
+        self.assertTrue(safe.exists(), "bare Bash grant should run safe commands")
+        danger = self.ws / "bare_danger.marker"
+        danger.write_text("keep")
+        self._exec(["Bash"], f"rm -rf {danger}")
+        self.assertTrue(danger.exists(), "bare Bash grant must not bypass the safety screen")
+
+    def test_bypass_mode_runs_undeclared(self) -> None:
+        marker = self.ws / "bypass.marker"
+        self._exec([], f"touch {marker}", mode="bypassPermissions")
+        self.assertTrue(marker.exists())
+
+
 class TestRulesStillWin(_Base):
     def test_deny_rule_beats_central_allow(self) -> None:
         # A configured deny rule for an otherwise-allowed tool must still deny —