feat(agent): replace sandbox with an optional sidecar uri (allowlist-gated routing)#4836
feat(agent): replace sandbox with an optional sidecar uri (allowlist-gated routing)#4836mmabrouk wants to merge 2 commits into
Conversation
dosubot
Bot
added
the
size:XL
|
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:
📝 WalkthroughWalkthroughAdds an optional ChangesSidecar URI routing change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
| harness: str = "pi_core" | ||
| sandbox: str = "local" | ||
| permission_policy: PermissionPolicy = "auto" | ||
| uri: Optional[str] = None # sidecar (runner) address; unset → env-var fallback |
There was a problem hiding this comment.
the uri should take the place of thr sandbox and the sandbox should be removed, as I always tell you we are still in the POC stage. We don't care about packward compatibikitz.
|
@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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ab3d478-bc89-4c56-8651-689403cdfcec
📒 Files selected for processing (6)
docs/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.md
mmabrouk
force-pushed
the
docs/sidecar-uri-config
branch
from
0f411b3 to
6ac6712
Compare
dosubot
Bot
added
size:XXL
mmabrouk
changed the base branch from
big-agents
to
feat/agent-config-structure-cleanup
dosubot
Bot
added
size:XL
mmabrouk
changed the title
|
Reused this design PR for the implementation (per queue-implement-feature). Changes mapped to scope: uri replaces sandbox (D7). Removed Routing. Allowlist gate, default-off (security). New Wire decision. FE. Dropped the sandbox Docs. agent-config-schema.md, agent-service-handler.md, service-to-agent-runner.md, running-the-agent.md (new env var), sidecar-uri-config project (DESIGN → IMPLEMENTED). Tests: SDK agents 344, service agent 45 (incl. full allowlist/precedence matrix), FE playground 121; ruff/prettier/eslint clean; golden untouched. Base set to #4840 (config-structure cleanup) so the diff is isolated. Not merged to big-agents. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Railway Preview Environment
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
services/oss/tests/pytest/unit/agent/test_default_agent_config.py (1)
42-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert raw
uriomission, not just parsedNone.These tests prove the defaults parse as
uri=None, but they do not lock the stronger contract this PR describes: the shipped default config omits theurikey entirely. If a later change starts emitting"uri": null, the current assertions still pass while the/inspectpayload shape changes. Please add absence checks for bothinspect_defaultandbuiltin_default.Also applies to: 64-79
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bf62cb2-d3f0-4f4e-9fa5-7dc11df4a2a9
📒 Files selected for processing (22)
docs/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/public-edge/agent-config-schema.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.mdsdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/utils/types.pysdks/python/oss/tests/pytest/unit/agents/test_dtos_agent_config.pyservices/oss/src/agent/app.pyservices/oss/src/agent/config.pyservices/oss/tests/pytest/unit/agent/test_default_agent_config.pyservices/oss/tests/pytest/unit/agent/test_select_backend.pyweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsxweb/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 skipped from review due to trivial changes (6)
- docs/design/agent-workflows/projects/sidecar-uri-config/context.md
- docs/design/agent-workflows/projects/sidecar-uri-config/status.md
- docs/design/agent-workflows/documentation/running-the-agent.md
- docs/design/agent-workflows/projects/sidecar-uri-config/README.md
- docs/design/agent-workflows/projects/sidecar-uri-config/research.md
- docs/design/agent-workflows/projects/sidecar-uri-config/plan.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (1)
services/oss/tests/pytest/unit/agent/test_default_agent_config.py (1)
42-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert raw
uriomission, not just parsedNone.These tests prove the defaults parse as
uri=None, but they do not lock the stronger contract this PR describes: the shipped default config omits theurikey entirely. If a later change starts emitting"uri": null, the current assertions still pass while the/inspectpayload shape changes. Please add absence checks for bothinspect_defaultandbuiltin_default.Also applies to: 64-79
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bf62cb2-d3f0-4f4e-9fa5-7dc11df4a2a9
📒 Files selected for processing (22)
docs/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/public-edge/agent-config-schema.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.mdsdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/utils/types.pysdks/python/oss/tests/pytest/unit/agents/test_dtos_agent_config.pyservices/oss/src/agent/app.pyservices/oss/src/agent/config.pyservices/oss/tests/pytest/unit/agent/test_default_agent_config.pyservices/oss/tests/pytest/unit/agent/test_select_backend.pyweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsxweb/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 skipped from review due to trivial changes (6)
- docs/design/agent-workflows/projects/sidecar-uri-config/context.md
- docs/design/agent-workflows/projects/sidecar-uri-config/status.md
- docs/design/agent-workflows/documentation/running-the-agent.md
- docs/design/agent-workflows/projects/sidecar-uri-config/README.md
- docs/design/agent-workflows/projects/sidecar-uri-config/research.md
- docs/design/agent-workflows/projects/sidecar-uri-config/plan.md
🛑 Comments failed to post (5)
docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md (1)
20-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Separate schema defaults from service-prefilled defaults in this table.
The
Defaultcolumn currently mixesAgentConfigSchemadefaults with/inspect-time service extras. In particular,skillsreads as though the schema itself defaults to one embedded skill, but the model default is[]and that skill is added only by the service-sidebuild_agent_v0_default(skill_slug=...). Clarifying that split here would keep the public contract page aligned with the actual schema/default builder behavior.sdks/python/agenta/sdk/agents/dtos.py (1)
960-963: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor an explicit
nullURI instead of falling back to defaults.
source.get("uri")makes “field omitted” and"uri": nullindistinguishable here. Ifdefaults.uriis set, a caller cannot clear that override:{agent: {uri: null}}still resolves to the default sidecar instead of the env/local fallback, which breaks the nullableuricontract.Suggested fix
- raw_uri = source.get("uri") - uri = _clean_uri(raw_uri) if raw_uri is not None else defaults.uri + if "uri" in source: + uri = _clean_uri(source.get("uri")) + else: + uri = defaults.uri📝 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.harness = str(source.get("harness") or defaults.harness).lower() if "uri" in source: uri = _clean_uri(source.get("uri")) else: uri = defaults.uri permission_policy = str(services/oss/src/agent/config.py (1)
103-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat blank
urioverrides as unset.A whitespace-only override currently goes down the rejection path because Line 110 checks truthiness before trimming. That makes an optional field fail loud instead of taking the documented env/local fallback.
Proposed fix
def resolve_runner_url(override: Optional[str]) -> Optional[str]: @@ - if override: + if override and override.strip(): return validate_runner_uri(override) return runner_url()📝 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.def resolve_runner_url(override: Optional[str]) -> Optional[str]: """Routing precedence: a validated request override, else the env var, else ``None``. ``override`` (the agent config's ``uri``) wins when set and allowlisted; a rejected override raises (``UnsupportedRunnerUriError``) rather than falling back. When ``override`` is unset this is exactly today's behavior: ``AGENTA_AGENT_RUNNER_URL`` if set, else ``None`` (the local runner CLI in ``AGENTA_AGENT_RUNNER_DIR``).""" if override and override.strip(): return validate_runner_uri(override)web/packages/agenta-playground/src/state/execution/agentRequest.ts (1)
177-193: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strip hidden
urioverrides before sending playground runs.The playground no longer surfaces
uri, but this helper still forwards any existingagent.uri/flaturiunchanged. Since the service now gives that field highest routing precedence, imported or previously saved configs can keep targeting a sidecar the author cannot see or clear.Proposed fix
const withAgentRunDefaults = (config: Record<string, unknown>): Record<string, unknown> => { const agent = config.agent if (agent && typeof agent === "object") { + const {uri: _uri, ...agentConfig} = agent as Record<string, unknown> return { ...config, - agent: {harness: "pi_core", ...(agent as Record<string, unknown>)}, + agent: {harness: "pi_core", ...agentConfig}, } } - return {harness: "pi_core", ...config} + const {uri: _uri, ...flatConfig} = config + return {harness: "pi_core", ...flatConfig} }📝 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./** * Default the agent run-selection field `harness` onto the AGENT CONFIG (`parameters.agent`), * not as a top-level params sibling. It is part of one `AgentConfig` now, so it belongs inside * the `agent` block. A value the resolved config already carries always wins; the schema nests * the config under `agent`, but a flat config (no `agent` key) is still defaulted at the top * level so a non-schema config keeps working. The sidecar `uri` is left unset (the server's * env-var routing fallback); it is an operator override, not a per-run default. */ const withAgentRunDefaults = (config: Record<string, unknown>): Record<string, unknown> => { const agent = config.agent if (agent && typeof agent === "object") { const {uri: _uri, ...agentConfig} = agent as Record<string, unknown> return { ...config, agent: {harness: "pi_core", ...agentConfig}, } } const {uri: _uri, ...flatConfig} = config return {harness: "pi_core", ...flatConfig} }web/packages/agenta-playground/src/state/execution/selectors.ts (1)
1288-1294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Legacy agent-mode fallback misses the nested
agent.harnessshape.This PR now defaults
harnessinside the agent block, but the last-resort heuristic still only checksconfig.harness. WhenworkflowTypeand the schema marker are both unavailable, nested agent configs will still be classified as non-agent.Proposed fix
const config = get(workflowMolecule.selectors.configuration(entityId)) as | Record<string, unknown> | null | undefined - return Boolean(config?.harness) + const agent = + config?.agent && typeof config.agent === "object" + ? (config.agent as Record<string, unknown>) + : null + const agentConfig = + config?.agent_config && typeof config.agent_config === "object" + ? (config.agent_config as Record<string, unknown>) + : null + return Boolean(config?.harness ?? agent?.harness ?? agentConfig?.harness) }), )📝 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.// Legacy heuristic — a stored config carrying top-level harness. // Kept as a last resort; delete once WP-6 is the sole source of truth. const config = get(workflowMolecule.selectors.configuration(entityId)) as | Record<string, unknown> | null | undefined const agent = config?.agent && typeof config.agent === "object" ? (config.agent as Record<string, unknown>) : null const agentConfig = config?.agent_config && typeof config.agent_config === "object" ? (config.agent_config as Record<string, unknown>) : null return Boolean(config?.harness ?? agent?.harness ?? agentConfig?.harness)
mmabrouk
force-pushed
the
docs/sidecar-uri-config
branch
from
6ac6712 to
a168741
Compare
mmabrouk
force-pushed
the
feat/agent-config-structure-cleanup
branch
from
8aa3c90 to
372b7a8
Compare
mmabrouk
force-pushed
the
docs/sidecar-uri-config
branch
from
a168741 to
cf0fa39
Compare
mmabrouk
force-pushed
the
feat/agent-config-structure-cleanup
branch
from
372b7a8 to
2610078
Compare
mmabrouk
force-pushed
the
docs/sidecar-uri-config
branch
from
cf0fa39 to
9322d03
Compare
|
CodeRabbit's review found no substantive code change for this PR. The 2 inline comments are both on the |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Okay, one thing I think that was not very well communicated: the URI should not only be a URL. The URI should be an identifier that allows it to see both, in our case:
- the URL for the sandbox, which is a Docker container or whatever
- which sandbox it's using, basically local or Daytona
Basically, the front end would show local and Daytona as options, especially right now in development. It will then build the URI that would include local and Daytona. In this case, it's totally right for the front end to hardcode the URL part. We don't need to send it to the front end for the moment. This is just for development.
In reality, this whole URI would be an environment variable. The idea is that that environment variable would include both the sandbox URL and which mode it uses. Obviously, from this URI, the service from this sidecar can be used. When you talk to the site, can you use a run.sh to provide this? I'd say yes, why not? It looks not the best solution, but it's an okay solution.
mmabrouk
force-pushed
the
feat/agent-config-structure-cleanup
branch
from
2610078 to
cec8c43
Compare
mmabrouk
force-pushed
the
docs/sidecar-uri-config
branch
from
9322d03 to
f444322
Compare
Design workspace for an optional `uri` on the agent run config that names the sidecar (agent runner) address. When set, the service routes `/run` there; when unset, it falls back to AGENTA_AGENT_RUNNER_URL / the local runner. Spun out of PR #4821 review comment 3469613625. Design only, no code. Key decisions: `uri` is a RunSelection field (where a run goes, like `sandbox`), not the neutral AgentConfig, and not a `/run` wire field (consumed service-side in select_backend; golden fixtures untouched). A caller-supplied address is gated by a server-side allowlist, default-off, because the service ships resolved secrets + bearer tokens to whatever URL it picks. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
Replace the per-run sandbox selector with an optional sidecar uri that routes the /run request, gated server-side by an allowlist (default empty = off). Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
mmabrouk
force-pushed
the
feat/agent-config-structure-cleanup
branch
from
cec8c43 to
3f051c8
Compare
mmabrouk
force-pushed
the
docs/sidecar-uri-config
branch
from
f444322 to
66ef5ba
Compare
mmabrouk
changed the base branch from
feat/agent-config-structure-cleanup
to
big-agents
|
Closing per JP (CTO): the sidecar-URI approach is not the right direction. The sandbox (local/daytona) interface stays as-is; the playground builds any composite identifier it needs on top, rather than exposing a user-facing |
1 participant
Replaces the per-run
sandboxselector on the agent config with an optional sidecarurithat routes the/runrequest, gated server-side by an allowlist. Builds on #4840(config-structure cleanup), which moved the run-selection fields onto
AgentConfig.From PR #4821 review comment 3469613625:
the reviewer asked for an optional
uripointing at the sidecar that the service uses toroute, falling back to env vars when unset. The follow-up decision (D7) was that
urishould replace
sandboxoutright — the address drives routing, and each sidecar picksits own sandbox provider (local or Daytona) from its own environment.
What changed
urireplacessandbox.sandboxis removed fromAgentConfig,AgentConfigSchema,build_agent_v0_default, and the playground control. A new optionalurifield is thesidecar (agent runner) address.
Routing.
select_backendnow routes by the config'suri. Precedence:Unset
uri(the default) is exactly today's behavior. The sidecar is no longer told whichsandbox to use per run; it reads
SANDBOX_AGENT_PROVIDERfrom its own env.Security: an allowlist gate, default-off. A caller-supplied address is where the
service ships resolved provider keys and bearer tokens, so it is an SSRF / secret-
exfiltration risk. A
uriis honored only when its origin is onAGENTA_AGENT_RUNNER_URI_ALLOWLIST(comma-separated origins). Default empty means everyoverride is rejected — the feature ships off; only the env-var / local path works until an
operator opts in. Matching is on parsed origin (not substring), restricted to
http/https.A disallowed
urifails loud (UnsupportedRunnerUriError), never a silent fallback (whichwould let a caller probe the allowlist or mask a misconfiguration).
Playground. No
uricontrol is surfaced — it is an operator/testing routing override,not a normal authoring field. The sandbox selector is removed from the form.
The wire (golden unchanged)
uriis not a/runwire field: it is consumed entirely service-side inselect_backend, so the golden fixtures are untouched. The/runwire still carries aconstant
sandbox: "local"the unchanged runner defaults correctly on.Deferred (runner-branch follow-up): removing the wire-level
sandboxfield would forcea
protocol.ts/ golden / runner change, which lives on a separate stack branch(
services/agent/**). This PR keeps the golden unchanged and leaves that removal to therunner branch, per POC scope.
Files
agents/dtos.py(removesandbox, adduri,_clean_uri),utils/types.py(
AgentConfigSchema+build_agent_v0_default).agent/config.py(AGENTA_AGENT_RUNNER_URI_ALLOWLIST,validate_runner_uri,resolve_runner_url,UnsupportedRunnerUriError),agent/app.py(select_backendroutesby
uri).AgentConfigControl.tsx(drop sandbox selector),agentRequest.ts/selectors.ts(drop the
sandboxdefault + heuristic arm).test_dtos_agent_config.py; servicetest_select_backend.py(full allowlist +precedence matrix),
test_default_agent_config.py; FEagentMode.test.ts,agentRequest.test.ts. Wire-contract golden untouched.agent-config-schema.md,agent-service-handler.md,service-to-agent-runner.md,running-the-agent.md(new env var), and thesidecar-uri-configproject (DESIGN →IMPLEMENTED).
Tests
https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc