From 094acda1e917d7f7052f11c520d1216720e46c96 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Thu, 30 Jul 2026 21:20:42 +0200 Subject: [PATCH 1/5] fix(sdk): ship Pi's built-in tools in the default agent template A saved agent shipped `tools: []`, which the runner reads as "grant Pi no built-in tools", so the agent had no read, bash, edit or write anywhere except the playground. The playground hid it: its build kit overlay adds read and bash back on every run, and nothing else does. The default template now ships Pi's own four defaults as typed builtin entries. The runner's grant semantics are untouched, and the permission mode still gates bash, edit and write behind an approval. Also: the Claude harness only warns about built-ins that differ from that exact set, the built-in picker no longer claims an empty selection leaves Pi's defaults on, and a built-in row shows its own name. Closes #5590 --- .../agent-release-gate/resources/qa_probe.py | 10 +- .../resources/qa_product.py | 10 +- .../unit/resources/test_workflow_catalog.py | 32 +- .../documentation/agent-configuration.md | 5 +- .../agent-workflows/documentation/tools.md | 26 +- .../agent-workflows/interfaces/README.md | 4 +- .../interfaces/in-service/harness-adapters.md | 14 +- .../public-edge/agent-config-schema.md | 31 +- .../projects/default-agent-builtins/README.md | 64 +++ .../default-agent-builtins/context.md | 138 ++++++ .../projects/default-agent-builtins/design.md | 451 ++++++++++++++++++ .../default-agent-builtins/open-questions.md | 158 ++++++ .../projects/default-agent-builtins/plan.md | 101 ++++ .../default-agent-builtins/research.md | 251 ++++++++++ .../projects/default-agent-builtins/status.md | 287 +++++++++++ .../default-agent-builtins/testing.md | 185 +++++++ .../sdk/agents/adapters/agenta_builtins.py | 12 +- .../agenta/sdk/agents/adapters/harnesses.py | 12 +- sdks/python/agenta/sdk/agents/pi_builtins.py | 17 + sdks/python/agenta/sdk/utils/types.py | 10 +- .../golden/pi_default_active_builtins.json | 4 + .../unit/agents/test_harness_adapters.py | 50 ++ .../unit/agents/test_pi_builtins_parity.py | 23 + .../pytest/unit/agents/test_wire_contract.py | 36 ++ services/oss/src/agent/config.py | 13 +- .../agent/test_config_template_fallback.py | 16 +- .../unit/agent/test_default_agent_template.py | 32 +- services/runner/config/agent.json | 7 +- .../src/engines/sandbox_agent/run-plan.ts | 7 +- .../unit/pi-default-builtins-parity.test.ts | 34 ++ .../tests/unit/sandbox-agent-run-plan.test.ts | 50 ++ ...create-ephemeral-app-from-template.test.ts | 115 +++++ .../SchemaControls/PiSettingsControl.tsx | 12 +- .../agentTemplate/itemDescriptors.tsx | 5 +- .../tests/unit/itemDescriptors.test.ts | 41 ++ .../tests/unit/agentRequest.test.ts | 37 ++ 36 files changed, 2266 insertions(+), 34 deletions(-) create mode 100644 docs/design/agent-workflows/projects/default-agent-builtins/README.md create mode 100644 docs/design/agent-workflows/projects/default-agent-builtins/context.md create mode 100644 docs/design/agent-workflows/projects/default-agent-builtins/design.md create mode 100644 docs/design/agent-workflows/projects/default-agent-builtins/open-questions.md create mode 100644 docs/design/agent-workflows/projects/default-agent-builtins/plan.md create mode 100644 docs/design/agent-workflows/projects/default-agent-builtins/research.md create mode 100644 docs/design/agent-workflows/projects/default-agent-builtins/status.md create mode 100644 docs/design/agent-workflows/projects/default-agent-builtins/testing.md create mode 100644 sdks/python/agenta/sdk/agents/pi_builtins.py create mode 100644 sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json create mode 100644 sdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.py create mode 100644 services/runner/tests/unit/pi-default-builtins-parity.test.ts create mode 100644 web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts create mode 100644 web/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.ts diff --git a/.agents/skills/agent-release-gate/resources/qa_probe.py b/.agents/skills/agent-release-gate/resources/qa_probe.py index c4ef604558..67ba4f1a39 100644 --- a/.agents/skills/agent-release-gate/resources/qa_probe.py +++ b/.agents/skills/agent-release-gate/resources/qa_probe.py @@ -79,7 +79,15 @@ def agent_template(harness: str, sandbox: str, model: str, provider: str) -> dic "connection": {"mode": "agenta", "slug": None}, "extras": {}, }, - "tools": [], + # Pi's default active built-ins, matching the shipped default agent template. An empty + # list means "grant nothing" to the runner, so seeding `[]` would gate the release on a + # configuration no real agent uses (issue #5590). + "tools": [ + {"type": "builtin", "name": "read"}, + {"type": "builtin", "name": "bash"}, + {"type": "builtin", "name": "edit"}, + {"type": "builtin", "name": "write"}, + ], "mcps": [], "skills": [], "harness": {"kind": harness}, diff --git a/.agents/skills/agent-release-gate/resources/qa_product.py b/.agents/skills/agent-release-gate/resources/qa_product.py index f3b1eb5a69..2c508116ae 100644 --- a/.agents/skills/agent-release-gate/resources/qa_product.py +++ b/.agents/skills/agent-release-gate/resources/qa_product.py @@ -749,7 +749,15 @@ def j5_commit(cell: dict) -> dict: "agent": { "instructions": {"agents_md": "seed"}, "llm": {"model": cell["model"], "provider": cell["provider"]}, - "tools": [], + # Pi's default active built-ins, matching the shipped default agent template. An + # empty list means "grant nothing" to the runner, so committing `[]` would gate + # the release on a configuration no real agent commits (issue #5590). + "tools": [ + {"type": "builtin", "name": "read"}, + {"type": "builtin", "name": "bash"}, + {"type": "builtin", "name": "edit"}, + {"type": "builtin", "name": "write"}, + ], "harness": {"kind": cell["harness"]}, "sandbox": {"kind": cell["sandbox"]}, } diff --git a/api/oss/tests/pytest/unit/resources/test_workflow_catalog.py b/api/oss/tests/pytest/unit/resources/test_workflow_catalog.py index 9787b7e1b1..348d3949bc 100644 --- a/api/oss/tests/pytest/unit/resources/test_workflow_catalog.py +++ b/api/oss/tests/pytest/unit/resources/test_workflow_catalog.py @@ -1,4 +1,10 @@ -from oss.src.resources.workflows.catalog import get_workflow_catalog_preset +from agenta.sdk.agents.pi_builtins import PI_DEFAULT_ACTIVE_BUILTINS +from agenta.sdk.engines.running.catalog import get_all_catalog_templates + +from oss.src.resources.workflows.catalog import ( + _build_template_data, + get_workflow_catalog_preset, +) def test_feedback_quality_rating_preset_is_preserved_from_sdk_catalog(): @@ -14,3 +20,27 @@ def test_feedback_quality_rating_preset_is_preserved_from_sdk_catalog(): assert preset.data.uri == "agenta:custom:feedback:v0" assert preset.data.schemas is not None assert preset.data.schemas.outputs is not None + + +def test_agent_template_data_materializes_the_default_builtin_tools(): + """The agent template a new agent is created from must carry Pi's built-ins (issue #5590). + + `_build_template_data` hoists the `agent` property's JSON-Schema default into a materialized + `data["parameters"]` block, then strips that non-primitive default back off the schema. That + hoist is the only thing that carries the shipped default into a newly created agent's + parameters, and the strip means the schema can no longer be the fallback source. Without this + test, an empty or dropped `tools` list would pass every builder-level test and still reach the + runner as "grant nothing", leaving saved agents with no read, bash, edit or write. + """ + expected_tools = [ + {"type": "builtin", "name": name} for name in PI_DEFAULT_ACTIVE_BUILTINS + ] + + entry = next( + entry for entry in get_all_catalog_templates() if entry["key"] == "agent" + ) + data = _build_template_data(entry["data"], settings_template=None) + + assert data is not None + assert data["parameters"]["agent"]["tools"] == expected_tools + assert "default" not in data["schemas"]["parameters"]["properties"]["agent"] diff --git a/docs/design/agent-workflows/documentation/agent-configuration.md b/docs/design/agent-workflows/documentation/agent-configuration.md index 802bff70cf..db4f6acd02 100644 --- a/docs/design/agent-workflows/documentation/agent-configuration.md +++ b/docs/design/agent-workflows/documentation/agent-configuration.md @@ -216,7 +216,7 @@ Legend: (a) catalog/schema, (b) SDK neutral config, (c) runtime. | Field | (a) schema | (b) SDK config | (c) runtime | Status | | --- | --- | --- | --- | --- | | model / provider | yes, `model: str` | yes, `Optional[str]` | wired to the runner | Loose string. No `ModelRef`, no provider enum. There is no separate provider field. | -| tools | yes, strict list | yes, lenient coercion | wired, resolved to builtin names + tool specs | Entries strict, list lenient. | +| tools | yes, strict list | yes, lenient coercion | wired, resolved to builtin names + tool specs | Entries strict, list lenient. The shipped default template fills it with Pi's four default built-ins (`read`, `bash`, `edit`, `write`); see [Tools](tools.md). | | mcp_servers | yes, strict list | yes | wired, resolved to runner MCP servers | Strict per entry. Claude supports external HTTP servers; Pi refuses them until its bridge exists. | | skills | yes, embed/inline list | yes | wired | Author-settable (`SkillConfig` inline or `@ag.embed` references). The playground build-kit overlay embeds one skill, the `build-an-agent` playbook; the `pi_agenta` harness additionally force-unions `getting-started`. See below. | | persona | no | no | wired but forced only | Not a config field. The Agenta harness hardcodes an append-system preamble. See below. | @@ -238,7 +238,8 @@ Pi (`pi_core`) and Claude harnesses get no forced skills or persona. Per-harness divergence is real in other ways, but not in permission enforcement anymore: the permission policy is now enforced on both Claude and Pi. Builtin tool names are dropped for -Claude with a warning, because builtins are Pi-only. Forced skills and persona are +Claude, because builtins are Pi-only; that drop warns only when the set differs from Pi's four +defaults, which is the set the shipped template carries. Forced skills and persona are Agenta-only. Pi's `system` and `append_system` overrides come through the `harness_kwargs` escape hatch on the neutral config, which is itself absent from the schema. diff --git a/docs/design/agent-workflows/documentation/tools.md b/docs/design/agent-workflows/documentation/tools.md index be1fbfb28f..826074680a 100644 --- a/docs/design/agent-workflows/documentation/tools.md +++ b/docs/design/agent-workflows/documentation/tools.md @@ -30,7 +30,7 @@ shares two fields through `ToolConfigBase`, and then a `type` discriminator pick | Config (`type`) | Carries | Example use | | --- | --- | --- | -| `builtin` | `name` | A harness-native tool such as Pi's `read` or `web_search`. | +| `builtin` | `name` | A harness-native tool such as Pi's `read` or `web_search`. The shipped default template grants Pi's four default built-ins (`read`, `bash`, `edit`, `write`); see [the grant list](#built-in-tools-the-harness-runs-them-natively-gated-through-the-same-relay). | | `gateway` | `provider`, `integration`, `action`, `connection`, optional `name` | A Composio action, like `github__create_issue` on a connected account. | | `code` | `name`, `runtime` (`python`/`node`), `script`, `input_schema`, `secrets` | An inline snippet the author writes, with named vault secrets injected. | | `client` | `name`, `input_schema` | A tool the browser fulfils, like "ask the user to pick a date." | @@ -401,6 +401,30 @@ separately, at session start. The extension edits Pi's active tool set at every non-builtin tool untouched. A builtin outside the grant list is simply absent from the model's active tools, so no call for it ever fires, and the permission hook never sees it. +Three values of the wire `tools` field mean three different things, and the difference is the +whole reason this list exists: + +- **Omitted.** The runner falls back to Pi's own default active set, `read`, `bash`, `edit`, + `write` (`PI_DEFAULT_ACTIVE_BUILTINS` in `run-plan.ts`). +- **`[]`.** Grant nothing. Every builtin is removed from the model's active tools. This is an + author saying "no builtins", not an author saying nothing. +- **A list of names.** Grant exactly those. + +The shipped default agent template names Pi's four defaults explicitly rather than leaving the +field empty, so a saved agent carries the same builtins Pi would have activated on its own. The +Python side of that list is `PI_DEFAULT_ACTIVE_BUILTINS` in +`sdks/python/agenta/sdk/agents/pi_builtins.py`; it and the runner's TypeScript constant are two +implementations of one contract, pinned against the shared golden fixture +`sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json`. + +Granting a builtin is not the same as allowing it to run. Under the default permission mode +`allow_reads`, a default agent runs `read` without asking and raises an approval on every +`bash`, `edit`, and `write` call. + +Agents saved before that default shipped still carry `tools: []` and therefore still have no +builtins. Nothing repairs them automatically. Their author has to open the agent, expand +Advanced, select the built-ins in the "Built-in tools" control, and commit a new revision. + ### External MCP servers: remote HTTP connections A declared user MCP server contains identity, an HTTP connection, credential references, and diff --git a/docs/design/agent-workflows/interfaces/README.md b/docs/design/agent-workflows/interfaces/README.md index 1720de6399..8d694da840 100644 --- a/docs/design/agent-workflows/interfaces/README.md +++ b/docs/design/agent-workflows/interfaces/README.md @@ -44,7 +44,7 @@ page. `Status` is read from each page's prose: **stable** (wired and unlikely to | [`/invoke`](public-edge/workflow-invoke.md) | public | `decorators/routing.py`, `models/workflows.py`, `agent/app.py` | stable | `unit/agent/`, `utils/test_messages_endpoint.py` | | [`/inspect`](public-edge/workflow-inspect.md) | public | `agent/schemas.py`, `agent/app.py` (builtin-URI binding), `models/workflows.py`, `decorators/routing.py` | stable | `unit/agents/test_dtos_agent_config.py`, `unit/agent/test_builtin_uri_binding.py` | | [`/messages`](public-edge/agent-messages.md) | public | `adapters/vercel/{routing,messages,stream}.py`, `agentRequest.ts` | evolving (create-or-resume not observable until storage lands) | `utils/test_messages_endpoint.py`, `unit/agents/test_ui_messages.py` | -| [Agent config schema](public-edge/agent-config-schema.md) | public | `agent/schemas.py`, `sdk/utils/types.py`, `agents/dtos.py` (`HARNESS_IDENTITIES`) | stable | `unit/agents/test_dtos_agent_config.py`, `unit/agents/test_harness_identity.py` | +| [Agent config schema](public-edge/agent-config-schema.md) | public | `agent/schemas.py`, `sdk/utils/types.py`, `agents/dtos.py` (`HARNESS_IDENTITIES`), `sdk/agents/pi_builtins.py` (`PI_DEFAULT_ACTIVE_BUILTINS`) | stable | `unit/agents/test_dtos_agent_config.py`, `unit/agents/test_harness_identity.py`, `unit/agents/test_pi_builtins_parity.py` + `golden/pi_default_active_builtins.json`, `services/oss unit/agent/test_default_agent_template.py` | | [`/run`](cross-service/service-to-agent-runner.md) | cross-service (the spine) | `protocol.ts`, `utils/wire.py`, `utils/ts_runner.py`, `server.ts`/`cli.ts` | stable (pinned by golden) | `unit/agents/test_wire_contract.py` + `golden/`, `services/agent/tests/unit/wire-contract.test.ts` | | [Runner to harness](cross-service/runner-to-harness.md) | cross-service (ACP) | `engines/sandbox_agent.ts` + `sandbox_agent/{run-plan,capabilities,permissions}.ts` | evolving | `services/agent/tests/unit/sandbox-agent-*.test.ts` | | [Runner to MCP server](cross-service/runner-to-mcp-server.md) | cross-service | `agents/mcp/`, `engines/sandbox_agent/{mcp,tool-mcp-assets,relay-guard}.ts`, `tools/{mcp-bridge,tool-mcp-http,tool-mcp-stdio,tool-mcp-env,relay,relay-client,relay-protocol,relay-watch}.ts` | evolving (internal channel delivered locally over loopback HTTP and on Daytona via the in-sandbox stdio shim, `client` tools included — a client call parks via a paused relay answer; user stdio disabled) | `services/runner/tests/unit/{mcp-servers,session-mcp-layering,tool-mcp-assets,tool-mcp-stdio,tool-relay-guard}.test.ts` | @@ -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/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` | +| [Harness adapters](in-service/harness-adapters.md) | in-service | `agents/adapters/harnesses.py`, `agents/adapters/claude_settings.py`, `agents/dtos.py`, `sdk/agents/pi_builtins.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,workflow,op_catalog,platform_tools}.py`, `agent/tools/resolver.py` | evolving | `unit/agents/tools/`, `unit/agents/platform/test_op_catalog.py` | | [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 afcefcae89..d8b71740dc 100644 --- a/docs/design/agent-workflows/interfaces/in-service/harness-adapters.md +++ b/docs/design/agent-workflows/interfaces/in-service/harness-adapters.md @@ -18,8 +18,12 @@ Each adapter implements `_to_harness_config(...)` and emits a different `/run` w harnesses. Pi has no native permission gate of its own (no `.claude/settings.json` equivalent), so the runner's tool relay enforces `permissions` for Pi at execution time; an `ask` verdict pauses the run and Pi gets the same human-in-the-loop approval Claude gets at its gate. -- **`ClaudeHarness`** delivers tools over MCP, not natively, and has no Pi built-ins (it warns - if any are set). It carries `permissions` and renders `.claude/settings.json` from four +- **`ClaudeHarness`** delivers tools over MCP, not natively, and has no Pi built-ins: it drops + the names. It warns only when the set differs from `PI_DEFAULT_ACTIVE_BUILTINS`, because the + shipped default template carries exactly that set and warning on it would fire on nearly every + Claude run. A set the author touched (a subset, a superset, an unrelated name) still warns, and + the message names the dropped tools. It carries `permissions` 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 @@ -45,6 +49,8 @@ The wire shapes, side by side: - `sdks/python/agenta/sdk/agents/adapters/harnesses.py`: the three adapters. - `sdks/python/agenta/sdk/agents/dtos.py`: the `PiAgentConfig`/`ClaudeAgentConfig`/ `AgentaAgentConfig` wire emitters. +- `sdks/python/agenta/sdk/agents/pi_builtins.py`: `PI_DEFAULT_ACTIVE_BUILTINS`, the set + `ClaudeHarness` stays silent about. ## Watch for when changing @@ -52,6 +58,10 @@ The wire shapes, side by side: tools natively; everyone else gets them over the MCP bridge. - **Prompt override behavior.** Pi replaces or appends; Claude reads options; Agenta composes. - **Forced Agenta behavior.** Instruction composition and the forced tool set are deliberate. +- **The Claude built-in warning predicate.** It is exact-set equality against + `PI_DEFAULT_ACTIVE_BUILTINS`, not a per-name filter. A per-name filter would silence an + authored subset such as `["bash"]`, which is exactly the case the warning exists for. If the + default template's built-in set changes, this predicate changes with it. - **Claude skill delivery.** Claude wires inline skills like the other harnesses; the runner materializes them under `.claude/skills`. (An earlier revision suppressed Claude's `wire_skills()` to `{}`; that override is gone, and `test_claude_carries_skills_for_project_local_materialization` diff --git a/docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md b/docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md index 94dc0f1a44..a918dcb805 100644 --- a/docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md +++ b/docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md @@ -21,7 +21,7 @@ The fields and the full schema follow. |---|---|---|---| | `agents_md` | string (textarea) | hello-world prompt | The agent's system prompt, its AGENTS.md. | | `model` | string (`grouped_choice`) | `"gpt-5.5"` | Model the agent runs on. A plain id (`"gpt-5.5"`) or a structured `{provider, connection}` ref. See [Model connection resolution](../in-service/model-connection-resolution.md). | -| `tools` | `(ToolConfig \| EmbedRef)[]` | `[]` | Runnable tools: `builtin`, `gateway`, `code`, `client`, `reference` (a workflow referenced as a tool — `type: "reference"` — the service runs server-side as a callback tool), or `platform` (an existing Agenta endpoint exposed to the agent — `type: "platform"` — the runner calls it directly). A workflow value can also be inlined via `@ag.embed`. See [Tool models and resolution](../in-service/tool-models-and-resolution.md). | +| `tools` | `(ToolConfig \| EmbedRef)[]` | four `builtin` entries: `read`, `bash`, `edit`, `write` (the schema's own default is `[]`; the shipped template fills it) | Runnable tools: `builtin`, `gateway`, `code`, `client`, `reference` (a workflow referenced as a tool — `type: "reference"` — the service runs server-side as a callback tool), or `platform` (an existing Agenta endpoint exposed to the agent — `type: "platform"` — the runner calls it directly). A workflow value can also be inlined via `@ag.embed`. See [Tool models and resolution](../in-service/tool-models-and-resolution.md). | | `mcp_servers` | `MCPServerConfig[]` | `[]` | External HTTP MCP servers; named header-secret references resolve from the vault per run. See [MCP models and resolution](../in-service/mcp-models-and-resolution.md). | | `harness` | `"pi_core" \| "claude" \| "pi_agenta"` (see slug+name note) | `"pi_core"` | The coding agent to drive. `pi_core` and `pi_agenta` both drive the `pi` ACP agent; `pi_agenta` adds Agenta's forced skills, prompt, and policy. | | `sandbox` | `"local" \| "daytona"` | `"local"` | Where it runs. | @@ -72,7 +72,12 @@ every field in its default state: { "agents_md": "You are a friendly hello-world agent running on the Agenta agent service.\n\n- Greet the user warmly.\n- Answer the user's message in one or two short sentences.", "model": "gpt-5.5", - "tools": [], + "tools": [ + { "type": "builtin", "name": "read" }, + { "type": "builtin", "name": "bash" }, + { "type": "builtin", "name": "edit" }, + { "type": "builtin", "name": "write" } + ], "mcp_servers": [], "harness": "pi_core", "sandbox": "local", @@ -92,6 +97,20 @@ every field in its default state: } ``` +The four `builtin` entries are Pi's own default active set, held in Python as +`PI_DEFAULT_ACTIVE_BUILTINS` (`sdks/python/agenta/sdk/agents/pi_builtins.py`) and pinned against +the runner's TypeScript constant of the same name by the shared golden fixture +`sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json`. Granting a +built-in is not the same as allowing it to run: under the default `permissions.default` of +`allow_reads`, `read` runs without asking, and `bash`, `edit`, and `write` raise an approval on +every call. The `/run` wire field is unchanged; the runner still reads `tools: []` as "grant +nothing" and an omitted `tools` as "Pi's defaults". See +[Tools](../../documentation/tools.md) for the grant list and the permission relay. + +Agents saved before this default shipped still carry an empty `tools` list. Nothing repairs them +automatically. Their author has to open the agent, expand Advanced, select the built-ins in the +"Built-in tools" control, and commit. + The skill embed above comes from the playground **build-kit overlay** (`build_agent_template_overlay` in `api/oss/src/apis/fastapi/applications/overlay.py`), not from the bare default: the overlay adds the default platform ops (`DEFAULT_BUILD_KIT_OPS`), @@ -270,6 +289,8 @@ not `SKILL.md` itself. `agenta:builtin:agent:v0`, whose default calls the same builder bare. - `sdks/python/agenta/sdk/agents/dtos.py`: the permissive runtime `AgentConfig` parser and `SandboxPermission`. +- `sdks/python/agenta/sdk/agents/pi_builtins.py`: `PI_DEFAULT_ACTIVE_BUILTINS`, the built-in + names the default `tools` list is built from. ## Watch for when changing @@ -279,6 +300,12 @@ not `SKILL.md` itself. without updating the catalog breaks the form silently. - **The default config.** It is shipped on `/inspect` and is what an untouched form runs. It has one source, `build_agent_v0_default`; change a default field there, not in each consumer. +- **The default built-in tools.** The `tools` default is built from + `PI_DEFAULT_ACTIVE_BUILTINS`, which must stay equal to the runner's TypeScript constant of the + same name. Both are pinned against + `sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json`; change the + fixture and both languages, or the parity tests fail. Widening the set past Pi's own defaults + also turns the runner's built-in gating relay on for agents that would otherwise skip it. - **Nested shapes.** `tools`, `mcp_servers`, `skills`, and `sandbox_permission` each have their own page and their own wire fields. A change here usually means a change there and a golden fixture. diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/README.md b/docs/design/agent-workflows/projects/default-agent-builtins/README.md new file mode 100644 index 0000000000..0e96390b89 --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/README.md @@ -0,0 +1,64 @@ +# Default agent built-in tools + +A new Pi agent works in the playground and has no tools anywhere else. The shipped default agent +template lists no tools, and an empty tool list means "grant nothing" by the time it reaches Pi. +This workspace plans the fix: ship Pi's four default built-ins in the default agent template. + +Reported as [#5590](https://github.com/Agenta-AI/agenta/issues/5590) (from the agent's side) and +[#5562](https://github.com/Agenta-AI/agenta/issues/5562) (from the automation's side). + +## Reading order + +| File | The question it answers | +| --- | --- | +| [context.md](context.md) | What does a user see, why does it happen, and what is in and out of scope | +| [research.md](research.md) | What the code does today, with file and line references | +| [design.md](design.md) | What we change, what we considered instead, and why | +| [plan.md](plan.md) | The order of work, split into landable pieces | +| [testing.md](testing.md) | Which tests to write and where, including the one that would have caught this | +| [open-questions.md](open-questions.md) | What is not decided and who decides it | +| [status.md](status.md) | Current state of the work | + +## Words used here + +- **Harness**: the coding agent the platform drives inside the sandbox. Today `pi_core` (Pi), + `pi_agenta` (Pi with a forced Agenta opinion layered on), and `claude` (Claude Code). +- **Runner**: the TypeScript service at `services/runner/` that receives a `/run` request, starts + the harness in a sandbox, and streams events back. +- **Sandbox**: the isolated filesystem and process space a run executes in, either `local` (a + temporary directory on the runner host) or `daytona` (a remote container). +- **Built-in**: one of the seven tools Pi implements inside itself: `read`, `bash`, `edit`, + `write`, `grep`, `find`, `ls`. The model calls a built-in the same way it calls any other tool, + but the code runs inside Pi rather than inside Agenta. +- **Grant list**: the `tools` field of a `/run` request. It names which Pi built-ins this run may + use. The runner deletes every built-in that is not named. +- **Agent template**: the saved agent configuration at `parameters.agent` of a workflow revision. + It holds instructions, model, tools, MCP servers, skills, and the execution selectors + (`harness`, `runner`, `sandbox`). +- **Build kit overlay**: an extra fragment of agent template the backend serves to the playground + and the playground merges into a run. It adds authoring tools and an authoring skill. It is + never saved into the agent. +- **Permission mode**: the agent-wide policy for whether a tool runs, pauses for approval, or is + refused. The four modes are `allow`, `ask`, `deny`, and `allow_reads`. The shipped default is + `allow_reads`: read-only tools run, everything else asks. + +## The change in three sentences + +`build_agent_v0_default()` gains four entries in its `tools` list, one for each of Pi's own +default built-ins (`read`, `bash`, `edit`, `write`), so a newly created agent carries them +wherever it runs instead of only inside the playground. Nothing about the runner's grant-list +semantics changes: an empty list still means "grant nothing", and permission gating still asks +before `bash`, `edit`, or `write` runs. The supporting work corrects what the author is told: the +built-in picker's help text currently claims an empty selection leaves Pi's defaults, which is the +opposite of what happens, and every built-in row in the Tools list is labelled "builtin" rather +than its own name. + +## Related work + +- [pi-builtin-gating](../pi-builtin-gating/README.md) built the grant-list enforcement and the + permission gate this project depends on. Its `context.md` names the empty-list trap; its + `design.md` explains why `tools: undefined` and `tools: []` must stay different. +- [approval-boundary](../approval-boundary/README.md) owns the one decision module every gate + calls. +- [build-kit-overlay-delivery](../../../build-kit-overlay-delivery/) owns how the playground + overlay reaches the frontend. diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/context.md b/docs/design/agent-workflows/projects/default-agent-builtins/context.md new file mode 100644 index 0000000000..08c538a5fa --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/context.md @@ -0,0 +1,138 @@ +# Why this work exists + +## What a user sees today + +An author creates an agent in the playground and leaves the configuration at its defaults. The +harness is `pi_core`. In the playground the agent runs shell commands, reads files, and writes +files. The author saves the agent, then runs the saved revision from a schedule, from the API, +or from any other caller that is not the playground. The agent answers: + +```text +I couldn't run this because no shell or filesystem tool is available in the current session. +No workspace, .env, script, or outputs were accessed. +``` + +Nothing was read. Nothing was written. The run reached the model, the model reported that it had +no tools, and the turn ended. + +Two reports describe this failure from two sides. [#5590](https://github.com/Agenta-AI/agenta/issues/5590) +reports it from the agent's side ("Pi agents run with no read, bash, edit or write tools outside +the playground"). [#5562](https://github.com/Agenta-AI/agenta/issues/5562) reports it from the +automation's side ("Automations need to have sessions. Otherwise they cannot write files"). + +Claude agents do not show this failure. Claude brings its own Read, Bash, and Write tools over +its own protocol. The failure is specific to Pi. + +## Words used throughout this workspace + +- **Harness**: the coding agent the platform drives inside the sandbox. Today `pi_core` (Pi), + `pi_agenta` (Pi with a forced Agenta opinion), and `claude` (Claude Code). +- **Runner**: the TypeScript service at `services/runner/` that receives a `/run` request, + starts the harness in a sandbox, and streams events back. +- **Sandbox**: the isolated filesystem and process space a run executes in. Either `local` (a + temporary directory on the runner host) or `daytona` (a remote container). +- **Built-in**: one of the seven tools Pi implements inside itself: `read`, `bash`, `edit`, + `write`, `grep`, `find`, `ls`. The model calls a built-in the same way it calls any tool, but + the code runs inside Pi rather than inside Agenta. +- **Grant list**: the `tools` field of a `/run` request. It names which Pi built-ins the run may + use. The runner deletes every built-in that is not named. +- **Agent template**: the saved agent configuration at `parameters.agent` of a workflow revision. + It holds instructions, model, tools, MCP servers, skills, and the execution selectors + (`harness`, `runner`, `sandbox`). +- **Build kit overlay**: an extra fragment of agent template the backend serves to the playground + only. It adds authoring tools and an authoring skill. It is never saved into the agent. + +## Why it happens + +The agent template's `tools` list is empty in the shipped default, and an empty grant list means +"grant nothing" all the way down to Pi. + +The chain has four links. + +1. `build_agent_v0_default()` in `sdks/python/agenta/sdk/utils/types.py:1412` emits + `"tools": []`. This is the value the playground pre-fills into a new agent and the value the + built-in agent interface advertises, so every agent saved from the default starts with an + empty tool list. + +2. The SDK resolves that list into `builtin_names`. `ToolResolver.resolve` in + `sdks/python/agenta/sdk/agents/tools/resolver.py:113` picks out the entries of type `builtin`. + An empty list yields an empty `builtin_names`. + +3. `PiAgentTemplate.wire_tools()` in `sdks/python/agenta/sdk/agents/dtos.py:881` always writes + `"tools": list(self.builtin_names)` into the `/run` body. It writes the field even when the + list is empty. + +4. The runner reads that field. `normalizePiBuiltinGrants` in + `services/runner/src/engines/sandbox_agent/run-plan.ts:196` distinguishes two cases: + + ```ts + function normalizePiBuiltinGrants(tools: string[] | undefined): string[] { + if (tools === undefined) return [...PI_DEFAULT_ACTIVE_BUILTINS]; + if (!Array.isArray(tools)) return []; + ``` + + A missing `tools` field means "use Pi's own defaults", which are `read`, `bash`, `edit`, + `write`. An empty array means "grant nothing". Because step 3 always writes the field, the + missing-field branch is unreachable from the platform. Every platform run takes the + empty-array branch. + + `replaceActiveBuiltinTools` in `services/runner/src/extensions/agenta.ts:157` then rewrites + Pi's active tool set, keeping only the granted built-ins. With no grants, Pi's active set + contains no `read`, no `bash`, no `edit`, and no `write`. The model is not refused at call + time. The tools are absent from its tool list, so it correctly reports that it has none. + +The two branches of `normalizePiBuiltinGrants` are deliberate. The +[pi-builtin-gating design](../pi-builtin-gating/design.md) states that `tools: undefined` and +`tools: []` must stay different, because an author who deselects every built-in must get an agent +with no built-ins. The problem is not that rule. The problem is that the shipped default expresses +"the author has not chosen" using the syntax for "the author chose none". + +## Why the playground is the exception + +The build kit overlay repairs the default for playground runs only. +`build_agent_template_overlay()` in `api/oss/src/core/workflows/build_kit.py:75` prepends two +built-in entries to the template's `tools` list: + +```python +"tools": [ + *[{"type": "builtin", "name": name} for name in AGENTA_FORCED_TOOLS], + ... +] +``` + +`AGENTA_FORCED_TOOLS` is `["read", "bash"]` +(`sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py:64`). The overlay exists to make the +authoring experience work: the build kit ships platform tools and a skill, and a skill is +unreadable without `read` and unrunnable without `bash`. The overlay is served to the playground +and merged into the run parameters there. It is never committed into the agent, so the moment the +author saves and runs the agent anywhere else, the two built-ins disappear with it. + +The overlay grants `read` and `bash` only, not `edit` and `write`. The issue reports an agent that +writes files in the playground. That works because Pi's `bash` can write files through shell +redirection, not because `write` was granted. + +## Goals + +- A newly created Pi agent has `read`, `bash`, `edit`, and `write` in its tool list wherever it + runs, not only in the playground. Whether a given call runs is then the permission model's + decision, which is what it should be. A scheduled agent under the shipped default permission mode + still pauses at its first shell command; + [design.md](design.md#what-this-fixes-and-what-the-reporter-will-still-hit) traces exactly how + far the reported run gets after this change. +- The author can see which built-ins the agent has, and can remove any of them. +- The permission behavior that ships today is unchanged. The default permission mode is + `allow_reads`, so `read` runs without asking and `bash`, `edit`, and `write` raise an approval. + Granting a tool is not the same as letting it run unattended, and this work does not blur that. + +## Non-goals + +- **Repairing agents already saved.** Every agent saved since the empty default shipped carries + `tools: []`. This work does not migrate them. The reasoning is in + [design.md](design.md#alternatives-considered). +- **Changing the runner's empty-array semantics.** `tools: []` keeps meaning "grant nothing". +- **Making unattended runs bypass approvals.** A scheduled Pi agent that calls `bash` under the + default permission mode still raises an approval. Whether an unattended run should be able to + proceed is a separate question, tracked in [open-questions.md](open-questions.md). +- **A per-built-in permission field.** `BuiltinToolConfig` still drops an authored `permission` + with a warning (`sdks/python/agenta/sdk/agents/tools/models.py:87`). Re-enabling it is a + separate change. diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/design.md b/docs/design/agent-workflows/projects/default-agent-builtins/design.md new file mode 100644 index 0000000000..8c9a8ebf5d --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/design.md @@ -0,0 +1,451 @@ +# What we change and why + +## The change + +`build_agent_v0_default()` in `sdks/python/agenta/sdk/utils/types.py:1412` stops emitting an +empty tool list. It emits Pi's four default built-ins in the typed form the schema describes: + +```python +"tools": [ + {"type": "builtin", "name": "read"}, + {"type": "builtin", "name": "bash"}, + {"type": "builtin", "name": "edit"}, + {"type": "builtin", "name": "write"}, +], +``` + +Everything downstream already handles this. The value validates against the strict +`AgentTemplateSchema` (`types.py:1228`), `AgentTemplate.from_params` parses it into four +`BuiltinToolConfig` entries, `ToolResolver.resolve` turns them into +`builtin_names = ["read", "bash", "edit", "write"]`, and `PiAgentTemplate.wire_tools()` puts those +four strings on the `/run` wire. The runner grants them and permission gating proceeds exactly as +it does today. + +Three changes support it, each described in its own section below. The built-in picker's help text +is corrected, because it currently tells the author the opposite of what happens. Each built-in row +in the Tools list shows its own name instead of the word "builtin". And the Claude harness stops +logging a warning for the default set, so the warning keeps meaning something. + +## Which built-ins, and why those four + +Pi's own default active set is `read`, `bash`, `edit`, `write` +(`PI_DEFAULT_ACTIVE_BUILTINS` at `services/runner/src/engines/sandbox_agent/run-plan.ts:192`). +The default template ships exactly that set. + +Three sets were on the table. + +**All seven** (`read`, `bash`, `edit`, `write`, `grep`, `find`, `ls`). Rejected. The three extra +tools are search and listing conveniences that `bash` already covers, and shipping a set wider +than Pi's own default means every default agent's grant list differs from +`PI_DEFAULT_ACTIVE_BUILTINS`. That difference forces `computeBuiltinGatingActive` +(`run-plan.ts:233`) to keep the gating relay on even for an author who has set the permission mode +to `allow`, adding a round trip per tool call for no benefit. An author who wants `grep` can add +it. + +**Only `read` and `bash`**, matching `AGENTA_FORCED_TOOLS`. Rejected. This is the set the +playground overlay grants, so it is the smallest set that closes the reported gap in the strict +sense. It is the wrong set to standardize on. The reason the playground has only these two is that +they are what a skill needs (`read` opens `SKILL.md`, `bash` runs its helper scripts), not a +judgment about what an agent needs. Shipping this set would leave a new agent unable to edit or +write a file except by shell redirection, which is a worse tool for the job and produces worse +diffs. It would also leave the platform default permanently out of step with Pi's default, which +is the confusion this bug is made of. + +**Pi's four defaults.** Chosen. Two reasons. It is the set a user of Pi expects, so the platform +stops silently subsetting the harness. And it is the set that makes the grant list a no-op +relative to Pi's own behavior, which means `sameStringSet(builtinGrants, PI_DEFAULT_ACTIVE_BUILTINS)` +in `computeBuiltinGatingActive` returns true and an all-`allow` agent keeps the fast path with no +relay round trips. + +The fast path has a cost the first draft did not name. When `computeBuiltinGatingActive` +(`run-plan.ts:233`) returns false, the runner never sets `AGENTA_AGENT_BUILTIN_GATING` +(`pi-assets.ts:369`), so the extension's inertness guard skips `registerBuiltinGating` +(`agenta.ts:373`) and `replaceActiveBuiltinTools` never runs. The four built-ins are then active +because *Pi* activates them, not because the runner enforced the grant list. Today those two sets +are identical, so the outcome is correct. If a future Pi release adds a fifth tool to its own +default active set, an agent under a blanket `allow` policy would get that tool even though its +saved grant list names exactly four. Choosing Pi's four defaults is what moves the shipped default +onto that unenforced path: today's `tools: []` is not equal to Pi's defaults, so gating is always +on. + +The honest fix is a runner change outside this project: separate "shape the active tool set" from +"gate each call", so an explicit grant list is always applied and only the approval relay takes the +fast path. That belongs to the pi-builtin-gating design, which owns +`computeBuiltinGatingActive`. Recorded in [open-questions.md](open-questions.md); it does not block +this change, because the two sets are equal in the pinned Pi version and the cross-language +constant pin in [testing.md](testing.md) is what would catch them diverging. + +The four are not equally powerful, and the permission model is what separates them. Under the +shipped default permission mode `allow_reads`, `read` is marked read-only in the identity table +(`services/runner/src/permission-plan.ts:40`) and runs without asking. `bash`, `edit`, and `write` +are not read-only and raise an approval on every call. Granting all four does not make a new agent +able to run shell commands unattended. + +## What `read` can reach on the local sandbox + +`read` runs without an approval, and on the shipped default sandbox that is a larger capability +than "read-only" suggests. Three facts stack. + +The default sandbox is `local` (`_DEFAULT_SANDBOX` at `types.py:1071`). A `local` run spawns +`sandbox-agent` on the runner host itself (`services/runner/src/engines/sandbox_agent/provider.ts:148`), +and the platform already states plainly that this is "unconfined host bash and not a tenant +boundary" (`services/oss/src/agent/config.py:60`). Pi's `read` takes "a path to the file to read +(relative or absolute)" and resolves it with `resolvePath(filePath, cwd)` +(`path-utils.js`, `resolveToCwd`), which returns an absolute path unchanged. There is no cwd jail. +So on a `local` sandbox, `read` under `allow_reads` is an approval-free read of any file the runner +process can open. + +Two things bound this, and neither removes it. + +It is not a new capability. The playground overlay already forces `read` and `bash` +(`AGENTA_FORCED_TOOLS`), and the playground's default sandbox is also `local`, so an author can do +this interactively today. What this change adds is the *unattended* and *non-playground* version: +a saved agent invoked from a schedule or the API now reads host files with nobody approving. + +It is deployment-controlled. `local` is only reachable where it is enabled, and +`AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS` gates it. But unset defaults to `["local"]` +(`sdks/python/agenta/sdk/agents/sandbox_providers.py:30`), so every default deployment has it on, +and the platform's own mitigation for multi-tenant use is to drop `local` from the enabled set. + +The conclusion is not to drop `read` from the default. An agent without `read` cannot do the job, +and `edit` and `write` are gated the same way `bash` is. The conclusion is that this change should +not ship into a shared deployment that still enables `local`, and that the design cannot describe +`read` as harmless just because it is classified read-only. Recorded as a pre-ship condition in +[status.md](status.md) and as an open question in [open-questions.md](open-questions.md); the +durable fixes (confine built-in filesystem operations to the run cwd, or stop enabling `local` by +default) are runner and deployment work, not template work. + +## What this fixes, and what the reporter will still hit + +This change makes the tools exist. It does not change whether they are allowed to run, and for the +scenario in [#5590](https://github.com/Agenta-AI/agenta/issues/5590) that distinction decides how +far the run gets. + +The reporter runs a saved agent from a schedule and asks it to change directory, source an env +file, and run a Python script. Every one of those steps is `bash`. + +After this change, that run reaches the model with `read`, `bash`, `edit`, and `write` in its tool +list. The model calls `read` and it works, because `read` is marked read-only +(`services/runner/src/permission-plan.ts:41`) and the default permission mode `allow_reads` allows +read-only tools. The model then calls `bash`. Under `allow_reads`, `bash` resolves to `ask` +(`permission-plan.ts:256`), no stored decision exists, and `decide()` returns `pendingApproval` +(`permission-plan.ts:155`). + +A pending approval on an unattended run does not wait. It ends the turn. +`pauseUserApproval` (`services/runner/src/engines/sandbox_agent/acp-interactions.ts:176`) emits an +`interaction_request`, writes a durable interaction record, and calls `onPause()`, which destroys +the ACP session (`services/runner/src/engines/sandbox_agent/pause.ts:33`). The turn ends with +`stopReason: "paused"` and the `bash` call settled as not executed +(`services/runner/src/engines/sandbox_agent/run-turn.ts:793`). The keep-alive path that parks a +sandbox and waits for a human requires a platform session id +(`services/runner/src/server.ts:372`), and the schedule dispatcher sends none +(`api/oss/src/tasks/asyncio/triggers/dispatcher.py:309`). + +So the reporter's observable outcome changes from "the agent says it has no tools" to "the agent +reads files, then stops at the first shell command". In the production detached dispatch path the +schedule's delivery row records `202 dispatched` and counts as a success either way +(`dispatcher.py:324`), so neither outcome is visible without opening the trace. + +This is the permission model working as designed. `allow_reads` means writes ask, and nobody is +there to answer. The author's supported lever today is to set the agent's permission mode to +`allow`, or to add an explicit allow rule for `Bash`, either of which makes the scheduled run +complete. + +Two things follow. First, this change is still the right change and still necessary: an agent with +no tools cannot be fixed by any permission setting, and an agent whose tools ask for approval can. +Second, it is not sufficient for the reporter's scenario on its own. Whether the platform should +do anything more, and what, is the first entry in [open-questions.md](open-questions.md). +[#5562](https://github.com/Agenta-AI/agenta/issues/5562), whose title asks for automations to have +sessions, is a report of that second half. + +## What each consumer of the default sees + +`build_agent_v0_default()` has three production call sites and they behave differently enough to +list. + +**`services/oss/src/agent/schemas.py:41`, the agent service `/inspect` schema.** The value is the +`default` on `parameters.agent`. The workflow catalog hoists object defaults out of the schema and +into a materialized `parameters` block (`api/oss/src/resources/workflows/catalog.py:104`), and the +frontend's create-agent factory copies that block into the new agent +(`web/packages/agenta-entities/src/workflow/state/appUtils.ts:181`). This is the path that fixes +the reported bug: a newly created agent's saved revision now contains the four entries. + +**`sdks/python/agenta/sdk/engines/running/interfaces.py:537`, the SDK built-in interface +`agenta:builtin:agent:v0`.** Same role, different publisher. It is pinned equal to the service +value by `services/oss/tests/pytest/unit/agent/test_default_agent_template.py:35`, so it changes +with the builder and stays equal. + +**`sdks/python/agenta/sdk/engines/running/utils.py:288`, the built-in's fallback parameters.** +This is the value a run gets when it binds `agenta:builtin:agent:v0` and supplies no parameters at +all, and it is a live path, not a dead one. `retrieve_configuration` (`utils.py:526`) reads this +registry, `seed_empty_parameters_from_configuration` (`utils.py:534`) calls it, and the resolver +middleware calls that on every invoke (`sdks/python/agenta/sdk/middlewares/running/resolver.py:571` +and again after reference hydration at `:596`). The workflow decorator reads the same registry at +registration time (`sdks/python/agenta/sdk/decorators/running.py:240`). Two existing tests drive +it: `sdks/python/oss/tests/pytest/unit/test_workflow_shapes_running.py:234` +(`test_inline_agent_revision_without_parameters_uses_default_template`) and +`api/oss/tests/pytest/unit/tools/test_platform_handlers.py:237` +(`test_test_run_parameters_less_agent_revision_succeeds_with_resolver_backed_child`). + +So the blast radius is wider than "agents created in the playground". An API or SDK caller that +invokes a revision bound to `agenta:builtin:agent:v0` with empty parameters also starts granting +Pi's four built-ins after this change. That is the intended behavior and it is consistent with the +rest of the change, but it must be stated rather than assumed inert. Both tests compute their +expectation from the builder, so neither needs editing. + +Two copies of the default do not go through the builder and are worth naming so a reader does not +assume they moved. + +`services/oss/src/agent/config.py:106` supplies `tools: []` as the request-time fallback for a +request that carries no agent template at all (threaded through +`services/oss/src/agent/app.py:58` into `AgentTemplate.from_params`). This path is reached only by +a caller that posts an agent invocation with no `parameters.agent`, which the platform does not +do. Leave it alone in this change. Aligning it is a separate cleanup with its own risk, and it is +listed in [open-questions.md](open-questions.md). + +`sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py:126` holds a documentation copy of the +config shape inside the build-an-agent skill, and its example shows `"tools": []`. That example +teaches the builder agent what a config looks like, so it should show the new default. It is a +text change with a drift test already in place +(`sdks/python/oss/tests/pytest/unit/agents/test_agenta_builtins_reference_files.py`). + +## The playground overlay keeps its two built-in entries + +`build_agent_template_overlay()` (`api/oss/src/core/workflows/build_kit.py:75`) prepends +`{"type": "builtin", "name": "read"}` and `{"type": "builtin", "name": "bash"}` from +`AGENTA_FORCED_TOOLS`. Once the default template carries all four, those two entries are +redundant for a default agent. + +They stay. + +The overlay's merge is safe with them. `identityMerge` in +`web/packages/agenta-playground/src/state/execution/buildKitOverlay.ts:65` keys a tool entry by +`platform:`, then `workflow:`, then `name:` (`buildKitOverlay.ts:47`). The +default's `{"type": "builtin", "name": "read"}` and the overlay's identical entry both key to +`name:read`, so the overlay replaces the base entry in its existing position. There is no +duplicate and no reordering. The merged list is byte-identical to the base list for those two +entries. + +The reason to keep them is that they are not there to repair the default. They are there because +the build kit ships a skill, and the playground must guarantee the skill is loadable regardless of +what the author's template says. An author who deliberately deselects `read` still needs it while +authoring, because otherwise the build-an-agent skill is announced in the system prompt and cannot +be opened. Removing the entries would make that guarantee depend on the default template, and the +default template is a value the author is free to edit. The comment above `AGENTA_FORCED_TOOLS` +(`agenta_builtins.py:58`) already states this as the reason they exist. + +The cost of keeping them is one line of redundancy in a value that is already pinned by a test +(`api/oss/tests/pytest/unit/applications/test_build_kit_overlay.py:66`). The cost of removing them +is a class of bug that only appears for an author who edited their tools, which is exactly the +author least likely to be testing the build kit. + +The same reasoning applies to `force_tools()` on the `pi_agenta` harness +(`sdks/python/agenta/sdk/agents/adapters/harnesses.py:141`). It stays. + +## The Claude harness warning + +`ClaudeHarness._to_harness_config` (`sdks/python/agenta/sdk/agents/adapters/harnesses.py:94`) +logs a warning whenever `builtin_names` is non-empty: + +```python +if config.builtin_names: + log.warning( + "ClaudeHarness ignores %d built-in tool(s); built-ins are a Pi concept", + len(config.builtin_names), + ) +``` + +Today this fires almost never. After the change it fires on every Claude run whose template came +from the default, which is most of them. A warning that fires on the normal path is noise, and +noise is how a real warning gets missed. + +Four options were considered. + +**Leave it.** Rejected. It would fire on nearly every Claude run and say nothing actionable. + +**Delete it.** Rejected. It is the only signal that an author who switched a configured Pi agent +to Claude has silently lost tools they chose. + +**Downgrade to debug.** Rejected for the same reason as deleting: it hides the case that matters. + +**Stay silent only for the untouched default set; warn for everything else.** Chosen. The warning +becomes: + +```python +if config.builtin_names and set(config.builtin_names) != set(PI_DEFAULT_ACTIVE_BUILTINS): + log.warning( + "ClaudeHarness ignores built-in tool(s) %s; built-ins are a Pi concept", + ", ".join(config.builtin_names), + ) +``` + +A default-derived template carries exactly the four Pi defaults and logs nothing. Anything else +warns, and the message names the tools, which the current message omits. + +The first draft filtered name by name instead (`[n for n in builtin_names if n not in defaults]`) +and was wrong. A subset is a deliberate authoring act too: an author who selected only `bash` and +then switched to Claude would lose it silently, because every name in their set is in Pi's default +set. The code has no provenance field telling it which sets are default-derived, so the only honest +predicate is exact-set equality: the one set we can be sure the author never touched. + +This is a heuristic, not a fact, and it is worth saying so. An author who deliberately selects +exactly Pi's four also gets silence. That is acceptable because the outcome is the same either way +(Claude drops them) and because the alternative is warning on the normal path. The real fix is for +agent creation to stop putting Pi built-ins in a Claude agent's template at all; see the note on +the harness preference below. + +This needs a shared constant for Pi's four default built-in names on the Python side. There is no +such constant today; the four names live only in TypeScript at `run-plan.ts:192`. Put it next to +the built-in vocabulary in `sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py`, beside +`AGENTA_FORCED_TOOLS`, and have `build_agent_v0_default()` build its `tools` entries from it so +the default and the warning cannot drift apart. + +The Python constant and the TypeScript `PI_DEFAULT_ACTIVE_BUILTINS` are now two copies of the same +list in two languages. They are already two copies today, just implicitly. They should be two +implementations of one pinned contract rather than one "mirroring" the other, so +[testing.md](testing.md) pins both against a shared fixture. + +Name the Python constant `PI_DEFAULT_ACTIVE_BUILTINS`, matching the TypeScript name exactly, and +make it a tuple. "Default built-in names" would be ambiguous with the seven-name vocabulary the +picker offers. Put it in a small neutral module for Pi harness facts rather than in +`agenta_builtins.py`: that module's own contract says it holds "the Agenta harness's forced +defaults: the things `AgentaHarness` always applies" (`agenta_builtins.py:1`), and Pi's native +active set is not an Agenta opinion. `AGENTA_FORCED_TOOLS` living there is correct precisely +because it *is* an Agenta opinion. Putting the two side by side would blur the distinction this +whole bug is made of. + +## A Claude agent created from the default carries four dead entries + +The create-agent factory copies the template and then overlays the author's last-used harness +(`web/packages/agenta-entities/src/workflow/state/appUtils.ts:186`, applied by +`applyAgentCreationPrefs` at `agentCreationPrefs.ts:32`). The preference sets `harness.kind` and +nothing else. So an author whose last agent was Claude creates a Claude agent that carries the four +Pi built-in entries in its tools list, where they do nothing: `ClaudeHarness` drops them, and the +Tools section shows four rows for tools the agent does not have. + +This is cosmetic, not a correctness bug, and it is not worth blocking the fix. It is also the case +that would be solved properly by making the default set harness-dependent at creation time rather +than by a warning filter. Recorded in [open-questions.md](open-questions.md) with the surface +question, because both are about the same thing: the tools list is authored per harness and stored +per agent. + +## No pinned wire contract moves + +The `/run` request field stays `tools?: string[]` +(`services/runner/src/protocol.ts:469`). No field is added, removed, renamed, or retyped. + +The shared golden fixtures under `sdks/python/oss/tests/pytest/unit/agents/golden/` do not change. +`test_wire_contract.py` builds its Pi payload from a hand-written +`PiAgentTemplate(builtin_tools=["read", "write"])` at `:125` and never touches +`build_agent_v0_default` or `AgentTemplate.from_params`, so the golden is unaffected. +`services/runner/tests/unit/wire-contract.test.ts` reads the same files and asserts on the parsed +request, not on a run plan built from it. + +What changes is a value, not a contract: the default a new agent starts from. + +## What the author sees and can change + +There is already a built-in picker. It is a multi-select over Pi's seven names in +`web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx:33`, it reads +and writes the same `parameters.agent.tools` array the Tools section shows +(`useModelHarness.tsx:1000`), and it renders inside Advanced under Permissions when the harness is +`pi_core` or `pi_agenta`. + +So a picker is not the scope question. Three defects in what exists are, and two of them become +visible only because of this change. + +**The help text is false, and this change makes it worse.** It reads "Optional Pi built-ins to +author explicitly; empty leaves Pi's harness defaults." Empty does not leave Pi's harness defaults. +Empty grants nothing. Even removing the field entirely grants nothing, because +`_parse_agent_fields` falls back to a default of `[]` (`dtos.py:1323`) and `wire_tools` always +emits the key. After this change the picker starts pre-populated with four selections, so an +author who clears it will read the help text, expect Pi's defaults, and get an agent with no +tools. Fix the text to say what happens: clearing the selection removes every Pi built-in from the +agent. + +**Every built-in row is labelled "builtin".** `describeTool`'s built-in branch +(`agentTemplate/itemDescriptors.tsx:195`) labels the row from the entry's `type` and ignores its +`name`. Today that is a curiosity because templates rarely carry built-ins. After this change, +every new agent's Tools section shows four identical rows reading "builtin". Read the top-level +`name` when it is present. + +**A built-in row opens a raw JSON editor.** `itemKinds.tsx:85` returns `"json"` for anything that +is not a function tool, a reference tool, or a gateway tool, so clicking a built-in row shows raw +JSON with no form. This is pre-existing and this change makes four of these rows appear in every +new agent. + +The recommendation splits these. + +In scope for this change: the help-text correction and the row label. Both are small, both are +directly caused by shipping built-ins in the default, and shipping the default without them +produces a visibly broken Tools list. + +Out of scope, as a follow-up: adding built-ins to the Tools section's own +`AgentToolSelectorPopover` and giving a built-in row a real form instead of raw JSON. The reason +is that the useful version of this work is not "add a picker". A picker already exists in a second +place, so adding a third editing surface for the same array without deciding which one is +canonical makes the configuration harder to reason about, not easier. That decision is worth doing +properly and it is not on the critical path for a bug where agents have no tools. It is filed in +[open-questions.md](open-questions.md). + +## Alternatives considered + +### Change the runner so an empty list means Pi's defaults + +Make `normalizePiBuiltinGrants` treat `[]` and `undefined` the same. + +Rejected. It removes the author's ability to say "no built-ins" and it contradicts a decision made +deliberately in the [pi-builtin-gating design](../pi-builtin-gating/design.md), which states that +`tools: undefined` must differ from `tools: []`. It would also fix agents saved today, which is why +it keeps coming up. That benefit is real, and it is not worth an agent configuration where +deselecting every tool silently re-grants four of them. + +### Make the SDK omit the field when the list is empty + +Change `PiAgentTemplate.wire_tools()` to drop `tools` when `builtin_names` is empty, so the +runner's missing-field branch fires. + +Rejected for the same reason, one layer up. It has the same effect as the option above and it is +harder to see, because the behavior would then depend on which of two identical-looking empty +values the SDK produced. + +An earlier draft added a second reason, that this would break the Claude golden fixture which pins +`"tools": []`. That reason is wrong and has been removed. `ClaudeAgentTemplate.wire_tools()` +hardcodes `"tools": []` itself (`dtos.py:921`) and never reads `builtin_names`, so editing +`PiAgentTemplate.wire_tools()` cannot move `run_request.claude.json`. The rejection rests on the +authoring-semantics argument alone, which is sufficient. + +### Repair agents already saved + +Backfill the four built-ins into every existing agent revision whose `tools` list is empty, either +by a migration or by a read-time upgrade in `AgentTemplate.from_params`. + +Out of scope by decision. Every agent saved since the empty default shipped carries `tools: []` +and stays broken until its author edits it. + +The trade-off is real and worth stating. Not repairing means the reported bug persists for every +agent that already exists, and the people who hit it are the people who already built something. +Repairing means the platform reinterprets a saved configuration value: an author who genuinely +deselected every built-in would find four of them back. There is no stored signal that +distinguishes "the default put an empty list here" from "I chose none", because they are the same +value. A migration would therefore have to guess, and it would guess wrong for exactly the author +who cared enough to configure it. + +The narrower version, backfilling only revisions whose `tools` list is empty and whose harness is +Pi and which were created before the fix ships, is implementable. It is deferred rather than +rejected. If the number of affected agents turns out to be large, revisit it as its own change +with its own review. + +The workaround in the meantime is one edit: open the agent, expand Advanced, and select the +built-ins in the "Built-in tools" control. Committing a new revision fixes that agent. + +### Change the default harness to `pi_agenta` + +`AgentaHarness` already forces `read` and `bash` into every run +(`harnesses.py:141`). Making `pi_agenta` the default harness would give new agents those two +tools without touching the template. + +Rejected. It grants two tools rather than four, so it does not fix editing or writing files. It +changes far more than tool availability: `pi_agenta` also forces an AGENTS.md preamble, a persona +appended to the system prompt, and a platform skill. Changing a harness default to fix a tools +default is a large, indirect change with side effects an author did not ask for. diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/open-questions.md b/docs/design/agent-workflows/projects/default-agent-builtins/open-questions.md new file mode 100644 index 0000000000..b4e857039c --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/open-questions.md @@ -0,0 +1,158 @@ +# Open questions + +## Does a scheduled agent need a way to run write-capable tools? + +**Needs a product decision.** + +A saved agent with the shipped default permission mode `allow_reads` stops at its first `bash`, +`edit`, or `write` call when nobody is watching. The turn ends with `stopReason: "paused"`, the +sandbox is destroyed, and the schedule's delivery row records a success. The evidence is in +[design.md](design.md#what-this-fixes-and-what-the-reporter-will-still-hit). + +Granting Pi's built-ins in the default template is necessary and is not sufficient for the +scenario in [#5590](https://github.com/Agenta-AI/agenta/issues/5590). The author can already +unblock their own agent by setting the permission mode to `allow` or adding an allow rule for +`Bash`, so nothing is impossible today. The question is whether the platform should make that +easier, and how. + +Three shapes exist, and they are not equivalent. + +- **Leave it.** The author sets `allow` when they want an unattended agent. This is honest and + costs nothing to build. It means the default configuration cannot complete an unattended task, + and the failure is invisible: the delivery says success. +- **Make the failure visible.** Keep the behavior and surface it. A run that ends + `stopReason: "paused"` with no client that can answer is a failed delivery, not a successful + one, and the schedule should say so. This is the smallest change that stops the silent stop. +- **Give an unattended run its own policy.** Let a schedule or trigger carry a permission + decision, so an author can say "this automation may run shell commands" without weakening the + agent's interactive default. Nothing today can carry such a value: the trigger dispatcher builds a + request with only references, selector, and inputs + (`api/oss/src/tasks/asyncio/triggers/dispatcher.py:309`), and the permission plan comes only from + the saved variant. + +This question is out of scope for this workspace and is the substance of +[#5562](https://github.com/Agenta-AI/agenta/issues/5562). It needs its own design. + +## Should the runner enforce a grant list even when the approval gate is off? + +**Not this project's to decide, but this change is what makes it matter.** + +`computeBuiltinGatingActive` (`run-plan.ts:233`) returns false when the permission plan cannot gate +a built-in and the grant list equals Pi's own defaults. The runner then never sets +`AGENTA_AGENT_BUILTIN_GATING`, so the extension skips `registerBuiltinGating` entirely and +`replaceActiveBuiltinTools` never runs. One flag turns off both the approval relay and the +active-set enforcement, and only one of those is a performance concern. + +Today the outcome is still correct, because the set the runner would enforce and the set Pi +activates on its own are the same four names. The exposure is a future Pi release that adds a fifth +tool to its default active set: an agent under a blanket `allow` policy would get it despite a +saved grant list naming four. This change is what moves the shipped default onto that path, since +today's `tools: []` never equals Pi's defaults and gating is therefore always on. + +The fix is to split the two concerns: always apply the grant list, and let only the approval relay +take the fast path. That is a change to `computeBuiltinGatingActive` and the extension's inertness +guard, both owned by [pi-builtin-gating](../pi-builtin-gating/README.md). It is filed there rather +than done here. The cross-language constant pin in [testing.md](testing.md) is the interim guard: +it fails the moment the two lists diverge. + +## Should `read` be approval-free on the local sandbox? + +**Needs a decision before this ships to a shared deployment.** + +`read` is classified read-only (`permission-plan.ts:40`) and `allow_reads` runs it without asking. +On the shipped default sandbox `local`, that is an approval-free read of any file the runner +process can open: the run executes on the runner host (`provider.ts:148`), Pi's `read` accepts +absolute paths and applies no cwd jail, and `AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS` defaults to +`["local"]` when unset. The full trace is in +[design.md](design.md#what-read-can-reach-on-the-local-sandbox). + +The capability exists today through the playground overlay. What this change adds is the +unattended, non-playground version of it. Three responses are available and they are not +equivalent: stop enabling `local` by default, confine built-in filesystem operations to the run +cwd in the runner, or reclassify `read` so it also asks. The first is deployment policy, the second +is runner work, the third would make the default agent ask before every file read and is probably +too blunt. + +This is not a reason to drop `read` from the default template. It is a reason not to ship the +default template into a shared deployment that still enables `local`. + +## Should agents saved before the fix be repaired? + +**Decided: no, for now.** Recorded here because the trade-off should stay visible rather than be +forgotten. + +Every agent saved since the empty default shipped carries `tools: []` and keeps failing outside the +playground until its author edits it. Repairing them means reinterpreting a stored value, and there +is no signal that separates "the default put an empty list here" from "I deselected everything", +because they are the same value. The full reasoning is in +[design.md](design.md#repair-agents-already-saved). + +Revisit if the count of affected agents turns out to be large. The narrow version, backfilling only +Pi agents whose tools list is empty and whose revision predates the fix, is implementable as its +own change. + +## Which surface owns editing built-in tools? + +**Needs a decision before the follow-up work, not before this fix.** + +Built-ins are editable today from a multi-select in Advanced under Permissions +(`web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx`). They also +appear as rows in the Tools section, where clicking one opens a raw JSON editor. Two surfaces edit +the same array with different affordances, and neither is obviously the canonical one. + +The options are to make the Tools section canonical and drop the Advanced multi-select, to keep the +multi-select and make Tools rows read-only, or to keep both and give the Tools row a real form. The +third preserves the current split and is the least decisive. + +This change corrects the two defects that shipping built-ins in the default makes visible (the +false help text and the row label). It does not add a third editing surface, because adding one +before deciding which surface is canonical makes the configuration harder to reason about. See +[design.md](design.md#what-the-author-sees-and-can-change). + +Two related questions belong with this one, because all three are about the same thing: the tools +list is authored per harness and stored per agent. + +**Should the default set depend on the harness?** A new agent created while the last-used harness +preference is `claude` carries four Pi built-in entries that Claude drops +(`appUtils.ts:186`, `agentCreationPrefs.ts:32`). Making creation harness-aware would fix that +properly, and would also remove the need for the exact-set heuristic in the Claude warning +(see [design.md](design.md#the-claude-harness-warning)). + +**Should "unset" and "explicitly empty" be different values?** They are the same value everywhere +today: `AgentTemplateSchema.tools` and `AgentTemplate.tools` both use `default_factory=list` +(`types.py:1228`, `dtos.py:604`), and the picker writes `undefined` when the author clears the +selection (`PiSettingsControl.tsx:83`). This is exactly the ambiguity that makes repairing existing +agents impossible. Making the shape tri-state, and persisting `[]` on a deliberate clear, would not +repair a single existing agent, but it stops the platform creating more of them. Worth doing on its +own; out of scope here. + +## Do the four built-in names now collide with an author's own tool? + +**Known consequence of this change. Not fixed here, and worth watching.** + +`read`, `bash`, `edit` and `write` are now occupied names for every agent created from the default +template. Built-in names and resolved tool specs share one namespace: `_validate_unique_names` +(`sdks/python/agenta/sdk/agents/tools/resolver.py:84`) walks `[*builtin_names, *tool_specs]` and +raises `DuplicateToolNameError` on the first repeat, which fails the whole run. An author-defined +client tool, gateway tool, or workflow tool called `write` was legal before this change and now +aborts the run of any agent that still carries the shipped built-ins. + +The blast radius is small (the names are short and generic, but an author who wanted one has to +have picked exactly it) and the failure is loud rather than silent, which is why the behavior is +left alone. The options, if it does bite: namespace built-ins on the wire so the two sets cannot +collide, let an author tool of the same name shadow the built-in rather than fail, or keep failing +and say so in the picker before the run. + +## The agent service's own fallback default + +**Fixed alongside the builder default, after review.** + +`services/oss/src/agent/config.py` supplies the fallback for a request that carries no agent +template at all, and it does not call `build_agent_v0_default()`. It shipped `tools: []` in both +copies: the on-disk `services/runner/config/agent.json` and the in-code `DEFAULT_TOOLS` used when +that file is missing. Both now carry the same four `builtin` entries, sourced from +`PI_DEFAULT_ACTIVE_BUILTINS` so they cannot drift, and the stale sync comment at the top of +`config.py` was corrected. + +It still hand-copies the default model and AGENTS.md text. Folding those into the builder is a +small cleanup worth doing on its own. diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/plan.md b/docs/design/agent-workflows/projects/default-agent-builtins/plan.md new file mode 100644 index 0000000000..496bac6386 --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/plan.md @@ -0,0 +1,101 @@ +# Execution plan + +Four pieces, in dependency order. Each one lands on its own branch and is reviewable alone. The +first piece fixes the reported bug; the rest keep the result honest and legible. + +## Piece 1: ship Pi's built-ins in the default template + +This is the fix. + +Files: + +- A small neutral Pi-facts module in `sdks/python/agenta/sdk/agents/`: add + `PI_DEFAULT_ACTIVE_BUILTINS`, a tuple of Pi's four default built-in names, named exactly like the + TypeScript constant at `services/runner/src/engines/sandbox_agent/run-plan.ts:192`. Not in + `agenta_builtins.py`: that module owns the `pi_agenta` harness's forced Agenta opinions, and Pi's + native active set is not one. Reasoning in + [design.md](design.md#the-claude-harness-warning). +- `sdks/python/agenta/sdk/utils/types.py:1429`: build the `tools` list from that constant as + `{"type": "builtin", "name": name}` entries. +- `services/oss/tests/pytest/unit/agent/test_default_agent_template.py`: add the new assertion and + narrow the existing authoring-extras assertions, as described in + [testing.md](testing.md#tests-that-must-be-updated). +- `sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py`: add the default-to-wire test. +- A new source-reading test pinning the Python constant against the TypeScript array. + +Verification before moving on: `cd services && py-run-tests`, `cd sdks/python && py-run-tests`, +`cd api && py-run-tests`. The `api` suite is included because +`api/oss/tests/pytest/unit/tools/test_platform_handlers.py:237` computes its expectation from the +builder and should adapt without edits; if it does not, something else reads the default. + +## Piece 2: stop the Claude harness warning firing on the default set + +Files: + +- `sdks/python/agenta/sdk/agents/adapters/harnesses.py:94`: stay silent only when the set is + exactly the four defaults, and name the dropped tools in the message. Not a name-by-name filter: + that would silence a deliberately authored subset such as `["bash"]`. Reasoning in + [design.md](design.md#the-claude-harness-warning). +- `sdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.py`: assert that the exact + default set produces no warning, and that a subset (`["bash"]`), a superset + (the four plus `grep`), and a non-default name each still warn. + +This depends on piece 1 only for the shared constant. It should ship with piece 1 rather than +after it: piece 1 alone makes the warning fire on nearly every Claude run. + +## Piece 3: correct what the author sees + +Frontend only. Nothing here changes the wire. + +Files, all under `web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/`: + +- `PiSettingsControl.tsx`: the same false claim appears three times, not once. The file header + comment says "an absent entry means Pi uses its own defaults" (`:5`), the `onChange` prop comment + says "undefined removes an empty tools field" as if that restored defaults (`:25`), and the help + text says "empty leaves Pi's harness defaults" (`:111`). Correct all three: neither an empty list + nor a removed field leaves Pi's defaults; both grant nothing. +- `agentTemplate/itemDescriptors.tsx:195`: label a built-in row from its top-level `name` when + present, falling back to `type` for provider built-ins that carry no name. +- `web/packages/agenta-entity-ui/tests/unit/`: cover the label change. +- `web/packages/agenta-playground/tests/unit/agentRequest.test.ts`: add the overlay case where the + base template already carries all four built-ins. + +Run `pnpm lint-fix` inside `web` before committing. + +## Piece 4: update the documentation that describes the default + +The interface documentation states the default agent config field by field, and the build-an-agent +skill teaches the same shape to the builder agent. Both currently show an empty tools list. + +Files: + +- `docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md:75`: the default + config example. Note that this example is stale in other ways too (it shows the pre-migration + flat shape and an older model), so scope the edit to the `tools` line rather than rewriting the + block, or rewrite it fully as a separate change. +- `docs/design/agent-workflows/documentation/tools.md`: the built-in row and the grant-list + section should say that the default template ships Pi's four defaults. +- `sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py:126`: the config-shape example + bundled into the build-an-agent skill. Its drift test is + `sdks/python/oss/tests/pytest/unit/agents/test_agenta_builtins_reference_files.py`. + +## Order and independence + +Piece 1 must land first; everything else refers to its constant or describes its result. Pieces 2, +3, and 4 are independent of each other and can be reviewed on separate branches. Piece 3 touches +only `web/`, so it conflicts with nothing. + +They are separate review lanes, not separate releases. Pieces 1, 2, and 3 ship together. Piece 1 +alone makes the Claude warning fire on nearly every Claude run (piece 2's problem) and leaves a +Tools section showing four rows all labelled "builtin" under a help text that actively misleads +(piece 3's problem). Piece 4 is documentation and can trail. + +Piece 1 closes [#5590](https://github.com/Agenta-AI/agenta/issues/5590) for newly created agents +only. It does not repair existing agents, and it does not make an unattended write-capable run +complete. Say that on the issue rather than closing it flat. + +## What this plan does not do + +Agents saved before the fix keep an empty tools list and keep failing outside the playground. The +reasoning is in [design.md](design.md#repair-agents-already-saved). The per-agent workaround is one +edit in the Advanced section followed by a commit. diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/research.md b/docs/design/agent-workflows/projects/default-agent-builtins/research.md new file mode 100644 index 0000000000..e422e278f4 --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/research.md @@ -0,0 +1,251 @@ +# What the code does today + +Every claim here was read from the repository. File paths are repository-relative. + +## The two fields named `tools` + +The single largest source of confusion in this area is that two different fields are called +`tools`, at two different layers, with two different shapes. + +| Layer | Field | Shape | Meaning | +| --- | --- | --- | --- | +| Agent template (saved config) | `parameters.agent.tools` | list of tool-config objects discriminated by `type`: `builtin`, `gateway`, `code`, `client`, `reference`, `platform`, or an `@ag.embed` reference | Everything the agent can call | +| Runner `/run` request | `tools` | list of plain strings | The Pi built-ins this run may use | + +Everything in the template that is not `type: "builtin"` leaves the template through +`customTools`, not through `tools`. So a template `tools` list of ten entries can produce a +`/run` `tools` list of zero. + +The strict schema arm is `ToolConfig` +(`sdks/python/agenta/sdk/agents/tools/models.py:245`), so the published JSON Schema describes +only the typed-dict form. The runtime coercion in +`sdks/python/agenta/sdk/agents/tools/compat.py:62` is looser and also accepts a bare string +(`"read"`) and a bare `{"name": "read"}`. Any value written into the shipped default must use +the typed form, because the default is validated against the strict schema. + +## The chain from saved config to Pi's active tool list + +1. `AgentTemplate.from_params` (`sdks/python/agenta/sdk/agents/dtos.py:637`) reads + `parameters.agent.tools`. `_parse_agent_fields` (`dtos.py:1323`) uses the template value when + it is not `None`, and otherwise falls back to the composition's default template. +2. `_coerce_tools` (`dtos.py:622`) turns each entry into a `ToolConfig`. +3. `ToolResolver.resolve` (`sdks/python/agenta/sdk/agents/tools/resolver.py:113`) splits the + list. Entries of type `builtin` become `builtin_names`; everything else becomes `tool_specs`. +4. `PiHarness._to_harness_config` (`sdks/python/agenta/sdk/agents/adapters/harnesses.py:75`) + copies `builtin_names` onto the `PiAgentTemplate` unchanged. +5. `PiAgentTemplate.wire_tools()` (`dtos.py:881`) emits `"tools": list(self.builtin_names)`. The + key is always present. +6. `request_to_wire` (`sdks/python/agenta/sdk/agents/utils/wire.py:82`, spread at `:142`) folds + that into the `/run` body, which `sdks/python/agenta/sdk/agents/adapters/sandbox_agent.py:85` + POSTs to the runner. +7. `normalizePiBuiltinGrants` (`services/runner/src/engines/sandbox_agent/run-plan.ts:196`) turns + the field into the grant list. Missing field yields `PI_DEFAULT_ACTIVE_BUILTINS` + (`run-plan.ts:192`, the four names `read`, `bash`, `edit`, `write`). Present-but-empty yields + an empty list. +8. `replaceActiveBuiltinTools` (`services/runner/src/extensions/agenta.ts:157`) rewrites Pi's + active tool set at `before_agent_start` (`agenta.ts:213`), keeping the granted built-ins in + place and deleting the rest. Non-built-in tools keep their positions. + +Step 5 is why the "missing field" branch in step 7 never fires from the platform. + +## Permission gating already works and is separate from granting + +Granting a built-in is not the same as letting it run. The two are enforced at different points. + +- The grant list decides whether the tool exists at all. It is applied once, before the agent + starts. +- The permission plan decides whether an existing tool may run this time. The `tool_call` hook at + `services/runner/src/extensions/agenta.ts:224` reports every built-in call, and `piDialogAllows` + (`agenta.ts:102`) blocks unless the runner answers `allow`. This landed in commit `3606e5d5cb` + on 2026-07-10 and is on `main`. + +The read-only table at `services/runner/src/permission-plan.ts:40` marks `read`, `grep`, `find`, +and `ls` read-only, and `bash`, `edit`, `write` not read-only. Under the shipped default +permission mode `allow_reads` (`sdks/python/agenta/sdk/utils/types.py:1072`), the read-only four +run without asking and the other three raise an approval. + +`computeBuiltinGatingActive` (`run-plan.ts:233`) decides whether to run the gating machinery at +all. It turns gating on when the resolved permission plan could gate a built-in, or when the +grant list differs from `PI_DEFAULT_ACTIVE_BUILTINS`. A default agent has permission mode +`allow_reads`, so `plan.default !== "allow"` and gating is on regardless of the grant list. + +## What the playground does differently + +`build_agent_template_overlay()` (`api/oss/src/core/workflows/build_kit.py:75`) returns a +fragment that prepends `{"type": "builtin", "name": n}` for each of `AGENTA_FORCED_TOOLS` +(`sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py:64`, the value `["read", "bash"]`), +then the platform ops, then the reserved client-tool embeds. + +The backend produces the overlay but never merges it. Two consumers serve it: +`api/oss/src/apis/fastapi/applications/router.py:1915` ships it as +`additional_context.playground_build_kit.agent_template_overlay`, and +`api/oss/src/core/workflows/static_catalog.py:229` registers it as the `__ag__build_kit` static +workflow, which is marked non-embeddable and is rejected at commit +(`api/oss/src/core/workflows/service.py:1340`). + +The frontend applies it, per run only. +`web/packages/agenta-playground/src/state/execution/agentRequest.ts:326` calls +`withBuildKitOverlay` on a throwaway copy of the run parameters. The merge lives in +`web/packages/agenta-playground/src/state/execution/buildKitOverlay.ts`. For list sections +(`tools`, `skills`, `mcps`) it merges by identity, computed at `buildKitOverlay.ts:47`: +`platform:`, else `workflow:`, else `name:`. An overlay entry whose identity +matches a base entry replaces that base entry in place. An overlay entry with no match is +appended. + +The consequence for this project: if the base template already carries +`{"type": "builtin", "name": "read"}`, the overlay's identical entry replaces it in place. No +duplicate appears, and the merged list is unchanged apart from ordering. A base entry stored as +the bare string `"read"` would not match (`isRecord` fails at `buildKitOverlay.ts:48`) and would +produce a duplicate, which is another reason the default must use the typed-dict form. + +Note that the overlay grants only `read` and `bash`, so the playground has never had `edit` or +`write` either. An agent that appears to write files in the playground is writing them through +`bash` redirection. + +## The default template and its consumers + +`build_agent_v0_default()` lives at `sdks/python/agenta/sdk/utils/types.py:1412` and emits +`"tools": []` at `:1429`. Three production call sites read it. + +| Call site | What it is | +| --- | --- | +| `services/oss/src/agent/schemas.py:41` | The `default` on `parameters.agent` in the agent service's `/inspect` schema | +| `sdks/python/agenta/sdk/engines/running/interfaces.py:537` | The `default` on the same field of the SDK built-in interface `agenta:builtin:agent:v0` | +| `sdks/python/agenta/sdk/engines/running/utils.py:288` | The fallback parameters for a run that binds `agenta:builtin:agent:v0` with no parameters at all | + +The `/inspect` default and the built-in interface default are pinned equal to each other and to +the builder by `services/oss/tests/pytest/unit/agent/test_default_agent_template.py`. + +### How a newly created agent gets that value + +The frontend does not read a JSON Schema `default`. The backend hoists object defaults out of +the schema and into a materialized `parameters` block: +`api/oss/src/resources/workflows/catalog.py:104` extracts each parameter property's `default` +into `data["parameters"]`, then `_normalize_parameter_schema_defaults` (`catalog.py:74`) strips +the non-primitive default back off the schema. The templates endpoint +(`api/oss/src/apis/fastapi/workflows/router.py:166`) serves the result. + +The frontend factory `createEphemeralAppFromTemplate` +(`web/packages/agenta-entities/src/workflow/state/appUtils.ts:134`) copies +`template.data.parameters`, overlays the author's last-used harness, model, and connection, and +`web/oss/src/components/pages/agent-home/hooks/useCreateAgent.ts:84` posts the result as the +agent's first revision. So the value in `build_agent_v0_default()` is literally what lands in a +new agent's saved configuration. + +### Other independent copies of the default + +These do not call the builder and can drift. + +- `services/oss/src/agent/config.py:26` duplicates the default model and AGENTS.md text, and its + `load_config()` supplies `tools: []` at `:106`. This is the request-time fallback used when a + request carries no template at all (`services/oss/src/agent/app.py:58`, threaded to the SDK at + `sdks/python/agenta/sdk/agents/handler.py:265`), and it never touches + `build_agent_v0_default()`. +- `services/runner/config/agent.json` and `services/runner/config/AGENTS.md` are the on-disk + editable copies of the same default. +- `sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py:126` holds a documentation copy of + the config shape bundled into the build-an-agent skill, with `"tools": []` in its example. +- `sdks/python/oss/tests/pytest/unit/test_skill_template_catalog.py:70` hand-copies the template + and claims to mirror the default. + +## The harness that already forces built-ins + +`AgentaHarness` (harness kind `pi_agenta`) unions `AGENTA_FORCED_TOOLS` into `builtin_names` at +`sdks/python/agenta/sdk/agents/adapters/harnesses.py:141` through `force_tools()` +(`agenta_builtins.py:774`). So a `pi_agenta` agent always has `read` and `bash`, anywhere it +runs. `PiHarness` (`pi_core`, the shipped default) forces nothing. + +## The Claude warning + +`ClaudeHarness._to_harness_config` (`harnesses.py:94`) drops built-ins and logs: + +```python +if config.builtin_names: + log.warning( + "ClaudeHarness ignores %d built-in tool(s); built-ins are a Pi concept", + len(config.builtin_names), + ) +``` + +Today this warning almost never fires, because almost no template carries built-ins. If the +default template carries four, every Claude run started from a default-derived template logs it. + +## What the author can see and change in the UI + +There is already a built-in picker, and it is not in the Tools section. + +`web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx` renders a +multi-select labelled "Built-in tools" over exactly Pi's seven names (`PiSettingsControl.tsx:33`). +It reads and writes the same `parameters.agent.tools` array +(`web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/useModelHarness.tsx:1000`), and it +appears in the Advanced section under Permissions, only when the harness is `pi_core` or +`pi_agenta` (`useModelHarness.tsx:186`). + +Three facts about it matter for this work. + +- Its help text reads "Optional Pi built-ins to author explicitly; empty leaves Pi's harness + defaults." That is false. An empty selection produces no grants and Pi loses all built-ins. +- Its write path (`PiSettingsControl.tsx:83`) calls `onChange(nextTools.length ? nextTools : undefined)`. + Deselecting every built-in on a template with no other tools removes the `tools` key entirely. + Removing the key does not reach the runner's missing-field branch: `_parse_agent_fields` falls + back to the composition default, which is `[]`, and `wire_tools` emits `[]`. +- It moves every selected built-in to the end of the `tools` array on each write. + +The Tools section itself has no built-in picker. +`web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AgentToolSelectorPopover.tsx:9` +says so explicitly in its header comment. A built-in that is already in the list renders as a row +in the "Built-in" group of `ToolManagementList.tsx:284`, and clicking it opens a JSON-only drawer +because `itemKinds.tsx:85` returns `"json"` for anything that is not a function tool, a reference +tool, or a gateway tool. + +The row label is wrong too. `describeTool`'s built-in branch +(`web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsx:195`) +labels the row from the entry's `type` and ignores its `name`. Every Pi built-in therefore renders +with the identical label "builtin". The branch was written for provider built-ins such as +`{type: "web_search_preview"}`, where `type` is the name. + +## Tests that touch this behavior + +### Tests that would fail on a default-template change + +`services/oss/tests/pytest/unit/agent/test_default_agent_template.py:68` and `:74` assert +`inspect_default["tools"] == []` and `builtin_default["tools"] == []`. These are the only +assertions in the repository that would break. + +### Tests that adapt on their own + +`api/oss/tests/pytest/unit/tools/test_platform_handlers.py:237` and +`sdks/python/oss/tests/pytest/unit/test_workflow_shapes_running.py:261` both compute their +expectation from `build_agent_v0_default()`. + +### The shared golden fixtures are not affected + +`sdks/python/oss/tests/pytest/unit/agents/golden/` holds six files. Two carry a `tools` field: +`run_request.pi_core.json` (`["read", "write"]`) and `run_request.claude.json` (`[]`). + +`sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py` builds a `PiAgentTemplate` with +`builtin_tools=["read", "write"]` hardcoded at `:125`, runs it through `request_to_wire`, and +asserts equality against the golden at `:243`. It never imports `build_agent_v0_default` and +never calls `AgentTemplate.from_params`. `services/runner/tests/unit/wire-contract.test.ts` reads +the same files in place (`services/runner/tests/utils/golden.ts:15`) and asserts on the parsed +request; it does not build a run plan from them. + +So changing the default template moves no golden fixture and no pinned wire contract. The `/run` +field shape is unchanged: it stays `tools?: string[]` at `services/runner/src/protocol.ts:469`. + +### Existing runner coverage of the grant list + +- `services/runner/tests/unit/builtin-grant-list.test.ts` pins that `tools: ["read"]` yields + exactly `["read"]` and that `replaceActiveBuiltinTools` drops the rest. +- `services/runner/tests/unit/sandbox-agent-run-plan.test.ts:205` is the closest existing test to + this bug. It pins that an omitted `tools` key yields the four defaults with gating off, and that + `tools: []` yields no grants with gating on. It asserts the runner's semantics are correct. It + cannot catch this bug, because the bug is that the platform never sends the omitted form. +- `services/runner/tests/unit/extension-tools.test.ts:224` pins that + `replaceActiveBuiltinTools` preserves the positions of non-built-in tools. +- `services/runner/tests/unit/sandbox-agent-pi-assets.test.ts:203` pins the env vars + `AGENTA_AGENT_BUILTIN_GATING` and `AGENTA_AGENT_BUILTIN_GRANTS`. + +The gap the bug fell through is that no test drives the default template all the way to a `/run` +body. The Python side tests the default template's contents, and the runner side tests the wire +field's semantics, and nothing joins them. diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/status.md b/docs/design/agent-workflows/projects/default-agent-builtins/status.md new file mode 100644 index 0000000000..c389cf08dc --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/status.md @@ -0,0 +1,287 @@ +# Status + +**State: implemented, uncommitted. All four pieces are in the working tree; no branch and no pull +request yet.** + +Last updated 2026-07-30, after implementation. + +## Where the work stands + +| Piece | State | +| --- | --- | +| 1. Ship Pi's built-ins in the default template | Implemented | +| 2. Narrow the Claude harness warning | Implemented | +| 3. Correct the picker's three false statements and the built-in row label | Implemented | +| 4. Update the documentation that describes the default | Implemented | + +All four landed together as one change, per [plan.md](plan.md#order-and-independence). + +## What shipped + +**Source.** + +- `sdks/python/agenta/sdk/agents/pi_builtins.py` (new): `PI_DEFAULT_ACTIVE_BUILTINS`, a tuple of + Pi's four default built-in names. A leaf module, deliberately not in `agenta_builtins.py`. +- `sdks/python/agenta/sdk/utils/types.py`: `build_agent_v0_default()` builds its `tools` list from + that constant as `{"type": "builtin", "name": name}` entries. +- `sdks/python/agenta/sdk/agents/adapters/harnesses.py`: the Claude warning fires only when the + built-in set differs from `PI_DEFAULT_ACTIVE_BUILTINS`, and names the tools. +- `services/runner/src/engines/sandbox_agent/run-plan.ts`: `PI_DEFAULT_ACTIVE_BUILTINS` is now + exported so the parity test can read it. No runner behavior changed. +- `web/packages/agenta-entity-ui/.../PiSettingsControl.tsx`: all three false "Pi's defaults" claims + corrected, plus the `"Pi defaults"` placeholder. +- `web/packages/agenta-entity-ui/.../agentTemplate/itemDescriptors.tsx`: a built-in row reads its + own `name`, falling back to `type` for provider built-ins that carry none. + +**Tests.** + +- `sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json` (new): the + shared cross-language fixture, asserted from Python by + `unit/agents/test_pi_builtins_parity.py` (new) and from TypeScript by + `services/runner/tests/unit/pi-default-builtins-parity.test.ts` (new). +- `sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py`: the crossing test. It runs the + real chain from the shipped default to the `/run` body (`build_agent_v0_default` → + `AgentTemplate.from_params` → `ToolResolver.resolve` → `PiHarness._to_harness_config` → + `request_to_wire`) and asserts `tools == ["read", "bash", "edit", "write"]`. This is the test + that would have caught the original bug. +- `services/oss/tests/pytest/unit/agent/test_default_agent_template.py`: new + `test_published_default_grants_pi_default_builtins`; the authoring-extras test now asserts that + no `platform` or embed entry is present rather than that the list is empty. +- `sdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.py`: the exact default set is + silent; a subset, a superset, and a non-default name each warn. +- `api/oss/tests/pytest/unit/resources/test_workflow_catalog.py`: the catalog's schema-default + hoist materializes the four entries into `parameters.agent.tools`. +- `services/runner/tests/unit/sandbox-agent-run-plan.test.ts`: the exact grant list the platform + now sends, under `allow` (gating off, the fast path) and under `allow_reads` (gating on). +- `web/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.ts` (new), + `web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts` (new), and + a no-duplicate overlay case in `web/packages/agenta-playground/tests/unit/agentRequest.test.ts`. + +**Documentation.** `interfaces/public-edge/agent-config-schema.md`, +`interfaces/in-service/harness-adapters.md`, `interfaces/README.md` (both index rows), +`documentation/tools.md`, `documentation/agent-configuration.md`, and the config-shape example in +`sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py`. + +Test results: SDK 2330 passed, services 112 passed, API 1495 passed, runner 1308 passed, and the +three frontend package suites 285 / 213 / 929 passed. Every remaining error in the Python suites is +`AssertionError: AGENTA_API_URL must be set`, the acceptance tests that need a running stack. + +## Deferred during implementation + +- **The default-config block in `interfaces/public-edge/agent-config-schema.md` is still stale in + other ways.** Its `tools` line is now correct, but the surrounding object still shows the + pre-migration flat shape (`agents_md`, `model`, `mcp_servers`, `harness: "pi_core"`) and an older + model id, while the builder emits the nested shape. [plan.md](plan.md) scoped the edit to the + `tools` line for exactly this reason. Regenerating that whole block, and the same page's field + table, from real `build_agent_v0_default()` output is worth its own change. The same flat-versus- + nested drift runs through `documentation/agent-configuration.md`. +- **The manual verification in [testing.md](testing.md#manual-verification) has not been run.** No + stack was deployed for this work. Both checks remain open. + +## Decisions taken + +- **The default agent template ships Pi's four default built-ins** (`read`, `bash`, `edit`, + `write`), rather than the runner changing what an empty grant list means. Reasoning in + [design.md](design.md#which-built-ins-and-why-those-four) and + [design.md](design.md#alternatives-considered). +- **Scope is new agents only.** Agents saved before the fix are not repaired. +- **The playground overlay keeps its `read` and `bash` entries.** They guarantee the build kit's + skill is loadable regardless of what the author's template says, and the overlay's identity merge + means they cannot duplicate the default's entries. +- **The Claude harness stays silent only for the exact default set** (settled after the Codex + review; the first draft filtered name by name, which would have silenced a deliberately authored + subset). Anything else warns and names the tools. +- **No built-in picker is added to the Tools section in this change.** One already exists in + Advanced, and deciding which surface is canonical is separate work. +- **The new Python constant is `PI_DEFAULT_ACTIVE_BUILTINS`, and it does not live in + `agenta_builtins.py`** (settled after the Codex review). That module owns the `pi_agenta` + harness's forced Agenta opinions; Pi's native active set is not one. + +## Verified during planning + +- The proposed default value validates against the strict `AgentTemplateSchema` and parses into + four `BuiltinToolConfig` entries through `AgentTemplate.from_params`. Checked by running the real + models, not by reading them. +- The shared golden fixtures under `sdks/python/oss/tests/pytest/unit/agents/golden/` are built + from hand-written templates, not from `build_agent_v0_default()`, so no pinned wire contract + moves. Re-confirmed in the Codex review, including that no runner, API, or web snapshot embeds + the builder's output. +- `services/oss/tests/pytest/unit/agent/test_default_agent_template.py:68` and `:74` are the only + assertions in the repository that break. + +## Pre-ship conditions + +Both came out of the Codex review and neither existed in the first draft. + +1. **Do not ship into a shared deployment that still enables the `local` sandbox.** The shipped + default is `sandbox: local`, `read` runs without approval under `allow_reads`, and a `local` run + executes on the runner host with no cwd jail. See + [design.md](design.md#what-read-can-reach-on-the-local-sandbox). + + **Still open.** This is a deployment decision, not a code change, and nothing in the + implementation can satisfy it. Whoever deploys must confirm that + `AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS` excludes `local` on any shared deployment, remembering + that unset defaults to `["local"]`. + +2. **Fix the release-gate seeds before using the gate as evidence.** Both hand-write + `"tools": []`, so the gate would report green on the shape this change replaces. See + [testing.md](testing.md#manual-verification). + + **Satisfied.** `.agents/skills/agent-release-gate/resources/qa_probe.py` and + `resources/qa_product.py` now seed the four typed built-in entries. `qa_product.py`'s + `template()` helper keeps its `tools or []` fallback: that is a "the caller named no tools" + default for per-cell fixtures, and the MCP cell passes `tools=[]` deliberately to isolate MCP + tools, so changing it would alter what those cells assert. + +## Blocking nothing, waiting on nothing + +The open questions in [open-questions.md](open-questions.md) did not block the work; the first of +them determines whether follow-up work is needed for the scheduled-run half of +[#5562](https://github.com/Agenta-AI/agenta/issues/5562). + +When the issue is closed, say that this fixes newly created agents only. It does not repair agents +already saved, and it does not make an unattended write-capable run complete on its own. + +## Codex review + +**Round 1 (2026-07-30, gpt-5.6-sol at xhigh, read-only).** Codex read the whole workspace, the +prior [pi-builtin-gating](../pi-builtin-gating/README.md) design, the Python and TypeScript chain, +the tests and goldens, and Pi's own shipped source in `node_modules`. Its verdict was "do not +approve as written": the seam is right, the security analysis and two factual claims are not. + +Every finding below was re-verified against the code before being accepted or rejected. Nothing was +taken on Codex's word. + +**Nothing changes the recommended approach.** Codex agreed the default template is the right seam +and agreed with the rejection of both alternatives (making `[]` mean Pi's defaults, and making the +SDK omit the field). No finding argues for the rejected route or for a fourth seam. What changed is +that the design now states a security surface it had not stated, admits a cost in the option it +chose, and carries two pre-ship conditions. + +### Accepted + +1. **The `read` grant is an approval-free host-file read on the default sandbox.** The design + treated `read` running without approval as benign because it is classified read-only. Verified: + the default sandbox is `local` (`types.py:1071`), a `local` run spawns the harness on the runner + host (`provider.ts:148`), which the platform's own code calls "unconfined host bash and not a + tenant boundary" (`services/oss/src/agent/config.py:60`), Pi's `read` documents "relative or + absolute" paths and resolves them with `resolvePath(filePath, cwd)` with no jail, and + `AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS` defaults to `["local"]` when unset + (`sandbox_providers.py:30`). One correction to Codex's framing: this change does not create the + capability. The playground overlay already forces `read` and the playground also runs `local`, + so it exists interactively today. What the change adds is the unattended, non-playground version + of it. Folded into design.md as its own section, into open-questions.md as a decision, and into + the pre-ship conditions above. + +2. **Turning the gate off also turns grant enforcement off.** Under a blanket `allow` policy with + the exact four grants, `computeBuiltinGatingActive` returns false, so the runner never sets + `AGENTA_AGENT_BUILTIN_GATING` (`pi-assets.ts:369`), the extension's inertness guard skips + `registerBuiltinGating` (`agenta.ts:373`), and `replaceActiveBuiltinTools` never runs. Verified. + The four built-ins are then active because Pi activates them, not because the grant list was + applied. The design sold this as a pure win ("keeps the fast path with no relay round trips"); + it is also the reason a future Pi release that adds a fifth default tool would hand it to an + agent whose saved grant list names four. Choosing Pi's four is what moves the shipped default + onto that path. The fix belongs to pi-builtin-gating, not here; the cross-language constant pin + is the interim guard. Folded into design.md and open-questions.md. + +3. **The consumer inventory contained a false claim.** design.md said the built-in's fallback + parameters at `utils.py:288` have no live caller. Verified false: `retrieve_configuration` + (`utils.py:526`) is read by `seed_empty_parameters_from_configuration` (`utils.py:534`), which + the resolver middleware calls on every invoke (`resolver.py:571`, `:596`), and the workflow + decorator reads the same registry (`decorators/running.py:240`). Two existing tests drive it: + `test_workflow_shapes_running.py:234` and `test_platform_handlers.py:237`. So the change also + affects API and SDK callers that invoke a revision bound to `agenta:builtin:agent:v0` with no + parameters, not only agents created in the playground. Corrected in design.md. + +4. **The Claude-golden reason for rejecting the `wire_tools()` alternative was wrong.** + `ClaudeAgentTemplate.wire_tools()` hardcodes `"tools": []` itself (`dtos.py:921`) and never + reads `builtin_names`, so editing `PiAgentTemplate.wire_tools()` could not move + `run_request.claude.json`. Verified. The rejection stands on the authoring-semantics argument + alone; the false supporting reason is removed from design.md. + +5. **Piece 2's warning filter would silence real misconfiguration.** Filtering name by name against + Pi's defaults means an author who selected only `["bash"]` and then switched to Claude gets no + warning, because every name in their set is in the default set. The code has no provenance + field, so the only honest predicate is exact-set equality. Piece 2 and its tests rewritten in + design.md and plan.md. + +6. **A Claude agent created from the default carries four dead Pi entries.** The create-agent + factory overlays the last-used harness and nothing else (`appUtils.ts:186`, + `agentCreationPrefs.ts:32`), so a Claude agent minted from the Pi default keeps the four + built-in rows. Verified. Cosmetic, not blocking; recorded in design.md and filed with the + surface question in open-questions.md, because the real fix is harness-aware creation. + +7. **The test plan stops at the wire and never reaches the product path.** The crossing test proves + builder-to-wire and would have failed on the original bug, but nothing covers catalog + materialization, the frontend factory, or the commit. Two cheap additions folded into + testing.md. Codex also caught that the sketch calls a `resolve_tools_offline` that does not + exist, and that hand-constructing `PiAgentTemplate` skips `PiHarness._to_harness_config`, which + removes one of the two properties the test is for. Both corrected. + +8. **The release-gate seeds hand-write `tools: []`.** Verified at `qa_probe.py:82` and + `qa_product.py:752`. The gate named in testing.md as the harness for manual check 2 would have + reported green on the exact shape this change replaces. Now a pre-ship condition. + +9. **The regex-over-TypeScript pin is a maintenance trap.** Replaced in testing.md with a shared + golden fixture asserted from both languages, the way `permission_decisions.json` already is. + Also verified that CI runs the Python half on a `services/**` change, so the pin does fire in + both directions. + +10. **The constant's name and home.** Renamed to `PI_DEFAULT_ACTIVE_BUILTINS`, matching the + TypeScript name exactly, and moved out of `agenta_builtins.py`. That module's own contract says + it holds "the Agenta harness's forced defaults: the things `AgentaHarness` always applies" + (`agenta_builtins.py:1`); Pi's native active set is not an Agenta opinion, and putting it + beside `AGENTA_FORCED_TOOLS` would blur the exact distinction this bug is made of. + +11. **Piece 3 has three false strings, not one.** The file header comment (`:5`), the `onChange` + prop comment (`:25`), and the help text (`:111`) all assert that an absent or empty `tools` + leaves Pi's defaults. plan.md now names all three. + +12. **The pieces are separate review lanes, not separate releases.** Piece 1 alone makes the Claude + warning fire on nearly every Claude run and shows four misleading rows. plan.md now says they + ship together, and that piece 1 closes #5590 only for new agents. + +### Accepted as observations, not as scope + +- **Malformed permission rules fail open.** `normalizeRules` (`permission-plan.ts:191`) silently + returns `[]` for a non-array, so `{default: "allow", rules: "garbage"}` becomes a blanket allow + and, with the four grants, turns gating off. Verified. It is real and it is defense in depth: the + SDK builds `rules` through `wire_author_permission_rules`, so the platform never sends this + shape. The fix belongs to the permission module, not to a template default. Not folded into the + plan. +- **Unset versus explicitly empty should stop being the same value.** Verified that + `default_factory=list` collapses them on both models and that the picker writes `undefined` on a + deliberate clear. This is the ambiguity that makes repairing existing agents impossible, so it is + worth fixing, but it is a schema and UI change with its own review. Recorded in + open-questions.md. +- **The build-kit overlay's tool identity is too broad.** `name:` (`buildKitOverlay.ts:47`) + would collide a built-in `read` with any other tool type named `read`; `builtin:` would be + tighter. Verified, and Codex also confirmed the design's own claim that the current `read` and + `bash` entries replace in place without duplicating. A `web/` change unrelated to this fix. +- **`BuiltinToolConfig`'s comment is stale.** `models.py:87` still says "no runner gate sees them + on Pi", which the pi-builtin-gating work made untrue. Verified. A one-line comment fix for + whoever is next in that file. + +### Rejected + +- **"Add a runtime capability handshake so a stale extension bundle cannot fail open."** The + failure mode is real and verified (`installPiExtensionLocal` checks that the bundle exists and + copies, `pi-assets.ts:397`, but nothing attests that it registered the hooks). It is rejected + *here* because it is not this project's finding: pi-builtin-gating already identified it, already + called it a silent gate bypass rather than polish, and already filed it as its next slice with a + design direction (a handshake record written at `before_agent_start`). Duplicating it in this + workspace would create a second owner for one problem. This change does not make it worse: a + stale bundle today means Pi's four defaults run ungated, which is the same outcome. +- **"Set an explicit rollout order: deploy gating-capable runners before the Python default starts + granting tools."** Rejected as stated, because it describes a skew that cannot occur here. Gating + landed on `main` in commit `3606e5d5cb` on 2026-07-10 and every runner build since carries it; + the template default and the runner are not independently versioned deployables in this + repository. Codex's related observation that `/health` exposes only a protocol major and that the + Python path never probes it is accurate, but a `/run` field whose shape does not change needs no + negotiation. + +## Provenance + +Design workspace created 2026-07-30. Research read against the `gitbutler/workspace` branch. Codex +review round 1 folded in above the same day, and the change implemented against that branch the +same day. diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/testing.md b/docs/design/agent-workflows/projects/default-agent-builtins/testing.md new file mode 100644 index 0000000000..b9eac8dd29 --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/testing.md @@ -0,0 +1,185 @@ +# How this is tested + +All targets below are existing test suites in this repository. Run them with the commands in +[docs/designs/testing/README.md](../../../../designs/testing/README.md): `cd services && py-run-tests`, +`cd sdks/python && py-run-tests`, `cd api && py-run-tests`, and `pnpm test` inside +`services/runner`. + +## The test that would have caught this + +Nothing in the repository drives the shipped default template all the way to a `/run` body. The +Python tests check what the default contains +(`services/oss/tests/pytest/unit/agent/test_default_agent_template.py`), and the runner tests check +what the wire field means +(`services/runner/tests/unit/sandbox-agent-run-plan.test.ts:205`, which already pins that `[]` and +an omitted key differ). Both suites were green while every agent shipped with no tools, because +neither one crosses the boundary. + +Add the crossing test to +`sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py`, which already owns the +"what actually goes on the wire" question: + +```python +def test_default_template_grants_pi_default_builtins_on_the_wire(): + """A default-derived agent must reach the runner with Pi's built-ins granted. + + The runner reads `tools: []` as "grant nothing" and deletes every built-in from Pi's active + set, so an empty list here means a saved agent has no read, bash, edit or write anywhere + outside the playground (issue #5590). + """ + template = AgentTemplate.from_params({"agent": build_agent_v0_default()}) + resolved = resolve_tools_offline(template.tools) + payload = request_to_wire(... PiAgentTemplate(builtin_names=resolved.builtin_names, ...) ...) + + assert payload["tools"] == ["read", "bash", "edit", "write"] +``` + +Two properties make this the right test. It starts from the shipped default rather than a +hand-written template, so it fails if anyone empties the default again. And it asserts on the wire +payload rather than on the template, so it fails if a future change to `wire_tools`, the resolver, +or the harness adapter drops built-ins on the way. + +The tool resolution step needs no network. `ToolResolver.resolve` +(`sdks/python/agenta/sdk/agents/tools/resolver.py:113`) derives `builtin_names` by filtering for +`BuiltinToolConfig`, and the default template has no gateway or reference tools to resolve. + +Two cautions on the sketch above, which is pseudocode. `resolve_tools_offline` does not exist; the +test has to build the resolution step out of what does. And the middle of the chain must be the +real one. Constructing `PiAgentTemplate(builtin_names=...)` by hand skips +`PiHarness._to_harness_config` (`harnesses.py:75`), which is the layer that copies `builtin_names` +onto the harness template. Skipping it removes one of the two properties the test is for. Go +through the harness adapter, and only fall back to hand-filtering the configs if the suite cannot +build a `SessionConfig`. + +## Tests that must be updated + +`services/oss/tests/pytest/unit/agent/test_default_agent_template.py:68` and `:74` assert +`inspect_default["tools"] == []` and `builtin_default["tools"] == []`. These are the only +assertions in the repository that break. + +They should not simply be flipped to the new list. The test they sit in is +`test_authoring_extras_absent_from_every_published_default`, whose point is that the playground +build kit's extras never leak into the published default. Pi's built-ins are not authoring extras, +so they belong in a separate assertion with its own reason: + +```python +def test_published_default_grants_pi_default_builtins(): + """A new agent must be able to read, run shell commands, and edit and write files wherever it + runs, not only in the playground (issue #5590). The runner reads an empty tools list as + "grant nothing".""" + expected = [{"type": "builtin", "name": name} for name in PI_DEFAULT_BUILTIN_NAMES] + assert _inspect_agent_default()["tools"] == expected + assert _builtin_agent_default()["tools"] == expected +``` + +Leave `test_authoring_extras_absent_from_every_published_default` asserting the things it was +written for: no platform ops, no authoring skill, no elevated sandbox permissions. Change its +`tools` assertions to check that no `platform` or embed entry is present, rather than that the +list is empty. + +## Pinning the two copies of Pi's default built-in list + +The design introduces a Python constant for Pi's four default built-ins, and +`services/runner/src/engines/sandbox_agent/run-plan.ts:192` already holds the same list as +`PI_DEFAULT_ACTIVE_BUILTINS`. Two copies in two languages drift. + +Pin them with a shared fixture asserted from both sides, the way `permission_decisions.json` +already is: a small golden under `sdks/python/oss/tests/pytest/unit/agents/golden/` holding the +four names, read by a Python test against the new constant and by a runner test against +`PI_DEFAULT_ACTIVE_BUILTINS`. Neither language owns the list; both are implementations of one +pinned contract. + +An earlier draft proposed a Python test that regexes the array literal out of `run-plan.ts`. It is +rejected. It binds a Python unit test to TypeScript formatting and to a file path, so a +reformat or a move breaks a test that has nothing to do with either, and the failure names the +wrong cause. The extra machinery of a fourth golden file is small next to that. + +Both sides run in CI on either change: `.github/workflows/12-check-unit-tests.yml` triggers on +both `sdks/python/**` and `services/**`, so a `run-plan.ts` edit runs the SDK job that holds the +Python half of the pin. + +## Runner tests + +No runner behavior changes, so no runner test changes. Two existing tests should be confirmed +still green rather than edited, because they pin the semantics this design deliberately does not +touch: + +- `services/runner/tests/unit/sandbox-agent-run-plan.test.ts:205`, "distinguishes omitted tools + from an explicit empty grant set". +- `services/runner/tests/unit/builtin-grant-list.test.ts`, the regression pin for the grant list + going dead in commit `0e71bd0f7a`. + +One addition is worth making while the area is open. `sandbox-agent-run-plan.test.ts` has no case +for the exact grant list the platform now sends. Add one asserting that +`tools: ["read", "bash", "edit", "write"]` under a blanket `allow` policy yields those four grants +and leaves `builtinGatingActive` false, which is the fast path the choice of set was made to +preserve, and that the same grant list under `allow_reads` turns gating on. + +## Frontend tests + +The two frontend changes in scope are the picker's help text and the built-in row label. + +The row label has a natural home in +`web/packages/agenta-entity-ui/tests/unit/`, which already uses +`{type: "builtin", name: "read"}` as a fixture in `toolPermission.test.ts:128`. Assert that +`describeTool({type: "builtin", name: "read"})` returns the name `read` rather than `builtin`, and +that a provider built-in with no `name` still falls back to its `type`. + +The help text is copy and needs no unit test. + +Confirm `web/packages/agenta-playground/tests/unit/agentRequest.test.ts` stays green. Its overlay +cases (`:301` onward) exercise `withBuildKitOverlay`, and the identity merge is the mechanism that +keeps the overlay from duplicating the default's `read` and `bash`. Add a case where the base +template already carries all four built-ins and assert the merged list contains each name exactly +once, since that is the specific interaction this change creates. + +## What the crossing test still does not cover + +The crossing test proves builder-to-wire. The bug's actual path is longer: builder → the API +catalog's materialized `parameters` → the frontend factory → the committed revision → a run. Every +link in that second half is untested, and a break in any of them reproduces the same symptom. + +Two of them are cheap to close and worth adding. + +- **The catalog materializes the tools.** Assert that the templates endpoint's + `template.data.parameters.agent.tools` carries the four entries, against + `_build_template_data` (`api/oss/src/resources/workflows/catalog.py:106`). This is the step that + hoists an object default out of the JSON Schema, and it drops non-primitive defaults from the + schema afterwards, so it is not obviously a pass-through. +- **The factory copies them into a new agent.** Assert that + `createEphemeralAppFromTemplate` (`web/packages/agenta-entities/src/workflow/state/appUtils.ts:134`) + preserves `parameters.agent.tools` for a Pi template, including when the last-used-harness + preference is applied (`agentCreationPrefs.ts:32`), which rewrites `harness.kind` and must leave + `tools` alone. + +The remaining links (commit, then invoke the committed revision) are the manual checks below. + +## Manual verification + +The unit tests cannot show that a real Pi agent regains its tools. Two checks against a running +stack close that gap. + +1. Create a new agent from the default and commit it without editing anything. Confirm its saved + revision's `parameters.agent.tools` holds the four built-in entries, and that the Tools section + lists them by name. + + **Done, 2026-07-30.** Two checks against a running stack, both passing. The agent service's + `/inspect` returns the four entries as the schema default the playground pre-fills from. And + `GET /workflows/catalog/templates/` returns them under `data.parameters` for + `agenta:builtin:agent:v0`, which is the source `createAppFromTemplate` prefers when it mints a + new agent (`web/packages/agenta-entities/src/workflow/api/createFromTemplate.ts:185`). So the + value a new agent is created from carries the four tools. The UI listing was not checked by + hand; `itemDescriptors` unit tests cover the row labels. +2. Run that committed revision outside the playground, through a schedule or a direct invoke of + the committed revision. Ask it to read a file. It should read the file rather than report that + it has no tools. Then ask it to run a shell command and confirm the approval behavior described + in [design.md](design.md) and [open-questions.md](open-questions.md), rather than a silent + "no tools available" answer. + +The `agent-release-gate` skill drives the same product endpoint at the wire level and asserts on +the frame stream, which is the right harness for check 2, but it needs a fix first. Both of its +seeds hand-write `"tools": []` into the agent template they commit +(`.agents/skills/agent-release-gate/resources/qa_probe.py:82` and +`resources/qa_product.py:752`). Run as-is, the gate would keep exercising the old empty-list shape +and would report green on exactly the configuration this change exists to replace. Update both +seeds to the shipped default before using the gate as evidence. diff --git a/sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py b/sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py index e18a058245..9f751dfce3 100644 --- a/sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py +++ b/sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py @@ -127,7 +127,12 @@ { "instructions": { "agents_md": "" }, "llm": { "model": "gpt-5.5", "provider": "openai", "connection": { "mode": "agenta" } }, - "tools": [], + "tools": [ + { "type": "builtin", "name": "read" }, + { "type": "builtin", "name": "bash" }, + { "type": "builtin", "name": "edit" }, + { "type": "builtin", "name": "write" } + ], "mcps": [], "skills": [], "harness": { "kind": "pi_agenta" }, @@ -173,7 +178,10 @@ runner default for that one tool). The six `type` values: - `builtin` — a harness built-in: `{ "type": "builtin", "name": "read" }`. (A per-builtin - `permission` is dropped — builtins are granted by selection, not gated.) + `permission` is dropped — builtins are granted by selection, not gated.) A new agent starts + with Pi's four defaults (`read`, `bash`, `edit`, `write`), as shown above. Keep them unless the + user asks you to drop one: an empty `tools` list grants NO built-ins, so an agent that ships + with `[]` cannot read or write a file at all. - `gateway` — a server-side gateway action (Composio). Do not hand-write it: run `discover_tools` and copy what it returns, adding the `connection` slug once the connection is ready. `{ "type": "gateway", "provider": "composio", "integration": "github", diff --git a/sdks/python/agenta/sdk/agents/adapters/harnesses.py b/sdks/python/agenta/sdk/agents/adapters/harnesses.py index 8fcf837086..752ed4a25b 100644 --- a/sdks/python/agenta/sdk/agents/adapters/harnesses.py +++ b/sdks/python/agenta/sdk/agents/adapters/harnesses.py @@ -31,6 +31,7 @@ SessionConfig, ) from ..interfaces import Environment, Harness +from ..pi_builtins import PI_DEFAULT_ACTIVE_BUILTINS from ..tools.models import ToolSpec, coerce_tool_spec from .agenta_builtins import ( compose_append_system, @@ -91,10 +92,15 @@ class ClaudeHarness(Harness): def _to_harness_config(self, config: SessionConfig) -> ClaudeAgentTemplate: # Claude has no Pi built-in tools; drop them rather than ship a name Claude cannot # honor. Tools go over MCP, and the shared permission plan is carried through. - if config.builtin_names: + # Exact-set equality, not a per-name filter: the shipped default carries exactly Pi's + # defaults, so that is the only set we can assume the author never touched, and a + # per-name filter would silence an authored subset such as ["bash"]. + if config.builtin_names and set(config.builtin_names) != set( + PI_DEFAULT_ACTIVE_BUILTINS + ): log.warning( - "ClaudeHarness ignores %d built-in tool(s); built-ins are a Pi concept", - len(config.builtin_names), + "ClaudeHarness ignores built-in tool(s) %s; built-ins are a Pi concept", + ", ".join(config.builtin_names), ) # Skills stay on the harness config; the runner materializes them under `.claude/skills` # in the session cwd so Claude ACP can load the same resolved inline packages. diff --git a/sdks/python/agenta/sdk/agents/pi_builtins.py b/sdks/python/agenta/sdk/agents/pi_builtins.py new file mode 100644 index 0000000000..f80cd3cb44 --- /dev/null +++ b/sdks/python/agenta/sdk/agents/pi_builtins.py @@ -0,0 +1,17 @@ +"""Facts about the Pi harness itself, independent of any Agenta opinion. + +``PI_DEFAULT_ACTIVE_BUILTINS`` is Pi's OWN default active built-in set: what Pi turns on when +nobody tells it otherwise. It is deliberately not in :mod:`.adapters.agenta_builtins`, which +holds the ``pi_agenta`` harness's forced Agenta opinions (``AGENTA_FORCED_TOOLS`` and friends). + +The TypeScript side holds the same list under the same name +(``services/runner/src/engines/sandbox_agent/run-plan.ts``). Neither language owns it: both are +pinned against the shared golden fixture +``sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json``. +""" + +from __future__ import annotations + +from typing import Tuple + +PI_DEFAULT_ACTIVE_BUILTINS: Tuple[str, ...] = ("read", "bash", "edit", "write") diff --git a/sdks/python/agenta/sdk/utils/types.py b/sdks/python/agenta/sdk/utils/types.py index 2df5c5fba6..bdba2bfdb1 100644 --- a/sdks/python/agenta/sdk/utils/types.py +++ b/sdks/python/agenta/sdk/utils/types.py @@ -10,6 +10,7 @@ from agenta.sdk.agents.dtos import HARNESS_IDENTITIES, SandboxPermission from agenta.sdk.agents.mcp import MCPServerConfig +from agenta.sdk.agents.pi_builtins import PI_DEFAULT_ACTIVE_BUILTINS from agenta.sdk.agents.tools import ToolConfig from agenta.sdk.agents.wire_models import run_contract_schemas from agenta.sdk.utils.assets import supported_llm_models, model_metadata @@ -1417,7 +1418,7 @@ def build_agent_v0_default( """The default agent-template value, shared by the builtin interface and the service. The agent-template value that sits at ``parameters.agent`` (Step 1 of the agent-template - migration): the portable definition flat (instructions / llm / empty tools / mcps) plus the + migration): the portable definition flat (instructions / llm / tools / mcps) plus the nested execution parts (``harness`` / ``runner`` / ``sandbox``). ``include_sandbox_permission`` adds the declared Layer-2 boundary the playground pre-fills (network egress on, strict). ``skill_slug`` adds one ``@ag.embed`` reference under ``skills`` to a stored skill the backend @@ -1426,7 +1427,12 @@ def build_agent_v0_default( template: Dict[str, Any] = { "instructions": {"agents_md": _DEFAULT_AGENTS_MD}, "llm": {"provider": _DEFAULT_AGENT_PROVIDER, "model": _DEFAULT_AGENT_MODEL}, - "tools": [], + # Pi's own default active built-ins, in the typed form the strict schema describes. The + # runner reads an empty list as "grant nothing", which left a saved agent with no + # read/bash/edit/write anywhere outside the playground overlay (issue #5590). + "tools": [ + {"type": "builtin", "name": name} for name in PI_DEFAULT_ACTIVE_BUILTINS + ], "mcps": [], } if skill_slug is not None: diff --git a/sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json b/sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json new file mode 100644 index 0000000000..095cbd23f2 --- /dev/null +++ b/sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json @@ -0,0 +1,4 @@ +{ + "description": "Pi's own default active built-in tool names. Neither language owns this list: the Python constant PI_DEFAULT_ACTIVE_BUILTINS (agenta/sdk/agents/pi_builtins.py) and the TypeScript PI_DEFAULT_ACTIVE_BUILTINS (services/runner/src/engines/sandbox_agent/run-plan.ts) are two implementations of this one pinned contract. The shipped default agent template grants exactly these, so the two must not drift.", + "names": ["read", "bash", "edit", "write"] +} diff --git a/sdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.py b/sdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.py index bbaab7729b..be334dfde8 100644 --- a/sdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.py +++ b/sdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.py @@ -37,6 +37,7 @@ force_skills, ) from agenta.sdk.agents.adapters.harnesses import _normalize_tool_specs, _opt_str +from agenta.sdk.agents.pi_builtins import PI_DEFAULT_ACTIVE_BUILTINS _CALLBACK = ToolCallback(endpoint="https://api.example/tools/call", authorization=None) @@ -327,6 +328,55 @@ def test_claude_no_warning_without_builtins(make_env, monkeypatch): assert recorded == [] +def _recorded_warnings(monkeypatch) -> list: + """Swap the adapter module's logger and collect the args of every warning it emits.""" + recorded: list = [] + monkeypatch.setattr( + harnesses, + "log", + type("L", (), {"warning": lambda self, *a, **k: recorded.append(a)})(), + ) + return recorded + + +def test_claude_stays_silent_for_the_untouched_default_builtin_set( + make_env, monkeypatch +): + """The shipped default template carries exactly Pi's four defaults (issue #5590), so + warning on that set would fire on nearly every Claude run and drown the case the warning + exists for: an author who configured built-ins and then switched to Claude.""" + recorded = _recorded_warnings(monkeypatch) + harness = ClaudeHarness(make_env(supported=[HarnessKind.CLAUDE])) + + harness._to_harness_config( + _session_config(builtin_tools=list(PI_DEFAULT_ACTIVE_BUILTINS)) + ) + + assert recorded == [] + + +@pytest.mark.parametrize( + "builtin_tools", + [ + pytest.param(["bash"], id="subset"), + pytest.param([*PI_DEFAULT_ACTIVE_BUILTINS, "grep"], id="superset"), + pytest.param(["ls"], id="non_default_name"), + ], +) +def test_claude_warns_for_any_set_other_than_the_default( + make_env, monkeypatch, builtin_tools +): + # Only the exact default set is assumed untouched; every other set is an authoring act + # Claude silently drops. The message names the tools so the author can see which. + recorded = _recorded_warnings(monkeypatch) + harness = ClaudeHarness(make_env(supported=[HarnessKind.CLAUDE])) + + harness._to_harness_config(_session_config(builtin_tools=builtin_tools)) + + assert recorded, f"expected a warning for {builtin_tools}" + assert recorded[0][1] == ", ".join(builtin_tools) + + def test_claude_threads_permissions_and_renders_settings_file(make_env): import json diff --git a/sdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.py b/sdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.py new file mode 100644 index 0000000000..eb5dd019c4 --- /dev/null +++ b/sdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.py @@ -0,0 +1,23 @@ +"""Cross-language parity for Pi's default active built-in set. + +Two implementations name the same four tools and must never drift: + - Python: ``PI_DEFAULT_ACTIVE_BUILTINS`` in ``agenta/sdk/agents/pi_builtins.py``, which the + shipped default agent template builds its ``tools`` entries from. + - TypeScript: ``PI_DEFAULT_ACTIVE_BUILTINS`` in + ``services/runner/src/engines/sandbox_agent/run-plan.ts``, which the runner falls back to + when the ``/run`` request omits ``tools``. + +Neither language owns the list. Both assert the SAME shared fixture +(``golden/pi_default_active_builtins.json``, loaded through the ``golden`` fixture in +``conftest.py``); the TypeScript half lives in the runner's unit tests. If the two disagree, +that is a real drift -- fix the side that moved, do not bend the fixture. +""" + +from __future__ import annotations + +from agenta.sdk.agents.pi_builtins import PI_DEFAULT_ACTIVE_BUILTINS + + +def test_python_constant_matches_the_shared_golden(golden): + fixture = golden("pi_default_active_builtins.json") + assert list(PI_DEFAULT_ACTIVE_BUILTINS) == fixture["names"] 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 f3fb58e528..1c3620867c 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 @@ -21,11 +21,13 @@ from agenta.sdk.agents import ( AgentaAgentTemplate, + AgentTemplate, ClaudeAgentTemplate, Endpoint, HarnessKind, Message, PiAgentTemplate, + PiHarness, ResolvedConnection, RunContext, RunContextReference, @@ -33,8 +35,10 @@ RunContextTrace, RunContextWorkflow, SandboxPermission, + SessionConfig, SkillTemplate, ToolCallback, + ToolResolver, TraceContext, ) from agenta.sdk.agents.utils.wire import ( @@ -42,6 +46,7 @@ result_from_wire, sanitize_runner_error, ) +from agenta.sdk.utils.types import build_agent_v0_default # The full set of top-level keys ``request_to_wire`` may emit. The TS ``AgentRunRequest`` # interface must declare a superset of these. Adding a key here without adding it to @@ -309,6 +314,37 @@ def test_request_to_wire_pi_matches_golden(golden): assert "harnessFiles" not in payload +async def test_default_template_grants_pi_default_builtins_on_the_wire(make_env): + """A default-derived agent must reach the runner with Pi's built-ins granted (issue #5590). + + The runner reads ``tools: []`` as "grant nothing" and deletes every built-in from Pi's + active set, so an empty list here means a saved agent has no read, bash, edit or write + anywhere outside the playground. This starts from the SHIPPED default rather than a + hand-written template, and it runs the real chain (template parse, tool resolution, the Pi + harness adapter, the wire serializer), because any of those layers could drop the built-ins + on the way. + """ + template = AgentTemplate.from_params({"agent": build_agent_v0_default()}) + resolved = await ToolResolver().resolve(template.tools) + harness = PiHarness(make_env(supported=[HarnessKind.PI])) + config = harness._to_harness_config( + SessionConfig( + agent=template, + builtin_names=resolved.builtin_names, + tool_specs=resolved.tool_specs, + ) + ) + + payload = request_to_wire( + harness=HarnessKind.PI, + sandbox="local", + config=config, + messages=[Message(role="user", content="hi")], + ) + + assert payload["tools"] == ["read", "bash", "edit", "write"] + + def test_request_to_wire_omits_run_context_when_none(): # No run context passed -> no `runContext` key (a run that needs no `call.context` binding stays # byte-identical to before, the same discipline skills/mcpServers/sandboxPermission use). diff --git a/services/oss/src/agent/config.py b/services/oss/src/agent/config.py index abf9a620f4..faf121088e 100644 --- a/services/oss/src/agent/config.py +++ b/services/oss/src/agent/config.py @@ -11,6 +11,7 @@ from pathlib import Path from typing import Any, List, Optional +from agenta.sdk.agents.pi_builtins import PI_DEFAULT_ACTIVE_BUILTINS from agenta.sdk.agents.sandbox_providers import enabled_sandbox_providers from agenta.sdk.utils.logging import get_module_logger @@ -22,8 +23,13 @@ # Fallback config used when the editable files are missing or a field is absent. # Kept in sync with the catalog template and the `/inspect` schema defaults -# (schemas.py: _DEFAULT_MODEL / _DEFAULT_AGENTS_MD). +# (schemas.py: _DEFAULT_MODEL / _DEFAULT_AGENTS_MD), including the shipped grant list: an +# empty `tools` reaches the runner as "grant no built-ins", so the fallback carries Pi's own +# defaults exactly as the catalog template does (issue #5590). DEFAULT_MODEL = "gpt-5.6-luna" +DEFAULT_TOOLS: List[Any] = [ + {"type": "builtin", "name": name} for name in PI_DEFAULT_ACTIVE_BUILTINS +] DEFAULT_AGENTS_MD = ( "You are a friendly hello-world agent running on the Agenta agent service.\n\n" "- Greet the user warmly.\n" @@ -102,7 +108,7 @@ def load_config() -> AgentTemplate: ) model: str = DEFAULT_MODEL - tools: List[str] = [] + tools: List[Any] = list(DEFAULT_TOOLS) meta_path = base / "agent.json" if meta_path.exists(): meta = json.loads(meta_path.read_text(encoding="utf-8")) @@ -111,7 +117,8 @@ def load_config() -> AgentTemplate: else: log.warning( "agent: template not found at %s; falling back to the built-in default " - "model %r with no tools (set AGENTA_AGENT_TEMPLATE_DIR if this path is unexpected)", + "model %r with the default built-in tools (set AGENTA_AGENT_TEMPLATE_DIR if this " + "path is unexpected)", meta_path, DEFAULT_MODEL, ) diff --git a/services/oss/tests/pytest/unit/agent/test_config_template_fallback.py b/services/oss/tests/pytest/unit/agent/test_config_template_fallback.py index c4ada319a4..45febf2def 100644 --- a/services/oss/tests/pytest/unit/agent/test_config_template_fallback.py +++ b/services/oss/tests/pytest/unit/agent/test_config_template_fallback.py @@ -7,6 +7,8 @@ import logging +from agenta.sdk.agents.pi_builtins import PI_DEFAULT_ACTIVE_BUILTINS + from oss.src.agent import config as agent_config @@ -21,6 +23,18 @@ def test_load_config_uses_real_template_when_present(): assert template.agents_md != agent_config.DEFAULT_AGENTS_MD +def test_on_disk_template_grants_pi_default_builtins(): + """The on-disk `agent.json` is a second copy of the shipped default (issue #5590). An empty + `tools` list there reaches the runner as "grant no built-ins", so a request that carries no + template of its own would run with no read, bash, edit or write.""" + expected = [ + {"type": "builtin", "name": name} for name in PI_DEFAULT_ACTIVE_BUILTINS + ] + + assert agent_config.load_config().tools == expected + assert agent_config.DEFAULT_TOOLS == expected + + def test_missing_template_logs_warning_and_falls_back(monkeypatch, tmp_path, caplog): monkeypatch.setenv("AGENTA_AGENT_TEMPLATE_DIR", str(tmp_path / "does-not-exist")) @@ -29,7 +43,7 @@ def test_missing_template_logs_warning_and_falls_back(monkeypatch, tmp_path, cap assert template.agents_md == agent_config.DEFAULT_AGENTS_MD assert template.model == agent_config.DEFAULT_MODEL - assert template.tools == [] + assert template.tools == agent_config.DEFAULT_TOOLS messages = [ record.message % record.args if record.args else record.message diff --git a/services/oss/tests/pytest/unit/agent/test_default_agent_template.py b/services/oss/tests/pytest/unit/agent/test_default_agent_template.py index abc61e5826..741a49c026 100644 --- a/services/oss/tests/pytest/unit/agent/test_default_agent_template.py +++ b/services/oss/tests/pytest/unit/agent/test_default_agent_template.py @@ -13,6 +13,7 @@ from __future__ import annotations from agenta.sdk.agents import AgentTemplate +from agenta.sdk.agents.pi_builtins import PI_DEFAULT_ACTIVE_BUILTINS from agenta.sdk.engines.running.interfaces import agent_v0_interface from agenta.sdk.utils.types import build_agent_v0_default @@ -59,23 +60,48 @@ def test_inspect_default_parses_into_the_runtime_selection(): assert config.permission_default == "allow_reads" +def _authoring_extra_tools(tools: list) -> list: + """The build-kit authoring entries: platform ops and the reserved client-tool embeds.""" + return [ + tool + for tool in tools + if not isinstance(tool, dict) + or tool.get("type") == "platform" + or "@ag.embed" in tool + ] + + def test_authoring_extras_absent_from_every_published_default(): # Platform tools, the authoring skill, and elevated sandbox permissions belong to the - # playground build-kit overlay, not to the published default template. + # playground build-kit overlay, not to the published default template. Pi's built-ins are + # not authoring extras (they are the shipped grant list), so they are asserted separately + # by `test_published_default_grants_pi_default_builtins`. inspect_default = _inspect_agent_default() builtin_default = _builtin_agent_default() - assert inspect_default["tools"] == [] + assert _authoring_extra_tools(inspect_default["tools"]) == [] assert "skills" not in inspect_default assert "permissions" not in inspect_default["sandbox"] assert "execute_code" not in inspect_default["sandbox"] assert "write_files" not in inspect_default["sandbox"] - assert builtin_default["tools"] == [] + assert _authoring_extra_tools(builtin_default["tools"]) == [] assert "permissions" not in builtin_default["sandbox"] assert "skills" not in builtin_default +def test_published_default_grants_pi_default_builtins(): + """A new agent must be able to read, run shell commands, and edit and write files wherever + it runs, not only in the playground (issue #5590). The runner reads an empty tools list as + "grant nothing", so shipping `tools: []` left every saved agent with no built-ins outside + the playground overlay.""" + expected = [ + {"type": "builtin", "name": name} for name in PI_DEFAULT_ACTIVE_BUILTINS + ] + assert _inspect_agent_default()["tools"] == expected + assert _builtin_agent_default()["tools"] == expected + + def test_harness_default_is_pi_core_in_every_source(): # The harness default is the single builder value (`harness.kind`), surfaced identically by the # SDK builtin and the service `/inspect`. diff --git a/services/runner/config/agent.json b/services/runner/config/agent.json index 6080c358b3..11f4678f80 100644 --- a/services/runner/config/agent.json +++ b/services/runner/config/agent.json @@ -1,4 +1,9 @@ { "model": "gpt-5.6-luna", - "tools": [] + "tools": [ + { "type": "builtin", "name": "read" }, + { "type": "builtin", "name": "bash" }, + { "type": "builtin", "name": "edit" }, + { "type": "builtin", "name": "write" } + ] } diff --git a/services/runner/src/engines/sandbox_agent/run-plan.ts b/services/runner/src/engines/sandbox_agent/run-plan.ts index 656c2fba84..bd01966aea 100644 --- a/services/runner/src/engines/sandbox_agent/run-plan.ts +++ b/services/runner/src/engines/sandbox_agent/run-plan.ts @@ -192,7 +192,12 @@ function hasCodeTool(specs: ResolvedToolSpec[]): boolean { return specs.some((spec) => spec.kind === "code"); } -const PI_DEFAULT_ACTIVE_BUILTINS = ["read", "bash", "edit", "write"]; +/** + * Pi's own default active built-in set. Exported so the cross-language parity test can pin it + * against the shared golden the Python `PI_DEFAULT_ACTIVE_BUILTINS` also asserts; not part of the + * engine's public surface. + */ +export const PI_DEFAULT_ACTIVE_BUILTINS = ["read", "bash", "edit", "write"]; const PI_BUILTIN_TOOL_NAMES = Object.keys(PI_BUILTIN_TOOL_IDENTITY); const PI_BUILTIN_TOOL_NAME_SET = new Set(PI_BUILTIN_TOOL_NAMES); diff --git a/services/runner/tests/unit/pi-default-builtins-parity.test.ts b/services/runner/tests/unit/pi-default-builtins-parity.test.ts new file mode 100644 index 0000000000..096701d041 --- /dev/null +++ b/services/runner/tests/unit/pi-default-builtins-parity.test.ts @@ -0,0 +1,34 @@ +/** + * Cross-language parity for Pi's default active built-in set. + * + * Two implementations name the same four tools and must never drift: + * - TS: `PI_DEFAULT_ACTIVE_BUILTINS` in `../../src/engines/sandbox_agent/run-plan.ts`, the set + * the runner falls back to when a `/run` request omits `tools`, and the set + * `computeBuiltinGatingActive` compares a grant list against for the no-gating fast path. + * - Python: `PI_DEFAULT_ACTIVE_BUILTINS` in `agenta/sdk/agents/pi_builtins.py`, which the shipped + * default agent template builds its `tools` entries from. + * + * Neither language owns the list. Both sides assert the SAME shared fixture, loaded in place (no + * copy) via `loadGolden`: + * `sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json`. The Python + * half asserts it in `sdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.py`. If the + * two disagree, that is a real drift between the implementations, not a fixture bug — fix the side + * that moved, do not bend the fixture. + * + * Run: pnpm test (or: pnpm exec vitest run tests/unit/pi-default-builtins-parity.test.ts) + */ +import { describe, it } from "vitest"; +import assert from "node:assert/strict"; + +import { loadGolden } from "../utils/golden.ts"; +import { PI_DEFAULT_ACTIVE_BUILTINS } from "../../src/engines/sandbox_agent/run-plan.ts"; + +const fixture = loadGolden("pi_default_active_builtins.json") as { + names: string[]; +}; + +describe("Pi default active built-ins parity fixture", () => { + it("the runner constant matches the shared golden", () => { + assert.deepEqual(PI_DEFAULT_ACTIVE_BUILTINS, fixture.names); + }); +}); diff --git a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts index d3d7f3f67e..2f21be384b 100644 --- a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts +++ b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts @@ -448,6 +448,56 @@ describe("buildRunPlan", () => { assert.equal(none.plan.useToolRelay, false); }); + it("keeps the fast path off for the shipped default's explicit four-builtin grant list", () => { + // The shipped default agent template now sends exactly Pi's own default set (issue #5590). + // Under a blanket allow that must stay equal to PI_DEFAULT_ACTIVE_BUILTINS, so gating stays + // off and no approval relay round trip is added per tool call — the reason this set was chosen. + const result = buildRunPlan( + { + harness: "pi_core", + messages: [{ role: "user", content: "hello" }], + permissions: { default: "allow", rules: [] }, + tools: ["read", "bash", "edit", "write"], + } as AgentRunRequest, + { createLocalCwd: () => "/tmp/local-cwd" }, + ); + + assert.equal(result.ok, true); + if (!result.ok) return; + assert.deepEqual(result.plan.builtinGrants, [ + "read", + "bash", + "edit", + "write", + ]); + assert.equal(result.plan.builtinGatingActive, false); + assert.equal(result.plan.useToolRelay, false); + }); + + it("turns builtin gating on for the same grant list under the default allow_reads mode", () => { + // allow_reads is the shipped default permission mode, and it can gate a builtin, so the + // fast path above is the blanket-allow case only. + const result = buildRunPlan( + { + harness: "pi_core", + messages: [{ role: "user", content: "hello" }], + permissions: { default: "allow_reads", rules: [] }, + tools: ["read", "bash", "edit", "write"], + } as AgentRunRequest, + { createLocalCwd: () => "/tmp/local-cwd" }, + ); + + assert.equal(result.ok, true); + if (!result.ok) return; + assert.deepEqual(result.plan.builtinGrants, [ + "read", + "bash", + "edit", + "write", + ]); + assert.equal(result.plan.builtinGatingActive, true); + }); + it("turns builtin gating on when the permission kill switch is set", () => { process.env.SANDBOX_AGENT_DENY_PERMISSIONS = "true"; diff --git a/web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts b/web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts new file mode 100644 index 0000000000..782be5b397 --- /dev/null +++ b/web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts @@ -0,0 +1,115 @@ +/** + * Unit tests for `createEphemeralAppFromTemplate` — the factory that mints a new agent's local + * entity from the API catalog's materialized template parameters. + * + * The shipped default grants Pi's four built-ins as `parameters.agent.tools` (issue #5590), and + * this factory is the link between that catalog payload and the config the user commits. It also + * overlays the last-used-harness preference, which rewrites `harness.kind` and must leave `tools` + * alone. Nothing covered that before, and a drop here reproduces the "agent has no tools" symptom + * with every Python test still green. + */ +import {QueryClient} from "@tanstack/react-query" +import {projectIdAtom} from "@agenta/shared/state" +import {getDefaultStore} from "jotai" +import {queryClientAtom} from "jotai-tanstack-query" +import {beforeEach, describe, expect, it, vi} from "vitest" + +const {fetchWorkflowCatalogTemplatesMock, inspectWorkflowMock} = vi.hoisted(() => ({ + fetchWorkflowCatalogTemplatesMock: vi.fn(), + inspectWorkflowMock: vi.fn(), +})) + +vi.mock("../../src/workflow/api", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + fetchWorkflowCatalogTemplates: fetchWorkflowCatalogTemplatesMock, + inspectWorkflow: inspectWorkflowMock, + } +}) + +import {createEphemeralAppFromTemplate} from "../../src/workflow/state/appUtils" +import {agentCreationPrefsAtom} from "../../src/workflow/state/agentCreationPrefs" +import {workflowLocalServerDataAtomFamily} from "../../src/workflow/state/store" + +const PROJECT_ID = "proj-1" + +/** Pi's default built-in grants, in the typed form the default template ships. */ +const PI_DEFAULT_BUILTINS = ["read", "bash", "edit", "write"].map((name) => ({ + type: "builtin", + name, +})) + +const agentTemplate = () => ({ + key: "agent", + data: { + uri: "agenta:builtin:agent:v0", + parameters: { + agent: { + harness: {kind: "pi_core"}, + llm: {model: "gpt-5"}, + tools: PI_DEFAULT_BUILTINS.map((tool) => ({...tool})), + }, + }, + schemas: {inputs: null, outputs: null, parameters: null}, + }, +}) + +function readAgentConfig(localId: string): Record { + const entity = getDefaultStore().get(workflowLocalServerDataAtomFamily(localId)) as any + return entity?.data?.parameters?.agent +} + +describe("createEphemeralAppFromTemplate (agent tools)", () => { + beforeEach(() => { + const store = getDefaultStore() + store.set(queryClientAtom, new QueryClient()) + store.set(projectIdAtom, PROJECT_ID) + store.set(agentCreationPrefsAtom, {version: 1}) + fetchWorkflowCatalogTemplatesMock.mockReset() + fetchWorkflowCatalogTemplatesMock.mockResolvedValue({ + count: 1, + templates: [agentTemplate()], + }) + inspectWorkflowMock.mockReset() + // Inspect only refines schemas; the factory falls back to the catalog ones when it fails. + inspectWorkflowMock.mockRejectedValue(new Error("no network in unit tests")) + }) + + it("preserves the template's built-in tools on the new agent", async () => { + const localId = await createEphemeralAppFromTemplate({type: "agent"}) + expect(localId).not.toBeNull() + expect(readAgentConfig(localId!).tools).toEqual(PI_DEFAULT_BUILTINS) + }) + + it("preserves the tools when the last-used-harness preference rewrites harness.kind", async () => { + getDefaultStore().set(agentCreationPrefsAtom, {version: 1, harness: "claude"}) + + const localId = await createEphemeralAppFromTemplate({type: "agent"}) + const agent = readAgentConfig(localId!) + + expect(agent.harness).toEqual({kind: "claude"}) + expect(agent.tools).toEqual(PI_DEFAULT_BUILTINS) + }) + + it("preserves the tools when the model/provider preferences are applied too", async () => { + getDefaultStore().set(agentCreationPrefsAtom, { + version: 1, + harness: "pi_agenta", + model: "claude-opus-4", + provider: "anthropic", + connectionMode: "self_managed", + }) + + const localId = await createEphemeralAppFromTemplate({type: "agent"}) + const agent = readAgentConfig(localId!) + + expect(agent.llm).toMatchObject({model: "claude-opus-4", provider: "anthropic"}) + expect(agent.tools).toEqual(PI_DEFAULT_BUILTINS) + }) + + it("preserves the tools on the deferred-inspect path (playground onboarding)", async () => { + const localId = await createEphemeralAppFromTemplate({type: "agent", deferInspect: true}) + expect(readAgentConfig(localId!).tools).toEqual(PI_DEFAULT_BUILTINS) + }) +}) diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx index 7ffd62c9c9..009d92ff4f 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx @@ -2,8 +2,9 @@ * PiSettingsControl * * Pi-family harness settings that are authored through the agent template's existing `tools` - * list. Built-ins persist as `{type: "builtin", name}` entries; an absent entry means Pi uses its - * own defaults. + * list. Built-ins persist as `{type: "builtin", name}` entries, and only the listed names are + * granted. Neither an empty list nor a removed `tools` field falls back to Pi's own defaults: + * the SDK always emits the field, so both reach the runner as "grant nothing" (issue #5590). */ import {memo, useCallback, useMemo} from "react" @@ -22,7 +23,8 @@ interface BuiltinTool { export interface PiSettingsControlProps { /** The agent template's top-level `tools` value. */ tools?: unknown[] | null - /** Called with the next top-level `tools` value; undefined removes an empty tools field. */ + /** Called with the next top-level `tools` value; undefined when nothing is left to store, + * which grants no built-ins rather than restoring Pi's defaults. */ onChange: (tools: unknown[] | undefined) => void /** Disable the control. */ disabled?: boolean @@ -108,7 +110,7 @@ export const PiSettingsControl = memo(function PiSettingsControl({ @@ -118,7 +120,7 @@ export const PiSettingsControl = memo(function PiSettingsControl({ value={selected} onChange={(value) => writeSelected(value)} options={PI_BUILTIN_OPTIONS} - placeholder="Pi defaults" + placeholder="No built-ins" disabled={disabled} /> diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsx index 9ddff8beeb..7cd5164994 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsx @@ -194,12 +194,15 @@ export function describeTool(tool: unknown): ItemDescriptor { // Built-in / provider tool: a bare `type` with no editable `function`. if (!fn || typeof fn !== "object") { + // Pi built-ins name themselves ({type:"builtin", name:"read"}); provider built-ins such as + // {type:"web_search_preview"} carry no name, so their `type` is the name. + const builtinName = typeof t.name === "string" && t.name ? (t.name as string) : undefined const typeValue = typeof t.type === "string" && t.type !== "function" ? (t.type as string) : Object.keys(t).find((k) => k !== "type" && k !== "function") return { - name: typeValue ?? "Built-in tool", + name: builtinName ?? typeValue ?? "Built-in tool", mono: "io", color: "#0d9488", tags: ["built-in"], diff --git a/web/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.ts b/web/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.ts new file mode 100644 index 0000000000..b561d1e565 --- /dev/null +++ b/web/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.ts @@ -0,0 +1,41 @@ +/** + * Unit tests for `describeTool`'s built-in branch — the row label a tool shows in the Tools list. + * + * The branch was written for provider built-ins such as `{type: "web_search_preview"}`, where the + * `type` IS the name. Pi built-ins persist as `{type: "builtin", name}`, so labelling from `type` + * renders every one of them as "builtin" — four identical rows on any agent created from the + * default template (issue #5590). Runs under @agenta/entity-ui's own vitest runner. + */ +import {describe, expect, it} from "vitest" + +import {describeTool} from "../../src/DrillInView/SchemaControls/agentTemplate/itemDescriptors" + +describe("describeTool (built-in tools)", () => { + it("labels a Pi built-in by its name, not by its type", () => { + const descriptor = describeTool({type: "builtin", name: "read"}) + expect(descriptor.name).toBe("read") + expect(descriptor.typeLabel).toBe("built-in") + expect(descriptor.tags).toEqual(["built-in"]) + }) + + it("labels each of Pi's default built-ins distinctly", () => { + const names = ["read", "bash", "edit", "write"].map( + (name) => describeTool({type: "builtin", name}).name, + ) + expect(names).toEqual(["read", "bash", "edit", "write"]) + }) + + it("falls back to the type for a provider built-in that carries no name", () => { + expect(describeTool({type: "web_search_preview"}).name).toBe("web_search_preview") + }) + + it("falls back to the only key for a bare provider entry with no type", () => { + expect(describeTool({code_interpreter: {}}).name).toBe("code_interpreter") + }) + + it("still describes a function tool by its function name", () => { + const descriptor = describeTool({type: "function", function: {name: "get_weather"}}) + expect(descriptor.name).toBe("get_weather") + expect(descriptor.typeLabel).toBe("definition") + }) +}) diff --git a/web/packages/agenta-playground/tests/unit/agentRequest.test.ts b/web/packages/agenta-playground/tests/unit/agentRequest.test.ts index 5521b27a92..8d4f3609e9 100644 --- a/web/packages/agenta-playground/tests/unit/agentRequest.test.ts +++ b/web/packages/agenta-playground/tests/unit/agentRequest.test.ts @@ -336,6 +336,43 @@ describe("buildAgentRequest", () => { expect(config).toEqual(before) }) + it("does not duplicate Pi built-ins the default template already carries", async () => { + // The shipped default grants Pi's four built-ins (issue #5590) and the kit overlay + // prepends `read` and `bash`. The `name:` identity merge is what keeps the two + // copies from both landing in the run's tools list. + const builtin = (name: string) => ({type: "builtin", name}) + const config = { + agent: { + tools: [builtin("read"), builtin("bash"), builtin("edit"), builtin("write")], + }, + } + seed(store, "e", { + config, + overlay: { + tools: [ + builtin("read"), + builtin("bash"), + {type: "platform", op: "commit_revision"}, + ], + }, + buildKitEnabled: true, + }) + + const req = await buildAgentRequest("e", [], {sessionId: "s1", store}) + const template = (req!.requestBody.data as any).parameters.agent + + expect(template.tools).toEqual([ + builtin("read"), + builtin("bash"), + builtin("edit"), + builtin("write"), + {type: "platform", op: "commit_revision"}, + ]) + for (const name of ["read", "bash", "edit", "write"]) { + expect(template.tools.filter((tool: any) => tool?.name === name)).toHaveLength(1) + } + }) + it("applies the build-kit overlay to a BARE template (no agent wrapper)", async () => { // `withAgentRunDefaults` leaves a config with no `agent` key as a bare template, so the // overlay must merge at the top level — not no-op (the bare published default case). From b9f467043d4bfadd64288ea6c7dfe8c9d7b36a5d Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 1 Aug 2026 17:10:02 +0200 Subject: [PATCH 2/5] fix(services): keep default tools when agent.json omits the key and gate QA seeds to Pi --- .../agent-release-gate/resources/qa_probe.py | 25 +++++++++------ .../resources/qa_product.py | 25 +++++++++------ services/oss/src/agent/config.py | 6 +++- .../agent/test_config_template_fallback.py | 31 +++++++++++++++++++ 4 files changed, 68 insertions(+), 19 deletions(-) diff --git a/.agents/skills/agent-release-gate/resources/qa_probe.py b/.agents/skills/agent-release-gate/resources/qa_probe.py index 67ba4f1a39..0921ea3c0f 100644 --- a/.agents/skills/agent-release-gate/resources/qa_probe.py +++ b/.agents/skills/agent-release-gate/resources/qa_probe.py @@ -70,6 +70,21 @@ def resolve_credentials(env_file: str | pathlib.Path | None = None) -> None: KEY = resolved["AGENTA_API_KEY"] +def default_tools(harness: str) -> list: + """The shipped default grant list for `harness`. + + On Pi this is Pi's default active built-ins, matching the shipped default agent template: an + empty list means "grant nothing" to the runner, so seeding `[]` would gate the release on a + configuration no real agent uses (issue #5590). Only Pi reads `builtin` grants; a Claude cell + brings its own tools, so seeding Pi's names there would test nothing. + """ + if not harness.startswith("pi"): + return [] + return [ + {"type": "builtin", "name": name} for name in ("read", "bash", "edit", "write") + ] + + def agent_template(harness: str, sandbox: str, model: str, provider: str) -> dict: return { "instructions": {"agents_md": "Be terse. Do exactly what is asked."}, @@ -79,15 +94,7 @@ def agent_template(harness: str, sandbox: str, model: str, provider: str) -> dic "connection": {"mode": "agenta", "slug": None}, "extras": {}, }, - # Pi's default active built-ins, matching the shipped default agent template. An empty - # list means "grant nothing" to the runner, so seeding `[]` would gate the release on a - # configuration no real agent uses (issue #5590). - "tools": [ - {"type": "builtin", "name": "read"}, - {"type": "builtin", "name": "bash"}, - {"type": "builtin", "name": "edit"}, - {"type": "builtin", "name": "write"}, - ], + "tools": default_tools(harness), "mcps": [], "skills": [], "harness": {"kind": harness}, diff --git a/.agents/skills/agent-release-gate/resources/qa_product.py b/.agents/skills/agent-release-gate/resources/qa_product.py index 2c508116ae..3e0c72a20f 100644 --- a/.agents/skills/agent-release-gate/resources/qa_product.py +++ b/.agents/skills/agent-release-gate/resources/qa_product.py @@ -238,6 +238,21 @@ def outcome_for_input(t: "Turn", wanted_input: dict) -> str | None: return None +def default_tools(harness: str) -> list: + """The shipped default grant list for `harness`. + + On Pi this is Pi's default active built-ins, matching the shipped default agent template: an + empty list means "grant nothing" to the runner, so seeding `[]` would gate the release on a + configuration no real agent uses (issue #5590). Only Pi reads `builtin` grants; a Claude cell + brings its own tools, so seeding Pi's names there would test nothing. + """ + if not harness.startswith("pi"): + return [] + return [ + {"type": "builtin", "name": name} for name in ("read", "bash", "edit", "write") + ] + + def template( cell: dict, tools: list | None = None, @@ -749,15 +764,7 @@ def j5_commit(cell: dict) -> dict: "agent": { "instructions": {"agents_md": "seed"}, "llm": {"model": cell["model"], "provider": cell["provider"]}, - # Pi's default active built-ins, matching the shipped default agent template. An - # empty list means "grant nothing" to the runner, so committing `[]` would gate - # the release on a configuration no real agent commits (issue #5590). - "tools": [ - {"type": "builtin", "name": "read"}, - {"type": "builtin", "name": "bash"}, - {"type": "builtin", "name": "edit"}, - {"type": "builtin", "name": "write"}, - ], + "tools": default_tools(cell["harness"]), "harness": {"kind": cell["harness"]}, "sandbox": {"kind": cell["sandbox"]}, } diff --git a/services/oss/src/agent/config.py b/services/oss/src/agent/config.py index faf121088e..a6a5ddc8b1 100644 --- a/services/oss/src/agent/config.py +++ b/services/oss/src/agent/config.py @@ -113,7 +113,11 @@ def load_config() -> AgentTemplate: if meta_path.exists(): meta = json.loads(meta_path.read_text(encoding="utf-8")) model = meta.get("model") or DEFAULT_MODEL - tools = meta.get("tools", []) or [] + # Only an explicit `tools` key overrides the shipped grant list: an older agent.json + # written before the defaults existed must not silently strip them, while an explicit + # empty list still means "grant no built-ins" (issue #5590). + if "tools" in meta: + tools = meta["tools"] or [] else: log.warning( "agent: template not found at %s; falling back to the built-in default " diff --git a/services/oss/tests/pytest/unit/agent/test_config_template_fallback.py b/services/oss/tests/pytest/unit/agent/test_config_template_fallback.py index 45febf2def..29c3c7272f 100644 --- a/services/oss/tests/pytest/unit/agent/test_config_template_fallback.py +++ b/services/oss/tests/pytest/unit/agent/test_config_template_fallback.py @@ -4,6 +4,7 @@ from __future__ import annotations +import json import logging @@ -35,6 +36,36 @@ def test_on_disk_template_grants_pi_default_builtins(): assert agent_config.DEFAULT_TOOLS == expected +def _write_template(tmp_path, monkeypatch, meta: dict): + """Point `load_config` at a throwaway template dir holding `meta` as its agent.json.""" + (tmp_path / "AGENTS.md").write_text("Be terse.", encoding="utf-8") + (tmp_path / "agent.json").write_text(json.dumps(meta), encoding="utf-8") + monkeypatch.setenv("AGENTA_AGENT_TEMPLATE_DIR", str(tmp_path)) + + +def test_agent_json_without_tools_key_keeps_the_defaults(monkeypatch, tmp_path): + """An agent.json written before the grant list existed carries no `tools` key. Reading that + as an empty list would strip read/bash/edit/write from every deployment still on the older + file (issue #5590).""" + _write_template(tmp_path, monkeypatch, {"model": "gpt-5.6-luna"}) + + assert agent_config.load_config().tools == agent_config.DEFAULT_TOOLS + + +def test_agent_json_with_empty_tools_grants_nothing(monkeypatch, tmp_path): + # An explicit empty list is the documented way to ship an agent with no built-ins. + _write_template(tmp_path, monkeypatch, {"model": "gpt-5.6-luna", "tools": []}) + + assert agent_config.load_config().tools == [] + + +def test_agent_json_tools_override_the_defaults(monkeypatch, tmp_path): + tools = [{"type": "builtin", "name": "read"}] + _write_template(tmp_path, monkeypatch, {"model": "gpt-5.6-luna", "tools": tools}) + + assert agent_config.load_config().tools == tools + + def test_missing_template_logs_warning_and_falls_back(monkeypatch, tmp_path, caplog): monkeypatch.setenv("AGENTA_AGENT_TEMPLATE_DIR", str(tmp_path / "does-not-exist")) From fe49b9011fece78487c8c4123e54c005d37ae511 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 1 Aug 2026 17:10:31 +0200 Subject: [PATCH 3/5] doc(agent-workflows): refresh the default-builtins workspace to the shipped state --- docs/design/agent-workflows/interfaces/README.md | 2 +- .../projects/default-agent-builtins/design.md | 7 +++---- .../projects/default-agent-builtins/plan.md | 14 ++++++++++---- .../projects/default-agent-builtins/research.md | 12 +++++++++--- .../projects/default-agent-builtins/status.md | 10 ++++++---- .../projects/default-agent-builtins/testing.md | 2 +- 6 files changed, 30 insertions(+), 17 deletions(-) diff --git a/docs/design/agent-workflows/interfaces/README.md b/docs/design/agent-workflows/interfaces/README.md index 8d694da840..ae19583a5b 100644 --- a/docs/design/agent-workflows/interfaces/README.md +++ b/docs/design/agent-workflows/interfaces/README.md @@ -44,7 +44,7 @@ page. `Status` is read from each page's prose: **stable** (wired and unlikely to | [`/invoke`](public-edge/workflow-invoke.md) | public | `decorators/routing.py`, `models/workflows.py`, `agent/app.py` | stable | `unit/agent/`, `utils/test_messages_endpoint.py` | | [`/inspect`](public-edge/workflow-inspect.md) | public | `agent/schemas.py`, `agent/app.py` (builtin-URI binding), `models/workflows.py`, `decorators/routing.py` | stable | `unit/agents/test_dtos_agent_config.py`, `unit/agent/test_builtin_uri_binding.py` | | [`/messages`](public-edge/agent-messages.md) | public | `adapters/vercel/{routing,messages,stream}.py`, `agentRequest.ts` | evolving (create-or-resume not observable until storage lands) | `utils/test_messages_endpoint.py`, `unit/agents/test_ui_messages.py` | -| [Agent config schema](public-edge/agent-config-schema.md) | public | `agent/schemas.py`, `sdk/utils/types.py`, `agents/dtos.py` (`HARNESS_IDENTITIES`), `sdk/agents/pi_builtins.py` (`PI_DEFAULT_ACTIVE_BUILTINS`) | stable | `unit/agents/test_dtos_agent_config.py`, `unit/agents/test_harness_identity.py`, `unit/agents/test_pi_builtins_parity.py` + `golden/pi_default_active_builtins.json`, `services/oss unit/agent/test_default_agent_template.py` | +| [Agent config schema](public-edge/agent-config-schema.md) | public | `agent/schemas.py`, `sdk/utils/types.py`, `agents/dtos.py` (`HARNESS_IDENTITIES`), `sdk/agents/pi_builtins.py` (`PI_DEFAULT_ACTIVE_BUILTINS`) | stable | `unit/agents/test_dtos_agent_config.py`, `unit/agents/test_harness_identity.py`, `unit/agents/test_pi_builtins_parity.py` + `golden/pi_default_active_builtins.json`, `services/oss/tests/pytest/unit/agent/test_default_agent_template.py` | | [`/run`](cross-service/service-to-agent-runner.md) | cross-service (the spine) | `protocol.ts`, `utils/wire.py`, `utils/ts_runner.py`, `server.ts`/`cli.ts` | stable (pinned by golden) | `unit/agents/test_wire_contract.py` + `golden/`, `services/agent/tests/unit/wire-contract.test.ts` | | [Runner to harness](cross-service/runner-to-harness.md) | cross-service (ACP) | `engines/sandbox_agent.ts` + `sandbox_agent/{run-plan,capabilities,permissions}.ts` | evolving | `services/agent/tests/unit/sandbox-agent-*.test.ts` | | [Runner to MCP server](cross-service/runner-to-mcp-server.md) | cross-service | `agents/mcp/`, `engines/sandbox_agent/{mcp,tool-mcp-assets,relay-guard}.ts`, `tools/{mcp-bridge,tool-mcp-http,tool-mcp-stdio,tool-mcp-env,relay,relay-client,relay-protocol,relay-watch}.ts` | evolving (internal channel delivered locally over loopback HTTP and on Daytona via the in-sandbox stdio shim, `client` tools included — a client call parks via a paused relay answer; user stdio disabled) | `services/runner/tests/unit/{mcp-servers,session-mcp-layering,tool-mcp-assets,tool-mcp-stdio,tool-relay-guard}.test.ts` | diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/design.md b/docs/design/agent-workflows/projects/default-agent-builtins/design.md index 8c9a8ebf5d..5810d4b621 100644 --- a/docs/design/agent-workflows/projects/default-agent-builtins/design.md +++ b/docs/design/agent-workflows/projects/default-agent-builtins/design.md @@ -295,10 +295,9 @@ agent creation to stop putting Pi built-ins in a Claude agent's template at all; the harness preference below. This needs a shared constant for Pi's four default built-in names on the Python side. There is no -such constant today; the four names live only in TypeScript at `run-plan.ts:192`. Put it next to -the built-in vocabulary in `sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py`, beside -`AGENTA_FORCED_TOOLS`, and have `build_agent_v0_default()` build its `tools` entries from it so -the default and the warning cannot drift apart. +such constant today; the four names live only in TypeScript at `run-plan.ts:192`. Have +`build_agent_v0_default()` build its `tools` entries from that constant so the default and the +warning cannot drift apart. Where the constant lives is settled two paragraphs below. The Python constant and the TypeScript `PI_DEFAULT_ACTIVE_BUILTINS` are now two copies of the same list in two languages. They are already two copies today, just implicitly. They should be two diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/plan.md b/docs/design/agent-workflows/projects/default-agent-builtins/plan.md index 496bac6386..b92ef6cd1f 100644 --- a/docs/design/agent-workflows/projects/default-agent-builtins/plan.md +++ b/docs/design/agent-workflows/projects/default-agent-builtins/plan.md @@ -1,7 +1,9 @@ # Execution plan -Four pieces, in dependency order. Each one lands on its own branch and is reviewable alone. The -first piece fixes the reported bug; the rest keep the result honest and legible. +Four pieces, in dependency order. Each one is reviewable alone, and all four landed together on +`fix/pi-default-builtins` as [PR #5597](https://github.com/Agenta-AI/agenta/pull/5597), for the +reason given in [Order and independence](#order-and-independence). The first piece fixes the +reported bug; the rest keep the result honest and legible. ## Piece 1: ship Pi's built-ins in the default template @@ -21,7 +23,10 @@ Files: narrow the existing authoring-extras assertions, as described in [testing.md](testing.md#tests-that-must-be-updated). - `sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py`: add the default-to-wire test. -- A new source-reading test pinning the Python constant against the TypeScript array. +- A shared golden fixture holding the four names, asserted from Python against the new constant and + from TypeScript against `PI_DEFAULT_ACTIVE_BUILTINS`, as described in + [testing.md](testing.md#pinning-the-two-copies-of-pis-default-built-in-list). Not a test that + reads the TypeScript source. Verification before moving on: `cd services && py-run-tests`, `cd sdks/python && py-run-tests`, `cd api && py-run-tests`. The `api` suite is included because @@ -65,7 +70,8 @@ Run `pnpm lint-fix` inside `web` before committing. ## Piece 4: update the documentation that describes the default The interface documentation states the default agent config field by field, and the build-an-agent -skill teaches the same shape to the builder agent. Both currently show an empty tools list. +skill teaches the same shape to the builder agent. Both showed an empty tools list before this +change. Files: diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/research.md b/docs/design/agent-workflows/projects/default-agent-builtins/research.md index e422e278f4..671ee683a2 100644 --- a/docs/design/agent-workflows/projects/default-agent-builtins/research.md +++ b/docs/design/agent-workflows/projects/default-agent-builtins/research.md @@ -1,6 +1,9 @@ # What the code does today -Every claim here was read from the repository. File paths are repository-relative. +Every claim here was read from the repository on 2026-07-30, before the fix landed, so this file +records the state that produced the bug. The empty `tools` list it describes is what +[PR #5597](https://github.com/Agenta-AI/agenta/pull/5597) replaced with Pi's four default +built-ins. File paths are repository-relative. ## The two fields named `tools` @@ -61,8 +64,11 @@ Granting a built-in is not the same as letting it run. The two are enforced at d The read-only table at `services/runner/src/permission-plan.ts:40` marks `read`, `grep`, `find`, and `ls` read-only, and `bash`, `edit`, `write` not read-only. Under the shipped default -permission mode `allow_reads` (`sdks/python/agenta/sdk/utils/types.py:1072`), the read-only four -run without asking and the other three raise an approval. +permission mode `allow_reads` (`sdks/python/agenta/sdk/utils/types.py:1072`), a granted read-only +tool runs without asking and a granted tool that is not read-only raises an approval. That table +classifies names; it does not grant them. Only a granted built-in runs at all, so `grep`, `find`, +and `ls` stay unavailable unless an author grants them. Under the four-name default this design +ships, `read` runs without asking and `bash`, `edit`, and `write` each raise an approval. `computeBuiltinGatingActive` (`run-plan.ts:233`) decides whether to run the gating machinery at all. It turns gating on when the resolved permission plan could gate a built-in, or when the diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/status.md b/docs/design/agent-workflows/projects/default-agent-builtins/status.md index c389cf08dc..5866b11b57 100644 --- a/docs/design/agent-workflows/projects/default-agent-builtins/status.md +++ b/docs/design/agent-workflows/projects/default-agent-builtins/status.md @@ -1,9 +1,10 @@ # Status -**State: implemented, uncommitted. All four pieces are in the working tree; no branch and no pull -request yet.** +**State: implemented and in review. All four pieces are committed on the branch +`fix/pi-default-builtins` and open as +[PR #5597](https://github.com/Agenta-AI/agenta/pull/5597).** -Last updated 2026-07-30, after implementation. +Last updated 2026-08-01, after the change went up for review. ## Where the work stands @@ -284,4 +285,5 @@ chose, and carries two pre-ship conditions. Design workspace created 2026-07-30. Research read against the `gitbutler/workspace` branch. Codex review round 1 folded in above the same day, and the change implemented against that branch the -same day. +same day. The work was committed to `fix/pi-default-builtins` and opened as +[PR #5597](https://github.com/Agenta-AI/agenta/pull/5597) on 2026-07-30. diff --git a/docs/design/agent-workflows/projects/default-agent-builtins/testing.md b/docs/design/agent-workflows/projects/default-agent-builtins/testing.md index b9eac8dd29..cdf50c513c 100644 --- a/docs/design/agent-workflows/projects/default-agent-builtins/testing.md +++ b/docs/design/agent-workflows/projects/default-agent-builtins/testing.md @@ -67,7 +67,7 @@ def test_published_default_grants_pi_default_builtins(): """A new agent must be able to read, run shell commands, and edit and write files wherever it runs, not only in the playground (issue #5590). The runner reads an empty tools list as "grant nothing".""" - expected = [{"type": "builtin", "name": name} for name in PI_DEFAULT_BUILTIN_NAMES] + expected = [{"type": "builtin", "name": name} for name in PI_DEFAULT_ACTIVE_BUILTINS] assert _inspect_agent_default()["tools"] == expected assert _builtin_agent_default()["tools"] == expected ``` From 21c04f6e7c4459d2c909608098d230ab45526146 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 1 Aug 2026 17:11:24 +0200 Subject: [PATCH 4/5] test: pin the tightened approval guard and type the template tests --- .../tests/unit/sandbox-agent-run-plan.test.ts | 35 +++++++++++++++++++ ...create-ephemeral-app-from-template.test.ts | 15 ++++++-- .../tests/unit/agentRequest.test.ts | 10 ++++-- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts index 2f21be384b..e2ca969ec8 100644 --- a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts +++ b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts @@ -149,6 +149,41 @@ describe("buildRunPlan", () => { }); }); + it("refuses a stalled history whose newest tool envelope is an unresolved call", () => { + // The approval was answered earlier; the newest envelope is a call nobody replied to, so + // this is not an approval reply and there is still no prompt to send. + const result = buildRunPlan( + { + messages: [ + { + role: "assistant", + content: [ + { type: "tool_call", toolCallId: "tc-1", toolName: "Write" }, + { + type: "tool_result", + toolCallId: "tc-1", + toolName: "Write", + output: { approved: true, interactionToken: "tok-1" }, + }, + ], + }, + { + role: "assistant", + content: [ + { type: "tool_call", toolCallId: "tc-2", toolName: "Bash" }, + ], + }, + ], + } as AgentRunRequest, + { createLocalCwd: () => "/tmp/unused" }, + ); + + assert.deepEqual(result, { + ok: false, + error: "No user message to send (prompt/messages empty).", + }); + }); + it("accepts an out-of-band approval reply that carries no user text", () => { // What a caller answering from the durable interaction row sends: the parked call plus its // {approved} envelope, and nothing else. The conversation is rebuilt from the record log diff --git a/web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts b/web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts index 782be5b397..31675e4cbb 100644 --- a/web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts +++ b/web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts @@ -55,9 +55,18 @@ const agentTemplate = () => ({ }, }) -function readAgentConfig(localId: string): Record { - const entity = getDefaultStore().get(workflowLocalServerDataAtomFamily(localId)) as any - return entity?.data?.parameters?.agent +/** Minimal shape of the agent config these assertions read off the local entity. */ +interface AgentConfigShape { + harness?: {kind?: string} + llm?: Record + tools?: {type?: string; name?: string}[] +} + +function readAgentConfig(localId: string): AgentConfigShape { + const entity = getDefaultStore().get(workflowLocalServerDataAtomFamily(localId)) + const agent = entity?.data?.parameters?.agent + if (!agent) throw new Error(`no agent config on the local entity for ${localId}`) + return agent as AgentConfigShape } describe("createEphemeralAppFromTemplate (agent tools)", () => { diff --git a/web/packages/agenta-playground/tests/unit/agentRequest.test.ts b/web/packages/agenta-playground/tests/unit/agentRequest.test.ts index 8d4f3609e9..7318a3bf18 100644 --- a/web/packages/agenta-playground/tests/unit/agentRequest.test.ts +++ b/web/packages/agenta-playground/tests/unit/agentRequest.test.ts @@ -100,6 +100,11 @@ function seed( ) } +/** Minimal shape of the agent template these assertions read back off the request body. */ +interface AgentTemplateShape { + tools: {type?: string; name?: string; op?: string}[] +} + const authoringSkill = { "@ag.embed": {"@ag.references": {workflow: {slug: "__ag__getting_started_with_agenta"}}}, } @@ -359,7 +364,8 @@ describe("buildAgentRequest", () => { }) const req = await buildAgentRequest("e", [], {sessionId: "s1", store}) - const template = (req!.requestBody.data as any).parameters.agent + const template = (req!.requestBody.data as {parameters: {agent: AgentTemplateShape}}) + .parameters.agent expect(template.tools).toEqual([ builtin("read"), @@ -369,7 +375,7 @@ describe("buildAgentRequest", () => { {type: "platform", op: "commit_revision"}, ]) for (const name of ["read", "bash", "edit", "write"]) { - expect(template.tools.filter((tool: any) => tool?.name === name)).toHaveLength(1) + expect(template.tools.filter((tool) => tool?.name === name)).toHaveLength(1) } }) From 356aa032ec3d20ab627e764da12cc54d1a3c23fe Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 1 Aug 2026 17:11:37 +0200 Subject: [PATCH 5/5] doc(sdk): distinguish selection from execution permission for built-ins --- sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py b/sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py index 9f751dfce3..63135ff539 100644 --- a/sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py +++ b/sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py @@ -178,7 +178,8 @@ runner default for that one tool). The six `type` values: - `builtin` — a harness built-in: `{ "type": "builtin", "name": "read" }`. (A per-builtin - `permission` is dropped — builtins are granted by selection, not gated.) A new agent starts + `permission` is dropped — selection grants the built-in; whether a call needs approval stays + the runner's permission mode.) A new agent starts with Pi's four defaults (`read`, `bash`, `edit`, `write`), as shown above. Keep them unless the user asks you to drop one: an empty `tools` list grants NO built-ins, so an agent that ships with `[]` cannot read or write a file at all.