From 6d9934103d95e45b6a53a7ff5722d36659077fcc Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 15 Jun 2026 01:39:21 +0800 Subject: [PATCH] docs: address PR review on RFC 012 (concurrency, vm guard, prompt budget) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Concurrency: change from "may serialize" to mandatory serialize-by-default via a per-run dispatch queue in the SDK bindings, with a non-overlap test as a hard acceptance criterion (the binding shape otherwise makes Promise.all dispatch concurrently before the tool contract has concurrency-safety metadata). - node:vm guard: make it enforceable, not a README warning — CodeRuntime exposes safe:boolean, the VM stub throws unless constructed { unsafe:true }, and code-mode refuses to register run_code over an unsafe runtime unless separately acknowledged (allowUnsafeRuntime); refusal path is tested. - Prompt budget: drop the "zero prompt tokens" claim (the SDK .d.ts is injected into the system prompt, so types do consume context) and add the explicit budget/caching tradeoff — Code Mode's saving is on output/round-trips, not the input-side tool description. --- docs/rfc/012-optional-code-mode.md | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/docs/rfc/012-optional-code-mode.md b/docs/rfc/012-optional-code-mode.md index e72f240373..1970899083 100644 --- a/docs/rfc/012-optional-code-mode.md +++ b/docs/rfc/012-optional-code-mode.md @@ -16,17 +16,27 @@ This RFC proposes an **optional** Code Mode for the DeepSeek Harness, covering * The design follows the codebase's capability-seam pattern ([ADR 0009](../adr/0009-capability-seams.md), the `bash` template) as a three-package split, plus one consumer plugin. Nothing in `dsh-session`, `dsh-agent`, `dsh-agent-loop`, `dsh-llm`, `dsh-tools`, or `dsh-system-prompt` changes. -**Prior art.** `@cloudflare/codemode` validates this shape directly and several of its decisions are adopted below. Its `Executor` interface is deliberately tiny — `execute(code, fns) → { result, error?, logs? }` — with a production `DynamicWorkerExecutor` (isolated Workers) and a six-line `NodeVMExecutor` example as two implementations behind it: exactly the interface/implementation split ADR 0009 prescribes. It generates TypeScript type definitions from tools for the model's context and runs the generated JavaScript in a sandbox, capturing console output alongside the return value. It normalizes model output into an async arrow function via AST parsing (acorn) and sanitizes tool names into valid JS identifiers (`my-tool` → `my_tool`, `delete` → `delete_`). It blocks outbound network by default. The transferable lessons — minimal executor contract, host-side type generation costing zero prompt tokens, capture-output-and-return-value, name sanitization, AST-normalize the code, isolate by default — are folded into the design below. What does **not** transfer is the substrate: Cloudflare's isolation is Workers-specific; our equivalent hardened substrate is the deferred follow-up. +**Prior art.** `@cloudflare/codemode` validates this shape directly and several of its decisions are adopted below. Its `Executor` interface is deliberately tiny — `execute(code, fns) → { result, error?, logs? }` — with a production `DynamicWorkerExecutor` (isolated Workers) and a six-line `NodeVMExecutor` example as two implementations behind it: exactly the interface/implementation split ADR 0009 prescribes. It generates TypeScript type definitions from tools for the model's context and runs the generated JavaScript in a sandbox, capturing console output alongside the return value. It normalizes model output into an async arrow function via AST parsing (acorn) and sanitizes tool names into valid JS identifiers (`my-tool` → `my_tool`, `delete` → `delete_`). It blocks outbound network by default. The transferable lessons — minimal executor contract, host-side type derivation, capture-output-and-return-value, name sanitization, AST-normalize the code, isolate by default — are folded into the design below. What does **not** transfer is the substrate: Cloudflare's isolation is Workers-specific; our equivalent hardened substrate is the deferred follow-up. + +**Prompt-budget tradeoff (Code Mode is not unconditionally cheaper).** Deriving the SDK types host-side costs no extra *discovery* round-trip, but the generated `.d.ts` is injected into the system prompt (§3a), so the type definitions themselves **do** consume context — and for an all-tools SDK that cost scales with every registered tool and can be comparable to, or larger than, the native JSON schemas it replaces. Code Mode's saving is on the **output/result** side (the model curates what comes back; intermediate results never re-enter context) and on **round-trips** (compose many calls in one program), not on the input-side tool description. The net win is workload-dependent: it pays off for multi-call, large-intermediate-result workflows and can cost *more* for a single call against a large tool surface. The `.d.ts` section is a prefix-stable prompt prefix, so prompt caching amortizes its per-turn cost across a session; the RFC notes that caching is what keeps the injected SDK affordable, and that a deployment with a very large tool surface should weigh the SDK size against native schemas rather than assume Code Mode is strictly cheaper. **1. Interface package `packages/code-runtime/`** — a new package `@deepseek-ai/dsh-code-runtime` owning `ctx.codeRuntime`, depending only on `cordis`. It defines an abstract `CodeRuntime extends Service` plus the execution vocabulary. The runtime knows **nothing** about `ctx.tools`: it is handed a set of named async functions (the resolved SDK bindings), runs the program, and captures output. The result shape mirrors Cloudflare's proven-minimal contract so an error is a *field on a resolved result*, not a throw the runtime is expected to make: - `CodeRunRequest = { code: string; sdk: SdkBinding[]; signal?: AbortSignal }` - `CodeRunResult = { result: unknown; logs: string[]; error?: string }` +- a readonly `safe: boolean` on the `CodeRuntime` service — `false` for an unsandboxed stub, `true` only for a real isolating substrate; consumers gate on it (§2). - `SdkBinding = { namespace: string; fns: Record Promise> }` Per the "explicit > implicit at seams" convention, the request spells out every field the runtime acts on; defaulting (e.g. an output cap, a timeout derived from `signal`) is the implementation's explicit job, not a hidden `?? default` inside `run()`. The split into interface + implementation is justified under ADR 0009 because there is **genuinely more than one planned implementation** — the node:vm stub *and* the hardened substrate (a real isolate, or the generated program run as a sandboxed process through the existing `ctx.bash` seam) that is scheduled follow-up work, not speculative optionality. ADR 0009 warns against splitting preemptively when only one implementation is conceivable; here a second is not just conceivable but required before any untrusted use, so the seam earns its keep. -**2. Implementation package `packages/code-runtime-vm/`** — a new package `@deepseek-ai/dsh-code-runtime-vm`, the `node:vm` reference stub. It type-erases the model's TypeScript via the compiler's `transpileModule` (or sucrase) — the types exist only to guide the model; the runtime is plain JS — then wraps the body in an async IIFE for top-level `await` (Cloudflare's `NodeVMExecutor` does literally `new AsyncFunction("codemode", "return await (${code})()")`), runs it in a `vm.Context` whose globals are a capturing `console` and the SDK namespace objects, awaits the IIFE, and captures the return value, the buffered logs, and any thrown error (as `error: string`). It applies an **output cap** (truncate captured logs) and a **timeout tied to `request.signal`**. These caps limit blast radius; **they are not a security boundary**. node:vm is **not** isolation: withholding `require`/`process` does not contain anything (code escapes via `constructor`/prototype reflection), and per [AGENTS.md](../../AGENTS.md) the harness must never hand model output the ambient environment. `code-runtime-vm` is therefore documented as **reference / test-only / unsafe-for-untrusted-input**, acceptable in the MVP only because the code runs at harness trust. Signal handling is best-effort: it aborts in-flight sub-dispatches but cannot reliably interrupt a hot synchronous loop (`while(true){}`) in node:vm — another reason the hardened substrate is deferred, not optional-forever. +**2. Implementation package `packages/code-runtime-vm/`** — a new package `@deepseek-ai/dsh-code-runtime-vm`, the `node:vm` reference stub. It type-erases the model's TypeScript via the compiler's `transpileModule` (or sucrase) — the types exist only to guide the model; the runtime is plain JS — then wraps the body in an async IIFE for top-level `await` (Cloudflare's `NodeVMExecutor` does literally `new AsyncFunction("codemode", "return await (${code})()")`), runs it in a `vm.Context` whose globals are a capturing `console` and the SDK namespace objects, awaits the IIFE, and captures the return value, the buffered logs, and any thrown error (as `error: string`). It applies an **output cap** (truncate captured logs) and a **timeout tied to `request.signal`**. These caps limit blast radius; **they are not a security boundary**. node:vm is **not** isolation: withholding `require`/`process` does not contain anything (code escapes via `constructor`/prototype reflection), and per [AGENTS.md](../../AGENTS.md) the harness must never hand model output the ambient environment. + +**The unsafe-runtime guard is enforceable, not a README warning.** Because a README caveat is not a control — and AGENTS.md's "never hand model output ambient authority" is a hard rule, not advice — the design makes the danger refuse to run by construction. Two layers: + +- **The runtime declares its trust level.** `CodeRuntime` carries a readonly `safe: boolean` (a `node:vm`-class stub returns `safe = false`; a real isolate/sandboxed-process substrate returns `safe = true`). The `code-runtime-vm` constructor *additionally* requires an explicit opt-in — `new VmCodeRuntime({ unsafe: true })` — and **throws** if that flag is absent, so merely depending on the package and wiring it cannot silently produce a live unsafe runtime; the operator must type the word `unsafe`. +- **The consumer refuses to expose `run_code` over an unsafe runtime by default.** When `code-mode` initializes, if `ctx.codeRuntime.safe === false` it does **not** register `run_code` unless the plugin itself is configured with an explicit acknowledgement (e.g. `code-mode` config `allowUnsafeRuntime: true`). Absent that, it logs a typed error and registers nothing — so a real model never reaches an unsandboxed runtime by a single config slip. The refusal path is tested: with the acknowledgement unset and an unsafe runtime, `run_code` is absent (and the wire tool list is unchanged from native); with both opt-ins set, it registers and runs. This keeps the unsafe reference backend usable for tests and trusted local demos while making production misuse take two deliberate, greppable flags rather than one mistake. + +`code-runtime-vm` is therefore documented as **reference / test-only / unsafe-for-untrusted-input**, acceptable in the MVP only because the code runs at harness trust *and* both opt-in flags must be set. Signal handling is best-effort: it aborts in-flight sub-dispatches but cannot reliably interrupt a hot synchronous loop (`while(true){}`) in node:vm — another reason the hardened substrate is deferred, not optional-forever. **3. Consumer plugin `packages/code-mode/`** — a new package `@deepseek-ai/dsh-code-mode`, the plugin that wires everything together. It declares `inject = ['tools', 'systemPrompt', 'codeRuntime']` — Cordis throws on access to a service that is not injected, and keeps the plugin inactive until all three exist (the same pattern as `tool-bash`'s `inject = ['tools', 'bash']`), which also gives correct load-ordering relative to `code-runtime`/`code-runtime-vm`. The plugin contributes four things, all through existing seams: @@ -48,7 +58,7 @@ Per the "explicit > implicit at seams" convention, the request spells out every **SDK codegen.** A pure `jsonSchemaToTs(schema)` in `code-mode` maps the JSON-schema subset the `defineTool` DSL produces (object/string/number/boolean/array, `properties`, `required[]`, `enum` → string-literal union, nested objects, array `items`) to a TS type literal. It is **total**: any unsupported construct (`$ref`, `oneOf`/`anyOf`, `integer`, `null`, `additionalProperties`, or any raw MCP shape it does not recognize) degrades to `unknown` without throwing — it never crashes codegen. Typing is best-effort, not a guarantee, because MCP tools accept arbitrary JSON Schema and `ToolSchema.parameters` is typed only as `Record`. Because `ToolSchema.name` is an arbitrary string (not necessarily a valid TS identifier), the SDK is generated as a **namespace with quoted access** (e.g. `tools["some-mcp-tool"](args)`) plus safe camelCase aliases where the name is a clean identifier; alias collisions and TS reserved words fall back to quoted-only access (no duplicate alias emitted). This mirrors Cloudflare's `sanitizeToolName`. `run_code` itself is filtered out of the SDK. The MVP surfaces text content only; image and other block types in sub-results are deferred (noted as a limitation). -**Concurrency — possible, not guaranteed safe.** The SDK functions are async, so the model *can* write `await Promise.all([...])` and Code Mode would dispatch those calls concurrently — the parallel-tools capability the loop itself still lacks. But the tool contract carries **no concurrency-safety metadata today**, and parallel tool execution is an open TODO in `dsh-tools`. So this RFC does **not** claim parallel SDK calls are free or naturally safe. MVP policy: document the capability with a footgun warning (concurrent calls share the same `tools/execute` seam and may race inside a not-yet-hardened tool), and make a future tool-metadata flag (read-only / concurrency-safe) the prerequisite for endorsing it. The MVP may serialize sub-dispatches; the SDK `.d.ts` can annotate which tools are safe to parallelize once that metadata exists. +**Concurrency — serialized by default (the binding must enforce it).** The SDK functions are async, so a model writing `await Promise.all([tools.a(...), tools.b(...)])` would *start both* immediately, and each would call `ctx.tools.execute` right away — i.e. the binding shape makes concurrent dispatch the **default**, not an opt-in. Because the tool contract carries **no concurrency-safety metadata today** (parallel tool execution and a concurrency-safety hint are an open TODO in both `dsh-tools` and [docs/architecture.md](../architecture.md): "phase 1 executes tool calls sequentially"), concurrent dispatch through a not-yet-hardened tool may race. So a prose "may serialize" is not sufficient. **Decision: the MVP SDK bindings enforce serialization** — each `run_code` invocation owns a per-run dispatch queue, and every `invoke()` chains onto it (`tail = tail.then(() => ctx.tools.execute(...))`), so even `Promise.all` over SDK calls executes them one at a time in submission order. This is a hard acceptance criterion, with a test that issues `Promise.all([...])` from a program and asserts the underlying `ctx.tools.execute` calls did **not** overlap (e.g. a probe tool records enter/exit and the test asserts no interleaving). The `.d.ts` may *describe* the model-visible functions as async (they are), but the implementation guarantees serial execution. Lifting serialization is deferred: only once a tool can declare itself read-only / concurrency-safe does the binding allow those specific tools to overlap. The same per-run queue is where the before/after abort checks (§3c) live, so an aborted run drains no further queued dispatches. **Tool visibility tiers (design intentionally skipped).** A natural extension is to mark each tool with a *visibility tier*: some tools "direct-call eligible" (still offered as native wire tools alongside `run_code`), some "code-mode only" (reachable solely from within a `run_code` program, never on the wire), and the default "both." This would let a deployment keep a few high-frequency or approval-gated tools as direct calls while routing the long tail through Code Mode, or hide composition-only primitives from the native surface entirely. This RFC notes the possibility but **intentionally skips the detailed design** — the per-tool metadata, how it interacts with the `agent/request` enforcement in 3b, and the presentation split in 3a are left to a follow-up. The MVP is the simple two-state model: Code Mode on (everything via `run_code`) or off (everything native). @@ -56,20 +66,20 @@ Per the "explicit > implicit at seams" convention, the request spells out every ## Plan -1. Scaffold the interface package `packages/code-runtime/` per [the cookbook](../cookbook/adding-a-package.md): abstract `CodeRuntime extends Service` (`super(ctx, 'codeRuntime')`), the `declare module 'cordis'` ctx key, the `CodeRunRequest`/`CodeRunResult`/`SdkBinding` vocabulary, method contracts documented in JSDoc (what `run` captures, abort semantics, that an error is a result field not a throw). HMR-safety test (dispose the contributing fiber, assert `ctx.codeRuntime` is gone). -2. Scaffold the implementation package `packages/code-runtime-vm/`: the node:vm stub — transpile/type-erase, async-IIFE wrap, capturing `console`, SDK globals, return-value/logs/error capture, output cap, signal-tied timeout. Tests for output capture, return value, error-as-field, abort, and a README documenting the "not a sandbox, trusted-only" caveat prominently. -3. Scaffold the consumer plugin `packages/code-mode/`: `jsonSchemaToTs` codegen with namespace/quoted-access + alias handling (unit tests, including non-identifier MCP names and unsupported-shape → `unknown`); the registered lazy `ctx.systemPrompt.section()` carrying the SDK `.d.ts`; the `agent/request` listener (`prepend: true`) collapsing `request.tools` to `[run_code]` after `await next()`; the `run_code` tool with the dispatch bridge (deterministic sub-call ids, before/after abort checks, `CodeRunError` on error results); and the `code/dispatch` event declared here via `SessionEventMap` merge. Declare `inject = ['tools', 'systemPrompt', 'codeRuntime']`. -4. Tests: HMR-safety (dispose removes the tool, the section, and the listener); a waterfall test that the wire tool list is exactly `[run_code]` (spy adapter, asserting via `agent/request` and optionally `llm/stream`); an integration test that a program calling two tools returns only its printed/returned output (verify the world, not the self-report); `deriveMessages()` ignores `code/dispatch`; abort mid-program stops further dispatches; `CodeRunError` surfaces as `isError: true`. -5. Wire an example: `examples/coding-agent-code-mode` (or a config flag on the existing example) loading the trio. Any example that runs a real model through `code-runtime-vm` is marked explicitly unsafe, or uses a mock model — never a real model against the unsandboxed stub by default. Add a `yarn demo:*` entry. +1. Scaffold the interface package `packages/code-runtime/` per [the cookbook](../cookbook/adding-a-package.md): abstract `CodeRuntime extends Service` (`super(ctx, 'codeRuntime')`) with a readonly `safe: boolean`, the `declare module 'cordis'` ctx key, the `CodeRunRequest`/`CodeRunResult`/`SdkBinding` vocabulary, method contracts documented in JSDoc (what `run` captures, abort semantics, that an error is a result field not a throw, what `safe` means). HMR-safety test (dispose the contributing fiber, assert `ctx.codeRuntime` is gone). +2. Scaffold the implementation package `packages/code-runtime-vm/`: the node:vm stub — `safe = false`, a constructor that **throws unless given `{ unsafe: true }`**, transpile/type-erase, async-IIFE wrap, capturing `console`, SDK globals, return-value/logs/error capture, output cap, signal-tied timeout. Tests for output capture, return value, error-as-field, abort, the constructor refusal without `unsafe`, and a README documenting the "not a sandbox, trusted-only" caveat prominently. +3. Scaffold the consumer plugin `packages/code-mode/`: `jsonSchemaToTs` codegen with namespace/quoted-access + alias handling (unit tests, including non-identifier MCP names and unsupported-shape → `unknown`); the registered lazy `ctx.systemPrompt.section()` carrying the SDK `.d.ts`; the `agent/request` listener (`prepend: true`) collapsing `request.tools` to `[run_code]` after `await next()`; the **unsafe-runtime gate** (refuse to register `run_code` when `ctx.codeRuntime.safe === false` unless `allowUnsafeRuntime` is set); the `run_code` tool with the dispatch bridge (per-run serialization queue, deterministic sub-call ids, before/after abort checks, `CodeRunError` on error results); and the `code/dispatch` event declared here via `SessionEventMap` merge. Declare `inject = ['tools', 'systemPrompt', 'codeRuntime']`. +4. Tests: HMR-safety (dispose removes the tool, the section, and the listener); a waterfall test that the wire tool list is exactly `[run_code]` (spy adapter, asserting via `agent/request` and optionally `llm/stream`); an integration test that a program calling two tools returns only its printed/returned output (verify the world, not the self-report); a **serialization test** that `Promise.all([...])` over SDK calls does not overlap the underlying `ctx.tools.execute` invocations (a probe tool records enter/exit; assert no interleaving); `deriveMessages()` ignores `code/dispatch`; abort mid-program stops further dispatches; `CodeRunError` surfaces as `isError: true`; and the **unsafe-runtime refusal test** (§3, the VM-guard): with the unsafe flag unset, a non-mock agent's `run_code` is refused; with it set, the program runs. +5. Wire an example: `examples/coding-agent-code-mode` (or a config flag on the existing example) loading the trio. Running it against the node:vm stub requires both opt-ins (`VmCodeRuntime({ unsafe: true })` and `code-mode`'s `allowUnsafeRuntime`); the example sets them explicitly and comments why, or uses a mock model — a real model never reaches the unsandboxed stub without those deliberate flags. Add a `yarn demo:*` entry. 6. Docs: update [docs/architecture.md](../architecture.md) (a `ctx.codeRuntime` row in the service map, a Code Mode note under the tool pipeline / capability seams sections); add a [cookbook](../cookbook/) note on writing a `CodeRuntime` backend; and **file the follow-up RFC for the hardened execution substrate** (the isolate/sandboxed-process design, plus the tool-visibility-tier design skipped here). Append the `| 012 | … | proposed |` row to [the RFC index](README.md). ## Risks -node:vm is not a sandbox. This is the single biggest caveat. Withholding `require`/`process` is not a boundary; the MVP runs at harness trust only; the hardened substrate is a hard prerequisite before any untrusted use and is the explicit subject of a follow-up RFC. The reference stub and every README around it say so loudly. +node:vm is not a sandbox. This is the single biggest caveat. Withholding `require`/`process` is not a boundary; the MVP runs at harness trust only; the hardened substrate is a hard prerequisite before any untrusted use and is the explicit subject of a follow-up RFC. The guard is enforceable, not just documented: the runtime exposes `safe: boolean`, the VM stub throws unless constructed with `{ unsafe: true }`, and `code-mode` refuses to register `run_code` over an unsafe runtime unless separately acknowledged (`allowUnsafeRuntime`) — production misuse requires two deliberate, greppable flags, and the refusal path is tested. Wrong seam would leak tools. If the wire tool list were enforced only in `system-prompt/assemble`, a later `agent/request` listener could re-add tools. Mitigation: enforce `request.tools = [run_code]` in the `agent/request` waterfall (the authoritative seam, run last before `llm.stream()`) with `prepend: true`, and assert exactly one wire tool in tests. The residual `llm/stream` caveat is documented, not hidden. -Concurrency is not yet safe. The RFC does not endorse parallel SDK calls as free; it documents the footgun and makes a tool concurrency-safety metadata flag the prerequisite. The MVP may serialize sub-dispatches. +Concurrency before the contract supports it. The binding shape makes concurrent dispatch the default, and the tool contract has no concurrency-safety metadata yet, so unguarded `Promise.all` over SDK calls could race a not-yet-hardened tool. Mitigation: the MVP bindings enforce a per-run serialization queue (every `invoke` chains onto the previous), with a test asserting `Promise.all` from a program does not overlap the underlying `ctx.tools.execute` calls. Per-tool parallelism is unlocked only once a tool can declare itself concurrency-safe. Two presentation modes to keep coherent. A tool added later must work in both native and Code Mode. Mitigation: both the codegen thunk and the `agent/request` listener read `ctx.tools.schemas()`, so coverage is automatic; a test asserts every registered schema produces valid `.d.ts`, including non-identifier MCP names via quoted access.