[fix] Stop blaming the user's key for unsubstituted sandbox credentials - #6362
Conversation
A Daytona run whose opaque Secret placeholder reaches the model proxy raw
(substitution not yet propagated) failed with 'model authentication failed --
add the project's OpenAI key', which is wrong three ways for a free-credits
Gemini user: the key is fine, they never had an OpenAI key, and the advice
is unactionable. Found live on EU cloud 2026-08-29: 7 of 7 placeholder 401s
were the first call of a fresh sandbox, 10-24s after Secret creation.
- runner: recognize the placeholder-shaped refusal (LiteLLM 'Virtual Key
expected. Received=dtn_...' and raw dtn_secret_ echoes) as its own
transient class 'credential_delivery_failed' with retry-flavored copy.
- runner: a custom-deployment auth failure names the connection instead of
the dialect family ('the starter-credits connection's API key', not 'the
project's OpenAI key').
- web: transient failure classes (credential_delivery_failed,
starter_credits_unavailable, rate_limited) offer a Try again button on the
last turn, reusing the Stopped->Resend regenerate wiring.
Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
dosubot
Bot
added
the
size:M
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
bug
dev experience
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe runner detects unsubstituted Daytona credential placeholders and returns a dedicated transient error. Custom deployments use a neutral model-connection API-key hint. The chat UI offers “Try again” for retryable failures on the last resendable turn and confirms retries that may repeat side effects. ChangesCredential delivery retry
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The new Try again action can regenerate a failed turn whose write-capable tools may already have changed external state; because the current safeguards do not cover all interrupted or errored executions, a retry could repeat a write without warning. This bounded correctness and security risk should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant runTurn
participant classifyRunError
participant AgentTurn
participant AgentConversation
participant RunErrorBody
runTurn->>classifyRunError: pass connection metadata and raw error
classifyRunError-->>runTurn: return credential_delivery_failed
runTurn-->>AgentTurn: provide failed turn
AgentTurn->>RunErrorBody: pass retry handler for the last turn
RunErrorBody-->>AgentTurn: render Try again for retryable code
AgentTurn->>AgentConversation: invoke resend
AgentConversation-->>AgentTurn: confirm side effects before regeneration
🚥 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 |
…rst call The guard for the placeholder-401 race (#6362): Daytona applies a new Secret's substitution rule asynchronously with no confirmation signal, and ~3% of fresh sandboxes' first model calls on EU cloud carried the raw dtn_ placeholder and failed the user's turn. On a freshly created Daytona sandbox whose model key rides a Secret and whose connection declares an endpoint base URL, the runner now probes the credential's own /chat/completions from INSIDE the sandbox: a response echoing dtn_ means the placeholder went through raw, so wait 2s and probe again (budget 25s); anything else means the header was substituted and the run proceeds. The probe starts right after sandbox create and is awaited at the END of acquire, so it runs concurrently with the ~10s of mounts/workspace/session work and the common case pays nothing. Fail open everywhere: an exec error, an empty body, or a spent budget logs and lets the run proceed — the worst outcome is the pre-existing behavior, now at least classified as credential_delivery_failed. The raw key value never appears runner-side: the shell expands the env var inside the sandbox. Every observed incident was a first call of a fresh sandbox (7 of 7), so once one probe substitutes, the sandbox is good for its lifetime. Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
…rst call The guard for the placeholder-401 race (#6362): Daytona applies a new Secret's substitution rule asynchronously with no confirmation signal, and ~3% of fresh sandboxes' first model calls on EU cloud carried the raw dtn_ placeholder and failed the user's turn. On a freshly created Daytona sandbox whose model key rides a Secret and whose connection declares an endpoint base URL, the runner now probes the credential's own /chat/completions from INSIDE the sandbox: a response echoing dtn_ means the placeholder went through raw, so wait 2s and probe again (budget 25s); anything else means the header was substituted and the run proceeds. The probe starts right after sandbox create and is awaited at the END of acquire, so it runs concurrently with the ~10s of mounts/workspace/session work and the common case pays nothing. Fail open everywhere: an exec error, an empty body, or a spent budget logs and lets the run proceed — the worst outcome is the pre-existing behavior, now at least classified as credential_delivery_failed. The raw key value never appears runner-side: the shell expands the env var inside the sandbox. Every observed incident was a first call of a fresh sandbox (7 of 7), so once one probe substitutes, the sandbox is good for its lifetime. Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
…rst call The guard for the placeholder-401 race (#6362): Daytona applies a new Secret's substitution rule asynchronously with no confirmation signal, and ~3% of fresh sandboxes' first model calls on EU cloud carried the raw dtn_ placeholder and failed the user's turn. On a freshly created Daytona sandbox whose model key rides a Secret and whose connection declares an endpoint base URL, the runner now probes the credential's own /chat/completions from INSIDE the sandbox: a response echoing dtn_ means the placeholder went through raw, so wait 2s and probe again (budget 25s); anything else means the header was substituted and the run proceeds. The probe starts right after sandbox create and is awaited at the END of acquire, so it runs concurrently with the ~10s of mounts/workspace/session work and the common case pays nothing. Fail open everywhere: an exec error, an empty body, or a spent budget logs and lets the run proceed — the worst outcome is the pre-existing behavior, now at least classified as credential_delivery_failed. The raw key value never appears runner-side: the shell expands the env var inside the sandbox. Every observed incident was a first call of a fresh sandbox (7 of 7), so once one probe substitutes, the sandbox is good for its lifetime. Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
…rst call The guard for the placeholder-401 race (#6362): Daytona applies a new Secret's substitution rule asynchronously with no confirmation signal, and ~3% of fresh sandboxes' first model calls on EU cloud carried the raw dtn_ placeholder and failed the user's turn. On a freshly created Daytona sandbox whose model key rides a Secret and whose connection declares an endpoint base URL, the runner now probes the credential's own /chat/completions from INSIDE the sandbox: a response echoing dtn_ means the placeholder went through raw, so wait 2s and probe again (budget 25s); anything else means the header was substituted and the run proceeds. The probe starts right after sandbox create and is awaited at the END of acquire, so it runs concurrently with the ~10s of mounts/workspace/session work and the common case pays nothing. Fail open everywhere: an exec error, an empty body, or a spent budget logs and lets the run proceed — the worst outcome is the pre-existing behavior, now at least classified as credential_delivery_failed. The raw key value never appears runner-side: the shell expands the env var inside the sandbox. Every observed incident was a first call of a fresh sandbox (7 of 7), so once one probe substitutes, the sandbox is good for its lifetime. Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
Adding a second integration to an agent evicted its warm session twice over: the integration names were composed into the prompt strings (appendSystemPrompt for Pi, agentsMd for the file-based harnesses) and into the search_tools tool description, and both surfaces are part of the session fingerprint. A one-word list change cost a full sandbox rebuild, fresh Daytona Secrets, and the substitution race of #6362. Now the guidance rides the wire as gatewayGuidance {text, carrier}: - the SDK adapters keep the carrier choice but stop splicing; the prompt strings leave the adapter purely authored; - the runner splices guidance-first at environment build (buildRunPlan), so the text refreshes exactly when a session is built and never evicts one: configFingerprint and the desired-state facets exclude the field by design, with the reason documented at both sites; - the wording presents the names as examples ('For instance, some of the integrations you have: ... Others may exist, and this list can go stale — search_tools is the source of truth'), so a list that goes stale mid-session stays honest; - the derived search_tools/run_tool descriptions drop the names sentence and are now byte-identical for any integration set, so only the FIRST connection (which genuinely adds the two tools) changes session config. Golden run_request.gateway_connection.json pins the new field; the connection-free payloads are byte-identical to before. Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
…rst call The guard for the placeholder-401 race (#6362): Daytona applies a new Secret's substitution rule asynchronously with no confirmation signal, and ~3% of fresh sandboxes' first model calls on EU cloud carried the raw dtn_ placeholder and failed the user's turn. On a freshly created Daytona sandbox whose model key rides a Secret and whose connection declares an endpoint base URL, the runner now probes the credential's own /chat/completions from INSIDE the sandbox: a response echoing dtn_ means the placeholder went through raw, so wait 2s and probe again (budget 25s); anything else means the header was substituted and the run proceeds. The probe starts right after sandbox create and is awaited at the END of acquire, so it runs concurrently with the ~10s of mounts/workspace/session work and the common case pays nothing. Fail open everywhere: an exec error, an empty body, or a spent budget logs and lets the run proceed — the worst outcome is the pre-existing behavior, now at least classified as credential_delivery_failed. The raw key value never appears runner-side: the shell expands the env var inside the sandbox. Every observed incident was a first call of a fresh sandbox (7 of 7), so once one probe substitutes, the sandbox is good for its lifetime. Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Review summary
Reviewed exact head 08ada4641c8d8a738f04c834ce2c5ec64ec216e5.
I recommend addressing the retry safety finding before merge.
1. High: Try again can repeat tools that already changed external systems
The new transient-error action passes directly to onResend, which calls regenerate({messageId}) without the side-effect check used by the existing rewind path.
A reproducible sequence is:
- A tool performs a write, such as creating a GitHub issue.
- A later model call in the same turn hits
rate_limitedor another retryable failure. - The error card shows Try again.
- Clicking it removes the failed assistant turn, including the completed tool record, and resends only the preceding user message.
- The agent can perform the write again.
I confirmed with an AI SDK probe that regenerating an assistant message containing a completed tool-create_issue part sends only the preceding user message. The repository already has sideEffectingToolsInRange and a confirmation dialog in handleRewind for this exact risk.
Smallest useful fix: reuse that existing side-effect detection and confirmation before retrying, or hide retry when a completed write tool exists. Add one test for a completed write followed by rate_limited.
2. Medium: Starter-credit users receive an impossible key instruction
For every named custom deployment, keyHintFor now says to add the connection's API key to the project vault. The new test explicitly expects this for starter-credits.
The seeded starter-credit connection is managed by Agenta, write-only, hidden from Settings, and rejects general updates. A genuine non-placeholder 401 therefore tells the user to edit a connection they cannot edit. It also exposes the internal slug starter-credits instead of the user-facing name Agenta.
Smallest useful fix: use neutral custom-connection copy unless the runner knows that the credential is user-editable. Do not special-case the starter-credits slug because the seeding code explicitly says the slug is not proof of ownership.
Implementation constraint from Mahmoud
Only implement these fixes if they can stay small, local, and clear. Do not add unwarranted complexity, duplicate recovery logic, or grow a chain of special cases that makes the code hard to follow.
Prefer reusing the existing retry safety helper and warning path. For the managed-connection message, prefer honest neutral copy in this pull request. If a precise fix requires broad new wire fields or a large connection-metadata refactor, keep that work out of this pull request and record it as a focused follow-up instead.
Verification
- Runner error tests: 33 passed locally.
- Runner type check passed locally.
- Web error-card tests: 5 passed locally.
- Web type check and changed-file lint passed locally.
- Rewind safety tests: 6 passed locally.
- No existing submitted review or inline comment duplicated these findings.
Two review findings: 1. High: the error card's Try again passed straight to regenerate, skipping the side-effect check the rewind path already has. A retryable model error can land AFTER a completed write tool, and the retry would run the write again. The resend handler now runs the same sideEffectingToolsInRange check and asks for confirmation, with retry-specific copy. Pinned with a failed-turn range test. 2. Medium: the custom-deployment auth hint named the connection slug, but the runner cannot tell a user-created connection from a managed hidden one (starter-credits), so the hint could leak an internal slug and point the user at a connection they cannot edit. The hint is now neutral. Claude-Session: https://claude.ai/code/session_0165tsjmvf3qvTPFcb9EV44g
|
Both findings are addressed in a283fd8.
No new wire fields and no connection-metadata refactor; the user-editable flag stays a follow-up. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/runner/src/engines/sandbox_agent/errors.ts (1)
35-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the interface documentation with the neutral hint.
keyHintForno longer namesconnection.slugfor custom deployments, butConciseErrorOptions.connectionstill says that the hint names the connection. Update that description to document the neutral wording.web/oss/src/components/AgentChatSlice/AgentConversation.tsx (1)
625-627: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep each changed web comment to one short line.
web/oss/src/components/AgentChatSlice/AgentConversation.tsx#L625-L627: replace the three-line retry-safety comment with one concise line.web/packages/agenta-chat/tests/unit/assets/rewind.test.ts#L66-L68: replace the three-line test rationale with one concise line.As per coding guidelines: Hard rule. At most ONE short line per comment.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6914e583-5a53-4cf8-a022-2eadcdc9572a
📒 Files selected for processing (4)
services/runner/src/engines/sandbox_agent/errors.tsservices/runner/tests/unit/sandbox-agent-errors.test.tsweb/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/packages/agenta-chat/tests/unit/assets/rewind.test.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
1 participant
Context
A free-credits user on EU cloud ran Gemini in the playground and got: "pi_core: model authentication failed. Add the project's OpenAI key to the project vault, or log in (OAuth)." Every part of that advice was wrong. The key was fine, the user never had an OpenAI key, and the model was Gemini.
The real cause, found in the LiteLLM proxy log: the sandbox sent its Daytona Secret placeholder instead of the real key. Daytona substitutes
dtn_secret_<id>placeholders into egress requests, and that substitution propagates asynchronously. When a fresh sandbox's first model call beats it (all 7 incidents in 72h were first calls, 10 to 24 seconds after Secret creation), the raw placeholder reaches the proxy and is refused:The runner classified that as a generic 401 and blamed the user's key. The "OpenAI" wording came from the custom deployment's dialect family, so every custom-connection auth failure mislabels the key the same way.
Changes
The runner now recognizes the placeholder-shaped refusal as its own class.
Before:
After:
Two more pieces:
credential_delivery_failed,starter_credits_unavailable,rate_limited). It reuses the same regenerate wiring as the Stopped/Resend pair, on the last turn only, never while a run is busy.The
codewire field is an open string, so the new value needs no protocol or golden-fixture change.Tests
services/runner:pnpm test(2538 passed) andpnpm run typecheck. New cases cover the LiteLLM placeholder body, a rawdtn_secret_echo, the connection-named hint, and the non-custom fallback.web/oss:AgentMessage.runError.test.tsx(5 passed) covers the Try again button appearing forcredential_delivery_failed, staying hidden without a handler, and staying hidden for non-transient codes.What to QA
https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt