[feat] Deliver agent credentials through Daytona Secrets#5277
[feat] Deliver agent credentials through Daytona Secrets#5277mmabrouk wants to merge 18 commits into
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:
✨ 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 |
dosubot
Bot
added
the
size:XL
| usage: Literal["opaque_http", "local_use"] | ||
|
|
||
|
|
||
| class WireModelConnection(_WireModel): |
There was a problem hiding this comment.
Review seam: modelConnection is deliberately the resolved, consumer-owned contract. Provider, deployment, endpoint, public environment, and typed credentials travel together; the author-facing connection selector stays on the SDK side and does not become runner policy. — Codex
| // Keep this runner-side boundary aligned with the resolver-owned contract in | ||
| // sdks/python/agenta/sdk/agents/connections/endpoints.py. Environment is public config; | ||
| // every other provider value must arrive as a typed credential. | ||
| const PUBLIC_MODEL_ENVIRONMENT_BINDINGS = new Set([ |
There was a problem hiding this comment.
Security seam: these explicit allowlists are the only values allowed to bypass opaque Secret substitution. When adding a provider, classify each field as public configuration, approved local_use, or opaque_http, and keep this list aligned with the Python resolver contract. — Codex
| ): Promise<void> => { | ||
| // A Secret remains mounted until Daytona confirms the sandbox is absent. Never reverse this | ||
| // order, including timer cleanup and create compensation after an id was returned. | ||
| await destroySandboxIdempotently(activeProvider, sandboxId); |
There was a problem hiding this comment.
Lifecycle invariant: Daytona must confirm that the sandbox is absent before its Secret records are deleted. This order applies to explicit destroy, delayed cleanup, rotation, and reconnect failure; reversing it can leave a live sandbox with broken or misbound credentials. — Codex
| currentAllocation = allocation; | ||
| return sandboxId; | ||
| } catch (cause) { | ||
| // The vendored provider creates the remote sandbox before it starts the daemon and only |
There was a problem hiding this comment.
Intentional fail-safe: an underlying create rejection is ambiguous because the remote sandbox may already exist before daemon startup fails. PR A retains the allocation instead of deleting a Secret that a partial sandbox could still mount. The stacked reconciliation design owns eventual orphan recovery. — Codex
|
Stacked durability design: #5278 contains only the managed-resource reconciliation plan. API, migration, worker, and deployment implementation remain paused for design review. — Codex |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/runner/src/engines/sandbox_agent/pi-assets.ts (1)
302-347: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winHandle the new Pi-extension failure path in
acquireEnvironment
prepareLocalPiAssetscan now throw when the Agenta extension is required but missing, but the two call sites treat that differently: the early call still bypassesacquireEnvironment’s{ ok: false, error }path, and the mount-guidance path swallows the same error as a generic mount failure and keeps going. Wrap the first call in the structured failure path, and letmountLocalAgentCwdrethrow this specific case instead of returningfalse.
🧹 Nitpick comments (3)
sdks/python/oss/tests/pytest/unit/agents/connections/test_dtos_model_ref.py (1)
92-108: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExtend the wire-boundary assertion to cover all retired top-level credential fields.
The test only checks
("provider", "connection", "secrets")are absent from the payload, but the runner'sLEGACY_MODEL_CREDENTIAL_FIELDSguard (run-plan.ts) also treats"deployment","credentialMode", and"endpoint"as retired top-level fields that must never appear outsidemodelConnection. Extending this assertion keeps the SDK-side wire-boundary test symmetric with the runner-side rejection list, catching a future leak of any of these fields.♻️ Proposed extension
assert "modelConnection" not in payload - for removed in ("provider", "connection", "secrets"): + for removed in ("provider", "connection", "secrets", "deployment", "credentialMode", "endpoint"): assert removed not in payloadservices/runner/src/extensions/agenta.ts (1)
362-398: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the model-override decode
decodePiModelProviderOverride(modelProviderOverrideRaw)can throw on a malformed env var and prevent this extension from registering tracing, tools, and builtin gating for the run. Catch the error and ignore the override so the rest still loads.sdks/python/agenta/sdk/agents/handler.py (1)
167-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "degradation" wording elsewhere in this file now contradicts the fail-closed contract.
This docstring correctly states resolution is fail-closed, but the
AgentCompositionclass docstring (still describing "capability-gated + degrading connection resolve" / "not the permissive copy") and the field comment "capability gating + degradation policy" describe the old graceful-degradation behavior. Aligning that wording avoids future readers assuming a permissive fallback still exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 648fceea-0258-4e1a-b103-3c7eb98dd487
⛔ Files ignored due to path filters (1)
services/runner/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (65)
sdks/python/agenta/sdk/agents/adapters/sandbox_agent.pysdks/python/agenta/sdk/agents/capabilities.pysdks/python/agenta/sdk/agents/connections/__init__.pysdks/python/agenta/sdk/agents/connections/endpoints.pysdks/python/agenta/sdk/agents/connections/errors.pysdks/python/agenta/sdk/agents/connections/models.pysdks/python/agenta/sdk/agents/connections/resolver.pysdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/agents/handler.pysdks/python/agenta/sdk/agents/interfaces.pysdks/python/agenta/sdk/agents/mcp/__init__.pysdks/python/agenta/sdk/agents/mcp/models.pysdks/python/agenta/sdk/agents/mcp/resolver.pysdks/python/agenta/sdk/agents/platform/connections.pysdks/python/agenta/sdk/agents/utils/wire.pysdks/python/agenta/sdk/agents/wire_models.pysdks/python/agenta/sdk/redaction/context.pysdks/python/agenta/sdk/redaction/seed.pysdks/python/oss/tests/pytest/unit/agents/connections/test_dtos_model_ref.pysdks/python/oss/tests/pytest/unit/agents/connections/test_models.pysdks/python/oss/tests/pytest/unit/agents/connections/test_resolver.pysdks/python/oss/tests/pytest/unit/agents/golden/run_request.claude.jsonsdks/python/oss/tests/pytest/unit/agents/golden/run_request.pi_core.jsonsdks/python/oss/tests/pytest/unit/agents/mcp/test_resolver.pysdks/python/oss/tests/pytest/unit/agents/platform/test_connections_http.pysdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pysdks/python/oss/tests/pytest/unit/agents/test_wire_models.pyservices/runner/package.jsonservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/daemon.tsservices/runner/src/engines/sandbox_agent/daytona-secret-plan.tsservices/runner/src/engines/sandbox_agent/daytona-secret-provider.tsservices/runner/src/engines/sandbox_agent/daytona-secrets.tsservices/runner/src/engines/sandbox_agent/daytona.tsservices/runner/src/engines/sandbox_agent/errors.tsservices/runner/src/engines/sandbox_agent/mcp.tsservices/runner/src/engines/sandbox_agent/pi-assets.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/extensions/agenta.tsservices/runner/src/extensions/model-provider-override.tsservices/runner/src/protocol.tsservices/runner/src/server.tsservices/runner/tests/setup/hermetic-env.tsservices/runner/tests/unit/daytona-secret-plan.test.tsservices/runner/tests/unit/daytona-secret-provider.test.tsservices/runner/tests/unit/daytona-secrets.test.tsservices/runner/tests/unit/extension-tools.test.tsservices/runner/tests/unit/mcp-servers.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-pi-assets.test.tsservices/runner/tests/unit/sandbox-agent-provider.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-keepalive-engine.test.tsservices/runner/tests/unit/session-mcp-layering.test.tsservices/runner/tests/unit/session-pool.test.tsservices/runner/tests/unit/ssrf-guard.test.tsservices/runner/tests/unit/wire-contract.test.tsservices/runner/tests/utils/qa-transcripts.ts
💤 Files with no reviewable changes (1)
- sdks/python/agenta/sdk/agents/adapters/sandbox_agent.py
| # ``resolved_connection`` is the single source of model routing and credentials. | ||
| resolved_connection: Optional[ResolvedConnection] = None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\bSessionConfig\s*\(' --type py -A12 | rg -nP 'secrets\s*='
rg -nP '\.secrets\b' --type py -g '!**/mcp/**' -C2Repository: Agenta-AI/agenta
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and locate SessionConfig-related definitions/usages.
ast-grep outline sdks/python/agenta/sdk/agents/dtos.py --view expanded
# Show the SessionConfig class and nearby fields/methods.
python3 - <<'PY'
from pathlib import Path
p = Path('sdks/python/agenta/sdk/agents/dtos.py')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 'class SessionConfig' in line:
start = max(1, i-40)
end = min(len(lines), i+140)
for j in range(start, end+1):
print(f"{j}:{lines[j-1]}")
break
PY
# Search for any remaining SessionConfig(secrets=...) or .secrets references in Python files.
rg -n --type py '\bSessionConfig\s*\(' -A12 .
rg -n --type py '\.secrets\b' .Repository: Agenta-AI/agenta
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact test that still passes `secrets=...` to `SessionConfig`.
python3 - <<'PY'
from pathlib import Path
p = Path('sdks/python/oss/tests/pytest/unit/agents/test_dtos_secret_repr.py')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 'SessionConfig(' in line or '.secrets' in line or 'secrets=' in line:
start = max(1, i-8)
end = min(len(lines), i+12)
for j in range(start, end+1):
print(f"{j}:{lines[j-1]}")
print('---')
PY
# Narrow search to SessionConfig construction sites that still mention secrets.
rg -n --type py -A6 -B2 'SessionConfig\s*\(' sdks services clients | rg -n 'secrets\s*=|\.secrets\b'
# Narrow search to any `.secrets` references within the agents SDK/tests only.
rg -n --type py '\.secrets\b' sdks/python/agenta/sdk sdks/python/oss/tests/pytest/unit/agents services/oss/tests/pytest/unit/agent -C2Repository: Agenta-AI/agenta
Length of output: 14855
Remove stale SessionConfig.secrets references
sdks/python/oss/tests/pytest/unit/agents/test_dtos_secret_repr.py and services/oss/tests/pytest/unit/agent/test_invoke_handler.py still pass/assert SessionConfig.secrets, but SessionConfig no longer defines that field. Update these call sites to the current credentials source.
| // Clear-then-apply (Security rule 5): on a managed run (credentialMode "env") the daemon | ||
| // inherits NONE of the sidecar's own provider keys, so only the resolved `plan.secrets` are | ||
| // inherits NONE of the sidecar's own provider keys, so only the resolved model environment is | ||
| // present and an inherited key for another provider cannot leak. For runtime_provided/none/ | ||
| // un-migrated runs the harness uses its own login, so the inherited keys stay. | ||
| const clearProviderEnv = plan.credentialMode === "env"; | ||
| const clearProviderEnv = | ||
| plan.credentialMode === "env" || plan.credentialMode === "none"; | ||
| const env = (deps.buildDaemonEnv ?? buildDaemonEnv)(plan.acpAgent, { | ||
| clearProviderEnv, | ||
| }); | ||
| Object.assign(env, plan.secrets); // apply only the resolved provider keys | ||
| Object.assign(env, plan.modelEnvironment); // apply only the resolved provider keys |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm what credentialMode "none" represents and whether it implies a harness-own-login run.
rg -n "credentialMode" services/runner/src/engines/sandbox_agent/run-plan.ts -B3 -A5Repository: Agenta-AI/agenta
Length of output: 3414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sandbox agent call site and nearby comments.
sed -n '748,775p' services/runner/src/engines/sandbox_agent.ts
# Inspect the daemon-side contract referenced by the review comment.
rg -n "BuildDaemonEnvOptions|clearProviderEnv|runtime_provided|credentialMode === \"env\"" services/runner/src/engines -B3 -A6
# Inspect the run-plan contract around credentialMode semantics.
sed -n '90,120p' services/runner/src/engines/sandbox_agent/run-plan.ts
sed -n '648,670p' services/runner/src/engines/sandbox_agent/run-plan.tsRepository: Agenta-AI/agenta
Length of output: 15213
clearProviderEnv should not clear for credentialMode === "none"
run-plan.ts and daemon.ts both treat "none" as a non-managed run that keeps the harness’s inherited provider/auth keys. Clearing here strips those keys before the daemon starts and breaks harness-login runs. If "none" is meant to behave differently, those docs need to change too.
| for (const [name, value] of Object.entries( | ||
| input.modelConnection?.environment ?? {}, | ||
| )) { | ||
| assertPublicEnvironmentBinding(name); | ||
| if (!value) fail(`model environment binding '${name}' is empty`); | ||
| const normalized = name.toLowerCase(); | ||
| if (directBindings.has(normalized)) { | ||
| fail(`duplicate direct environment binding '${name}'`); | ||
| } | ||
| directBindings.add(normalized); | ||
| environment[name] = value; | ||
| } | ||
|
|
||
| const add = (candidate: Omit<DaytonaSecretCandidate, "ordinal">): void => { | ||
| assertBinding(candidate.binding.name); | ||
| const consumerKey = | ||
| candidate.consumer.kind === "model" ? "model" : candidate.consumer.server; | ||
| const key = `${candidate.consumer.kind}:${consumerKey}:${candidate.binding.kind}:${candidate.binding.name.toLowerCase()}`; | ||
| if (seen.has(key)) { | ||
| fail(`duplicate credential binding '${candidate.binding.name}'`); | ||
| } | ||
| seen.add(key); | ||
| candidates.push({ ...candidate, ordinal: candidates.length }); | ||
| }; | ||
|
|
||
| const connection = input.modelConnection; | ||
| if (connection) { | ||
| const opaqueCredentials = (connection.credentials ?? []).filter( | ||
| (credential) => credential.usage === "opaque_http", | ||
| ); | ||
| const host = | ||
| opaqueCredentials.length > 0 && connection.endpoint?.baseUrl | ||
| ? exactHttpsHost(connection.endpoint.baseUrl) | ||
| : undefined; | ||
| for (const credential of connection.credentials ?? []) { | ||
| if (credential.usage === "local_use") { | ||
| assertLocalUseBinding(credential.binding.name); | ||
| const normalized = credential.binding.name.toLowerCase(); | ||
| if (directBindings.has(normalized)) { | ||
| fail( | ||
| `duplicate direct environment binding '${credential.binding.name}'`, | ||
| ); | ||
| } | ||
| directBindings.add(normalized); | ||
| environment[credential.binding.name] = credential.value; | ||
| continue; | ||
| } | ||
| if (!host) { | ||
| fail( | ||
| "opaque model credentials require endpoint.baseUrl for exact-host restriction", | ||
| ); | ||
| } | ||
| add({ | ||
| consumer: { kind: "model" }, | ||
| binding: credential.binding, | ||
| allowedHost: host, | ||
| value: credential.value, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Same env-var name can be both a direct/public binding and an opaque Secret candidate for the model consumer.
The local_use branch (Lines 180-191) checks directBindings before writing, but the opaque_http path (falls through to add() at Lines 192-203) never checks directBindings, and add()'s own seen set (Lines 158-168) only dedupes among secret candidates — it never consults directBindings. So a modelConnection with environment: { AWS_REGION: "..." } and a credential also bound to { kind: "environment", name: "AWS_REGION" } with usage: "opaque_http" passes validation, producing both a plaintext plan.environment.AWS_REGION and a Secret candidate for the same name — an ambiguous/colliding final environment binding that downstream merge logic must silently resolve one way or the other.
🔒 Proposed fix: make add() aware of directBindings for the model consumer
const add = (candidate: Omit<DaytonaSecretCandidate, "ordinal">): void => {
assertBinding(candidate.binding.name);
+ if (
+ candidate.consumer.kind === "model" &&
+ candidate.binding.kind === "environment" &&
+ directBindings.has(candidate.binding.name.toLowerCase())
+ ) {
+ fail(`duplicate direct environment binding '${candidate.binding.name}'`);
+ }
const consumerKey =
candidate.consumer.kind === "model" ? "model" : candidate.consumer.server;As per the existing security-seam guidance on this file (Line 31): "these explicit allowlists are the only values allowed to bypass opaque Secret substitution... keep this list aligned with the Python resolver contract" — the same rigor should extend to preventing one binding name from satisfying both the public/direct path and the opaque path simultaneously.
📝 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.
| for (const [name, value] of Object.entries( | |
| input.modelConnection?.environment ?? {}, | |
| )) { | |
| assertPublicEnvironmentBinding(name); | |
| if (!value) fail(`model environment binding '${name}' is empty`); | |
| const normalized = name.toLowerCase(); | |
| if (directBindings.has(normalized)) { | |
| fail(`duplicate direct environment binding '${name}'`); | |
| } | |
| directBindings.add(normalized); | |
| environment[name] = value; | |
| } | |
| const add = (candidate: Omit<DaytonaSecretCandidate, "ordinal">): void => { | |
| assertBinding(candidate.binding.name); | |
| const consumerKey = | |
| candidate.consumer.kind === "model" ? "model" : candidate.consumer.server; | |
| const key = `${candidate.consumer.kind}:${consumerKey}:${candidate.binding.kind}:${candidate.binding.name.toLowerCase()}`; | |
| if (seen.has(key)) { | |
| fail(`duplicate credential binding '${candidate.binding.name}'`); | |
| } | |
| seen.add(key); | |
| candidates.push({ ...candidate, ordinal: candidates.length }); | |
| }; | |
| const connection = input.modelConnection; | |
| if (connection) { | |
| const opaqueCredentials = (connection.credentials ?? []).filter( | |
| (credential) => credential.usage === "opaque_http", | |
| ); | |
| const host = | |
| opaqueCredentials.length > 0 && connection.endpoint?.baseUrl | |
| ? exactHttpsHost(connection.endpoint.baseUrl) | |
| : undefined; | |
| for (const credential of connection.credentials ?? []) { | |
| if (credential.usage === "local_use") { | |
| assertLocalUseBinding(credential.binding.name); | |
| const normalized = credential.binding.name.toLowerCase(); | |
| if (directBindings.has(normalized)) { | |
| fail( | |
| `duplicate direct environment binding '${credential.binding.name}'`, | |
| ); | |
| } | |
| directBindings.add(normalized); | |
| environment[credential.binding.name] = credential.value; | |
| continue; | |
| } | |
| if (!host) { | |
| fail( | |
| "opaque model credentials require endpoint.baseUrl for exact-host restriction", | |
| ); | |
| } | |
| add({ | |
| consumer: { kind: "model" }, | |
| binding: credential.binding, | |
| allowedHost: host, | |
| value: credential.value, | |
| }); | |
| } | |
| const add = (candidate: Omit<DaytonaSecretCandidate, "ordinal">): void => { | |
| assertBinding(candidate.binding.name); | |
| if ( | |
| candidate.consumer.kind === "model" && | |
| candidate.binding.kind === "environment" && | |
| directBindings.has(candidate.binding.name.toLowerCase()) | |
| ) { | |
| fail(`duplicate direct environment binding '${candidate.binding.name}'`); | |
| } | |
| const consumerKey = | |
| candidate.consumer.kind === "model" ? "model" : candidate.consumer.server; | |
| const key = `${candidate.consumer.kind}:${consumerKey}:${candidate.binding.kind}:${candidate.binding.name.toLowerCase()}`; | |
| if (seen.has(key)) { | |
| fail(`duplicate credential binding '${candidate.binding.name}'`); | |
| } | |
| seen.add(key); | |
| candidates.push({ ...candidate, ordinal: candidates.length }); | |
| }; |
| if ( | ||
| sandboxCredentialsRotated(parked, incoming) || | ||
| parked.transientCredentialsHash !== incoming.transientCredentialsHash | ||
| ) | ||
| return "credentials-rotated"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) How is toolCallback.authorization produced per turn? Is it re-minted?
rg -nP -C3 'toolCallback' services/runner/src --type=ts -g '!**/tests/**'
rg -nP -C3 '(authorization|reMint|reminted|mint)\b' services/runner/src/engines/sandbox_agent --type=ts -g '!**/tests/**'
# 2) Idle-continuation consumers of credentialEpochMismatch
rg -nP -C3 'credentialEpochMismatch' services/runner/src --type=ts
# 3) Existing tests that vary ONLY toolCallback auth between parked/incoming
rg -nP -C6 'computeCredentialEpoch|toolCallback' services/runner/tests/unit/session-pool.test.tsRepository: Agenta-AI/agenta
Length of output: 28509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server.ts around idle continuation and approval-resume =="
sed -n '540,690p' services/runner/src/server.ts
echo
echo "== session-pool.ts around credential epoch logic =="
sed -n '330,440p' services/runner/src/engines/sandbox_agent/session-pool.ts
echo
echo "== tests around computeCredentialEpoch / credentialEpochMismatch =="
sed -n '460,730p' services/runner/tests/unit/session-pool.test.ts
echo
echo "== where toolCallback.authorization is sourced from in server/src =="
rg -n -C3 'toolCallback\.authorization|toolCallback:\s*{|\bauthorization\b.*toolCallback' services/runner/src --type=tsRepository: Agenta-AI/agenta
Length of output: 19175
Idle continuations should ignore per-turn callback auth changes
credentialEpochMismatch treats a changed transientCredentialsHash as credentials-rotated, so the idle-continuation path will evict and cold-start any parked session whose toolCallback.authorization is re-minted each turn. That conflicts with the approval-resume path, which intentionally ignores the transient hash. Either drop the transient hash from the idle check too, or make callback auth stable for the whole parked session. Add a test for the transient-only-diff case.
| const baseUrl = (value as { baseUrl?: unknown }).baseUrl; | ||
| if (typeof baseUrl !== "string" || baseUrl.trim() !== baseUrl) { | ||
| throw new Error("model provider override has an invalid baseUrl"); | ||
| } | ||
|
|
||
| let url: URL; | ||
| try { | ||
| url = new URL(baseUrl); | ||
| } catch { | ||
| throw new Error("model provider override baseUrl must be a valid URL"); | ||
| } | ||
| if ( | ||
| url.protocol !== "https:" || | ||
| !url.hostname || | ||
| url.username || | ||
| url.password || | ||
| url.search || | ||
| url.hash | ||
| ) { | ||
| throw new Error( | ||
| "model provider override baseUrl must be an HTTPS URL without credentials, query, or fragment", | ||
| ); | ||
| } | ||
|
|
||
| return { provider, baseUrl }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== target file ==\n'
git ls-files 'services/runner/src/extensions/model-provider-override.ts' 'services/runner/src/**/daytona-secret-plan.ts' 'services/runner/src/**/secret-plan*.ts' | cat
printf '\n== outline: target file ==\n'
ast-grep outline services/runner/src/extensions/model-provider-override.ts --view expanded || true
printf '\n== outline: candidate comparator file(s) ==\n'
for f in $(git ls-files 'services/runner/src/**/daytona-secret-plan.ts' 'services/runner/src/**/secret-plan*.ts'); do
echo "--- $f ---"
ast-grep outline "$f" --view expanded || true
done
printf '\n== search for exactHttpsHost and baseUrl validation ==\n'
rg -n "exactHttpsHost|baseUrl must|model provider override|public fully qualified host|localhost|\.internal|\.local|\.home|\.lan|isIP\(" services/runner/src -SRepository: Agenta-AI/agenta
Length of output: 4665
🏁 Script executed:
set -euo pipefail
printf '\n== model-provider-override.ts (lines 1-120) ==\n'
sed -n '1,120p' services/runner/src/extensions/model-provider-override.ts | cat -n
printf '\n== daytona-secret-plan.ts (lines 80-130) ==\n'
sed -n '80,130p' services/runner/src/engines/sandbox_agent/daytona-secret-plan.ts | cat -n
printf '\n== usages of PiModelProviderOverride ==\n'
rg -n "PiModelProviderOverride|validatePiModelProviderOverride|encodePiModelProviderOverride|decodePiModelProviderOverride|PI_MODEL_PROVIDER_OVERRIDE_ENV" services/runner/src -SRepository: Agenta-AI/agenta
Length of output: 6692
🏁 Script executed:
set -euo pipefail
printf '\n== pi-assets.ts around encode usage ==\n'
sed -n '60,95p' services/runner/src/engines/sandbox_agent/pi-assets.ts | cat -n
printf '\n== agenta.ts around decode usage ==\n'
sed -n '350,390p' services/runner/src/extensions/agenta.ts | cat -n
printf '\n== any direct use of baseUrl field ==\n'
rg -n "\.baseUrl\b|baseUrl:" services/runner/src/engines/sandbox_agent services/runner/src/extensions -SRepository: Agenta-AI/agenta
Length of output: 5103
Tighten baseUrl to the same public-host policy as the other outbound URL checks.
validatePiModelProviderOverride() still accepts IP literals, localhost/internal domains, and explicit non-default ports, so values like https://127.0.0.1/ or https://169.254.169.254/ pass through even though this override is forwarded into the Pi extension without any later hostname check. Reuse the stricter exactHttpsHost-style rules here.
1 participant
Context
Daytona agent runs need model keys and authenticated MCP headers, but sending those values as ordinary sandbox environment variables exposes plaintext credentials to the remote sandbox control plane. The first implementation of this feature coupled that delivery problem to durable API leases, new tables, and migrations.
This recut establishes the narrow security boundary first. It changes only the Python agent SDK contract and the runner. Existing connection, workflow, and frontend APIs stay unchanged.
Changes
The SDK now resolves managed model and HTTP MCP credentials into typed runner-wire descriptors. Managed credentials fail closed when resolution is incomplete; only explicitly self-managed credentials remain runtime-provided.
When
AGENTA_DAYTONA_OPAQUE_SECRETS=process_local, the runner creates per-sandbox Daytona Secret records and sends only Secret placeholders in the sandbox creation request. It deletes the sandbox before deleting its Secrets, compensates partial creation failures, serializes reconnect and cleanup, and recreates the sandbox when the credential fingerprint changes.The same path covers model credentials, every HTTP MCP header, custom provider endpoints, and Pi provider overrides. Remote endpoints must use HTTPS with a public fully qualified host. Runs that require the Pi extension now fail clearly if the extension cannot be delivered.
This PR intentionally keeps ownership process-local. A hard runner crash can leave a Daytona Secret behind. The stacked design PR specifies durable managed-resource reconciliation as the follow-up instead of adding API migrations here.
Tests / notes
QA_OK.What to QA
AGENTA_DAYTONA_OPAQUE_SECRETS=process_local, run Claude on Daytona with a managed Anthropic connection, and confirm the turn completes.