diff --git a/docs/design/agent-workflows/documentation/adapters/claude-code.md b/docs/design/agent-workflows/documentation/adapters/claude-code.md index 2be7ea12be..7e440be68f 100644 --- a/docs/design/agent-workflows/documentation/adapters/claude-code.md +++ b/docs/design/agent-workflows/documentation/adapters/claude-code.md @@ -55,12 +55,28 @@ attached. See [tools.md](../tools.md#status-and-known-gaps). ## Permissions -Claude gates tool use behind a permission prompt. In an Agenta run there is no human at the -keyboard to answer it, so the runner answers for it. By default it auto-approves, because the -tools are backend-resolved and trusted. The per-run permission policy (or an env override) -can flip this to deny, which rejects tool use instead. This is handled on -`session.onPermissionRequest`, a hook Pi does not need because Pi does not gate tools this -way. +Claude gates tool use behind its own permission prompt. There are two places a per-tool +permission lands, and both matter. + +The first is static, written before the session starts: the claude adapter renders a +`.claude/settings.json` file (`sdks/python/agenta/sdk/agents/adapters/claude_settings.py`, +delivered on the `harnessFiles` wire seam) whose `permissions.allow` / `permissions.ask` / +`permissions.deny` lists Claude Code reads via `settingSources`. Each backend-resolved EXECUTABLE +tool (callback/code) gets a per-tool rule `mcp__agenta-tools__`, because that is how Claude +addresses a tool of the internal `agenta-tools` MCP server above. A tool whose +`effective_permission()` is `allow` gets an allow rule, so Claude runs it without raising a gate; +`deny` gets a deny rule; `ask` or unset gets no allow rule, so the gate still fires. + +The second is dynamic: a gate that does fire arrives at `session.onPermissionRequest`, where the +runner answers for the (absent) human. When a human surface exists the runner parks the undecided +gate for a HITL approval round-trip; otherwise it falls to the per-run permission policy (auto or +deny). Pi does not need this hook because Pi does not gate tools this way. + +Both layers are needed because Claude's gate fires BEFORE the runner relay that would otherwise +honor an `allow`. Without the settings.json rule, an `allow` resolved tool always parked +(finding F-046); the rule is what lets it run. The `ask`/unset path is left to the gate on +purpose, so HITL approval is preserved. Note that `permission_policy: "auto"` is NOT a blanket +bypass — it still means "gate, then HITL or policy", not "allow everything". ## Tracing from the event stream diff --git a/docs/design/agent-workflows/interfaces/README.md b/docs/design/agent-workflows/interfaces/README.md index 88d645d705..2fafc07c49 100644 --- a/docs/design/agent-workflows/interfaces/README.md +++ b/docs/design/agent-workflows/interfaces/README.md @@ -55,7 +55,7 @@ page. `Status` is read from each page's prose: **stable** (wired and unlikely to | [Neutral runtime DTOs](in-service/neutral-runtime-dtos.md) | in-service | `agents/dtos.py` | stable | `unit/agents/test_dtos_*.py`, `test_harness_identity.py` | | [Runtime ports](in-service/runtime-ports.md) | in-service | `agents/interfaces.py` | evolving (`LocalBackend` stub) | `unit/agents/test_environment_lifecycle.py`, `test_harness_adapters.py` | | [Backend adapter](in-service/backend-adapter.md) | in-service | `agents/adapters/sandbox_agent.py` | stable | `unit/agents/test_runner_adapter_config.py`, `test_environment_lifecycle.py` | -| [Harness adapters](in-service/harness-adapters.md) | in-service | `agents/adapters/harnesses.py`, `agents/dtos.py` | stable | `unit/agents/test_harness_adapters.py`, `test_dtos_harness_configs.py` | +| [Harness adapters](in-service/harness-adapters.md) | in-service | `agents/adapters/harnesses.py`, `agents/adapters/claude_settings.py`, `agents/dtos.py` | stable | `unit/agents/test_harness_adapters.py`, `test_dtos_harness_configs.py`, `unit/agents/adapters/test_claude_settings.py` | | [Browser protocol adapter](in-service/browser-protocol-adapter.md) | in-service | `agents/adapters/vercel/{routing,messages,stream,sse}.py` | stable | `unit/agents/test_ui_messages.py`, `utils/test_messages_endpoint.py` | | [Tool models and resolution](in-service/tool-models-and-resolution.md) | in-service | `agents/tools/models.py`, `platform/gateway.py`, `agent/tools/resolver.py` | stable | `unit/agents/tools/` | | [MCP models and resolution](in-service/mcp-models-and-resolution.md) | in-service | `agents/mcp/{models,resolver,wire}.py` | evolving (stdio wired; remote deferred; resolution feature-gated) | `unit/agents/mcp/` | diff --git a/docs/design/agent-workflows/interfaces/in-service/harness-adapters.md b/docs/design/agent-workflows/interfaces/in-service/harness-adapters.md index 4dfd2c6711..1c0c3c1448 100644 --- a/docs/design/agent-workflows/interfaces/in-service/harness-adapters.md +++ b/docs/design/agent-workflows/interfaces/in-service/harness-adapters.md @@ -17,9 +17,11 @@ Each adapter implements `_to_harness_config(...)` and emits a different `/run` w overrides (`system`, `append_system`), and drops `permission_policy` because Pi never gates tool use. - **`ClaudeHarness`** delivers tools over MCP, not natively, and has no Pi built-ins (it warns - if any are set). It carries `permission_policy` and renders `.claude/settings.json` from - `harness_kwargs` and the sandbox permission, shipped as `harnessFiles`. It carries inline - skill packages on the wire like the others; the runner materializes them under + if any are set). It carries `permission_policy` and renders `.claude/settings.json` from four + sources — the author's `harness_kwargs["claude"]["permissions"]` slice, the sandbox permission, + each user MCP server's permission (`mcp__` rules), and each resolved EXECUTABLE tool's + permission (`mcp__agenta-tools__` rules; F-046) — shipped as `harnessFiles`. It carries + inline skill packages on the wire like the others; the runner materializes them under `.claude/skills` in the session cwd, matching Claude's project-local skill layout. - **`AgentaHarness`** runs on the same Pi engine but forces Agenta's opinion: it composes the base instructions over the author's, forces the Agenta tool set, and layers the Agenta @@ -54,3 +56,10 @@ The wire shapes, side by side: now pins the carry-on-wire behavior.) - **Harness options.** The `harness_kwargs` bag is keyed by harness; each adapter reads only its own slice. +- **Claude `agenta-tools` server-name coupling.** The per-resolved-tool settings.json rules use + the fixed name `mcp__agenta-tools__` (`INTERNAL_TOOL_MCP_SERVER` in + `adapters/claude_settings.py`). It MUST match the runner's internal tool-MCP server name + (`services/agent/src/tools/mcp-bridge.ts`, `relay-mcp-stdio.ts`, `tool-mcp-http.ts`, + `engines/sandbox_agent/mcp.ts`, `sdks/python/agenta/sdk/agents/adapters/claude_settings.py`). + Renaming the server on one side without the other silently + re-parks `allow` tools on Claude (the bug F-046 fixed). diff --git a/docs/design/agent-workflows/projects/capability-config/proposal.md b/docs/design/agent-workflows/projects/capability-config/proposal.md index a40fb76d25..1e037951f0 100644 --- a/docs/design/agent-workflows/projects/capability-config/proposal.md +++ b/docs/design/agent-workflows/projects/capability-config/proposal.md @@ -114,7 +114,20 @@ are two cases. Resolved tools (gateway, code) run in the runner, through the relay. The runner is the choke point, so it applies the permission directly: allow runs the call, ask parks it, deny -refuses it. This works the same on `pi` and `claude`. +refuses it. + +This is true on `pi`, but it was NOT true on `claude` until F-046 was fixed — and the fix +shows why the relay alone is not enough on Claude. A resolved executable tool is delivered to +Claude as a tool of the internal `agenta-tools` MCP server (`mcp__agenta-tools__`), and +**Claude Code raises its own permission gate before the tool ever reaches the runner relay**. +The runner parks every undecided gate when a human surface exists, so the per-tool `allow` the +relay would have honored is never consulted — an `allow` tool always parked. The fix renders the +per-resolved-tool permission as a Layer-1 `.claude/settings.json` rule too +(`adapters/claude_settings.py` `_rules_from_tool_specs`): `allow` -> a `permissions.allow` rule +(Claude runs it, no park), `ask`/unset -> no allow rule (the gate stays raised, HITL park +preserved; `client` tools excluded because they are not delivered over the internal MCP server), +`deny` -> a deny rule. So on Claude the permission is enforced at the settings layer +(before the gate) AND at the relay; on `pi` the relay is the sole choke point. Harness builtins (Claude `Bash`/`Edit`/`Read`, Pi `bash`/`read`) run inside the harness, where the runner cannot intercept them. For Claude, the settings.json `allow`/`deny`/`ask` rules set diff --git a/docs/design/agent-workflows/projects/capability-config/status.md b/docs/design/agent-workflows/projects/capability-config/status.md index 095d2d541d..0b9e0b0d29 100644 --- a/docs/design/agent-workflows/projects/capability-config/status.md +++ b/docs/design/agent-workflows/projects/capability-config/status.md @@ -137,6 +137,13 @@ its seven points: `network: off` guarantee. See `research.md` section 5. 2. **`read_only` enforcement strength.** Is `read_only` a hard runtime block on resolved tools, or only an advisory default for the permission? The honest first cut is advisory. + - ~~**Resolved-tool `permission:"allow"` parks on Claude (F-046).**~~ **Resolved by the F-046 + fix.** A resolved executable tool's per-tool permission is now rendered as a + `mcp__agenta-tools__` rule in `.claude/settings.json`, so an `allow` tool runs (no park) + while `ask`/unset keeps the HITL gate. The relay stays the choke point on `pi`. (See the S3b + "F-046 correction" note above and `proposal.md` §"Where Layer 3 is enforced".) Still open: the + batch `/invoke` sub-observation (HTTP 200 + empty content on a parked `ask` tool) — separate + from this permission fix. 3. ~~**Pi's real control surface (Phase 0).**~~ **Resolved by S0b.** Backend-dependent: supported in-process (`pi.ts:311` passes `tools`), unsupported over sandbox-agent ACP (`pi-acp@0.0.29` forwards nothing). Design: honor `builtin_names` in-process, fail loud over sandbox-agent. @@ -218,11 +225,21 @@ Smallest shippable, independently reviewable units. Each names its acceptance ch - **S3b — Layer 3 enforcement** (`done`, reviewed). Relay enforces resolved-tool permission (`resolvePermission`: deny→refusal string before any execution, allow→run, ask/unset→headless `permissionPolicy`); `permissionPolicy` threaded into `startToolRelay`, same resolver as the - Claude-builtin responder. `claude-settings.ts` renders per-MCP-server `mcp__` rules + Claude-builtin responder. The claude adapter (now Python: `adapters/claude_settings.py`, formerly + `claude-settings.ts`) renders per-MCP-server `mcp__` rules (name verified against `toAcpMcpServers`). Responder untouched (Claude builtins handled by S2 - settings.json + existing `PolicyResponder`); no fragile per-resolved-tool mcp rules. HITL "ask" + settings.json + existing `PolicyResponder`). HITL "ask" surfacing deferred to S5 (`TODO(S5)`). 166 TS pass. Reviewer APPROVED (deny-before-execute + MCP-name both verified against downstream consumers). + - **F-046 correction (2026-06-27).** S3b originally chose "no per-resolved-tool mcp rules", + on the premise that the relay is the choke point for resolved tools on every harness. That + premise was FALSE for Claude: a resolved executable tool is delivered as a tool of the internal + `agenta-tools` MCP server, and Claude Code's own permission gate fires BEFORE the relay, so the + runner parks it and the relay's `allow` is never consulted — a `permission:"allow"` tool ALWAYS + parked on Claude. Now WIRED: `_rules_from_tool_specs` in `adapters/claude_settings.py` renders a + per-tool `mcp__agenta-tools__` rule (allow→`permissions.allow`, ask/unset→no allow rule so + HITL park is preserved, deny→`permissions.deny`; `client` tools excluded). This does NOT make + `permission_policy:"auto"` a blanket bypass (that is a separate design question, out of scope). - **S4 — Playground form** (`done`, reviewed + fixed). New controls: `SandboxPermissionControl` (network mode/allowlist/filesystem/enforcement), `ClaudePermissionsControl` (mode + allow/deny/ask, gated to Claude harness, collapsible advanced), per-tool `ToolPermissionControl` @@ -320,6 +337,17 @@ gh pr create --head feat/agent-capability-config --base main `engines/pi.ts` (rejects restrictive sandbox_permission + deny/ask permissions), 8 tests, live rejection confirmed. 185 TS green. Note: `engines/pi.ts` is in the shared-files set (carries the guard); test `pi-capability-guard.test.ts` is new. +- 2026-06-27: Fixed F-046 (Claude resolved-tool permission). The Layer-1 claude-settings renderer + (`adapters/claude_settings.py`) now emits a per-resolved-tool `mcp__agenta-tools__` rule for + each EXECUTABLE (callback/code) tool, mapping `effective_permission()` → allow/ask/deny — mirroring + the per-MCP-server helper but against the runner's fixed internal `agenta-tools` server name (new + `INTERNAL_TOOL_MCP_SERVER` constant, coupled to `services/agent/src/tools/mcp-bridge.ts`). + `ClaudeAgentConfig.wire_harness_files` now passes `tool_specs` in. A `permission:"allow"` tool + runs on Claude instead of parking; `ask`/unset preserves the HITL park; `deny` blocks; `client` + tools excluded. Corrected the false S3b premise + the proposal claim that resolved-tool permission + "works the same on pi and claude". SDK claude-settings unit tests + the Claude golden + both wire + contract tests (Python + TS) updated. `permission_policy:"auto"`-as-blanket-bypass intentionally + NOT done (separate design question). - 2026-06-24: Landed Layers 1 + 3 + the playground (code-complete, reviewed). S2 (Claude `.claude/settings.json`, reviewed), S3a (permission plumbing), S3b (relay + MCP-rule enforcement, reviewed), S4 (playground form, reviewed) + S4b (fixed the `agenta_metadata`-strip diff --git a/sdks/python/agenta/sdk/agents/adapters/claude_settings.py b/sdks/python/agenta/sdk/agents/adapters/claude_settings.py index 13b25b7ce6..fe982657f5 100644 --- a/sdks/python/agenta/sdk/agents/adapters/claude_settings.py +++ b/sdks/python/agenta/sdk/agents/adapters/claude_settings.py @@ -7,7 +7,7 @@ ``settingSources: ["user", "project", "local"]`` (and applies ``permissions.defaultMode``); that file is the only clean Claude-config path because the sandbox-agent daemon strips ACP ``_meta``. -Three rule sources merge here: +Four rule sources merge here: - the AUTHOR's options (Layer 1), read from the generic ``harness_kwargs["claude"]["permissions"]`` slice: ``default_mode`` + per-tool ``allow``/``deny``/``ask`` strings. This is the only place the claude-specific shape of that slice is known. @@ -16,10 +16,19 @@ filesystem is read-only/off). A safety floor, not the primary enforcement. - rules DERIVED from per-MCP-server ``permission`` (Layer 3, S3b): each user MCP server with a set permission becomes a whole-server ``mcp__`` allow/ask/deny rule. - -Layer 3 enforcement is split by tool source: resolved tools (code / gateway-callback) run -runner-side and are enforced at the relay, NOT here. Only the per-MCP-server permission lands in -this file. + - rules DERIVED from each resolved EXECUTABLE tool's ``permission`` (Layer 3, tool path; F-046): + each callback/code tool becomes a per-tool ``mcp__agenta-tools__`` allow/ask/deny rule. + +Why the tool path needs a rule here (F-046): backend-resolved executable tools (callback/code) are +delivered to Claude as tools of the runner's internal ``agenta-tools`` MCP server. Claude Code +raises its OWN permission gate BEFORE running any tool, and the runner parks every undecided gate +when a human surface exists — so the tool's per-tool ``permission`` never reaches the runner relay +that honors ``allow``. Emitting an ``allow`` rule here is the only way an ``allow`` tool actually +runs on Claude instead of always parking. ``ask``/unset emits no allow rule (the gate stays raised +-> HITL park preserved); ``deny`` emits a deny rule (which also closes a local-Claude execution +gap). ``client`` tools are browser-fulfilled, never delivered over this channel, so they are +excluded. NOTE: this does NOT make ``permission_policy:"auto"`` a blanket bypass; ``auto`` still +means "gate -> HITL/policy". """ from __future__ import annotations @@ -34,6 +43,17 @@ # Where the rendered settings land, relative to the session cwd. SETTINGS_PATH = ".claude/settings.json" +# The fixed name of the runner's INTERNAL MCP server that delivers backend-resolved EXECUTABLE +# tools (callback/code) to the harness. Claude addresses one of a server's tools as +# ``mcp____``, so a per-tool permission rule for a resolved tool is +# ``mcp__agenta-tools__``. This name COUPLES to the runner constant and MUST stay in sync +# with the TypeScript runner, which advertises the same server name in: +# - ``services/agent/src/tools/mcp-bridge.ts`` (``name: "agenta-tools"``) +# - ``services/agent/src/tools/relay-mcp-stdio.ts`` and ``tool-mcp-http.ts`` (``serverInfo.name``) +# - ``services/agent/src/engines/sandbox_agent/mcp.ts`` +# If the runner renames this server, this constant must change with it. +INTERNAL_TOOL_MCP_SERVER = "agenta-tools" + def _string_list(value: Any) -> List[str]: """Keep only the string entries of an authored allow/deny/ask value; default to ``[]``.""" @@ -128,6 +148,47 @@ def _rules_from_mcp_permissions(mcp_servers: Any) -> Dict[str, List[str]]: return {"allow": allow, "ask": ask, "deny": deny} +def _rules_from_tool_specs(tool_specs: Any) -> Dict[str, List[str]]: + """Derive per-tool Claude rules from each resolved EXECUTABLE tool's Layer-3 ``permission`` (F-046). + + Mirrors :func:`_rules_from_mcp_permissions`, but per-tool against the fixed internal server name + ``agenta-tools``: a callback/code tool is delivered to Claude as a tool of that MCP server, so + its rule is ``mcp__agenta-tools__``. The tool's :meth:`ToolSpec.effective_permission` + (the single source of the allow/ask/deny ladder) routes it to the matching list; a tool whose + effective permission is unset (``None``) contributes nothing (falls back to the global + ``permission_policy``). ``client`` tools are browser-fulfilled and never delivered over this + channel, so they are excluded (this mirrors the runner's ``mcp-bridge`` filter). Accepts a list + of :class:`~agenta.sdk.agents.tools.models.ToolSpec` or plain dicts (coerced to a spec so the + same permission ladder applies). + """ + # Lazy import: ``tools.models`` does not import this adapter, but keeping the import local + # avoids loading the tool models when the claude adapter is used without resolved tools. + from ..tools.models import coerce_tool_spec + + allow: List[str] = [] + ask: List[str] = [] + deny: List[str] = [] + for raw in tool_specs or []: + try: + spec = coerce_tool_spec(raw) + except Exception: + # A malformed/nameless spec contributes nothing (mirrors the MCP helper's name guard). + continue + if spec.kind == "client": + continue + permission = spec.effective_permission() + if not permission: + continue + rule = f"mcp__{INTERNAL_TOOL_MCP_SERVER}__{spec.name}" + if permission == "allow": + allow.append(rule) + elif permission == "ask": + ask.append(rule) + elif permission == "deny": + deny.append(rule) + return {"allow": allow, "ask": ask, "deny": deny} + + def _get(obj: Any, key: str) -> Any: """Read ``key`` off a pydantic model (attribute) or a plain dict (item).""" if obj is None: @@ -141,15 +202,17 @@ def build_claude_settings_files( harness_kwargs: Optional[Dict[str, Any]], sandbox_permission: Any = None, mcp_servers: Any = None, + tool_specs: Any = None, ) -> List[Dict[str, str]]: """Build the Claude ``settings.json`` as a generic ``harnessFiles`` entry, or ``[]`` if none. Reads the author's Layer-1 options from ``harness_kwargs["claude"]["permissions"]``, merges - them with the Layer-2-derived rules (from ``sandbox_permission``) and the Layer-3-derived MCP - rules (from ``mcp_servers``), dedupes each list, and emits the smallest valid file: - ``permissions.defaultMode`` is set only when authored (and valid), and each allow/deny/ask list - appears only when non-empty. When there is nothing to write at all (no author options AND no - derived rules) it returns ``[]`` so the runner writes no file. + them with the Layer-2-derived rules (from ``sandbox_permission``), the Layer-3-derived MCP rules + (from ``mcp_servers``), and the Layer-3-derived per-resolved-tool rules (from ``tool_specs``, + F-046), dedupes each list, and emits the smallest valid file: ``permissions.defaultMode`` is set + only when authored (and valid), and each allow/deny/ask list appears only when non-empty. When + there is nothing to write at all (no author options AND no derived rules) it returns ``[]`` so + the runner writes no file. Returns ``[{"path": ".claude/settings.json", "content": }]`` or ``[]``. """ @@ -159,12 +222,22 @@ def build_claude_settings_files( # keeps first-seen order, so an author rule wins its position and derived rules append. sandbox_rules = _rules_from_sandbox_permission(sandbox_permission) mcp_rules = _rules_from_mcp_permissions(mcp_servers) + tool_rules = _rules_from_tool_specs(tool_specs) - allow = _dedupe([*author["allow"], *mcp_rules.get("allow", [])]) + allow = _dedupe( + [*author["allow"], *mcp_rules.get("allow", []), *tool_rules.get("allow", [])] + ) deny = _dedupe( - [*author["deny"], *sandbox_rules.get("deny", []), *mcp_rules.get("deny", [])] + [ + *author["deny"], + *sandbox_rules.get("deny", []), + *mcp_rules.get("deny", []), + *tool_rules.get("deny", []), + ] + ) + ask = _dedupe( + [*author["ask"], *mcp_rules.get("ask", []), *tool_rules.get("ask", [])] ) - ask = _dedupe([*author["ask"], *mcp_rules.get("ask", [])]) permissions: Dict[str, Any] = {} if "mode" in author: diff --git a/sdks/python/agenta/sdk/agents/dtos.py b/sdks/python/agenta/sdk/agents/dtos.py index 16395bd421..a8c1cf37fe 100644 --- a/sdks/python/agenta/sdk/agents/dtos.py +++ b/sdks/python/agenta/sdk/agents/dtos.py @@ -746,16 +746,22 @@ def wire_harness_files(self) -> Dict[str, Any]: """Render the Claude harness's permission settings into a ``.claude/settings.json`` file the runner drops in the cwd. This is the claude adapter (Layer 1 translation), done in Python: parse the author's ``harness_kwargs["claude"]["permissions"]`` slice, merge the - Layer-2 ``sandbox_permission`` derivation and the per-MCP-server Layer-3 permissions, and - emit one ``harnessFiles`` entry. Omitted when Claude has nothing to write (no author options - and no derived rules), so a boundary-free Claude run is byte-identical to before.""" + Layer-2 ``sandbox_permission`` derivation, the per-MCP-server Layer-3 permissions, and the + per-resolved-tool Layer-3 permissions (``tool_specs`` -> ``mcp__agenta-tools__`` rules, + F-046), and emit one ``harnessFiles`` entry. The resolved-tool rules matter because Claude + Code's own permission gate fires BEFORE the runner relay, so without an ``allow`` rule an + ``allow`` tool always parks. Omitted when Claude has nothing to write (no author options and + no derived rules), so a boundary-free Claude run is byte-identical to before.""" # Lazy import: ``adapters.claude_settings`` is light, but importing it at module top would # run ``adapters/__init__`` (which imports the harness adapters, which import this module), # so it is imported here to keep ``dtos`` free of that cycle. from .adapters.claude_settings import build_claude_settings_files files = build_claude_settings_files( - self.harness_kwargs, self.sandbox_permission, self.mcp_servers + self.harness_kwargs, + self.sandbox_permission, + self.mcp_servers, + self.tool_specs, ) if not files: return {} diff --git a/sdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.py b/sdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.py index 763c88e0f2..02fa8a8a84 100644 --- a/sdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.py +++ b/sdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.py @@ -11,9 +11,21 @@ import json -from agenta.sdk.agents.adapters.claude_settings import build_claude_settings_files +from agenta.sdk.agents.adapters.claude_settings import ( + INTERNAL_TOOL_MCP_SERVER, + build_claude_settings_files, +) from agenta.sdk.agents.dtos import SandboxPermission from agenta.sdk.agents.mcp import ResolvedMCPServer +from agenta.sdk.agents.tools.models import ( + CallbackToolSpec, + ClientToolSpec, + CodeToolSpec, +) + + +def _rule(name: str) -> str: + return f"mcp__{INTERNAL_TOOL_MCP_SERVER}__{name}" def _settings(files): @@ -174,3 +186,137 @@ def test_empty_inputs_render_nothing(): def test_non_claude_slice_renders_nothing(): # Only the `claude` slice is the claude adapter's concern; a `pi` slice contributes nothing. assert build_claude_settings_files({"pi": {"system": "You are Pi."}}) == [] + + +# --------------------------------------------------------------------------- F-046: +# per-resolved-tool rules for the internal `agenta-tools` MCP server. Backend-resolved +# executable tools (callback/code) are delivered to Claude as `mcp__agenta-tools__`; +# Claude's own permission gate fires before the runner relay, so the tool's permission must be +# rendered here or an `allow` tool always parks. + + +def test_allow_executable_tool_renders_allow_rule(): + # An explicit-`allow` callback tool produces `mcp__agenta-tools__` in `permissions.allow` + # so Claude runs it without raising its gate (no park). + spec = CallbackToolSpec( + name="capital_lookup", + description="d", + call_ref="workflow.x", + permission="allow", + ) + perms = _settings(build_claude_settings_files(None, None, None, [spec]))[ + "permissions" + ] + assert perms["allow"] == [_rule("capital_lookup")] + assert "ask" not in perms + assert "deny" not in perms + + +def test_read_only_executable_tool_derives_allow_rule(): + # No explicit permission + read_only=True -> effective `allow` -> an allow rule. + spec = CallbackToolSpec( + name="get_user", description="d", call_ref="tools__x", read_only=True + ) + perms = _settings(build_claude_settings_files(None, None, None, [spec]))[ + "permissions" + ] + assert perms["allow"] == [_rule("get_user")] + + +def test_code_tool_allow_renders_allow_rule(): + # `code` tools are executable too, so they get a rule. + spec = CodeToolSpec( + name="calc", description="d", code="print(1)", permission="allow" + ) + perms = _settings(build_claude_settings_files(None, None, None, [spec]))[ + "permissions" + ] + assert perms["allow"] == [_rule("calc")] + + +def test_ask_tool_not_in_allow(): + # An `ask` tool emits no allow rule -> the gate stays raised -> HITL park preserved. It rides + # the `ask` list (mirrors the per-MCP-server helper), never the `allow` list. + spec = CallbackToolSpec( + name="writer", description="d", call_ref="workflow.x", permission="ask" + ) + perms = _settings(build_claude_settings_files(None, None, None, [spec]))[ + "permissions" + ] + assert perms["ask"] == [_rule("writer")] + assert "allow" not in perms + + +def test_unset_tool_renders_no_rule(): + # No explicit permission, no read_only, no needs_approval -> effective permission is None -> + # no rule at all (falls back to the global `permission_policy`). With nothing else to write, + # the whole file is omitted. + spec = CallbackToolSpec(name="mystery", description="d", call_ref="workflow.x") + assert build_claude_settings_files(None, None, None, [spec]) == [] + + +def test_deny_tool_renders_deny_rule(): + # `deny` emits a deny rule (also closes a local-Claude execution-path gap). + spec = CallbackToolSpec( + name="danger", description="d", call_ref="workflow.x", permission="deny" + ) + perms = _settings(build_claude_settings_files(None, None, None, [spec]))[ + "permissions" + ] + assert perms["deny"] == [_rule("danger")] + assert "allow" not in perms + + +def test_client_tool_excluded(): + # `client` tools are browser-fulfilled, never delivered over the `agenta-tools` channel, so + # they contribute no rule even with an explicit `allow`. + spec = ClientToolSpec(name="ui_pick", description="d", permission="allow") + assert build_claude_settings_files(None, None, None, [spec]) == [] + + +def test_tool_rules_merge_with_author_and_mcp(): + # Author allow/deny first, then the per-MCP-server rule, then the per-tool rule append (deduped, + # first-seen order preserved). + server = ResolvedMCPServer( + name="github", transport="http", url="https://x", permission="allow" + ) + allow_tool = CallbackToolSpec( + name="capital_lookup", + description="d", + call_ref="workflow.x", + permission="allow", + ) + deny_tool = CodeToolSpec(name="rm", description="d", code="x", permission="deny") + perms = _settings( + build_claude_settings_files( + _claude({"allow": ["Read"], "deny": ["Write"]}), + None, + [server], + [allow_tool, deny_tool], + ) + )["permissions"] + assert perms["allow"] == ["Read", "mcp__github", _rule("capital_lookup")] + assert perms["deny"] == ["Write", _rule("rm")] + + +def test_tool_rules_accept_plain_dicts(): + # The builder coerces plain wire dicts so the same permission ladder applies; a `client` dict + # is excluded. + perms = _settings( + build_claude_settings_files( + None, + None, + None, + [ + { + "name": "get_user", + "description": "d", + "callRef": "tools__x", + "kind": "callback", + "readOnly": True, + }, + {"name": "ui_pick", "description": "d", "kind": "client"}, + ], + ) + )["permissions"] + assert perms["allow"] == [_rule("get_user")] diff --git a/sdks/python/oss/tests/pytest/unit/agents/golden/run_request.claude.json b/sdks/python/oss/tests/pytest/unit/agents/golden/run_request.claude.json index 85aa932eee..f85150cfbb 100644 --- a/sdks/python/oss/tests/pytest/unit/agents/golden/run_request.claude.json +++ b/sdks/python/oss/tests/pytest/unit/agents/golden/run_request.claude.json @@ -29,7 +29,7 @@ "harnessFiles": [ { "path": ".claude/settings.json", - "content": "{\n \"permissions\": {\n \"defaultMode\": \"acceptEdits\",\n \"allow\": [\n \"Read\",\n \"Bash(npm run:*)\"\n ],\n \"deny\": [\n \"WebFetch\"\n ]\n }\n}" + "content": "{\n \"permissions\": {\n \"defaultMode\": \"acceptEdits\",\n \"allow\": [\n \"Read\",\n \"Bash(npm run:*)\",\n \"mcp__agenta-tools__get_user\"\n ],\n \"deny\": [\n \"WebFetch\"\n ]\n }\n}" } ], "skills": [ diff --git a/sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py b/sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py index 0fb0d0aa1e..3a92b99c7e 100644 --- a/sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py +++ b/sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py @@ -234,6 +234,9 @@ def test_request_to_wire_claude_matches_golden(golden): assert "sandboxPermission" not in payload # The claude adapter (Python) translated the author's permissions slice into a rendered # `.claude/settings.json`, carried on the generic `harnessFiles` seam. The runner writes it blind. + # The `allow` list also carries the per-resolved-tool rule for the internal `agenta-tools` MCP + # server (F-046): `_CUSTOM_TOOL` is a read-only callback tool -> effective `allow` -> + # `mcp__agenta-tools__get_user`, so Claude runs it instead of parking on its own permission gate. assert payload["harnessFiles"] == [ { "path": ".claude/settings.json", @@ -241,7 +244,11 @@ def test_request_to_wire_claude_matches_golden(golden): { "permissions": { "defaultMode": "acceptEdits", - "allow": ["Read", "Bash(npm run:*)"], + "allow": [ + "Read", + "Bash(npm run:*)", + "mcp__agenta-tools__get_user", + ], "deny": ["WebFetch"], } }, diff --git a/services/agent/tests/unit/wire-contract.test.ts b/services/agent/tests/unit/wire-contract.test.ts index ffb6071a38..a402d21067 100644 --- a/services/agent/tests/unit/wire-contract.test.ts +++ b/services/agent/tests/unit/wire-contract.test.ts @@ -135,7 +135,14 @@ describe("wire contract: requests (vs Python golden)", () => { permissions: Record; }; assert.equal(settings.permissions.defaultMode, "acceptEdits"); - assert.deepEqual(settings.permissions.allow, ["Read", "Bash(npm run:*)"]); + // The allow list also carries the per-resolved-tool rule for the internal `agenta-tools` MCP + // server (F-046): the golden's `get_user` is a read-only callback tool -> effective `allow` -> + // `mcp__agenta-tools__get_user`, so Claude runs it instead of parking on its own permission gate. + assert.deepEqual(settings.permissions.allow, [ + "Read", + "Bash(npm run:*)", + "mcp__agenta-tools__get_user", + ]); assert.deepEqual(settings.permissions.deny, ["WebFetch"]); // Claude carries resolved inline skills on the same `skills` seam Pi uses; the runner // installs them into Claude's project-local `.claude/skills/` tree. This regressed