refactor(runner): canonical runner configuration names and typed config#5294
Conversation
dosubot
Bot
added
the
size:XL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
the
refactoring
📝 WalkthroughWalkthroughThe PR replaces local-only sandbox permission flags with validated ChangesSandbox provider migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
dosubot
Bot
added
size:XXL
|
🤖 Author note: Ready for review. This is the interface-freeze slice: one Tests: runner 1062 pass / 2 pre-existing transcript-replay failures (proven on HEAD, unrelated); api 15, services 12, sdk 11. One known-pending item, does not affect this review: the Helm |
One parse-and-validate boundary for the runner's operator-facing configuration, plus the final public AGENTA_RUNNER_* names everywhere the runner and API read them. New services/runner/src/config/runner-config.ts: typed RunnerConfig (server, providers, daytona, callback) parsed and validated once before the HTTP server listens, with one redacted startup summary. Every provider-selection and Daytona read now flows through the typed object instead of scattered process.env reads. Rename map (old -> new): SANDBOX_AGENT_PROVIDER -> AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER (+ AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS) SANDBOX_AGENT_LOG_LEVEL -> AGENTA_RUNNER_LOG_LEVEL DAYTONA_API_KEY/API_URL/TARGET/SNAPSHOT/IMAGE (runner) -> AGENTA_RUNNER_DAYTONA_* DAYTONA_AUTOSTOP/AUTODELETE -> AGENTA_RUNNER_DAYTONA_AUTOSTOP_MINUTES/_AUTODELETE_MINUTES Removed (no aliases): DAYTONA_SNAPSHOT_AGENT, AGENTA_AGENT_SANDBOX_PI_INSTALLED (replaced by probe + pinned-install repair), AGENTA_SESSION_HARNESS_MOUNTS (transcript mounts derive from the session contract), AGENTA_SANDBOX_LOCAL_ALLOWED (replaced by the shared enabled/default provider pair the API/services/SDK read with the same rules). Validation: unset enabled = local; empty list invalid; unknown/duplicate ids invalid; default must be enabled; daytona enabled without a credential fails; snapshot+image mutually exclusive; invalid lifecycle values fail; empty strings become absent at the parse boundary. The code evaluator's bare DAYTONA_* namespace is deliberately untouched. Claude-Session: https://claude.ai/code/session_01XhENr63WL9npkKrJGnzDc1
The API, OSS agent service, SDK, and web now read AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS / AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER with the same parsing rules as the runner, replacing the removed AGENTA_SANDBOX_LOCAL_ALLOWED gate. - api/oss/src/utils/env.py: RunnerConfig exposes enabled_sandbox_providers + default_sandbox_provider, validated with the runner's rules (shared semantics, reimplemented in Python). - Producer-side gates (services/oss select_backend, SDK handler) refuse any sandbox not in the enabled set; the runner stays the final authority. - web/entrypoint.sh derives NEXT_PUBLIC_AGENTA_SANDBOX_LOCAL_ENABLED from the enabled-provider list. - Tests: API parser unit cases mirroring the runner's; services + SDK gate cases updated. Claude-Session: https://claude.ai/code/session_01XhENr63WL9npkKrJGnzDc1
All hosting surfaces now set the runner from the canonical AGENTA_RUNNER_* names, with one operator-facing entry feeding both the api and runner services.
- Compose (7 files): runner blocks carry the enabled/default provider pair (default local) and AGENTA_RUNNER_DAYTONA_* (AUTOSTOP/AUTODELETE in minutes); DAYTONA_SNAPSHOT_AGENT and AGENTA_AGENT_SANDBOX_PI_INSTALLED removed.
- Env examples (4 files): AGENTA_SANDBOX_LOCAL_ALLOWED replaced by the provider pair; the shared daytona block split into a code-evaluator group (bare DAYTONA_*) and an agent-runner group (AGENTA_RUNNER_DAYTONA_*).
- Helm: agentRunner.providers.{enabled,default,daytona.*} per the design; the runner deployment renders the enabled list comma-joined and reads the Daytona key from providers.daytona.apiKeySecretRef; commonEnv exposes the provider pair to api/web/services and drops sandboxLocalAllowed. The code-evaluator daytona block stays bare and untouched.
- Railway: runner set_vars/set_optional_vars use the new names, and the pre-existing bug where unset_vars deleted the runner's DAYTONA_* credentials right after setting them is fixed by removing those names from the unset list.
Claude-Session: https://claude.ai/code/session_01XhENr63WL9npkKrJGnzDc1
The snapshot recipe installs pi globally, but the probe checked only the pinned .agenta-pi path, so the recipe's own snapshot would have paid a session-time reinstall on every run. The repair ladder now links a PATH-baked pi to the pinned path before falling back to the pinned install. Snapshot recipe docs updated to the canonical names. Claude-Session: https://claude.ai/code/session_01XhENr63WL9npkKrJGnzDc1
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
services/runner/src/engines/sandbox_agent.ts (1)
1-1: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCentralize "effective sandbox provider" resolution; verify
??vs||don't disagree on empty-string input.Five sites independently resolve "which provider does this request use, given
request.sandboxand the configured default," with two different fallback operators and an unverified single-argument call path:
services/runner/src/engines/sandbox_agent.ts#L713-719: uses(request.sandbox ?? loadRunnerConfig().providers.default) === "daytona"for the durable-cwd path decision. Align this operator withrun-plan.ts's resolution (or extract a shared helper both call), so an empty-stringrequest.sandboxcan't make this branch disagree withplan.isDaytona.services/runner/src/engines/sandbox_agent.ts#L662-668: calls single-argresolvesToLocalProvider(request.sandbox)to gate the local multi-runner ownership check; confirm this still falls back toloadRunnerConfig().providers.default(not a hardcoded"local") whenrequest.sandboxis omitted.services/runner/src/server.ts#L301-306: same single-argresolvesToLocalProvider(request.sandbox)call, plus its own??-based fallback for the daytona branch — same two concerns apply here.services/runner/src/engines/sandbox_agent/run-plan.ts#L258-286: usesrequest.sandbox || defaultProvider || "local"(falsy-based||), the one authoritative resolutionplan.isDaytonais derived from; the other sites should match this exactly rather than reimplement it.services/runner/tests/unit/session-pool.test.ts#L30-44: add a test case for the single-argument call pattern (resolvesToLocalProvider(request.sandbox), no second arg) to lock in the behavior the unchanged production call sites actually rely on.Since
resolvesToLocalProvider's implementation (likely insession-pool.ts) isn't in the provided files, please verify its default-argument behavior directly.♻️ Suggested direction: single shared resolver
// services/runner/src/config/runner-config.ts (or a small shared helper) export function resolveEffectiveProvider( requestSandbox: string | undefined, config: RunnerConfig = loadRunnerConfig(), ): SandboxProviderId { return (requestSandbox?.trim() || config.providers.default) as SandboxProviderId; }Then replace the
??/||/resolvesToLocalProvider(x)call sites above with this single function so all decisions agree.services/runner/tests/unit/sandbox-agent-orchestration.test.ts (1)
26-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "enable Daytona for tests"
beforeEachinto a shared helper.The same 4-line block (
AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS="local,daytona"+AGENTA_RUNNER_DAYTONA_API_KEY="test-key"+resetRunnerConfigCache()) is duplicated verbatim across three suites. A shared helper (e.g. exported fromhermetic-env.ts) would keep this in one place if the env names or reset semantics change again.
services/runner/tests/unit/sandbox-agent-orchestration.test.ts#L26-L34: replace the inlinebeforeEachbody with a call to the shared helper.services/runner/tests/unit/sandbox-agent-run-plan.test.ts#L16-L27: replace the inlinebeforeEachbody with a call to the shared helper.services/runner/tests/unit/sandbox-lifecycle.test.ts#L16-L24: replace the inlinebeforeEachbody with a call to the shared helper.♻️ Proposed shared helper
// services/runner/tests/setup/hermetic-env.ts (or a new tests/setup/daytona-env.ts) export function enableDaytonaForTests(): void { process.env.AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS = "local,daytona"; process.env.AGENTA_RUNNER_DAYTONA_API_KEY = "test-key"; resetRunnerConfigCache(); }-beforeEach(() => { - process.env.AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS = "local,daytona"; - process.env.AGENTA_RUNNER_DAYTONA_API_KEY = "test-key"; - resetRunnerConfigCache(); -}); +beforeEach(enableDaytonaForTests);hosting/kubernetes/helm/templates/runner-deployment.yaml (1)
53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProvider-registry env vars computed twice from the same Values path. Both sites independently derive
AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS/AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDERfrom.Values.agentRunner.providerswith duplicated defaulting logic (default (list "local")/default "local"), instead of the runner deployment reusing the helper that_helpers.tplexplicitly documents as the shared, "one operator entry" definition. Values match today, but any future change to one copy's defaulting/validation rule risks silent drift between what api/web/services observe as "enabled providers" and what the runner itself enforces.
hosting/kubernetes/helm/templates/runner-deployment.yaml#L53-L56: replace the local$enabledProviders/$providers.defaultcomputation with a call to a shared named template (e.g. extract_helpers.tpllines 1100-1105 intoagenta.agentRunner.providers.enabledEnv/.defaultEnvhelpers) so both files stay in sync.hosting/kubernetes/helm/templates/_helpers.tpl#L1100-L1105: extract this block into a reusable named template that both this shared-services block andrunner-deployment.yamlcall, instead of each computing the values inline.hosting/railway/oss/scripts/configure.sh (1)
305-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale deprecated
SANDBOX_AGENT_PROVIDER(and any prior legacy runner vars) not cleaned up.The old
SANDBOX_AGENT_PROVIDERdefault was removed fromset_vars runnerhere, but it's absent fromunset_vars runner(Line 326), so pre-existing Railway environments keep this now-unread variable indefinitely. Consider adding it (and any other renamed/removed runner variable) to theunset_vars runnercall for cleanup, now that it's excluded from the Daytona-credential caveat.♻️ Proposed cleanup
- unset_vars runner AGENTA_LICENSE SCRIPT_NAME REDIS_URI REDIS_URI_VOLATILE REDIS_URI_DURABLE SUPERTOKENS_CONNECTION_URI POSTGRES_URI_CORE POSTGRES_URI_TRACING POSTGRES_URI_SUPERTOKENS + unset_vars runner AGENTA_LICENSE SCRIPT_NAME REDIS_URI REDIS_URI_VOLATILE REDIS_URI_DURABLE SUPERTOKENS_CONNECTION_URI POSTGRES_URI_CORE POSTGRES_URI_TRACING POSTGRES_URI_SUPERTOKENS SANDBOX_AGENT_PROVIDERAlso applies to: 324-326
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa106185-9bd2-4630-8407-e284ee25caf2
📒 Files selected for processing (44)
api/oss/src/utils/env.pyapi/oss/tests/pytest/unit/utils/test_env_runner_config.pyhosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/ee/docker-compose.gh.local.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/ee/env.ee.dev.examplehosting/docker-compose/ee/env.ee.gh.examplehosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.local.ymlhosting/docker-compose/oss/docker-compose.gh.ssl.ymlhosting/docker-compose/oss/docker-compose.gh.ymlhosting/docker-compose/oss/env.oss.dev.examplehosting/docker-compose/oss/env.oss.gh.examplehosting/kubernetes/helm/templates/_helpers.tplhosting/kubernetes/helm/templates/runner-deployment.yamlhosting/railway/oss/scripts/configure.shsdks/python/agenta/sdk/agents/errors.pysdks/python/agenta/sdk/agents/handler.pysdks/python/agenta/sdk/agents/sandbox_providers.pysdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.pyservices/oss/src/agent/app.pyservices/oss/src/agent/config.pyservices/oss/tests/pytest/unit/agent/test_select_backend.pyservices/runner/sandbox-images/daytona/README.mdservices/runner/sandbox-images/daytona/build_snapshot.pyservices/runner/src/config/runner-config.tsservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/daemon.tsservices/runner/src/engines/sandbox_agent/daytona-provider.tsservices/runner/src/engines/sandbox_agent/daytona.tsservices/runner/src/engines/sandbox_agent/provider.tsservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/src/engines/sandbox_agent/session-pool.tsservices/runner/src/server.tsservices/runner/tests/setup/hermetic-env.tsservices/runner/tests/unit/runner-config.test.tsservices/runner/tests/unit/sandbox-agent-daemon.test.tsservices/runner/tests/unit/sandbox-agent-daytona.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-agent-provider.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsservices/runner/tests/unit/sandbox-lifecycle.test.tsservices/runner/tests/unit/session-pool.test.tsweb/entrypoint.sh
| if provider_id not in _KNOWN_SANDBOX_PROVIDERS: | ||
| raise ValueError( | ||
| f"AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS lists unknown provider " | ||
| f"'{provider_id}'; known: {', '.join(_KNOWN_SANDBOX_PROVIDERS)}." | ||
| ) | ||
| if provider_id in seen: | ||
| raise ValueError( | ||
| f"AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS lists provider '{provider_id}' " | ||
| f"more than once." | ||
| ) | ||
| seen.add(provider_id) | ||
| return ids | ||
|
|
||
|
|
||
| def _parse_default_sandbox_provider(raw: Optional[str], enabled: List[str]) -> str: | ||
| """Parse ``AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER``; unset -> ``local``; must be enabled.""" | ||
| value = (raw or "").strip().lower() or "local" | ||
| if value not in _KNOWN_SANDBOX_PROVIDERS: | ||
| raise ValueError( | ||
| f"AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER is unknown provider '{value}'; " | ||
| f"known: {', '.join(_KNOWN_SANDBOX_PROVIDERS)}." | ||
| ) | ||
| if value not in enabled: | ||
| raise ValueError( | ||
| f"AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER '{value}' is not in the enabled set " | ||
| f"[{', '.join(enabled)}]." | ||
| ) | ||
| return value |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error text has drifted from the SDK/runner's "in-lockstep" implementations.
The docstring above (lines 981-984) states this reimplements the runner's parser "so all three readers agree," but two messages have drifted from the SDK's sandbox_providers.py and the runner's runner-config.ts (both in this cohort/context): the unknown-provider message says "known:" here vs. "known providers:" there, and the default-not-in-enabled-set message here drops the "Add it to AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS or change the default." remediation sentence present in both other implementations. Operators debugging via the API get a less actionable error than via the runner.
🐛 Proposed fix to re-sync message text
if provider_id not in _KNOWN_SANDBOX_PROVIDERS:
raise ValueError(
f"AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS lists unknown provider "
- f"'{provider_id}'; known: {', '.join(_KNOWN_SANDBOX_PROVIDERS)}."
+ f"'{provider_id}'; known providers: {', '.join(_KNOWN_SANDBOX_PROVIDERS)}."
) if value not in enabled:
raise ValueError(
f"AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER '{value}' is not in the enabled set "
- f"[{', '.join(enabled)}]."
+ f"[{', '.join(enabled)}]. Add it to AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS "
+ f"or change the default."
)📝 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.
| if provider_id not in _KNOWN_SANDBOX_PROVIDERS: | |
| raise ValueError( | |
| f"AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS lists unknown provider " | |
| f"'{provider_id}'; known: {', '.join(_KNOWN_SANDBOX_PROVIDERS)}." | |
| ) | |
| if provider_id in seen: | |
| raise ValueError( | |
| f"AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS lists provider '{provider_id}' " | |
| f"more than once." | |
| ) | |
| seen.add(provider_id) | |
| return ids | |
| def _parse_default_sandbox_provider(raw: Optional[str], enabled: List[str]) -> str: | |
| """Parse ``AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER``; unset -> ``local``; must be enabled.""" | |
| value = (raw or "").strip().lower() or "local" | |
| if value not in _KNOWN_SANDBOX_PROVIDERS: | |
| raise ValueError( | |
| f"AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER is unknown provider '{value}'; " | |
| f"known: {', '.join(_KNOWN_SANDBOX_PROVIDERS)}." | |
| ) | |
| if value not in enabled: | |
| raise ValueError( | |
| f"AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER '{value}' is not in the enabled set " | |
| f"[{', '.join(enabled)}]." | |
| ) | |
| return value | |
| if provider_id not in _KNOWN_SANDBOX_PROVIDERS: | |
| raise ValueError( | |
| f"AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS lists unknown provider " | |
| f"'{provider_id}'; known providers: {', '.join(_KNOWN_SANDBOX_PROVIDERS)}." | |
| ) | |
| if provider_id in seen: | |
| raise ValueError( | |
| f"AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS lists provider '{provider_id}' " | |
| f"more than once." | |
| ) | |
| seen.add(provider_id) | |
| return ids | |
| def _parse_default_sandbox_provider(raw: Optional[str], enabled: List[str]) -> str: | |
| """Parse ``AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER``; unset -> ``local``; must be enabled.""" | |
| value = (raw or "").strip().lower() or "local" | |
| if value not in _KNOWN_SANDBOX_PROVIDERS: | |
| raise ValueError( | |
| f"AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER is unknown provider '{value}'; " | |
| f"known providers: {', '.join(_KNOWN_SANDBOX_PROVIDERS)}." | |
| ) | |
| if value not in enabled: | |
| raise ValueError( | |
| f"AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER '{value}' is not in the enabled set " | |
| f"[{', '.join(enabled)}]. Add it to AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS " | |
| f"or change the default." | |
| ) | |
| return value |
| enabled_sandbox_providers: List[str] = Field( | ||
| default_factory=_enabled_sandbox_providers_default | ||
| ) | ||
| default_sandbox_provider: str = Field( | ||
| default_factory=_default_sandbox_provider_default | ||
| ) | ||
|
|
||
| model_config = ConfigDict(extra="ignore") | ||
|
|
||
| @model_validator(mode="after") | ||
| def _warn_local_sandbox(self) -> "RunnerConfig": | ||
| global _SANDBOX_LOCAL_WARNED | ||
| if self.sandbox_local_allowed and not _SANDBOX_LOCAL_WARNED: | ||
| if "local" in self.enabled_sandbox_providers and not _SANDBOX_LOCAL_WARNED: | ||
| _SANDBOX_LOCAL_WARNED = True | ||
| warnings.warn( | ||
| "AGENTA_SANDBOX_LOCAL_ALLOWED is on (default): local sandbox is not a " | ||
| "tenant boundary. Set it to false to harden a shared/multi-tenant deployment.", | ||
| "local sandbox is enabled (default): it is unconfined host bash, not a " | ||
| "tenant boundary. Set AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS=daytona to " | ||
| "harden a shared/multi-tenant deployment.", | ||
| stacklevel=2, | ||
| ) | ||
| return self |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers and nearby context.
sed -n '980,1125p' api/oss/src/utils/env.py
# Find parseRunnerConfig and related helpers in the same file.
rg -n "parseRunnerConfig|_enabled_sandbox_providers_default|_default_sandbox_provider_default|RunnerConfig|field_validator|model_validator" api/oss/src/utils/env.pyRepository: Agenta-AI/agenta
Length of output: 7137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== env.py relevant excerpt =="
sed -n '980,1125p' api/oss/src/utils/env.py
echo
echo "== search for RunnerConfig usages =="
rg -n "RunnerConfig\(" -S .
echo
echo "== search for parseRunnerConfig definition/usages =="
rg -n "parseRunnerConfig" -S api/oss/src/utils/env.py .Repository: Agenta-AI/agenta
Length of output: 9305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from typing import List, Optional
from pydantic import BaseModel, Field, model_validator
KNOWN = {"local", "daytona"}
def parse_enabled(raw: Optional[str]) -> List[str]:
if raw is None:
return ["local"]
return [x.strip().lower() for x in raw.split(",")]
def parse_default(raw: Optional[str], enabled: List[str]) -> str:
value = (raw or "").strip().lower() or "local"
if value not in enabled:
raise ValueError(f"{value} not enabled")
return value
def enabled_default() -> List[str]:
return parse_enabled(None)
def default_default() -> str:
return parse_default(None, enabled_default())
class M(BaseModel):
enabled_sandbox_providers: List[str] = Field(default_factory=enabled_default)
default_sandbox_provider: str = Field(default_factory=default_default)
`@model_validator`(mode="after")
def warn(self):
return self
print("M() ->", M())
print("M(enabled_sandbox_providers=['daytona'], default_sandbox_provider='daytona') ->",
M(enabled_sandbox_providers=['daytona'], default_sandbox_provider='daytona'))
try:
print(M(enabled_sandbox_providers=['not-a-real-provider'], default_sandbox_provider='local'))
except Exception as e:
print(type(e).__name__, e)
try:
print(M(enabled_sandbox_providers=['daytona'], default_sandbox_provider='local'))
except Exception as e:
print(type(e).__name__, e)
PYRepository: Agenta-AI/agenta
Length of output: 537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from typing import List, Optional
from pydantic import BaseModel, Field, model_validator
def parse_enabled(raw: Optional[str]) -> List[str]:
if raw is None:
return ["local"]
return [x.strip().lower() for x in raw.split(",")]
def parse_default(raw: Optional[str], enabled: List[str]) -> str:
value = (raw or "").strip().lower() or "local"
if value not in enabled:
raise ValueError(f"{value} not enabled")
return value
def enabled_default() -> List[str]:
return parse_enabled(None)
def default_default() -> str:
return parse_default(None, enabled_default())
class M(BaseModel):
enabled_sandbox_providers: List[str] = Field(default_factory=enabled_default)
default_sandbox_provider: str = Field(default_factory=default_default)
`@model_validator`(mode="after")
def warn(self):
return self
print("M() ->", M())
print("M(enabled_sandbox_providers=['daytona'], default_sandbox_provider='daytona') ->",
M(enabled_sandbox_providers=['daytona'], default_sandbox_provider='daytona'))
for kwargs in [
dict(enabled_sandbox_providers=['not-a-real-provider'], default_sandbox_provider='local'),
dict(enabled_sandbox_providers=['daytona'], default_sandbox_provider='local'),
]:
try:
print("TRY", kwargs, "=>", M(**kwargs))
except Exception as e:
print("ERR", kwargs, "=>", type(e).__name__, e)
PYRepository: Agenta-AI/agenta
Length of output: 723
Move the sandbox-provider checks onto the model itself RunnerConfig(enabled_sandbox_providers=...) currently accepts invalid values because the parsing/consistency logic only runs through default_factory. Put the provider parsing and the enabled/default subset check in a field_validator or model_validator(mode="after") so the invariant holds for every construction path.
| {{- $providers := default dict $runner.providers -}} | ||
| {{- $enabledProviders := default (list "local") $providers.enabled -}} | ||
| {{- $daytona := default dict $providers.daytona -}} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify values.yaml uses the new agentRunner.providers.daytona path (and not the old agentRunner.daytona)
fd -a values.yaml hosting/kubernetes/helm --exec cat -n {}
rg -n 'agentRunner' hosting/kubernetes/helm/values.yaml -A3 -B1Repository: Agenta-AI/agenta
Length of output: 8074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the chart files around agentRunner and schema/docs references.
fd -a 'values.schema.json|README.md|*.md|*.yaml' hosting/kubernetes/helm | sed -n '1,200p'
echo '--- values.schema.json ---'
fd -a values.schema.json hosting/kubernetes/helm --exec sh -c 'echo "### {}"; rg -n "agentRunner|daytona|providers" "{}" -A4 -B2' \;
echo '--- docs / upgrade notes mentioning daytona or agentRunner ---'
rg -n 'agentRunner\.daytona|agentRunner\.providers\.daytona|daytona|upgrade|migration' hosting/kubernetes/helm -g '*.md' -g '*.yaml' -g '*.yml' -A2 -B2Repository: Agenta-AI/agenta
Length of output: 503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- chart file list ---'
fd -a --type f --hidden --glob 'values.schema.json' --glob '*.md' --glob '*.yaml' --glob '*.yml' hosting/kubernetes/helm | sed -n '1,200p'
echo '--- values.schema.json ---'
fd -a --type f --hidden --glob 'values.schema.json' hosting/kubernetes/helm -x sh -c '
echo "### {}"
rg -n "agentRunner|daytona|providers" "{}" -A6 -B3
'
echo '--- markdown/yaml references ---'
rg -n "agentRunner\.daytona|agentRunner\.providers\.daytona|daytona|provider[s]?" hosting/kubernetes/helm -g '*.md' -g '*.yaml' -g '*.yml' -A2 -B2Repository: Agenta-AI/agenta
Length of output: 13711
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any migration/upgrade note that mentions the agentRunner daytona path change.
rg -n 'agentRunner\.providers\.daytona|agentRunner\.daytona|providers\.daytona|daytona' \
. \
-g '*.md' -g '*.yaml' -g '*.yml' -g 'Chart.yaml' \
-A2 -B2Repository: Agenta-AI/agenta
Length of output: 50375
Keep the Daytona values path consistent
runner-deployment.yaml now reads agentRunner.providers.daytona, but values.yaml and values.schema.json still expose agentRunner.daytona. Existing overrides at the old path will be ignored on upgrade; update the chart defaults/schema (and upgrade notes) together, or add a compatibility shim.
| def _default_select_backend(agent_template: AgentTemplate) -> Backend: | ||
| """Env-driven default: `AGENTA_RUNNER_INTERNAL_URL` picks HTTP transport; else local cwd. | ||
|
|
||
| `local` (unconfined host bash, not a tenant boundary) is refused unless | ||
| `AGENTA_SANDBOX_LOCAL_ALLOWED` is on — a bare `agent_v0` gets this protocol-level | ||
| safety behavior for free, same as capability gating and MCP gating above. | ||
| A sandbox the deployment has not enabled is refused before any run — a bare `agent_v0` | ||
| gets this protocol-level safety behavior for free, same as capability gating and MCP | ||
| gating above. The runner remains the final authority; this is a pre-filter that reads the | ||
| same `AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS` registry. | ||
| """ | ||
| if agent_template.sandbox == "local" and not _sandbox_local_allowed(): | ||
| raise LocalSandboxNotAllowedError() | ||
| if not sandbox_provider_enabled(agent_template.sandbox): | ||
| raise LocalSandboxNotAllowedError(sandbox=agent_template.sandbox) | ||
| url = os.getenv("AGENTA_RUNNER_INTERNAL_URL", "").strip() or None | ||
| return SandboxAgentBackend(sandbox=agent_template.sandbox, url=url, cwd=os.getcwd()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether sandbox_provider_enabled is defined/imported anywhere relevant.
rg -nP '\bsandbox_provider_enabled\b' sdks/python/agenta -C3Repository: Agenta-AI/agenta
Length of output: 1455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' sdks/python/agenta/sdk/agents/handler.pyRepository: Agenta-AI/agenta
Length of output: 5082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "sandbox_provider_enabled|from agenta\.sdk\.agents\.sandbox_providers import|import .*sandbox_providers" sdks/python/agenta/sdk/agents/handler.py sdks/python/agenta/sdk/agents -C2Repository: Agenta-AI/agenta
Length of output: 1621
Missing import for sandbox_provider_enabled
_default_select_backend() calls sandbox_provider_enabled(...), but sdks/python/agenta/sdk/agents/handler.py doesn’t import it. That raises NameError whenever the default backend is selected.
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 86-86: undefined name 'sandbox_provider_enabled'
(F821)
🪛 GitHub Actions: 11 - check code styling / 1_Python lint.txt
[error] 86-87: ruff (F821) Undefined name sandbox_provider_enabled
🪛 GitHub Actions: 11 - check code styling / Python lint
[error] 86-86: ruff (F821) Undefined name sandbox_provider_enabled.
Sources: Coding guidelines, Linters/SAST tools
65fefb8 to
90f26c7
Compare
8e15fab to
d57bd38
Compare
dosubot
Bot
added
size:XL
|
Landed the Helm Reconciled against the now-merged MCP contract base (#5290, base
Validation: |
The runner gate rename referenced sandbox_provider_enabled without importing it (F821), crashing the sandbox pre-filter and failing SDK lint + unit tests. Claude-Session: https://claude.ai/code/session_01XhENr63WL9npkKrJGnzDc1
1 participant
What an operator sees
One
AGENTA_RUNNER_*namespace for the agent runner, parsed and validated once before the HTTP server accepts a request. A bad configuration fails startup with a clear message instead of silently misbehaving mid-run, and the runner logs one redacted summary of what it resolved:This is the interface-freeze slice: the public names cannot change after release, so it lands first.
Rename map (old -> new)
SANDBOX_AGENT_PROVIDERAGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER(+ newAGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS)SANDBOX_AGENT_LOG_LEVELAGENTA_RUNNER_LOG_LEVELDAYTONA_API_KEY(runner)AGENTA_RUNNER_DAYTONA_API_KEYDAYTONA_API_URL(runner)AGENTA_RUNNER_DAYTONA_API_URLDAYTONA_TARGET(runner)AGENTA_RUNNER_DAYTONA_TARGETDAYTONA_SNAPSHOT(runner)AGENTA_RUNNER_DAYTONA_SNAPSHOTDAYTONA_IMAGE(runner)AGENTA_RUNNER_DAYTONA_IMAGEDAYTONA_AUTOSTOP/DAYTONA_AUTODELETEAGENTA_RUNNER_DAYTONA_AUTOSTOP_MINUTES/_AUTODELETE_MINUTESThe code evaluator's bare
DAYTONA_*namespace (api/services and its Helm/Railway wiring) is deliberately untouched -- it is a separate consumer and gets its own namespace in a later slice.Removed (no aliases, no fallback)
DAYTONA_SNAPSHOT_AGENT-- the runner uses its pinned default snapshot.AGENTA_AGENT_SANDBOX_PI_INSTALLED-- replaced by a probe: the runner checks for the pinned Pi executable in the sandbox and installs it if a custom image lacks it, failing the run loudly if the install fails.AGENTA_SESSION_HARNESS_MOUNTS-- transcript mounts derive from the session contract.AGENTA_SANDBOX_LOCAL_ALLOWED-- replaced by the shared enabled/default provider pair below.One shared value for API and web
The API, OSS agent service, SDK, and web read the same
AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS/AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDERpair the runner reads, with the same parsing rules (re-implemented per language, unit-tested to agree on every input). The producer-side gate refuses a sandbox the deployment has not enabled before any run; the runner remains the final authority, so even a drifted API produces an honest "provider not enabled on this deployment" failure instead of a wrong run.Validation (fails startup)
Unset enabled =
local; an explicitly empty list is invalid; unknown and duplicate ids are invalid; the default must be enabled; Daytona enabled without a provisioning credential fails; snapshot and image are mutually exclusive; invalid lifecycle minutes (zero, negative, non-numeric, fractional) fail; empty strings become absent at the parse boundary (so a Compose${VAR:-}never becomes an invalid empty value). The runner constructs its Daytona client explicitly from the typed config and bridges the credential into the ambient name the vendored SDK reads; the daemon force-blanks both names from every harness environment so user code never sees them.Test coverage
pnpm testgreen except two pre-existing transcript-replay failures unrelated to this change.pnpm run typecheckclean.Hosting (Compose, Helm, Railway) is updated in this same lane.
https://claude.ai/code/session_01XhENr63WL9npkKrJGnzDc1