-
Notifications
You must be signed in to change notification settings - Fork 574
refactor(runner): canonical runner configuration names and typed config #5294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
59b47b0
3c70efb
4d7b287
b26c3c4
d57bd38
06e1b82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| import os | ||
| import hashlib | ||
| import warnings | ||
| from typing import List, Optional | ||
| from uuid import getnode | ||
| from json import loads | ||
| from urllib.parse import urlparse, quote_plus | ||
|
|
||
| from pydantic import BaseModel, ConfigDict, model_validator | ||
| from pydantic import BaseModel, ConfigDict, Field, model_validator | ||
|
|
||
|
|
||
| _TRUTHY = {"true", "1", "t", "y", "yes", "on", "enable", "enabled"} | ||
|
|
@@ -969,28 +970,103 @@ def enabled(self) -> bool: | |
|
|
||
| _SANDBOX_LOCAL_WARNED = False | ||
|
|
||
| # Sandbox providers this runner can provision. Kept in lockstep with the runner's | ||
| # KNOWN_SANDBOX_PROVIDER_IDS and the SDK's KNOWN_SANDBOX_PROVIDERS so all three readers agree. | ||
| _KNOWN_SANDBOX_PROVIDERS = ("local", "daytona") | ||
|
|
||
|
|
||
| def _parse_enabled_sandbox_providers(raw: Optional[str]) -> List[str]: | ||
| """Parse ``AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS`` with the runner's rules. | ||
|
|
||
| Unset -> ``["local"]``; explicit empty invalid; ids normalized lowercase; unknown and | ||
| duplicate ids invalid. Reimplements the TypeScript runner parser so the API's provider | ||
| pre-filter matches the runner's final authority (runner-selfhosting-cleanup/interface.md | ||
| sections 2 and 4). | ||
| """ | ||
| if raw is None: | ||
| return ["local"] | ||
| trimmed = raw.strip() | ||
| if trimmed == "": | ||
| raise ValueError( | ||
| "AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS is set but empty; unset it for the " | ||
| "default 'local', or list at least one provider." | ||
| ) | ||
| ids = [part.strip().lower() for part in trimmed.split(",")] | ||
| seen: set = set() | ||
| for provider_id in ids: | ||
| if provider_id == "": | ||
| raise ValueError( | ||
| f"AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS has an empty entry in '{trimmed}'." | ||
| ) | ||
| 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 | ||
|
|
||
|
|
||
| def _enabled_sandbox_providers_default() -> List[str]: | ||
| return _parse_enabled_sandbox_providers( | ||
| os.getenv("AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS") | ||
| ) | ||
|
|
||
|
|
||
| def _default_sandbox_provider_default() -> str: | ||
| return _parse_default_sandbox_provider( | ||
| os.getenv("AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER"), | ||
| _enabled_sandbox_providers_default(), | ||
| ) | ||
|
|
||
|
|
||
| class RunnerConfig(BaseModel): | ||
| """Agent runner (services/runner) sidecar configuration.""" | ||
|
|
||
| concurrency_limit: int = int(os.getenv("AGENTA_RUNNER_CONCURRENCY_LIMIT") or "1000") | ||
|
|
||
| # `local` sandbox runs unconfined host bash — not a tenant boundary; on by default | ||
| # for zero-config self-host. Canonical declaration; services/oss reads the same var directly. | ||
| sandbox_local_allowed: bool = ( | ||
| os.getenv("AGENTA_SANDBOX_LOCAL_ALLOWED") or "true" | ||
| ).lower() in _TRUTHY | ||
| # The sandbox-provider registry the runner and API share. `local` is unconfined host bash — | ||
| # not a tenant boundary; enabled by default for zero-config self-host. Canonical declaration; | ||
| # services/oss and the SDK read the same variables directly with the same rules. | ||
| 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 | ||
|
Comment on lines
+1052
to
1072
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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 |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 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.pyand the runner'srunner-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