[feat] Deliver agent credentials through Daytona Secrets - #5670
Conversation
Recut of #5277 onto current main. Behind AGENTA_DAYTONA_OPAQUE_SECRETS=process_local, the runner creates per-sandbox Daytona Secret records and sends only Secret placeholders in the sandbox create request instead of plaintext credential env vars. The SDK resolves managed model and HTTP MCP credentials into typed wire descriptors and fails closed when resolution is incomplete. Flag off, behavior matches main. Re-expressed over main's later reworks: typed run failures, model capability catalog, current-turn/attachment delivery, Pi built-ins rework, session-storage rework, mount-credential tracking, @daytona/sdk 0.198.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change replaces legacy model and secret fields with typed model and MCP credentials. It adds fail-closed resolution, run-scoped redaction, Daytona Secret delivery, provider overrides, and credential-rotation handling for resumed sessions. ChangesCredential routing and runtime delivery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Railway Preview Environment
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
services/runner/tests/unit/wire-contract.test.ts (1)
34-58: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
connectioninKNOWN_REQUEST_KEYS.
protocol.tsstill declaresconnection?: { mode: string; slug?: string }, and the runner still readsrequest.connectionfor the Pi custom-provider path. The TS runtime check rejects any request top-level key not inKNOWN_REQUEST_KEYS, so/runrequests carryingconnectionwould fail the wire-contract test.🐛 Proposed fix
"modelCapabilities", + "connection", "modelConnection",Source: Coding guidelines
sdks/python/agenta/sdk/agents/utils/wire.py (1)
83-94: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the remaining
secrets=caller forrequest_to_wire.
./sdks/python/agenta/sdk/agents/integration/agents/_fake_runner_backend.py:76passessecrets=self._secrets, butrequest_to_wireno longer accepts that keyword and will raiseTypeError.services/runner/src/server.ts (1)
1406-1413: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winOne-shot dev path can still log an unredacted credential-bearing stack.
This catch handles an error thrown by the one-shot JSON path (Lines 1396-1400), where
run(request)is awaited without a local try/catch. Becauserequestis declared withlet request: AgentRunRequest;inside the nested try block (Lines ~1372-1400), it is out of scope here, so thisconsole.errorcannot apply the sameseedForRun(request).redactString(...)protection just added at Lines 1198-1204 for the streaming path. An error whose message or stack echoes a credential value (an auth failure that repeats the key, a dumped env) reaches stderr unredacted through this path.Hoist the
requestdeclaration so it stays in scope for this catch, and redact the stack the same way the streaming path now does.🔒 Proposed fix
inFlight += 1; + let request: AgentRunRequest = {}; try { const raw = await readBody(req); - let request: AgentRunRequest; try { request = raw.trim() ? (JSON.parse(raw) as AgentRunRequest) : {}; } catch (err) { return send(res, 400, { ok: false, error: `Invalid JSON: ${err instanceof Error ? err.message : String(err)}`, }); } ... } finally { inFlight -= 1; } ... } catch (err) { const message = err instanceof Error ? err.message : "Internal error"; - console.error(err instanceof Error ? (err.stack ?? err.message) : err); + console.error( + err instanceof Error && err.stack + ? seedForRun(request).redactString(err.stack, "stderr") + : err, + ); return send(res, 500, { ok: false, error: message }); }
🧹 Nitpick comments (11)
sdks/python/agenta/sdk/agents/handler.py (1)
348-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract a shared event-redaction helper.
agent_event_streamandagent_batchboth build{"type": event.type, "data": event.data}and redact it with the samesink="agent_event"call. Extract a small helper to keep the two call sites in sync if the redaction call shape changes later.♻️ Proposed helper extraction
+def _redact_event(event) -> Dict[str, Any]: + return get_active_redactor().redact_json( + {"type": event.type, "data": event.data}, sink="agent_event" + ) + + async def agent_event_stream( harness, session_config, msgs, *, record_usage: RecordUsageFn = ambient_record_usage ): ... if event.type == "done": event_stop_reason = (event.data or {}).get("stopReason") - yield get_active_redactor().redact_json( - {"type": event.type, "data": event.data}, sink="agent_event" - ) + yield _redact_event(event)async for event in run: - events.append( - get_active_redactor().redact_json( - {"type": event.type, "data": event.data}, sink="agent_event" - ) - ) + events.append(_redact_event(event))Also applies to: 387-391
sdks/python/agenta/sdk/agents/wire_models.py (1)
79-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing a base type for model and MCP credential shapes.
WireCredentialBinding/WireCredentialandWireMcpCredentialBinding/WireMcpCredentialrepeat the same{binding: {kind, name}, value, usage}shape, differing only in thekind/usageliterals. A shared generic base would remove the duplication if a third credential domain is added later.Also applies to: 317-345
sdks/python/agenta/sdk/agents/dtos.py (1)
841-857: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
wire_model_connectionbelow the field declarations.
PiAgentTemplatedeclarestool_specs,system, andappend_systemafter this method. Pydantic still collects the fields, so behavior is unchanged. The base class and the siblingwire_tools/wire_promptmethods keep fields first, then methods. Match that order for readability.sdks/python/oss/tests/pytest/unit/agents/connections/test_dtos_model_ref.py (1)
62-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParametrize the model shapes.
The
forloop hides which model shape failed. A failure reports one assertion without the input.pytest.mark.parametrizenames each case in the report.♻️ Proposed refactor
-def test_wire_model_connection_empty_before_resolution(): - for model in ( - "openai-codex/gpt-5.5", - {"provider": "openai", "model": "gpt-5.5"}, - { - "provider": "openai", - "model": "gpt-5.5", - "connection": {"mode": "agenta", "slug": "openai-prod"}, - }, - { - "provider": "openai", - "model": "gpt-5.5", - "connection": {"mode": "self_managed"}, - }, - ): - config = PiAgentTemplate(model=model) - assert config.wire_model_connection() == {} +@pytest.mark.parametrize( + "model", + [ + "openai-codex/gpt-5.5", + {"provider": "openai", "model": "gpt-5.5"}, + { + "provider": "openai", + "model": "gpt-5.5", + "connection": {"mode": "agenta", "slug": "openai-prod"}, + }, + { + "provider": "openai", + "model": "gpt-5.5", + "connection": {"mode": "self_managed"}, + }, + ], +) +def test_wire_model_connection_empty_before_resolution(model): + config = PiAgentTemplate(model=model) + assert config.wire_model_connection() == {}Confirm that
pytestis imported in this module before you apply the change.sdks/python/oss/tests/pytest/unit/agents/connections/test_resolver.py (1)
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare
_credential_environmentbetween the two test modules.
sdks/python/oss/tests/pytest/unit/agents/platform/test_connections_http.pydefines the same helper. Move one copy into a sharedconftest.pyor a test-utility module, then import it in both places. Add a parameter type hint at the same time.services/runner/src/engines/sandbox_agent/provider.ts (2)
95-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a codepoint sort in
canonicalJson.
localeCompareis locale- and ICU-sensitive. The fingerprint is compared only inside one process today, so behavior is correct now. A plain codepoint comparison removes the dependency on the runtime locale and makes the digest reproducible if the fingerprint is ever persisted or compared across processes.♻️ Proposed change
- .sort(([left], [right]) => left.localeCompare(right)); + .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0));
155-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
buildDaytonaCreate's attachment handling instead of repeating it.
buildDaytonaCreatealready addssecretswhensecretAttachmentsis non-empty (Lines 83-85).buildDaytonarepeats that logic on top ofcreateFields. The duplicated rule can diverge. Consider passing the attachments intobuildDaytonaCreateinsidebuildDaytona, or dropping the now-unused parameter frombuildDaytonaCreate.services/runner/src/engines/sandbox_agent/run-plan.ts (1)
470-495: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider building the secret plan after the MCP capability gates.
buildDaytonaSecretPlanvalidatesrequest.mcpServers. The Pi user-MCP rejection (Line 544) and the reserved-server-name rejection (Line 553) run later. On a flag-on Pi + Daytona run that declares an MCP server with an invalid URL, the caller now receives a secret-plan message instead ofPI_USER_MCP_UNSUPPORTED_MESSAGE. Moving the plan construction below those gates keeps the more actionable message. No credential is allocated at plan time, so this is message quality only.services/runner/src/engines/sandbox_agent/daytona-secret-provider.ts (1)
21-28: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove the unused
planfield fromRegistryEntry.
plansMatchcompares onlycreateFingerprint.entry.planis written at Line 202 and never read. It holds plaintext credential values in memory for the lifetime of the entry. Dropping it reduces the in-memory credential surface.Also applies to: 48-50
services/runner/src/engines/sandbox_agent/environment.ts (1)
618-626: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse an options object for
buildSandboxProvider.The call order matches the current signature, but two adjacent optional object parameters still allow a silent mismatch in this package. Convert
buildSandboxProviderto a single options object, following thebuildDaemonEnvpattern.services/runner/src/extensions/model-provider-override.ts (1)
11-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReject non-public
baseUrlhosts invalidatePiModelProviderOverride.
validatePiModelProviderOverrideaccepts custom HTTPS addresses without host checks, so values likehttps://127.0.0.1/orhttps://169.254.169.254/pass and are written intoAGENTA_AGENT_MODEL_PROVIDER_OVERRIDE. The Pi extension then registers the built-in provider with thatbaseUrl, which can route model requests through a private host. Add synchronous host checks here, following the same SSRF protections already used for MCP URLs, such as rejecting localhost and blocked IP literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b4248030-8db8-4b58-8634-37ae336b178b
📒 Files selected for processing (73)
sdks/python/agenta/sdk/agents/adapters/harnesses.pysdks/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.attachment.jsonsdks/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_harness_adapters.pysdks/python/oss/tests/pytest/unit/agents/test_redaction_scope.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pysdks/python/oss/tests/pytest/unit/agents/test_wire_models.pyservices/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/environment-setup.tsservices/runner/src/engines/sandbox_agent/environment.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/pi-model-config.tsservices/runner/src/engines/sandbox_agent/provider.tsservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/engines/sandbox_agent/runtime-policy.tsservices/runner/src/engines/sandbox_agent/session-identity.tsservices/runner/src/extensions/agenta.tsservices/runner/src/extensions/model-provider-override.tsservices/runner/src/protocol.tsservices/runner/src/redaction.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/redaction-sinks.test.tsservices/runner/tests/unit/sandbox-agent-daemon.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-agent-pi-assets.test.tsservices/runner/tests/unit/sandbox-agent-pi-model-config.test.tsservices/runner/tests/unit/sandbox-agent-provider.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsservices/runner/tests/unit/server.test.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-mcp-layering.test.tsservices/runner/tests/unit/session-pool.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
- SDK: build_resolved_connection wraps classify_environment failures in InvalidConnectionConfigurationError (422), matching the effective_endpoint handling; malformed bindings no longer surface as 500s. Test added. - runner: buildDaytonaSecretPlan rejects an opaque model credential whose binding name (case-insensitive) collides with a direct environment binding, so one name can never ride both envVars and secrets in a create request. local_use bindings now settle before opaque ones so wire order cannot evade the check. Tests added. - runner: extracted one shared isDaytonaNotFound helper (daytona-secrets.ts) recognizing both the typed DaytonaNotFoundError and 404-shaped errors; used by Secret cleanup and the process-local sandbox lifecycle wrapper. Test added. - hermetic-env AGENTA_DAYTONA_OPAQUE_SECRETS scrub: already present at HEAD (SCRUBBED list + per-test re-scrub); no change needed. - daytona-secret-provider proxy staleness claim: rebutted, intentionally unchanged (all lifecycle methods are declared on the facade; attachments only affect create, which always rebuilds the delegate). - configFingerprint environment-values claim: rebutted, intentionally unchanged (modelConnection.environment carries non-secret config only by contract; secret material rides typed credentials whose values are already stripped from the fingerprint).
mmabrouk
left a comment
There was a problem hiding this comment.
This review is a guide for reading the diff, plus inline notes at the places where a design choice or a tradeoff is easy to miss.
Suggested reading order
services/runner/src/protocol.ts: the wire shapes. Model routing and credentials now arrive grouped undermodelConnection, and MCP secret headers ride typedconnection.credentials, replacing the old flatsecretsmap.sdks/python/agenta/sdk/agents/connections/models.pyand the newendpoints.py: how the SDK types each credential and splits secret values from plain environment values before anything goes on the wire.services/runner/src/engines/sandbox_agent/run-plan.ts: the feature flag gate and how the per-run secret plan is built.daytona-secret-plan.ts, thendaytona-secrets.ts, thendaytona-secret-provider.ts: the Daytona Secret lifecycle, from deciding what becomes a Secret to creating and cleaning them up.provider.ts: where the Secret wrapper hooks into sandbox provisioning.session-identity.ts: how reuse decisions changed (a shape-only fingerprint for "same setup" and a value hash for "did a credential rotate").src/redaction.ts: how credential values are kept out of records, spans, and logs.
One-line roles
daytona-secret-plan.ts: pure decision logic that splits a request's values into Daytona Secrets versus plain environment; no I/O.daytona-secrets.ts: a thin client for Daytona's Secret API with idempotent deletes and shared not-found handling.daytona-secret-provider.ts: wraps the sandbox provider so Secrets exist before the sandbox that uses them and are removed only after it is gone.extensions/model-provider-override.ts: shared validation for the public routing override (provider id and base URL, no secret material) that the runner hands to the in-Pi extension.connections/endpoints.py(SDK): resolves a connection into typed credentials plus a small allowlist of plain environment names, and computes the effective endpoint.
Verification already done
The branch went through two full review passes (an architecture and security pass, then a line-level pass on the recut), and live QA against a real Daytona deployment covering create, reconnect after park, credential rotation, and teardown. With the feature flag off, no secret plan is built and no wrapper is applied, so runner behavior is identical to main.
| // 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.
This module only decides what becomes a Daytona Secret. It performs no I/O: no network calls, no Daytona API use, nothing outside pure validation and classification. The actual Secret creation lives in daytona-secrets.ts. Note the allowlist rule here: only these four non-secret names (AWS/GCP region and project settings) may travel as plain environment values. Every other provider value must arrive as a typed credential, and assertPublicEnvironmentBinding hard-fails anything else.
| // Two passes: settle every direct (local_use) binding first, so the collision check in | ||
| // `add` sees the complete direct set regardless of credential order on the wire. | ||
| for (const credential of connection.credentials ?? []) { | ||
| if (credential.usage !== "local_use") continue; |
There was a problem hiding this comment.
CAVEAT, and a deliberate one: AWS and GCP "local_use" credentials stay as plaintext environment values inside the sandbox instead of becoming Daytona Secrets. The provider SDKs read several environment variables natively and compute request signatures from them, so a Daytona placeholder value would break signing. This is a documented exception with its own tight allowlist (LOCAL_USE_MODEL_CREDENTIAL_BINDINGS above), not an oversight. No opaque provider key is allowed to take this path.
| ); | ||
| } | ||
|
|
||
| async function deleteIdempotently( |
There was a problem hiding this comment.
This file is a thin CRUD wrapper around Daytona's Secret API. Deletes are idempotent on purpose: teardown paths can retry safely because deleting an already-deleted Secret is treated as success. The shared isDaytonaNotFound predicate just above covers both error shapes Daytona can produce, the typed DaytonaNotFoundError and a plain object with statusCode === 404.
| const facade: ProcessLocalDaytonaSecretProvider = { | ||
| name: "daytona", | ||
| async create(...args: unknown[]): Promise<string> { | ||
| const allocation = await allocateDaytonaSecrets(plan, api); |
There was a problem hiding this comment.
The lifecycle rule this wrapper enforces: Secrets are created before the sandbox that references them, and the sandbox is destroyed before its Secrets are deleted. That ordering means no live sandbox ever points at a deleted Secret. When create fails partway, the code compensates: if the provider was never built, the freshly allocated Secrets are deleted; if the remote create may have partially happened, Secrets are retained on purpose, because deleting them could strand a half-created sandbox.
| * Wrap Daytona provisioning with process-local Secret allocation. | ||
| * | ||
| * A parked sandbox and its allocation live in the registry together. Secret ownership is | ||
| * process-local BY DESIGN: the registry dies with the runner process, so a hard crash can |
There was a problem hiding this comment.
TRADEOFF, accepted deliberately: Secret ownership lives only in this process's memory. If the runner process crashes hard, the registry is gone and a Daytona Secret can be orphaned until Daytona's own auto-delete backstop reaps the sandbox. Durable reconciliation (recording ownership somewhere that survives a crash) is the explicit follow-up design in PR #5278, so reviewers should not expect recovery machinery here.
| connection: request.connection ?? null, | ||
| deployment: request.deployment ?? null, | ||
| endpoint: request.endpoint ?? null, | ||
| modelConnection: request.modelConnection |
There was a problem hiding this comment.
This is the split between the two hashes. configFingerprint is shape-only: credentials are stripped down to {binding, usage}, so no secret value ever enters it, and it answers "is this the same setup as the parked session". computeCredentialEpoch below is the one that hashes the actual values and answers "did a credential rotate". When the epoch changes, the parked sandbox is evicted and rebuilt instead of being reused with stale credentials.
| * they still transit runner memory and can be echoed by the model), so the deny-set seeds from | ||
| * the request, not from the delivered environment. | ||
| */ | ||
| export function requestSecretValues( |
There was a problem hiding this comment.
Every sink now seeds its deny-set from the typed wire shapes: durable records, spans, and (new in this PR) stderr exception stacks. requestSecretValues collects the model connection's credential values and environment values plus every MCP credential value, which is a superset of what actually lands in the sandbox environment. So even if a model echoes a credential back, or an error message includes one, the value is scrubbed before it reaches storage or logs.
| return resolved | ||
|
|
||
|
|
||
| def classify_environment( |
There was a problem hiding this comment.
This is where the SDK splits resolved values into two disjoint groups: plain environment (only the four allowlisted non-secret names, like AWS_REGION) and typed credentials (everything else). The runner enforces the same boundary on its side, so a secret value cannot slip through as environment. Managed resolution also fails closed: if a value is malformed, build_resolved_connection raises an error that maps to a 422 response instead of silently degrading the run to runtime-provided credentials.
| name: str = Field(min_length=1) | ||
|
|
||
|
|
||
| class ResolvedMCPCredential(BaseModel): |
There was a problem hiding this comment.
MCP header credentials are now typed instead of riding the plain headers dict. ResolvedMCPCredential masks its value on any Pydantic dump, so a log line or a serialized model can never carry the real secret; the value only surfaces through to_wire() at the materialization boundary. On a Daytona run there is one more layer: the value is swapped for a Daytona Secret placeholder before the sandbox create request, so plaintext never reaches the create call at all.
| * URL must pass the SSRF guard. Called early in acquire — BEFORE any sandbox or Daytona Secret | ||
| * is created — and again per server at ACP materialization (`toAcpMcpServers`). | ||
| */ | ||
| export async function validateUserMcpServers( |
There was a problem hiding this comment.
DECISION: this keeps main's SSRF rule (validateUserMcpUrl) rather than the stricter rule from the earlier PR. The operator-controlled allowlist may exempt a non-https host, which matters for internal MCP servers, and https is required for everything not on that allowlist. One small deliberate deviation from flag-off main: this structural validation now runs early in acquire, before any sandbox or Daytona Secret is created, so a bad MCP config fails the run before it has side effects.
Re-express the typed-wire delta on top of the Codex harness feature: - keep wire_harness_mode / harnessMode alongside modelConnection; drop the retired flat fields main's copies still carried (provider, secrets, credentialMode) from protocol.ts, wire_models.py, and the fingerprint - port main's new Codex tests (run-plan gates, wire golden) to the typed modelConnection shape and regenerate run_request.codex.json - runner: tsc clean, 99 files / 1534 unit tests green; SDK: 1920 passed
mmabrouk
left a comment
There was a problem hiding this comment.
Reviewed, please address comments. Answer each comment and suggest how we can fix it. For large item let's have the discussion in chat. For small item just go ahead and fix and commit
Gaps:
- Docs are not being updated. They should. Same goes for readme mentioning security.
- Custom secrets are not being taken into account. Maybe the wiring is not done (we are not sending them at all). In any case we need a plan on how to take them into account in addition to MCPs
| } | ||
|
|
||
| /** Opaque comparison key for every field baked into a parked Daytona sandbox at create time. */ | ||
| export function daytonaCreateFingerprint(input: { |
There was a problem hiding this comment.
does this mean that changing secrets break the finger print for daytona and moves things automatically into cold 2 mode (resending the conversation from scratch)? i
There was a problem hiding this comment.
Yes, with one clarification about which check does it.
This fingerprint is narrower than the session pool's. It covers only the fields baked into a Daytona sandbox at create time, and it guards reconnecting to a still-live sandbox: if the secret plan differs, the sandbox cannot serve this request, so the wrapper cleans up its Secret records and forces a fresh create.
It is not, however, what puts you in cold-2. By the time this runs, the pool has already made that decision: computeCredentialEpoch in session-identity.ts hashes the credential values, server.ts sees credentials-rotated, and the warm session is evicted before reconnect is ever attempted. So this fingerprint is a second, narrower guard rather than the cause. Changing a key does force a conversation restart today, and it did on main too, for that reason.
The improvement you are describing is real and this feature is what unlocks it. With the flag on, the sandbox holds a dtn_secret_<id> placeholder, not the key. So a value rotation does not need a new sandbox at all: update the Daytona Secret record behind the placeholder and the running sandbox keeps working. Only a change to the plan's shape (a different binding, a different allowed host, a credential added or removed) genuinely requires a new sandbox, because that is what is actually baked in.
Concretely that means splitting this fingerprint in two, a shape part and a value part, and adding an update path so a value-only change updates the Secret records instead of tearing down. One live constraint worth naming: our runner's Daytona key currently does not have permission to manage Secrets, so this whole path needs that fixed before it can be exercised against a real account regardless.
I have put this in the chat with the broader "what should invalidate what" question, since the two are the same design problem seen from different ends.
Four problems, all caught by suites the port never ran locally.
1. The author's connection choice stopped reaching the runner. The port dropped
`wire_model_ref()` because it also carried the retired flat `provider` field,
but that method was the only emitter of the top-level `connection` reference.
The runner still gates Pi's OpenAI-compatible models.json path on
`request.connection.mode === "agenta"` and names the provider from its slug
(`pi-model-config.ts`), so a named custom connection silently fell back to the
generic provider-override env. Restored as `wire_connection_ref()`, which emits
only `{mode, slug}` and stays empty for the project default so a plain run's
payload is unchanged.
2. `_fake_runner_backend.py` still passed `secrets=` to `request_to_wire`, which
the typed wire no longer accepts. It now mirrors the production sandbox-agent
backend: the parameter is accepted for interface parity and ignored, because
credentials ride inside `config` as `modelConnection`.
3. The services handler tests still built `ResolvedConnection(env=...)` and
expected a resolution failure to degrade into an empty `runtime_provided`
plan. Ported to typed credentials, and the degradation test became a
fail-closed test. The shared no-credential stub now reads its provider from
the harness capability table, because a fail-closed resolve must return a
connection the post-resolve gate accepts for whichever harness is running.
4. The custom-connection replay test asserted the retired flat wire.
Also: the credential epoch now uses an HMAC keyed with a per-process random key
instead of a bare sha256 of the secret values. The epoch is only ever compared
within one process, so the key costs nothing, and the digest stops being
brute-forceable against candidate API keys. This is what CodeQL flagged.
Verified: runner tsc clean, 99 files / 1534 tests; SDK 2067 passed (unit +
integration) with the 10 pre-existing litellm xfails; services 100 passed.
Review found the feature had no documentation and, more importantly, no way for an operator to turn it on: the environment variable was read straight from process.env in the runner and was never plumbed through docker-compose or the Helm chart, so setting it on the host did nothing. - Renamed the flag to AGENTA_RUNNER_DAYTONA_OPAQUE_SECRETS. Every other runner Daytona setting is AGENTA_RUNNER_DAYTONA_*, and the bare AGENTA_DAYTONA_ prefix collided visually with the DAYTONA_* variables that configure the unrelated code-evaluator sandbox. The feature is unreleased, so no operator has set the old name. - Plumbed it through all seven docker-compose runner services, the four env examples, the Helm deployment template, values.yaml, and values.schema.json. - Documented it in the configuration reference (what it does, the two caveats: the Daytona API key needs Secrets permission, and AWS keys cannot be hidden) and in the how-agents-run concept page (why an agent reading its own environment is the threat, and what the sandbox sees with the flag on). Also expanded the wire-contract comments the review asked about: what the usage values mean and why there are exactly two, what endpoint covers beyond OpenAI-compatible routes (Azure apiVersion, AWS/Vertex region), why a Daytona secret candidate is called a candidate, why allowedHost can never be a wildcard, and why the retired-field guard rejects rather than ignores while `connection` is deliberately not among the retired fields.
CodeQL reports the alert on the createHmac call itself, and a suppression comment only applies to alerts on its own line. The preceding-comment form did not take, so the call is split out and carries the marker inline.
CodeQL flagged the credential epoch as an insecurely hashed password, and GitHub code scanning does not honour inline suppression comments, so the check stayed red. Rather than argue with the scanner, the design changed, and the result is better than what it replaced. The epoch answers one question: did the credential material change since this session was parked? It never needed a digest to do that. It now holds the material behind a `CredentialMaterial` value object and compares it with `timingSafeEqual`. Why holding the values is not a step backwards. A digest of an API key was never much protection, because keys carry little enough entropy that a leaked digest can be attacked offline, and the parked environment already holds the plaintext anyway. The realistic risk is a value reaching a log line, and that is now structurally impossible rather than a convention: the material sits in a private field with no getter, and every route from an object to text (`String`, template literals, `JSON.stringify`, `util.inspect`, so `console.log`) returns a placeholder. This mirrors what the Python side already does, where `ResolvedCredential` masks its value on dump and hides it from `repr`. A test pins every one of those rendering routes, so a future refactor that drops an override fails loudly instead of leaking silently. No behavior change: the same rotations evict and the same re-minted per-turn bearers do not. Runner tsc clean, 99 files / 1535 tests.
The README still described `request.secrets`, which this change retired, and said nothing about hiding keys from the sandbox. It now describes the consumer-grouped shape, the reject-on-retired-field guard, and the Daytona Secrets path with its three caveats.
|
Thanks, this was a good review. Every inline comment has a reply. Here are the two gaps from the review body, plus a note on CI. DocsYou were right, and the gap was worse than missing prose: the feature could not be turned on at all. The environment variable was read straight from
Custom secretsYou are right that they are not handled. I traced it before answering, and the finding is that they never were, on this branch or on main. The vault already supports a So nothing regressed. What this PR does is make it much easier to add, because the consumer-grouped shape is exactly the extension point. I have put a plan in the chat. The key insight, which I got wrong at first and corrected: a custom secret is hideable too, as long as the user tells us which host it may be sent to. Other fixes in this round
On the failing CIThe remaining red checks are the acceptance suites, and they are not this PR. Every one of them fails identically: 194 to 314 assertions of The evidence that points at the Railway work rather than at us: acceptance passed on this branch's first commit, when previews still used the legacy deploy path. Since then the repo variable switched previews to clone mode, and this PR's deploy job now runs Everything this PR is responsible for is green: the runner suites, both SDK suites including integration, the services suites, the API suites, lint, format, the Helm render test, and CodeQL. |
…bare 403 A Daytona API key that can create sandboxes does not automatically have the separate permission to manage Secrets. When it does not, enabling AGENTA_RUNNER_DAYTONA_OPAQUE_SECRETS makes every run with a hideable credential fail at sandbox creation, and the raw provider error is a bare Forbidden that says nothing about the flag that caused it. This is the first thing an operator hits when turning the feature on, so the create path now recognizes a permission refusal and re-raises it as a message naming the variable to fix, the permission to grant, and the way to revert. The provider's own error is kept as the cause so the logs keep the detail. Every other failure keeps its original error untouched. The docs promote the same point from a bullet to a warning, and say plainly that the runner does not fall back to plaintext, because doing the unprotected thing silently would be worse than stopping. Runner tsc clean, 99 files / 1538 tests.
`RunPlan` had grown to thirty fields on one flat interface, and every consumer took the whole thing regardless of how little it read. Raised in review of #5670. The fields are grouped into `credentials`, `workspace`, `tools`, and `prompt`. The five identity fields (`harness`, `acpAgent`, `sandboxId`, `isPi`, `isDaytona`) stay at the top level because almost every consumer branches on them, and `sandboxPermission` stays there too since the declared security boundary is its own concern. The payoff is not the tidier interface. It is that a consumer can now say what it actually touches. `prepareWorkspace` used to take a nine-key `Pick` of the flat plan; it now takes three named slices and the parameter type reads as a description of the function. Several other consumers narrowed the same way, which means a future change to, say, credential delivery has a compiler-checked list of what depends on it. The one rename is `plan.prompt` to `plan.prompt.text`, forced by the group taking the name. Nothing else was renamed, no logic changed, and no `any` cast or `@ts-expect-error` was added. Runner tsc clean, 99 files / 1535 tests, the same counts as before the change.
…he gap Review catch. `wire_connection_ref()` emits a top-level `connection` field for a self-managed or named Agenta connection, and the runner reads its slug to register a custom OpenAI-compatible Pi run in Pi's models.json. But `WireRunRequest` no longer declared the field, so the exported contract schema omitted something the implementation both produces and requires. A client generated from that schema would drop it, and those runs would silently fall back to the generic provider-override path. The field went missing because it sat next to the flat credential fields this change retired and got swept out with them. It is not a credential. It is the author's connection CHOICE, which is non-secret routing config. Restored `WireConnection` and the `connection` field on `WireRunRequest`. Two reasons nothing caught this, both now closed: - `KNOWN_REQUEST_KEYS` was stale in exactly the same way, and its guard is a subset check over three sample payloads, none of which names a connection. Added the key, and added a test asserting the set EQUALS the schema's declared aliases in both directions. A key the schema declares but the producer never emits is dead contract surface, so equality is the honest assertion. - A validation-based test could not have caught it either: `_WireModel` sets `extra="allow"`, so a payload carrying a field the schema forgot still validates cleanly with the field quietly demoted to an extra. The new test is structural rather than validation-based for that reason, and the second new test round-trips a named connection through `model_dump` to prove it survives as a modelled field rather than an extra. I verified both guards fail when the schema field is removed, so neither is a test that can never break. Also corrected the runner's own doc comment on the field. It claimed "the current SDK resolver does NOT send it", which described the regression rather than the contract, and would have led the next reader to delete the models.json path as dead code. Verified: SDK 2069 passed with the 10 pre-existing litellm xfails, services 100 passed, runner tsc clean and 99 files / 1538 tests, ruff clean.
`RunPlan` had grown to thirty fields on one flat interface, and every consumer took the whole thing regardless of how little it read. Raised in review of #5670. The fields are grouped into `credentials`, `workspace`, `tools`, and `prompt`. The five identity fields (`harness`, `acpAgent`, `sandboxId`, `isPi`, `isDaytona`) stay at the top level because almost every consumer branches on them, and `sandboxPermission` stays there too since the declared security boundary is its own concern. The payoff is not the tidier interface. It is that a consumer can now say what it actually touches. `prepareWorkspace` used to take a nine-key `Pick` of the flat plan; it now takes three named slices and the parameter type reads as a description of the function. Several other consumers narrowed the same way, which means a future change to, say, credential delivery has a compiler-checked list of what depends on it. The one rename is `plan.prompt` to `plan.prompt.text`, forced by the group taking the name. Nothing else was renamed, no logic changed, and no `any` cast or `@ts-expect-error` was added. Runner tsc clean, 99 files / 1535 tests, the same counts as before the change.
|
Confirmed and fixed in What happened.
Why nothing caught it, and what now does. Two separate holes, both closed.
The I checked both guards actually fail when the schema field is removed, so neither is a test that can never break. One more thing your comment led me to. The runner's own doc comment on Verified: SDK 2069 passed with the 10 pre-existing litellm xfails, services 100 passed, runner type check clean with 99 files and 1538 tests, ruff clean. |
[refactor] Group RunPlan by concern instead of one flat bag
Context
Daytona agent runs need model API keys and authenticated MCP headers, but today the runner sends those values as plaintext sandbox environment variables, readable by the sandbox control plane, the agent process, and the shell. PR #5277 fixed this but was written in July against the now-dead
big-agentsbase; main has since taken about 1,200 commits through the same files (typed run failures, capability catalog, current-turn delivery, Pi built-ins rework, session-storage rework,@daytona/sdk0.198). This PR is a recut of #5277 onto current main, one commit, targetingmaindirectly. Supersedes #5277.Changes
Behind the flag
AGENTA_RUNNER_DAYTONA_OPAQUE_SECRETS=process_local, the runner creates per-sandbox Daytona Secret records and puts only Secret placeholders in the sandbox create request. The SDK resolves managed model and HTTP MCP credentials into typed wire descriptors and fails closed when resolution is incomplete; only explicitly self-managed credentials remain runtime-provided.Before (create request):
After (flag on):
The sandbox process sees only the
dtn_secret_...placeholder; Daytona substitutes the real value proxy-side at egress to the allowed host. The runner deletes the sandbox before its Secrets, compensates partial creation failures, and rebuilds the sandbox when the credential fingerprint rotates. With the flag on, the fingerprint wrapper applies to every Daytona run (even runs with zero opaque credentials), so a parked sandbox cannot reconnect across alocal_usecredential rotation. With the flag off, behavior matches main: plaintext delivery, no wrapper, no new failure modes.On the wire, the retired flat fields (
provider,deployment,endpoint,credentialMode,secrets) are replaced by a typedmodelConnectionobject, and MCP credentials ride insidemcpServers[].connection.credentials. The runner rejects the retired fields unconditionally. Redaction now seeds from the typed shapes on every sink, including exception stacks before they reach stderr, and the SDK scopes its redactor per run instead of accumulating deny-sets across runs in one task.Ownership is process-local by design: a hard runner crash can orphan a Daytona Secret until Daytona's auto-delete backstop. Durable managed-resource reconciliation stays a follow-up (see #5278), same as the original PR.
Tests / notes
tsc --noEmitclean; unit suite 94 files, 1,464 tests green; integration smoke 8 green.dtn_secret_placeholder, created a sandbox whose create request carried only the Secret name,printenvinside the sandbox showed only the placeholder, then deleted sandbox before Secret and verified both gone. Nothing left behind.AGENTA_RUNNER_DAYTONA_API_KEY) is denied on the Secrets API; the org's main key works. Grant the runner key the Secrets permission (or reissue it) before enabling the flag there.AGENTA_RUNNER_DAYTONA_OPAQUE_SECRETSduring review, to match every other runner Daytona setting, and is now plumbed through docker-compose, the env examples, and the Helm chart. It previously reached the runner through none of those, so it could not be enabled on a real deployment at all.Forbiddenfrom the provider.What to QA
AGENTA_RUNNER_DAYTONA_OPAQUE_SECRETS=process_localon a stack whose Daytona key has the Secrets permission, run Claude on Daytona with a managed Anthropic connection. The run completes; the sandbox env shows adtn_secret_placeholder, not the key.