diff --git a/.agents/skills/agent-release-gate/resources/qa_probe.py b/.agents/skills/agent-release-gate/resources/qa_probe.py index c4ef604558..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,7 +94,7 @@ def agent_template(harness: str, sandbox: str, model: str, provider: str) -> dic "connection": {"mode": "agenta", "slug": None}, "extras": {}, }, - "tools": [], + "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 f3b1eb5a69..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,7 +764,7 @@ def j5_commit(cell: dict) -> dict: "agent": { "instructions": {"agents_md": "seed"}, "llm": {"model": cell["model"], "provider": cell["provider"]}, - "tools": [], + "tools": default_tools(cell["harness"]), "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..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`) | 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/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` | @@ -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..5810d4b621 --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/design.md @@ -0,0 +1,450 @@ +# 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`. 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 +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..b92ef6cd1f --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/plan.md @@ -0,0 +1,107 @@ +# Execution plan + +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 + +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 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 +`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 showed an empty tools list before this +change. + +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..671ee683a2 --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/research.md @@ -0,0 +1,257 @@ +# What the code does today + +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` + +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`), 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 +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..5866b11b57 --- /dev/null +++ b/docs/design/agent-workflows/projects/default-agent-builtins/status.md @@ -0,0 +1,289 @@ +# Status + +**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-08-01, after the change went up for review. + +## 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. 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 new file mode 100644 index 0000000000..cdf50c513c --- /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_ACTIVE_BUILTINS] + 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..63135ff539 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,11 @@ 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 — 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. - `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..a6a5ddc8b1 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,16 +108,21 @@ 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")) 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 " - "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..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,9 +4,12 @@ from __future__ import annotations +import json import logging +from agenta.sdk.agents.pi_builtins import PI_DEFAULT_ACTIVE_BUILTINS + from oss.src.agent import config as agent_config @@ -21,6 +24,48 @@ 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 _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")) @@ -29,7 +74,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..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 @@ -448,6 +483,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..31675e4cbb --- /dev/null +++ b/web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts @@ -0,0 +1,124 @@ +/** + * 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}, + }, +}) + +/** 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)", () => { + 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..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"}}}, } @@ -336,6 +341,44 @@ 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 {parameters: {agent: AgentTemplateShape}}) + .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) => 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).