feat(agent): enforce sidecar trust + disable unenforceable sandbox boundaries#4831
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR updates agent configuration and harness option naming, publishes harness-filtered model metadata, normalizes ChangesHarness-aware agent config and model picker
Wire contract and inspect response
Sidecar routing, authorization, and sandbox gates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| Ordered roughly cheapest-to-strongest. They compose; this is not either/or. | ||
|
|
||
| 1. **Network isolation / localhost binding (make the implicit assumption real).** |
There was a problem hiding this comment.
Kept as decided near-term step 1; endorsement noted inline. (adcd126)
| 5. **Payload encryption of `secrets`** — held in reserve; only if a requirement forbids | ||
| plaintext secrets even under TLS. | ||
|
|
||
| Rationale: steps 1–2 are config-only, ship today, and make the implicit assumption real and |
There was a problem hiding this comment.
I agree let's only implement 1 and 2 now
There was a problem hiding this comment.
Scoped near-term work to steps 1 & 2 only; moved mTLS / short-lived scoped tokens / payload encryption to an explicit 'Later / deferred hardening' subsection. (adcd126)
| egress boundary is applied by the Daytona platform at sandbox creation. This is real | ||
| enforcement at the sandbox boundary. | ||
|
|
||
| - **Local sandbox network egress — NOT enforceable.** |
There was a problem hiding this comment.
that is alright. it might make sense to return an error the way we do when a capability is not implemented if that variable is set for local
There was a problem hiding this comment.
Documented: local sandbox errors when network is set (not-implemented-style, regardless of enforcement), mirroring the code.ts CODE_TOOL_UNSUPPORTED_MESSAGE gate / the typed *Unsupported*Errors in connections/errors.py. Code change is a separate task. (adcd126)
| restricted-network run on the local sandbox when `enforcement: "strict"` (fail loud), and | ||
| proceeds with a warning under `best_effort`. | ||
|
|
||
| - **Filesystem boundary — DECLARED, enforced NOWHERE.** |
There was a problem hiding this comment.
again, should be an error if specified currently since it is not implemented
There was a problem hiding this comment.
Documented: specifying filesystem errors (not implemented on any backend), same not-implemented pattern. (adcd126)
| on Daytona.** | ||
| Resolved `code` tools, gateway/callback tools, and stdio MCP servers do **not** run inside | ||
| the sandbox. They run in the **runner host process** via the relay | ||
| (`services/agent/src/tools/relay.ts`: `runCodeTool`, `callAgentaTool`, and the stdio MCP |
There was a problem hiding this comment.
we have remove code execution from the code. we should also disable the same way mcp servers implementation until we fix security issues. We simply put them as non implemented and remove the implemenattion in the sandbox sidecar like we did for tool calls.
for gateway it's alright. no need to do anything. that is not part of sandbox persmission, that is part of the permission (the other parameter that is about the sidecar and obviously there we should deal with these correctly
There was a problem hiding this comment.
Verified code exec is already removed (runCodeTool throws). Documented the decision to disable + remove the stdio MCP sidecar impl (mcp-bridge.ts/mcp-server.ts/stdio plumbing) the same way, until security is fixed. Clarified gateway = Layer-3 tool-permission, NOT Layer-2 sandbox_permission, so no action there. Code change is a separate task. (adcd126)
| the opt-out that accepts the boundary is not a hard guarantee. | ||
|
|
||
| - **Legacy in-process engine bypass.** | ||
| `backend: "pi"` (the legacy in-process `engines/pi.ts`) is not the sandbox-agent path and |
There was a problem hiding this comment.
we have i think another agent removing that. should be removed by the time you start working on this
There was a problem hiding this comment.
Verified: engines/pi.ts is gone (only sandbox_agent.ts/sandbox_agent//skills.ts remain). Doc now presents the legacy in-process engine as removed/historical only. (adcd126)
…view Address the 6 author inline comments on the proposal: - Scope near-term work to steps 1-2 only (network isolation + optional /run token); defer mTLS, short-lived scoped tokens, and payload encryption to an explicit Later/deferred subsection. - Document the decided not-implemented error behavior: the local sandbox errors when a network policy is set, and any specified filesystem policy errors, both mirroring the existing code-tool unsupported gate (code.ts CODE_TOOL_UNSUPPORTED_MESSAGE) and the typed connection *Unsupported*Errors. - Document the decision to disable stdio MCP-server execution in the sidecar the same way code execution was removed (not-implemented gate + remove mcp-bridge.ts/mcp-server.ts/stdio plumbing), and clarify that gateway tools are Layer-3 tool-permission, not Layer-2 sandbox_permission, so they need no change. - Update the legacy in-process pi engine to removed/historical (engines/pi.ts is gone). Verified code state: code execution already removed, stdio MCP still present, pi.ts removed, protocol.ts network comment already corrected by A3. Docs-only; no runner code changed. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
mmabrouk
force-pushed
the
docs/sidecar-trust-and-sandbox-enforcement
branch
from
10acf57 to
adcd126
Compare
|
Revised the proposal per your 6 inline comments (commit
Also: the enforcement matrix + one-line summary were rewritten to reflect all the above, and the protocol.ts comment section was updated — A3 already applied the network half of the correction; two follow-up comment edits (filesystem→error, local-network→error) are recorded for whoever lands that behavior. |
…view Address the 6 author inline comments on the proposal: - Scope near-term work to steps 1-2 only (network isolation + optional /run token); defer mTLS, short-lived scoped tokens, and payload encryption to an explicit Later/deferred subsection. - Document the decided not-implemented error behavior: the local sandbox errors when a network policy is set, and any specified filesystem policy errors, both mirroring the existing code-tool unsupported gate (code.ts CODE_TOOL_UNSUPPORTED_MESSAGE) and the typed connection *Unsupported*Errors. - Document the decision to disable stdio MCP-server execution in the sidecar the same way code execution was removed (not-implemented gate + remove mcp-bridge.ts/mcp-server.ts/stdio plumbing), and clarify that gateway tools are Layer-3 tool-permission, not Layer-2 sandbox_permission, so they need no change. - Update the legacy in-process pi engine to removed/historical (engines/pi.ts is gone). Verified code state: code execution already removed, stdio MCP still present, pi.ts removed, protocol.ts network comment already corrected by A3. Docs-only; no runner code changed. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
mmabrouk
force-pushed
the
docs/sidecar-trust-and-sandbox-enforcement
branch
from
adcd126 to
227de27
Compare
mmabrouk
changed the title
mmabrouk
changed the base branch from
big-agents
to
feat/agent-contract-versioning-docs
dosubot
Bot
added
size:XXL
|
Implementation landed (commit What changed, per the three decided scope items:
What disabling MCP removes: non-Pi harnesses (Claude) take tools only over MCP, so they can no longer receive custom tools, and user-declared stdio MCP servers are refused. Pi tools (native via the extension) and gateway/callback tools ( Out of scope, deliberately: Tests: Docs: project |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Railway Preview Environment
Updated at 2026-06-25T18:37:44.419Z |
…view Address the 6 author inline comments on the proposal: - Scope near-term work to steps 1-2 only (network isolation + optional /run token); defer mTLS, short-lived scoped tokens, and payload encryption to an explicit Later/deferred subsection. - Document the decided not-implemented error behavior: the local sandbox errors when a network policy is set, and any specified filesystem policy errors, both mirroring the existing code-tool unsupported gate (code.ts CODE_TOOL_UNSUPPORTED_MESSAGE) and the typed connection *Unsupported*Errors. - Document the decision to disable stdio MCP-server execution in the sidecar the same way code execution was removed (not-implemented gate + remove mcp-bridge.ts/mcp-server.ts/stdio plumbing), and clarify that gateway tools are Layer-3 tool-permission, not Layer-2 sandbox_permission, so they need no change. - Update the legacy in-process pi engine to removed/historical (engines/pi.ts is gone). Verified code state: code execution already removed, stdio MCP still present, pi.ts removed, protocol.ts network comment already corrected by A3. Docs-only; no runner code changed. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
mmabrouk
force-pushed
the
docs/sidecar-trust-and-sandbox-enforcement
branch
from
227de27 to
8bbcd82
Compare
…view Address the 6 author inline comments on the proposal: - Scope near-term work to steps 1-2 only (network isolation + optional /run token); defer mTLS, short-lived scoped tokens, and payload encryption to an explicit Later/deferred subsection. - Document the decided not-implemented error behavior: the local sandbox errors when a network policy is set, and any specified filesystem policy errors, both mirroring the existing code-tool unsupported gate (code.ts CODE_TOOL_UNSUPPORTED_MESSAGE) and the typed connection *Unsupported*Errors. - Document the decision to disable stdio MCP-server execution in the sidecar the same way code execution was removed (not-implemented gate + remove mcp-bridge.ts/mcp-server.ts/stdio plumbing), and clarify that gateway tools are Layer-3 tool-permission, not Layer-2 sandbox_permission, so they need no change. - Update the legacy in-process pi engine to removed/historical (engines/pi.ts is gone). Verified code state: code execution already removed, stdio MCP still present, pi.ts removed, protocol.ts network comment already corrected by A3. Docs-only; no runner code changed. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
mmabrouk
force-pushed
the
docs/sidecar-trust-and-sandbox-enforcement
branch
from
cdeb18a to
f0225ca
Compare
Added: bind the agent sidecar on 0.0.0.0 in compose (stream-error fix)This PR's trust model defaults the sidecar runner bind to Fix: set Committed to this lane ( |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/oss/src/agent/app.py (1)
219-225: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate the runner URI before resolving tools or credentials.
select_backend(agent_config)is the allowlist gate, but it currently runs after tool/MCP resolution and possible model credential resolution. A disallowedagent.urishould fail before those external/secret-adjacent operations.Proposed fix
params = parameters or {} agent_config = AgentConfig.from_params(params, defaults=_default_agent_config()) + backend = select_backend(agent_config) msgs = to_messages(messages or (inputs or {}).get("messages") or []) @@ - harness = make_harness( - agent_config.harness, Environment(select_backend(agent_config)) - ) + harness = make_harness(agent_config.harness, Environment(backend))Also applies to: 258-260
docs/design/agent-workflows/interfaces/public-edge/workflow-invoke.md (1)
26-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale
sandboxrun-selection reference; should beuri.This page still lists the agent run-selection fields as
harness,sandbox,permission_policy, but the rest of this PR removes the per-runsandboxselector and replaces it withuri(seeagent-config-schema.md: "urireplaced the oldsandboxselector" andagent-service-handler.mdlistsharness/uri/permission_policy).select_backendnow sendssandboxonly as a constant_DEFAULT_SANDBOX, not as an authored selection. Update the example comment and prose to keep the doc set consistent.📝 Suggested doc fix
- "parameters": { "agent": { /* config, incl. harness, sandbox, permission_policy */ } } + "parameters": { "agent": { /* config, incl. harness, uri, permission_policy */ } }-`parameters` carries the agent config under `agent`, and the run-selection fields (`harness`, -`sandbox`, `permission_policy`) live inside that same `agent` object. The handler reads the +`parameters` carries the agent config under `agent`, and the run-selection fields (`harness`, +`uri`, `permission_policy`) live inside that same `agent` object. The handler reads the
🧹 Nitpick comments (1)
sdks/python/agenta/sdk/engines/running/utils.py (1)
544-566: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueShallow copy only protects top-level keys.
dict(meta)copies only the top level, so nested values (e.g. theharness_capabilitiesdocument) remain shared between the caller, the registry, and everyretrieve_metaconsumer. The docstring's "later mutation of the caller's dict cannot leak into the registry" guarantee therefore doesn't hold for nested mutations. Current callers treat meta as read-only, so this is non-blocking, butcopy.deepcopyon both store and read would make the isolation guarantee real.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32e73fdd-96c7-4369-ac42-916d90ae4e7d
📒 Files selected for processing (87)
docs/design/agent-workflows/documentation/adapters/agenta.mddocs/design/agent-workflows/documentation/adapters/pi.mddocs/design/agent-workflows/documentation/agent-configuration.mddocs/design/agent-workflows/documentation/agent-template.mddocs/design/agent-workflows/documentation/architecture.mddocs/design/agent-workflows/documentation/ground-truth.mddocs/design/agent-workflows/documentation/ports-and-adapters.mddocs/design/agent-workflows/documentation/running-the-agent.mddocs/design/agent-workflows/interfaces/cross-service/service-to-agent-runner.mddocs/design/agent-workflows/interfaces/in-service/agent-service-handler.mddocs/design/agent-workflows/interfaces/in-service/harness-adapters.mddocs/design/agent-workflows/interfaces/in-service/neutral-runtime-dtos.mddocs/design/agent-workflows/interfaces/public-edge/agent-config-schema.mddocs/design/agent-workflows/interfaces/public-edge/agent-messages.mddocs/design/agent-workflows/interfaces/public-edge/workflow-inspect.mddocs/design/agent-workflows/interfaces/public-edge/workflow-invoke.mddocs/design/agent-workflows/projects/agent-config-structure-cleanup/status.mddocs/design/agent-workflows/projects/agent-model-picker/README.mddocs/design/agent-workflows/projects/agent-model-picker/context.mddocs/design/agent-workflows/projects/agent-model-picker/plan.mddocs/design/agent-workflows/projects/agent-model-picker/research.mddocs/design/agent-workflows/projects/agent-model-picker/status.mddocs/design/agent-workflows/projects/sidecar-trust-and-sandbox-enforcement/README.mddocs/design/agent-workflows/projects/sidecar-trust-and-sandbox-enforcement/status.mddocs/design/agent-workflows/projects/sidecar-uri-config/README.mddocs/design/agent-workflows/projects/sidecar-uri-config/context.mddocs/design/agent-workflows/projects/sidecar-uri-config/plan.mddocs/design/agent-workflows/projects/sidecar-uri-config/research.mddocs/design/agent-workflows/projects/sidecar-uri-config/security.mddocs/design/agent-workflows/projects/sidecar-uri-config/status.mddocs/design/agent-workflows/projects/wire-contract-schema/README.mddocs/design/agent-workflows/projects/wire-contract-schema/status.mdhosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.ymlsdks/python/agenta/__init__.pysdks/python/agenta/sdk/agents/__init__.pysdks/python/agenta/sdk/agents/adapters/claude_settings.pysdks/python/agenta/sdk/agents/adapters/harnesses.pysdks/python/agenta/sdk/agents/capabilities.pysdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/agents/utils/wire.pysdks/python/agenta/sdk/agents/wire_models.pysdks/python/agenta/sdk/decorators/routing.pysdks/python/agenta/sdk/decorators/running.pysdks/python/agenta/sdk/engines/running/utils.pysdks/python/agenta/sdk/models/workflows.pysdks/python/agenta/sdk/utils/types.pysdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.pysdks/python/oss/tests/pytest/unit/agents/connections/test_capabilities.pysdks/python/oss/tests/pytest/unit/agents/test_dtos_agent_config.pysdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pysdks/python/oss/tests/pytest/unit/agents/test_wire_models.pysdks/python/oss/tests/pytest/unit/test_inspect_response.pyservices/agent/README.mdservices/agent/src/engines/sandbox_agent/mcp.tsservices/agent/src/engines/sandbox_agent/run-plan.tsservices/agent/src/server.tsservices/agent/src/tools/mcp-bridge.tsservices/agent/src/tools/mcp-server.tsservices/agent/tests/unit/mcp-servers.test.tsservices/agent/tests/unit/sandbox-agent-orchestration.test.tsservices/agent/tests/unit/sandbox-agent-run-plan.test.tsservices/agent/tests/unit/server.test.tsservices/agent/tests/unit/tool-bridge.test.tsservices/oss/src/agent/app.pyservices/oss/src/agent/config.pyservices/oss/src/agent/schemas.pyservices/oss/tests/pytest/unit/agent/test_builtin_uri_binding.pyservices/oss/tests/pytest/unit/agent/test_default_agent_config.pyservices/oss/tests/pytest/unit/agent/test_select_backend.pyweb/oss/src/components/AgentChatSlice/assets/transport.tsweb/packages/agenta-entities/src/workflow/api/api.tsweb/packages/agenta-entities/src/workflow/index.tsweb/packages/agenta-entities/src/workflow/state/index.tsweb/packages/agenta-entities/src/workflow/state/inspectMeta.tsweb/packages/agenta-entities/src/workflow/state/store.tsweb/packages/agenta-entities/tests/unit/inspectResponseSchemaResolution.test.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ClaudePermissionsControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/connectionUtils.tsweb/packages/agenta-entity-ui/tests/unit/connectionUtils.test.tsweb/packages/agenta-playground/src/state/execution/agentRequest.tsweb/packages/agenta-playground/src/state/execution/selectors.tsweb/packages/agenta-playground/tests/unit/agentMode.test.tsweb/packages/agenta-playground/tests/unit/agentRequest.test.ts
💤 Files with no reviewable changes (1)
- sdks/python/agenta/init.py
✅ Files skipped from review due to trivial changes (24)
- docs/design/agent-workflows/projects/agent-model-picker/README.md
- docs/design/agent-workflows/interfaces/public-edge/agent-messages.md
- docs/design/agent-workflows/documentation/ground-truth.md
- sdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.py
- services/agent/src/tools/mcp-server.ts
- web/packages/agenta-entities/src/workflow/index.ts
- docs/design/agent-workflows/projects/agent-config-structure-cleanup/status.md
- docs/design/agent-workflows/interfaces/cross-service/service-to-agent-runner.md
- sdks/python/agenta/sdk/agents/utils/wire.py
- docs/design/agent-workflows/projects/sidecar-uri-config/README.md
- docs/design/agent-workflows/interfaces/in-service/neutral-runtime-dtos.md
- docs/design/agent-workflows/projects/sidecar-uri-config/context.md
- docs/design/agent-workflows/documentation/adapters/agenta.md
- docs/design/agent-workflows/interfaces/public-edge/workflow-inspect.md
- docs/design/agent-workflows/documentation/agent-template.md
- docs/design/agent-workflows/interfaces/in-service/harness-adapters.md
- docs/design/agent-workflows/projects/sidecar-uri-config/status.md
- docs/design/agent-workflows/projects/sidecar-uri-config/plan.md
- services/agent/README.md
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ClaudePermissionsControl.tsx
- docs/design/agent-workflows/projects/agent-model-picker/context.md
- docs/design/agent-workflows/projects/agent-model-picker/plan.md
- docs/design/agent-workflows/projects/agent-model-picker/status.md
- docs/design/agent-workflows/projects/wire-contract-schema/status.md
🚧 Files skipped from review as they are similar to previous changes (20)
- web/packages/agenta-entities/tests/unit/inspectResponseSchemaResolution.test.ts
- web/packages/agenta-entities/src/workflow/state/index.ts
- web/packages/agenta-entities/src/workflow/state/inspectMeta.ts
- sdks/python/oss/tests/pytest/unit/agents/test_wire_models.py
- services/agent/tests/unit/sandbox-agent-orchestration.test.ts
- sdks/python/oss/tests/pytest/unit/test_inspect_response.py
- sdks/python/oss/tests/pytest/unit/agents/connections/test_capabilities.py
- services/oss/src/agent/schemas.py
- sdks/python/agenta/sdk/models/workflows.py
- sdks/python/agenta/sdk/agents/capabilities.py
- services/agent/src/engines/sandbox_agent/mcp.ts
- web/packages/agenta-entities/src/workflow/api/api.ts
- services/agent/tests/unit/sandbox-agent-run-plan.test.ts
- sdks/python/agenta/sdk/decorators/routing.py
- services/agent/tests/unit/server.test.ts
- services/agent/src/tools/mcp-bridge.ts
- services/agent/src/engines/sandbox_agent/run-plan.ts
- sdks/python/agenta/sdk/agents/wire_models.py
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/connectionUtils.ts
- web/packages/agenta-entity-ui/tests/unit/connectionUtils.test.ts
| environment: | ||
| PORT: "8765" | ||
| # Bind on all interfaces: the co-located Python `services` container reaches the | ||
| # sidecar over the compose network at `sandbox-agent:8765`, so the trust-model | ||
| # default of `127.0.0.1` (same-host only) is unreachable cross-container. | ||
| AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Expose runner token env in compose for /run auth.
This service now binds to 0.0.0.0, but AGENTA_AGENT_RUNNER_TOKEN is not plumbed in this environment block. Because this container intentionally has no env_file, operators cannot enable the server’s token gate without editing compose again. Add an explicit token env entry so authorization can be turned on safely.
Suggested patch
environment:
PORT: "8765"
# Bind on all interfaces: the co-located Python `services` container reaches the
# sidecar over the compose network at `sandbox-agent:8765`, so the trust-model
# default of `127.0.0.1` (same-host only) is unreachable cross-container.
AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0}
+ AGENTA_AGENT_RUNNER_TOKEN: ${AGENTA_AGENT_RUNNER_TOKEN:-}
PI_CODING_AGENT_DIR: /pi-agent📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| environment: | |
| PORT: "8765" | |
| # Bind on all interfaces: the co-located Python `services` container reaches the | |
| # sidecar over the compose network at `sandbox-agent:8765`, so the trust-model | |
| # default of `127.0.0.1` (same-host only) is unreachable cross-container. | |
| AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0} | |
| environment: | |
| PORT: "8765" | |
| # Bind on all interfaces: the co-located Python `services` container reaches the | |
| # sidecar over the compose network at `sandbox-agent:8765`, so the trust-model | |
| # default of `127.0.0.1` (same-host only) is unreachable cross-container. | |
| AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0} | |
| AGENTA_AGENT_RUNNER_TOKEN: ${AGENTA_AGENT_RUNNER_TOKEN:-} |
| environment: | ||
| PORT: "8765" | ||
| # Bind on all interfaces: the co-located Python `services` container reaches the | ||
| # sidecar over the compose network at `sandbox-agent:8765`, so the trust-model | ||
| # default of `127.0.0.1` (same-host only) is unreachable cross-container. | ||
| AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add token env passthrough for optional /run authorization.
AGENTA_AGENT_RUNNER_HOST is opened to all interfaces, but AGENTA_AGENT_RUNNER_TOKEN is missing from the allowed env list. Since sandbox-agent has no env_file, this blocks enabling the server’s token check via normal env configuration.
Suggested patch
environment:
PORT: "8765"
# Bind on all interfaces: the co-located Python `services` container reaches the
# sidecar over the compose network at `sandbox-agent:8765`, so the trust-model
# default of `127.0.0.1` (same-host only) is unreachable cross-container.
AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0}
+ AGENTA_AGENT_RUNNER_TOKEN: ${AGENTA_AGENT_RUNNER_TOKEN:-}
PI_CODING_AGENT_DIR: /pi-agent📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| environment: | |
| PORT: "8765" | |
| # Bind on all interfaces: the co-located Python `services` container reaches the | |
| # sidecar over the compose network at `sandbox-agent:8765`, so the trust-model | |
| # default of `127.0.0.1` (same-host only) is unreachable cross-container. | |
| AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0} | |
| environment: | |
| PORT: "8765" | |
| # Bind on all interfaces: the co-located Python `services` container reaches the | |
| # sidecar over the compose network at `sandbox-agent:8765`, so the trust-model | |
| # default of `127.0.0.1` (same-host only) is unreachable cross-container. | |
| AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0} | |
| AGENTA_AGENT_RUNNER_TOKEN: ${AGENTA_AGENT_RUNNER_TOKEN:-} |
| environment: | ||
| PORT: "8765" | ||
| # Bind on all interfaces: the co-located Python `services` container reaches the | ||
| # sidecar over the compose network at `sandbox-agent:8765`, so the trust-model | ||
| # default of `127.0.0.1` (same-host only) is unreachable cross-container. | ||
| AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep auth configurable in GH stack by wiring runner token env.
This compose file exposes the runner on 0.0.0.0 but does not pass AGENTA_AGENT_RUNNER_TOKEN. That prevents enabling the /run auth gate without modifying compose again.
Suggested patch
environment:
PORT: "8765"
# Bind on all interfaces: the co-located Python `services` container reaches the
# sidecar over the compose network at `sandbox-agent:8765`, so the trust-model
# default of `127.0.0.1` (same-host only) is unreachable cross-container.
AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0}
+ AGENTA_AGENT_RUNNER_TOKEN: ${AGENTA_AGENT_RUNNER_TOKEN:-}
AGENTA_HOST: ${AGENTA_HOST:-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| environment: | |
| PORT: "8765" | |
| # Bind on all interfaces: the co-located Python `services` container reaches the | |
| # sidecar over the compose network at `sandbox-agent:8765`, so the trust-model | |
| # default of `127.0.0.1` (same-host only) is unreachable cross-container. | |
| AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0} | |
| environment: | |
| PORT: "8765" | |
| # Bind on all interfaces: the co-located Python `services` container reaches the | |
| # sidecar over the compose network at `sandbox-agent:8765`, so the trust-model | |
| # default of `127.0.0.1` (same-host only) is unreachable cross-container. | |
| AGENTA_AGENT_RUNNER_HOST: ${AGENTA_AGENT_RUNNER_HOST:-0.0.0.0} | |
| AGENTA_AGENT_RUNNER_TOKEN: ${AGENTA_AGENT_RUNNER_TOKEN:-} | |
| AGENTA_HOST: ${AGENTA_HOST:-} |
| agent = params.get("agent") | ||
| source = agent if isinstance(agent, dict) else params | ||
| harness = str(source.get("harness") or defaults.harness).lower() | ||
| raw_uri = source.get("uri") | ||
| uri = _clean_uri(raw_uri) if raw_uri is not None else defaults.uri |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Let explicit uri: null clear inherited runner overrides.
Line 965 treats explicit None the same as an absent key, so a caller cannot clear a default/stored runner URI except by sending blanks. Since this field controls where secret-bearing runner requests go, honor key presence.
Proposed fix
agent = params.get("agent")
source = agent if isinstance(agent, dict) else params
harness = str(source.get("harness") or defaults.harness).lower()
- raw_uri = source.get("uri")
- uri = _clean_uri(raw_uri) if raw_uri is not None else defaults.uri
+ uri = _clean_uri(source.get("uri")) if "uri" in source else defaults.uri
permission_policy = str(| const message = | ||
| err instanceof Error ? (err.stack ?? err.message) : String(err); | ||
| result = { ok: false, error: message }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Returning err.stack in the response body leaks internal details.
Switching the error string from err.message to err.stack embeds full stack traces (file paths, internal structure) into the /run result. Per the wire contract, the Python side maps ok:false into RuntimeError(f"Agent run failed: {error}"), so this trace can propagate upward and potentially surface to end users. Prefer logging the stack server-side and returning only err.message on the wire. The same pattern repeats at Lines 193-195.
🛡️ Proposed change
- const message =
- err instanceof Error ? (err.stack ?? err.message) : String(err);
+ if (err instanceof Error && err.stack) {
+ process.stderr.write(`[sandbox-agent] run error: ${err.stack}\n`);
+ }
+ const message = err instanceof Error ? err.message : String(err);
result = { ok: false, error: message };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const message = | |
| err instanceof Error ? (err.stack ?? err.message) : String(err); | |
| result = { ok: false, error: message }; | |
| if (err instanceof Error && err.stack) { | |
| process.stderr.write(`[sandbox-agent] run error: ${err.stack}\n`); | |
| } | |
| const message = err instanceof Error ? err.message : String(err); | |
| result = { ok: false, error: message }; |
| // Legacy pre-migration run-selection keys that now live inside `agent`. Stripped from the | ||
| // top level when `agent` is present so we never emit both wire shapes for one config. | ||
| const LEGACY_RUN_SELECTION_KEYS = ["harness", "sandbox", "permission_policy"] as const | ||
|
|
||
| const configFor = (appId?: string | null) => { | ||
| const resolved = resolveAppAgConfig(appId) | ||
| if (!resolved) return stubConfig() | ||
| return { | ||
| parameters: {harness: "pi_core", sandbox: "local", ...resolved.ag_config}, | ||
| references: resolved.references, | ||
| const agConfig = resolved.ag_config as Record<string, unknown> | ||
| const agent = agConfig.agent | ||
| let parameters: Record<string, unknown> | ||
| if (agent && typeof agent === "object") { | ||
| const rest = {...agConfig} | ||
| for (const key of LEGACY_RUN_SELECTION_KEYS) delete rest[key] | ||
| parameters = { | ||
| ...rest, | ||
| agent: { | ||
| harness: "pi_core", | ||
| sandbox: "local", | ||
| ...(agent as Record<string, unknown>), | ||
| }, | ||
| } | ||
| } else { | ||
| parameters = {harness: "pi_core", sandbox: "local", ...agConfig} | ||
| } | ||
| return {parameters, references: resolved.references} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether permission_policy is ever set at the top level of resolved ag_config
rg -nP --type=ts -C3 'permission_policy' web/oss/src/components/AgentChatSliceRepository: Agenta-AI/agenta
Length of output: 945
🏁 Script executed:
#!/bin/bash
# Find usage of configFor and search for permission_policy in tests or other files
rg -NP --type=ts "configFor" web/oss/src/components/AgentChatSlice
rg -NP --type=ts "permission_policy" --ignore-case --glob "!(web/oss/src/components/AgentChatSlice/assets/transport.ts)"Repository: Agenta-AI/agenta
Length of output: 379
🏁 Script executed:
#!/bin/bash
# Search for permission_policy across the repository to see how it's defined and used
rg -N "permission_policy" --type=ts
# Also check transport.ts around the configFor definition to see full scope
sed -n '55,90p' 'web/oss/src/components/AgentChatSlice/assets/transport.ts'Repository: Agenta-AI/agenta
Length of output: 154
🏁 Script executed:
#!/bin/bash
cat -n web/oss/src/components/AgentChatSlice/assets/transport.ts | sed -n '55,100p'Repository: Agenta-AI/agenta
Length of output: 2541
🏁 Script executed:
#!/bin/bash
# Search for stubConfig to understand fallback behavior
rg -Nb "stubConfig" web/oss/src/components/AgentChatSlice/assets/transport.ts
# Search for the definition of LEGACY_RUN_SELECTION_KEYS context to see if permission_policy is expected elsewhere
rg -Nb "resolved.ag_config" web/oss/srcRepository: Agenta-AI/agenta
Length of output: 361
🏁 Script executed:
#!/bin/bash
# Check the stubConfig definition to see if permission_policy is included there
sed -n '2000,2010p' web/oss/src/components/AgentChatSlice/assets/transport.ts
# Search for 'permission_policy' in the `resolved` usage or configuration mapping functions
rg -Nb "resolveAppAgConfig" web/oss/src/components/AgentChatSlice/assets/transport.ts | head -20Repository: Agenta-AI/agenta
Length of output: 352
🏁 Script executed:
#!/bin/bash
# Check the agConfig file to understand how the config is resolved and if permission_policy is included
find web/oss/src/components/AgentChatSlice/assets -name "agConfig*" -type f | xargs cat -n | head -50Repository: Agenta-AI/agenta
Length of output: 2419
🏁 Script executed:
#!/bin/bash
# Read the rest of agConfig.ts to see the resolution logic and checks for permissions
sed -n '50,150p' web/oss/src/components/AgentChatSlice/assets/agConfig.tsRepository: Agenta-AI/agenta
Length of output: 2435
🏁 Script executed:
#!/bin/bash
# Search for test files related to transport or agConfig in this directory
find web/oss/src/components/AgentChatSlice/assets -name "*.test.ts" -o -name "*.spec.ts" -o -name "__tests__"
# Also check the stubConfig function content directly which we saw starts at 2000
sed -n '2000,2020p' web/oss/src/components/AgentChatSlice/assets/transport.tsRepository: Agenta-AI/agenta
Length of output: 154
🏁 Script executed:
#!/bin/bash
# 1. Check if stubConfig sets permission_policy (start reading around the line number found previously)
sed -n '2000,2030p' web/oss/src/components/AgentChatSlice/assets/transport.ts
# 2. Search for test files for this component
find web/oss/src/components/AgentChatSlice -iname "*.test.ts" -o -iname "*.spec.ts" -o -iname "*.test.tsx" | head -10
# 3. Search for explicit permission_policy usage in the whole oss folder to see how it's typically structured
rg "permission_policy" web/oss/src --type=ts -nRepository: Agenta-AI/agenta
Length of output: 302
🏁 Script executed:
#!/bin/bash
# The previous sed command started at 2000, but looking at the file size (line count), it might be smaller or larger.
# Let's find the exact line number of 'stubConfig' function definition
rg -n "^const stubConfig" web/oss/src/components/AgentChatSlice/assets/transport.ts
# Also count lines
wc -l web/oss/src/components/AgentChatSlice/assets/transport.tsRepository: Agenta-AI/agenta
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Check the stubConfig implementation fully to see if it includes permission_policy
sed -n '36,60p' web/oss/src/components/AgentChatSlice/assets/transport.ts
# Also, search for the full file to ensure we've seen the whole context, and perhaps look at the usage site to see if it passes the result to a specific API endpoint
sed -n '1,50p' web/oss/src/components/AgentChatSlice/assets/transport.tsRepository: Agenta-AI/agenta
Length of output: 3520
permission_policy is stripped from the top level but never re-seeded under agent.
LEGACY_RUN_SELECTION_KEYS deletes permission_policy from the root of agConfig when an agent object is present. However, unlike harness and sandbox, it is not re-inserted into the agent block. This causes any top-level permission_policy to be silently lost, reverting to the server-side default.
Ensure the value is migrated from the root to agent, or defaulted if missing:
} else {
parameters = {harness: "pi_core", sandbox: "local", ...agConfig}
}Proposed fix:
- Before deleting keys from
rest, check ifpermission_policyexists inagConfig. - Add it to the
agentobject construction if it exists, or rely on a default similar toharness/sandboxif appropriate for your server schema.
Ensure permission_policy is present in the final agent object if it was intended.
| const modeOptions = useMemo( | ||
| () => allowedConnectionModes(capabilities, harness), | ||
| [capabilities, harness], | ||
| ) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reconcile auth mode when the harness filters it out.
modeOptions is filtered, but connection.mode can remain unsupported after a harness change; then the segmented control has no valid value while the saved ModelRef still carries the old mode.
Proposed fix
[setField, modelId, connection, config.model, capabilities, harness],
)
+
+ useEffect(() => {
+ if (modeOptions.length === 0 || modeOptions.includes(connection.mode)) return
+ writeModel({mode: modeOptions[0], slug: null})
+ }, [modeOptions, connection.mode, writeModel])
// On harness switch, clear a model the new harness can't reach (rather than sending anAlso applies to: 440-452
| const nextModelId = patch.modelId !== undefined ? patch.modelId : modelId | ||
| // When the model changes, derive the provider from the picked model; otherwise keep it. | ||
| let nextProvider: string | null | ||
| if (patch.provider !== undefined) { | ||
| nextProvider = patch.provider | ||
| } else if (patch.modelId !== undefined) { | ||
| nextProvider = | ||
| providerForModel(capabilities, harness, nextModelId) ?? connection.provider | ||
| } else { | ||
| nextProvider = connection.provider | ||
| } | ||
| setField( | ||
| "model", | ||
| composeModelValue({ | ||
| modelId: patch.modelId !== undefined ? patch.modelId : modelId, | ||
| provider: patch.provider !== undefined ? patch.provider : connection.provider, | ||
| modelId: nextModelId, | ||
| provider: nextProvider, | ||
| mode: patch.mode !== undefined ? patch.mode : connection.mode, | ||
| slug: patch.slug !== undefined ? patch.slug : connection.slug, | ||
| // Carry through extra ModelRef keys (params, ...) the form does not edit. | ||
| existing: config.model, | ||
| }), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear the custom connection slug when the provider changes.
writeModel can switch provider from the selected model while keeping the previous provider’s connection.slug, producing mismatched ModelRefs such as Anthropic + an OpenAI custom slug.
Proposed fix
} else {
nextProvider = connection.provider
}
+ const providerChanged = nextProvider !== connection.provider
+ const nextSlug =
+ patch.slug !== undefined ? patch.slug : providerChanged ? null : connection.slug
setField(
"model",
composeModelValue({
modelId: nextModelId,
provider: nextProvider,
mode: patch.mode !== undefined ? patch.mode : connection.mode,
- slug: patch.slug !== undefined ? patch.slug : connection.slug,
+ slug: nextSlug,
// Carry through extra ModelRef keys (params, ...) the form does not edit.
existing: config.model,
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nextModelId = patch.modelId !== undefined ? patch.modelId : modelId | |
| // When the model changes, derive the provider from the picked model; otherwise keep it. | |
| let nextProvider: string | null | |
| if (patch.provider !== undefined) { | |
| nextProvider = patch.provider | |
| } else if (patch.modelId !== undefined) { | |
| nextProvider = | |
| providerForModel(capabilities, harness, nextModelId) ?? connection.provider | |
| } else { | |
| nextProvider = connection.provider | |
| } | |
| setField( | |
| "model", | |
| composeModelValue({ | |
| modelId: patch.modelId !== undefined ? patch.modelId : modelId, | |
| provider: patch.provider !== undefined ? patch.provider : connection.provider, | |
| modelId: nextModelId, | |
| provider: nextProvider, | |
| mode: patch.mode !== undefined ? patch.mode : connection.mode, | |
| slug: patch.slug !== undefined ? patch.slug : connection.slug, | |
| // Carry through extra ModelRef keys (params, ...) the form does not edit. | |
| existing: config.model, | |
| }), | |
| const nextModelId = patch.modelId !== undefined ? patch.modelId : modelId | |
| // When the model changes, derive the provider from the picked model; otherwise keep it. | |
| let nextProvider: string | null | |
| if (patch.provider !== undefined) { | |
| nextProvider = patch.provider | |
| } else if (patch.modelId !== undefined) { | |
| nextProvider = | |
| providerForModel(capabilities, harness, nextModelId) ?? connection.provider | |
| } else { | |
| nextProvider = connection.provider | |
| } | |
| const providerChanged = nextProvider !== connection.provider | |
| const nextSlug = | |
| patch.slug !== undefined ? patch.slug : providerChanged ? null : connection.slug | |
| setField( | |
| "model", | |
| composeModelValue({ | |
| modelId: nextModelId, | |
| provider: nextProvider, | |
| mode: patch.mode !== undefined ? patch.mode : connection.mode, | |
| slug: nextSlug, | |
| // Carry through extra ModelRef keys (params, ...) the form does not edit. | |
| existing: config.model, | |
| }), |
| // On harness switch, clear a model the new harness can't reach (rather than sending an | ||
| // unsupported model). Permissive when the new harness publishes no models. | ||
| useEffect(() => { | ||
| if (!harness || !modelId) return | ||
| if (!harnessAllowsModel(capabilities, harness, modelId)) { | ||
| writeModel({modelId: null, provider: null}) | ||
| } | ||
| // Only react to harness/capabilities changes, not every model edit. | ||
| }, [harness, capabilities]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Backfill provider once inspect capabilities arrive.
If the fallback picker is used before /inspect resolves, it writes {model: id} with no provider. When capabilities later load and the model is valid, this effect leaves provider null even though the UI contract says selecting a model sets both.
Proposed fix
useEffect(() => {
if (!harness || !modelId) return
+ const inferredProvider = providerForModel(capabilities, harness, modelId)
if (!harnessAllowsModel(capabilities, harness, modelId)) {
- writeModel({modelId: null, provider: null})
+ writeModel({modelId: null, provider: null, slug: null})
+ return
+ }
+ if (inferredProvider && inferredProvider !== connection.provider) {
+ writeModel({provider: inferredProvider, slug: null})
}
- // Only react to harness/capabilities changes, not every model edit.
- }, [harness, capabilities])
+ }, [harness, capabilities, modelId, connection.provider, writeModel])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // On harness switch, clear a model the new harness can't reach (rather than sending an | |
| // unsupported model). Permissive when the new harness publishes no models. | |
| useEffect(() => { | |
| if (!harness || !modelId) return | |
| if (!harnessAllowsModel(capabilities, harness, modelId)) { | |
| writeModel({modelId: null, provider: null}) | |
| } | |
| // Only react to harness/capabilities changes, not every model edit. | |
| }, [harness, capabilities]) | |
| // On harness switch, clear a model the new harness can't reach (rather than sending an | |
| // unsupported model). Permissive when the new harness publishes no models. | |
| useEffect(() => { | |
| if (!harness || !modelId) return | |
| const inferredProvider = providerForModel(capabilities, harness, modelId) | |
| if (!harnessAllowsModel(capabilities, harness, modelId)) { | |
| writeModel({modelId: null, provider: null, slug: null}) | |
| return | |
| } | |
| if (inferredProvider && inferredProvider !== connection.provider) { | |
| writeModel({provider: inferredProvider, slug: null}) | |
| } | |
| // Only react to harness/capabilities changes, not every model edit. | |
| }, [harness, capabilities, modelId, connection.provider, writeModel]) |
…view Address the 6 author inline comments on the proposal: - Scope near-term work to steps 1-2 only (network isolation + optional /run token); defer mTLS, short-lived scoped tokens, and payload encryption to an explicit Later/deferred subsection. - Document the decided not-implemented error behavior: the local sandbox errors when a network policy is set, and any specified filesystem policy errors, both mirroring the existing code-tool unsupported gate (code.ts CODE_TOOL_UNSUPPORTED_MESSAGE) and the typed connection *Unsupported*Errors. - Document the decision to disable stdio MCP-server execution in the sidecar the same way code execution was removed (not-implemented gate + remove mcp-bridge.ts/mcp-server.ts/stdio plumbing), and clarify that gateway tools are Layer-3 tool-permission, not Layer-2 sandbox_permission, so they need no change. - Update the legacy in-process pi engine to removed/historical (engines/pi.ts is gone). Verified code state: code execution already removed, stdio MCP still present, pi.ts removed, protocol.ts network comment already corrected by A3. Docs-only; no runner code changed. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
mmabrouk
force-pushed
the
docs/sidecar-trust-and-sandbox-enforcement
branch
from
f0225ca to
f7baee1
Compare
…aders Flip the deferred remote-MCP path on in the runner while keeping stdio MCP disabled (PR #4831 disabled the runner-host stdio bridge for security). - toAcpMcpServers now delivers transport:"http" (+url): builds the ACP McpServer type:"http" variant ({name,url,headers}) and routes each resolved env entry into an HTTP request header. The resolved secret already rides the /run wire under env (the SDK resolver merges named secrets into env for both transports), so there is no SDK, protocol, or golden-fixture change. - transport:"stdio" still throws MCP_UNSUPPORTED_MESSAGE; run-plan still refuses any run carrying a stdio MCP server. A command-less stdio or url-less http server is skipped (logged), unchanged. - Tests: http delivered with the secret in a header, empty header list with no secret, stdio still rejected, mixed input skips non-deliverable entries. - Docs synced: runner-to-mcp-server + mcp-models-and-resolution inventory, public-edge agent-config-schema, the sidecar enforcement matrix (http enabled row), http-mcp-transport status. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
…ions The runner's capability handling degraded silently: a non-Pi run carrying tools whose harness probe reports mcpTools:false / toolCalls:false had its tools dropped without a trace, and a failed probe fell back to a static guess with no signal that it had. A wrong or missing flag changed behavior quietly instead of erroring. Make capability handling fail loud, mirroring the *_UNSUPPORTED_MESSAGE gates from #4831 and tools/code.ts CODE_TOOL_UNSUPPORTED_MESSAGE: - assertRequiredCapabilities (capabilities.ts) refuses a non-Pi run that carries tool specs when the probe reports it cannot receive tools, with a specific error naming the missing flags and the dropped tool count. Pi is exempt (its tools ride the native extension, not MCP); a no-tools run is unaffected. - probeCapabilities now returns ProbedCapabilities { capabilities, source } so callers know whether the flags were probed or a static guess; the gate logs the source so a failed probe is debuggable. - Debug assertions across the runner hot path (run-plan build, capability probe shape, capability gate, sandbox start) catch impossible states early with actionable [sandbox-agent invariant] messages. Docs: runner-to-harness inventory page updated to record the fail-loud gate and drop the stale 'a wrong flag silently changes behavior' claim. Tests: 182 vitest (was 168) + typecheck green in services/agent. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
Add a research project documenting the service->runner /run trust model and the verified sandbox enforcement state. Part 1 records that /run carries plaintext provider secrets plus the toolCallback and trace bearer tokens over unauthenticated plain HTTP (deliver_http in ts_runner.py, no auth check in server.ts), safe only on a trusted network. Proposes localhost/in-cluster-only + an optional shared /run token as the pragmatic default, with TLS/mTLS and short-lived scoped tokens as the hardening path. Part 2 corrects the record: SandboxPermission network egress IS enforced on Daytona (daytonaNetworkFields/buildSandboxProvider), not enforceable on the local sandbox (run-plan.ts fails loud under strict), filesystem is declared-only, and code/gateway tools plus stdio MCP run on the runner host outside the sandbox boundary. Includes an enforcement matrix and the exact protocol.ts:149-150 comment correction for the protocol.ts owner to apply. Docs only; no code changed. Composio, the tool gateway, connections, and MCP are unchanged. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
…view Address the 6 author inline comments on the proposal: - Scope near-term work to steps 1-2 only (network isolation + optional /run token); defer mTLS, short-lived scoped tokens, and payload encryption to an explicit Later/deferred subsection. - Document the decided not-implemented error behavior: the local sandbox errors when a network policy is set, and any specified filesystem policy errors, both mirroring the existing code-tool unsupported gate (code.ts CODE_TOOL_UNSUPPORTED_MESSAGE) and the typed connection *Unsupported*Errors. - Document the decision to disable stdio MCP-server execution in the sidecar the same way code execution was removed (not-implemented gate + remove mcp-bridge.ts/mcp-server.ts/stdio plumbing), and clarify that gateway tools are Layer-3 tool-permission, not Layer-2 sandbox_permission, so they need no change. - Update the legacy in-process pi engine to removed/historical (engines/pi.ts is gone). Verified code state: code execution already removed, stdio MCP still present, pi.ts removed, protocol.ts network comment already corrected by A3. Docs-only; no runner code changed. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
…undaries Implements the decided near-term scope of the sidecar-trust/sandbox-enforcement proposal (runner-only, services/agent/src): 1. Network isolation: server.ts binds to AGENTA_AGENT_RUNNER_HOST (default 127.0.0.1, never 0.0.0.0) and gates POST /run with an OPTIONAL shared token (AGENTA_AGENT_RUNNER_TOKEN, default OFF; constant-time compare; /health open). 2. Error-on-unimplemented sandbox_permission in run-plan.ts: a restricted network policy on the LOCAL sandbox and any filesystem value now fail loudly the not-implemented way (LOCAL_NETWORK_UNSUPPORTED_MESSAGE / FILESYSTEM_UNSUPPORTED_MESSAGE), regardless of enforcement. 3. stdio MCP disabled the way code execution was: MCP_UNSUPPORTED_MESSAGE in tools/mcp-bridge.ts; buildToolMcpServers + toAcpMcpServers throw, mcp-server.ts reduced to a refusing stub, and run-plan.ts rejects any run carrying a stdio MCP server. Removes custom-tool delivery to non-Pi harnesses + user stdio MCP. protocol.ts is untouched (the gates are runtime behavior, not new wire fields). Gateway/callback tools are unchanged (Layer-3 tool-permission). Docs synced. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
The sidecar trust model defaults the runner bind to 127.0.0.1 (same-host only). In docker-compose the Python services container and the sandbox-agent sidecar are SEPARATE containers, so 127.0.0.1 makes the sidecar unreachable: every agent run failed with 'All connection attempts failed' and the playground chat showed a stream error. The sidecar's own healthcheck hits 127.0.0.1 inside its container, so it passed and masked the break. Set AGENTA_AGENT_RUNNER_HOST=0.0.0.0 (overridable) on the sandbox-agent service in the EE dev, OSS dev, and OSS gh compose files so the co-located services container can reach it at sandbox-agent:8765. Verified live: the agent chat streams end to end again. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
mmabrouk
changed the base branch from
feat/agent-contract-versioning-docs
to
big-agents
mmabrouk
force-pushed
the
docs/sidecar-trust-and-sandbox-enforcement
branch
from
f7baee1 to
8be0209
Compare
…aders Flip the deferred remote-MCP path on in the runner while keeping stdio MCP disabled (PR #4831 disabled the runner-host stdio bridge for security). - toAcpMcpServers now delivers transport:"http" (+url): builds the ACP McpServer type:"http" variant ({name,url,headers}) and routes each resolved env entry into an HTTP request header. The resolved secret already rides the /run wire under env (the SDK resolver merges named secrets into env for both transports), so there is no SDK, protocol, or golden-fixture change. - transport:"stdio" still throws MCP_UNSUPPORTED_MESSAGE; run-plan still refuses any run carrying a stdio MCP server. A command-less stdio or url-less http server is skipped (logged), unchanged. - Tests: http delivered with the secret in a header, empty header list with no secret, stdio still rejected, mixed input skips non-deliverable entries. - Docs synced: runner-to-mcp-server + mcp-models-and-resolution inventory, public-edge agent-config-schema, the sidecar enforcement matrix (http enabled row), http-mcp-transport status. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
…ions The runner's capability handling degraded silently: a non-Pi run carrying tools whose harness probe reports mcpTools:false / toolCalls:false had its tools dropped without a trace, and a failed probe fell back to a static guess with no signal that it had. A wrong or missing flag changed behavior quietly instead of erroring. Make capability handling fail loud, mirroring the *_UNSUPPORTED_MESSAGE gates from #4831 and tools/code.ts CODE_TOOL_UNSUPPORTED_MESSAGE: - assertRequiredCapabilities (capabilities.ts) refuses a non-Pi run that carries tool specs when the probe reports it cannot receive tools, with a specific error naming the missing flags and the dropped tool count. Pi is exempt (its tools ride the native extension, not MCP); a no-tools run is unaffected. - probeCapabilities now returns ProbedCapabilities { capabilities, source } so callers know whether the flags were probed or a static guess; the gate logs the source so a failed probe is debuggable. - Debug assertions across the runner hot path (run-plan build, capability probe shape, capability gate, sandbox start) catch impossible states early with actionable [sandbox-agent invariant] messages. Docs: runner-to-harness inventory page updated to record the fail-loud gate and drop the stale 'a wrong flag silently changes behavior' claim. Tests: 182 vitest (was 168) + typecheck green in services/agent. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
… stdio MCP relay shim
QA finding F-042: an Agenta gateway/callback tool is never surfaced to the Claude
harness when it runs in a Daytona sandbox. The same tool works on Claude+LOCAL and
Pi+Daytona; only Claude x Daytona drops it.
Root cause is an advertisement gap, not an execution gap. On LOCAL, the runner
advertises gateway tools to Claude over a loopback HTTP MCP server. On Daytona that
loopback is correctly skipped (the in-sandbox harness's 127.0.0.1 is the sandbox's,
not the runner's), and Pi delivers via its bundled in-sandbox extension + the file
relay. Claude on Daytona had NO in-sandbox advertiser, so sessionInit.mcpServers was
empty and the model never learned the tool existed. The file relay loop (execution)
was already running, idle.
Fix: a tiny self-contained stdio MCP server, esbuild-bundled like the Pi extension,
uploaded into the sandbox and advertised to Claude as an ACP McpServerStdio entry
(typeless {name,command,args,env}). The sandbox-agent SDK forwards it verbatim into
the in-sandbox newSession; the Claude ACP adapter maps a typeless entry to a Claude
SDK {type:"stdio",...} server, launched IN the sandbox. On tools/call the shim writes
the same relay request the existing runner-side relay loop already executes. The
entire execution path, security model, and local path are unchanged.
- src/tools/relay-mcp-stdio.ts: the shim (newline-delimited MCP JSON-RPC over stdio).
- src/tools/relay-client.ts: the file-relay client factored out of dispatch.ts so the
bundle carries only file-relay code (no callback/code executor) — 5.3 kB, 0 network.
- src/engines/sandbox_agent/relay-shim.ts: bundle path + fail-loud upload helper.
- src/engines/sandbox_agent/mcp.ts: Daytona non-Pi branch builds the stdio entry.
- src/engines/sandbox_agent.ts: upload the shim + pass the path on Daytona non-Pi+tools.
- scripts/build-extension.mjs: also bundle dist/tools/relay-mcp-stdio.js.
Security: the stdio child runs INSIDE the Daytona sandbox (not the runner host, so it
does not reopen the #4831 runner-host-stdio hole), carries only public specs + the
relay dir (no secret), and every credentialed action stays server-side via the relay.
Layer-3 permission enforcement in startToolRelay is unchanged. Codex-reviewed (xhigh).
DRAFT: not live-verified on Daytona yet (the one remaining gate). 284 runner unit
tests + typecheck green; bundle builds and answers initialize over stdio.
Design: docs/design/agent-workflows/projects/claude-daytona-gateway/
Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
… stdio MCP relay shim
QA finding F-042: an Agenta gateway/callback tool is never surfaced to the Claude
harness when it runs in a Daytona sandbox. The same tool works on Claude+LOCAL and
Pi+Daytona; only Claude x Daytona drops it.
Root cause is an advertisement gap, not an execution gap. On LOCAL, the runner
advertises gateway tools to Claude over a loopback HTTP MCP server. On Daytona that
loopback is correctly skipped (the in-sandbox harness's 127.0.0.1 is the sandbox's,
not the runner's), and Pi delivers via its bundled in-sandbox extension + the file
relay. Claude on Daytona had NO in-sandbox advertiser, so sessionInit.mcpServers was
empty and the model never learned the tool existed. The file relay loop (execution)
was already running, idle.
Fix: a tiny self-contained stdio MCP server, esbuild-bundled like the Pi extension,
uploaded into the sandbox and advertised to Claude as an ACP McpServerStdio entry
(typeless {name,command,args,env}). The sandbox-agent SDK forwards it verbatim into
the in-sandbox newSession; the Claude ACP adapter maps a typeless entry to a Claude
SDK {type:"stdio",...} server, launched IN the sandbox. On tools/call the shim writes
the same relay request the existing runner-side relay loop already executes. The
entire execution path, security model, and local path are unchanged.
- src/tools/relay-mcp-stdio.ts: the shim (newline-delimited MCP JSON-RPC over stdio).
- src/tools/relay-client.ts: the file-relay client factored out of dispatch.ts so the
bundle carries only file-relay code (no callback/code executor) — 5.3 kB, 0 network.
- src/engines/sandbox_agent/relay-shim.ts: bundle path + fail-loud upload helper.
- src/engines/sandbox_agent/mcp.ts: Daytona non-Pi branch builds the stdio entry.
- src/engines/sandbox_agent.ts: upload the shim + pass the path on Daytona non-Pi+tools.
- scripts/build-extension.mjs: also bundle dist/tools/relay-mcp-stdio.js.
Security: the stdio child runs INSIDE the Daytona sandbox (not the runner host, so it
does not reopen the #4831 runner-host-stdio hole), carries only public specs + the
relay dir (no secret), and every credentialed action stays server-side via the relay.
Layer-3 permission enforcement in startToolRelay is unchanged. Codex-reviewed (xhigh).
DRAFT: not live-verified on Daytona yet (the one remaining gate). 284 runner unit
tests + typecheck green; bundle builds and answers initialize over stdio.
Design: docs/design/agent-workflows/projects/claude-daytona-gateway/
Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
… stdio MCP relay shim
QA finding F-042: an Agenta gateway/callback tool is never surfaced to the Claude
harness when it runs in a Daytona sandbox. The same tool works on Claude+LOCAL and
Pi+Daytona; only Claude x Daytona drops it.
Root cause is an advertisement gap, not an execution gap. On LOCAL, the runner
advertises gateway tools to Claude over a loopback HTTP MCP server. On Daytona that
loopback is correctly skipped (the in-sandbox harness's 127.0.0.1 is the sandbox's,
not the runner's), and Pi delivers via its bundled in-sandbox extension + the file
relay. Claude on Daytona had NO in-sandbox advertiser, so sessionInit.mcpServers was
empty and the model never learned the tool existed. The file relay loop (execution)
was already running, idle.
Fix: a tiny self-contained stdio MCP server, esbuild-bundled like the Pi extension,
uploaded into the sandbox and advertised to Claude as an ACP McpServerStdio entry
(typeless {name,command,args,env}). The sandbox-agent SDK forwards it verbatim into
the in-sandbox newSession; the Claude ACP adapter maps a typeless entry to a Claude
SDK {type:"stdio",...} server, launched IN the sandbox. On tools/call the shim writes
the same relay request the existing runner-side relay loop already executes. The
entire execution path, security model, and local path are unchanged.
- src/tools/relay-mcp-stdio.ts: the shim (newline-delimited MCP JSON-RPC over stdio).
- src/tools/relay-client.ts: the file-relay client factored out of dispatch.ts so the
bundle carries only file-relay code (no callback/code executor) — 5.3 kB, 0 network.
- src/engines/sandbox_agent/relay-shim.ts: bundle path + fail-loud upload helper.
- src/engines/sandbox_agent/mcp.ts: Daytona non-Pi branch builds the stdio entry.
- src/engines/sandbox_agent.ts: upload the shim + pass the path on Daytona non-Pi+tools.
- scripts/build-extension.mjs: also bundle dist/tools/relay-mcp-stdio.js.
Security: the stdio child runs INSIDE the Daytona sandbox (not the runner host, so it
does not reopen the #4831 runner-host-stdio hole), carries only public specs + the
relay dir (no secret), and every credentialed action stays server-side via the relay.
Layer-3 permission enforcement in startToolRelay is unchanged. Codex-reviewed (xhigh).
DRAFT: not live-verified on Daytona yet (the one remaining gate). 284 runner unit
tests + typecheck green; bundle builds and answers initialize over stdio.
Design: docs/design/agent-workflows/projects/claude-daytona-gateway/
Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
… stdio MCP relay shim
QA finding F-042: an Agenta gateway/callback tool is never surfaced to the Claude
harness when it runs in a Daytona sandbox. The same tool works on Claude+LOCAL and
Pi+Daytona; only Claude x Daytona drops it.
Root cause is an advertisement gap, not an execution gap. On LOCAL, the runner
advertises gateway tools to Claude over a loopback HTTP MCP server. On Daytona that
loopback is correctly skipped (the in-sandbox harness's 127.0.0.1 is the sandbox's,
not the runner's), and Pi delivers via its bundled in-sandbox extension + the file
relay. Claude on Daytona had NO in-sandbox advertiser, so sessionInit.mcpServers was
empty and the model never learned the tool existed. The file relay loop (execution)
was already running, idle.
Fix: a tiny self-contained stdio MCP server, esbuild-bundled like the Pi extension,
uploaded into the sandbox and advertised to Claude as an ACP McpServerStdio entry
(typeless {name,command,args,env}). The sandbox-agent SDK forwards it verbatim into
the in-sandbox newSession; the Claude ACP adapter maps a typeless entry to a Claude
SDK {type:"stdio",...} server, launched IN the sandbox. On tools/call the shim writes
the same relay request the existing runner-side relay loop already executes. The
entire execution path, security model, and local path are unchanged.
- src/tools/relay-mcp-stdio.ts: the shim (newline-delimited MCP JSON-RPC over stdio).
- src/tools/relay-client.ts: the file-relay client factored out of dispatch.ts so the
bundle carries only file-relay code (no callback/code executor) — 5.3 kB, 0 network.
- src/engines/sandbox_agent/relay-shim.ts: bundle path + fail-loud upload helper.
- src/engines/sandbox_agent/mcp.ts: Daytona non-Pi branch builds the stdio entry.
- src/engines/sandbox_agent.ts: upload the shim + pass the path on Daytona non-Pi+tools.
- scripts/build-extension.mjs: also bundle dist/tools/relay-mcp-stdio.js.
Security: the stdio child runs INSIDE the Daytona sandbox (not the runner host, so it
does not reopen the #4831 runner-host-stdio hole), carries only public specs + the
relay dir (no secret), and every credentialed action stays server-side via the relay.
Layer-3 permission enforcement in startToolRelay is unchanged. Codex-reviewed (xhigh).
DRAFT: not live-verified on Daytona yet (the one remaining gate). 284 runner unit
tests + typecheck green; bundle builds and answers initialize over stdio.
Design: docs/design/agent-workflows/projects/claude-daytona-gateway/
Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
1 participant
Context
The agent runner sidecar (
services/agent/) exposed two trust gaps that the design doc on this PR analyzed. First,POST /runcarries plaintext provider secrets and reusable bearer tokens, yet the sidecar accepted any caller on any interface with no authentication. Second, theSandboxPermissionboundary declared capabilities the runner does not actually enforce: anetworkpolicy on the local sandbox (which has no egress control), afilesystemjail (applied by no backend), and stdio MCP servers that run unconfined on the runner host (the same execution bypass that already had code execution removed). The boundary looked stronger than it was, and a run could proceed believing a jail held when it did not.This PR is now the implementation of the decided near-term scope from the design doc (steps 1 and 2 of Part 1, plus the Part 2 not-implemented decisions). It stays inside the runner (
services/agent/src/**). mTLS, scoped tokens, and payload encryption remain deferred.Changes
1. Network isolation + optional
/runtoken (server.ts).The server now binds to
AGENTA_AGENT_RUNNER_HOST(default127.0.0.1, never0.0.0.0), so by default the sidecar is reachable only from the same host as the co-located Python service. Set it to the private pod/internal interface in Kubernetes/Compose and never publish the port. An optional shared token gates/run: whenAGENTA_AGENT_RUNNER_TOKENis set, a request must present it (asAuthorization: Bearer <token>orX-Agenta-Runner-Token: <token>, constant-time compare) or get a 401. The check is default-off, so loopback/co-located deployments are unaffected./healthstays open for liveness probes.2. Error-on-unimplemented
sandbox_permission(run-plan.ts).A declared boundary the runner cannot enforce now fails loudly the way code tools do (
tools/code.ts), instead of being silently accepted underbest_effort:networkpolicy on the local sandbox returnsLOCAL_NETWORK_UNSUPPORTED_MESSAGE(the local sidecar has no per-run egress control), regardless ofenforcement. Daytona still enforces it.filesystemvalue (on any backend) returnsFILESYSTEM_UNSUPPORTED_MESSAGE, since no backend applies a filesystem jail.best_effortis no longer the escape hatch for these two. The Daytona strict-mode guard against runner-host (gateway/code) tools is kept.3. stdio MCP disabled, the way code execution was (
tools/mcp-bridge.ts,tools/mcp-server.ts,engines/sandbox_agent/mcp.ts,run-plan.ts).A stdio MCP server runs an arbitrary process on the runner host, outside the sandbox boundary. Until the security is fixed, the wire shapes (
McpServerConfig, themcpServersrequest field) stay, but delivery is gated by a single named constant:buildToolMcpServersandtoAcpMcpServersthrow it,mcp-server.tsis reduced to a refusing stub, andrun-plan.tsrejects any run that carries a stdio MCP server up front.What disabling MCP removes: non-Pi harnesses (e.g. Claude) take custom tools only over MCP, so they can no longer receive custom tools at all, and user-declared stdio MCP servers are refused. Pi is unaffected. Pi tools are delivered natively through the Pi extension, not MCP, and gateway/callback tools still POST back through
/tools/call.protocol.tsis not edited. The gates are runtime behavior, not new wire fields, and the file is owned by the harness-rename agent this cycle. Gateway/callback tools are unchanged; they are governed by Layer-3 tool-permission, not the Layer-2sandbox_permissionboundary.Scope / risk
MCP support for non-Pi harnesses (e.g. Claude) is disabled in this PR. A Claude agent that previously worked with custom tools will fail with the MCP-unsupported error. Pi-based agents with gateway/callback tools are unaffected. The loopback-only bind is the most visible deployment change: any operator who was testing by hitting the runner port directly from another host will lose access unless they set
AGENTA_AGENT_RUNNER_HOST.Tests / notes
pnpm test(168 passing) andpnpm run typecheckinservices/agent, prettier clean./runtoken gate (default-off accept, 401 without/with a wrong token, accept via both header forms,/healthopen); the local-network and filesystem not-implemented errors (bothenforcementvalues); stdio MCP refused at plan time and at delivery; a non-Pi run carrying a custom tool now surfaces the MCP-unsupported error.README.md/status.md(decisions marked implemented), the runnerREADME.mdMCP/tools section. The interface inventory pages (runner-to-mcp-server.md,sandbox-permission.md) are owned by a sibling lane; a hand-off to fold the disabled/error state into the inventory is recorded on the coordination board.How to QA
Prerequisites: Local dev stack with the agent sidecar running (
services/agent/Node runner). NoAGENTA_AGENT_RUNNER_TOKENset (default-off).Steps (network isolation):
127.0.0.1:/healthresponds without a token:curl http://127.0.0.1:3001/health # Expected: 200Steps (token gate):
3. Set
AGENTA_AGENT_RUNNER_TOKEN=test-secretand restart the runner.4. Send a
/runrequest without the token:Steps (sandbox_permission fail-fast):
6. Send a
/runrequest with a restricted network policy on the local backend:Steps (stdio MCP disabled):
7. Send a
/runrequest that includes an stdio MCP server:Expected result: Token gate blocks unauthorized requests and passes authorized ones. Local-network and filesystem policies fail loudly. stdio MCP servers are refused at plan time.
Automated tests:
Edge cases: A Pi-based agent using gateway tools must still work (gateway tools are not MCP and are unaffected). A Claude agent using custom tools will now fail; confirm the error message is clear, not a silent hang.
https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc