Skip to content

fix(think, sandbox-coding-agent): model id validation, ai@7 peer, sandbox cleanup and hardening - #2388

Merged
threepointone merged 5 commits into
mainfrom
fix/think-sandbox-review
Sep 27, 2026
Merged

threepointone merged 5 commits into
mainfrom
fix/think-sandbox-review

Conversation

@threepointone

@threepointone threepointone commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Follow-ups from a review of #1832 (bundled workers-ai-provider), #1830 (sandbox-coding-agent example) and #1831 (CodingAgent RFC).

@cloudflare/think

  • Peer range is now ai@^7 (and @ai-sdk/react@^4). The bundled workers-ai-provider@4 / @ai-sdk/*@4 produce v4 models, which ai@6 rejects with UnsupportedModelVersionError, so string model ids already failed on ai@6. Shipped as a patch; consider whether you want a minor.
  • resolveModel fails fast on ids that are neither @... nor <provider>/<model> (e.g. gpt-5), instead of failing at inference time in env.AI.run.
  • Tests: invalid ids, missing AI binding, LanguageModel passthrough, string models from beforeTurn/beforeStep.
  • README snippet no longer uses an un-imported createWorkersAI.

examples/sandbox-coding-agent

  • Sandboxes are destroyed when a run finishes and in clearDelegatedRuns (previously the 6th task within 15 minutes exceeded max_instances: 5). getWorkspaceDiff is replaced by getLastResult, since the container is gone after a run.
  • Container death, non-zero exits, a missing exit event, failed git commands and failed clones are reported as failures instead of "completed with no changes".
  • The Anthropic gateway proxy forwards only POST v1/messages and v1/messages/count_tokens, and rejects max_tokens > 32000. README gains a Security section (no auth on /agents/*, internet + bypassPermissions, gateway spend).
  • Dockerfile base image matches the SDK (0.12.2); Claude Code pinned to 2.1.283.
  • Sandbox ids include the orchestrator name; session ids must be UUIDs before use in shell commands.

design/rfc-coding-agent.md

  • Fixes broken §10 references; notes rfc-think-multi-session.md is rejected and aligns topology with the accepted rfc-user-chat-durable-objects.md; cross-references rfc-codex-harness-capability.md; adds open questions (container quotas/cleanup, egress cost/abuse, auth/multi-tenancy, concurrent turns, container-death contract).

Verified on a combined branch with the other review follow-ups: pnpm run check, agents chat/workers/react, ai-chat, and Think suites all pass.


Devin Review

- think: require ai@^7 / @ai-sdk/react@^4 (bundled providers are v4 models)
- think: resolveModel fails fast on malformed string model ids; tests for
  invalid ids, missing AI binding, LanguageModel pass-through, and string
  models from beforeTurn/beforeStep
- think README: dynamic-config snippet no longer uses an unimported helper
- sandbox-coding-agent: destroy containers when runs finish and on clear;
  sandbox id hashes orchestrator name + run id; missing exit / non-zero exit /
  failed git commands fail the run; validate Claude session ids; allowlist
  proxied Anthropic endpoints and cap max_tokens; README security section;
  align Dockerfile base image with SDK 0.12.2 and pin Claude Code 2.1.283
- rfc-coding-agent: fix dangling section refs, reconcile topology with the
  accepted user-chat DO RFC, cross-reference the Codex harness RFC, add open
  questions

Co-authored-by: Cursor <cursoragent@cursor.com>
@changeset-bot

changeset-bot Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ae2d3de

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@cloudflare/think Patch
@cloudflare/agent-think Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@agent-think

agent-think Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

✅ agents import sizes: no significant changes (9f70bc82 → ae2d3dee, workflow run)

@devin-ai-integration devin-ai-integration Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.

Devin Review found 3 potential issues.

Devin Review

Comment on lines +355 to +356
override async onAgentToolFinish(run: AgentToolRunInfo): Promise<void> {
await this.destroySandbox(run.runId);

@devin-ai-integration devin-ai-integration Bot Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Interrupted containers outlive completed child runs

When an interrupted child finishes after releaseIdleSandboxes sees it running, its container receives no cleanup. onStart runs the sweep only on orchestrator wakes; child completion does not trigger another wake. Those containers can exhaust the five-instance limit before they sleep.

Learn more

The orchestrator tracks each container in storage. A finish callback can mark a run interrupted while its child continues, so the callback leaves the container alive. The sweep runs once when the parent wakes and skips any child it finds running. The child can finish after that sweep without sending another finish callback to the parent. Without another parent wake, the container stays allocated until the sandbox's 15-minute idle sleep; the example permits only five concurrent containers.

Example: A child is still running when its parent recovers and reports interrupted. The wake sweep sees running and retains its container. The child finishes one minute later, but the parent has no other requests for ten minutes. Further delegated tasks can reach max_instances: 5 despite the earlier children having finished.

Recommended fix: Schedule a durable follow-up cleanup for tracked interrupted runs, such as a periodic alarm or scheduled callback that re-inspects the child until it is terminal and calls destroySandbox. Avoid relying on an unrelated request to wake the orchestrator.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — fixed in e163805. onAgentToolFinish now keeps the sandbox when the status is interrupted and childStillRunning !== false (unknown treated as maybe-running); it's freed later by the wake sweep, a later finish, or clearDelegatedRuns.

Comment thread examples/sandbox-coding-agent/src/server.ts
Comment thread packages/think/src/think.ts Outdated
@pkg-pr-new

pkg-pr-new Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

agents

npm i https://pkg.pr.new/agents@2388

@cloudflare/ai-chat

npm i https://pkg.pr.new/@cloudflare/ai-chat@2388

@cloudflare/codemode

npm i https://pkg.pr.new/@cloudflare/codemode@2388

hono-agents

npm i https://pkg.pr.new/hono-agents@2388

@cloudflare/shell

npm i https://pkg.pr.new/@cloudflare/shell@2388

@cloudflare/think

npm i https://pkg.pr.new/@cloudflare/think@2388

@cloudflare/voice

npm i https://pkg.pr.new/@cloudflare/voice@2388

@cloudflare/worker-bundler

npm i https://pkg.pr.new/@cloudflare/worker-bundler@2388

commit: ae2d3de

threepointone and others added 3 commits September 27, 2026 16:59
- think: resolveModel accepts only @cf/ and @hf/ Workers AI ids with a
  model name; other @-prefixed ids fail with the invalid-id error. @hf/ ids
  get the same Workers AI settings as @cf/ ids
- sandbox-coding-agent: keep the container of an interrupted run whose child
  may still be working; sweep tracked containers on wake and destroy those
  whose child run is no longer running (covers failed starts)

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

# Conflicts:
#	packages/think/src/think.ts
Co-authored-by: Cursor <cursoragent@cursor.com>

# Conflicts:
#	packages/think/src/tests/hooks.test.ts
#	packages/think/src/think.ts

@devin-ai-integration devin-ai-integration Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.

Devin Review found 3 new potential issues.

Devin Review

Comment thread examples/sandbox-coding-agent/src/server.ts
Comment on lines +195 to 205
if (failure) {
const tail = stderr.trim().split("\n").slice(-12).join("\n");
writer.write({ type: "text-start", id: "error" });
writer.write({
type: "text-delta",
id: "error",
delta:
`\n\n**Claude Code error**\n\n${detail}` +
`\n\n**Claude Code error**\n\n${failure}` +
(tail ? `\n\n\`\`\`\n${tail}\n\`\`\`` : "")
});
writer.write({ type: "text-end", id: "error" });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Recovered Claude failures become successful runs

If a child restarts after writing an error message but before recording its outcome, inspectAgentToolRun treats that assistant message as success. The parent reports the failed coding task as completed.

Learn more

A Claude failure writes human-readable error text into the assistant stream before throwing. If the child Durable Object restarts between persisting that text and finalizing the agent-tool row, reconciliation treats any assistant message after the run started as proof of success. This error message satisfies that condition, so inspectAgentToolRun reports completed even though Claude failed.

Example: Claude exits with code 1 and streams the error text; the child is evicted before startAgentToolRun stores error. On recovery, the error assistant message causes a completed status, and the orchestrator reports the task as done.

Recommended fix: Persist a durable failure marker the child reconciliation can read, or update AIChatAgent reconciliation to respect a persisted stream-error outcome before inferring completion from assistant messages. Test a restart between the error chunk and terminal row update.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accurate, but it's an SDK-level reconciliation gap rather than an issue in this example: AIChatAgent's stale-run reconciliation (and Think's mirror) infers completed from any assistant message after the run started, so an eviction between persisting the error text and finalizing the row reads as success. The right fix is in the reconciliation itself (respect a durable failure/stream-error marker before inferring completion), which affects every agent tool, so I'm tracking it as a separate follow-up rather than widening this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #2390: the child now records its stream error on the open run row as it happens, and stale-row reconciliation seals such a row as error instead of inferring completed from the assistant reply (both AIChatAgent and Think).

Comment thread examples/sandbox-coding-agent/src/server.ts
…ire a bounded max_tokens

Co-authored-by: Cursor <cursoragent@cursor.com>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

Devin Review

Comment on lines +379 to +380
if (result.status === "error" && (await this.isChildLive(run.runId))) {
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Finished child sandboxes exhaust container slots

When a child stream fails, onAgentToolFinish retains the live child's sandbox without arranging cleanup after it finishes. releaseIdleSandboxes runs only on orchestrator startup, so an active orchestrator leaves these sandboxes allocated. Repeated failures can exhaust the five-container limit and block new tasks.

Learn more

The parent tracks a sandbox when a run starts, and its finish hook normally destroys that sandbox. A child-stream failure can mark the parent run error while the child still reports running. This branch retains the sandbox, but releaseIdleSandboxes runs only in onStart. An orchestrator that stays active does not run that sweep again when the child finishes. Its retained sandbox continues occupying a container slot until the container sleeps or the orchestrator restarts.

Example: Five child streams fail while their children keep editing. Each child finishes later, but the same orchestrator remains active. All five sandboxes remain allocated, so a sixth task cannot start within the configured max_instances: 5 limit.

Recommended fix: Schedule a durable follow-up inspection for retained runs, or arrange a child-completion callback that destroys the sandbox once inspectAgentToolRun reports a terminal state. Ensure the follow-up survives orchestrator eviction and retries transient inspection failures.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.


- **`/agents/*` has no authentication.** Anyone who can reach the Worker can chat with any orchestrator by name, start containers, and spend your Workers AI and AI Gateway budget. Put it behind [Cloudflare Access](https://developers.cloudflare.com/cloudflare-one/policies/access/) or add an auth check before `routeAgentRequest` before deploying.
- **The container runs model-directed commands with internet access.** Claude Code runs with `--permission-mode bypassPermissions`, so it executes whatever shell commands it decides on, without approval, and the container can reach the public internet (`enableInternet = true`, needed for the `git clone`). Don't put anything in the container you wouldn't hand to an untrusted process.
- **The Anthropic proxy is bounded, not locked down.** It only forwards `POST /v1/messages` and `POST /v1/messages/count_tokens` and rejects message requests whose `max_tokens` is missing or above `MAX_OUTPUT_TOKENS`, but any process in the container can still call those endpoints on your account. Set spend limits / rate limits on the gateway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Cleanup example differs from the implementation

The README's finish-hook example destroys a sandbox after an error, even when its child is still editing. The implementation keeps that sandbox. Update the example so readers do not copy the conflicting cleanup behavior.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

@threepointone
threepointone merged commit 1031c0f into main Sep 27, 2026
14 of 16 checks passed
@threepointone
threepointone deleted the fix/think-sandbox-review branch September 27, 2026 18:49
@github-actions github-actions Bot mentioned this pull request Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant