From fe4da9244f53cdbf66bcd9ce3cdaa6fc09dca362 Mon Sep 17 00:00:00 2001 From: imccyu <276526105+imccyu@users.noreply.github.com> Date: Thu, 9 Jul 2026 22:12:30 +0800 Subject: [PATCH] fix(tool-cordis): validate a dynamic tool's execute return shape after the realm round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sandbox execute wrapper JSON round-tripped the return and blindly cast it to ToolExecuteReturn. A JSON-valid but wrong-shape return — a bare string, { content: 'ok' }, blocks without a type tag — sailed through: the registry spreads result.content, so { content: 'ok' } became ['o','k'], passed the session log's isJsonValue gate, and the DeepSeek serializer then flattened it to '(no output)' — silent corruption of the next model request and every replay, instead of a contained tool error. The round-tripped value is now shape-checked against the two ToolExecuteReturn forms (array of content blocks, or { content: blocks, meta? }); block checks are structural only (plain object + string type tag) because the ContentBlock union is merge-extensible. A wrong shape — and the formerly cryptic forgot-return/bare-string cases — fails that one call with a teaching error echoing a truncated preview of what was returned and the two valid forms. New specs pin the object-form pass-through (meta included), six rejection shapes, and the preview truncation; per-file 100% coverage holds. --- ...6-07-08-self-referential-cordis-toolset.md | 2 +- packages/cordis/tool-cordis/src/guard.ts | 74 +++++++++++++-- .../cordis/tool-cordis/tests/mount.spec.ts | 89 +++++++++++++++++++ 3 files changed, 158 insertions(+), 7 deletions(-) diff --git a/docs/rfc/implemented/feature/2026-07-08-self-referential-cordis-toolset.md b/docs/rfc/implemented/feature/2026-07-08-self-referential-cordis-toolset.md index 8a43c271c3..3ec6c75cbc 100644 --- a/docs/rfc/implemented/feature/2026-07-08-self-referential-cordis-toolset.md +++ b/docs/rfc/implemented/feature/2026-07-08-self-referential-cordis-toolset.md @@ -30,7 +30,7 @@ Mount code runs via `vm.createContext` + `runInContext`, wrapped as the body of Sandbox globals are deliberately small: a tagged write-through `console` (`[cordis:] …` on the host stdout/stderr, so a listener that fires long after the mount call still lands somewhere the user sees), the `harness.defineTool` / `harness.registerTool` registration pair, the encoding primitives fresh vm contexts lack (`btoa`/`atob` as host closures over `Buffer` — a sanctioned exception, `Buffer` itself is never exposed — plus `TextEncoder`/`TextDecoder`), and callable traps over the withheld Node APIs (`require`, `setTimeout`/`setInterval`/`setImmediate`/`clearTimeout`/`clearInterval`, `fetch`) that throw a redirect naming the cordis alternative. Only function-shaped globals are trapped; `process` and `Buffer` stay `undefined` so a `typeof` feature probe stays inert rather than detonating a throwing accessor. -Three boundary mechanisms make model-written code behave correctly across the realm seam. **Dual-realm `instanceof`**: most objects sandbox code touches are host-realm (tool `args`, event payloads, service returns), so a plain `x instanceof Array` in the vm would silently be false — a per-sandbox prelude gives the vm realm's own constructors a `Symbol.hasInstance` that checks both the vm constructor and its host counterpart, patching only vm-realm globals. **Realm normalization of tool results**: objects built inside the vm carry the vm realm's `Object.prototype`, which the session log's append-time plainness check (`isJsonValue` in `dsh-session`, a prototype-identity comparison) rejects, so the sandbox's `harness.defineTool` JSON round-trips every `execute` return into the host realm — which also projects it onto exactly what the log durably stores. **A whitelist context façade**: the `ctx` a mounted plugin's `apply` receives is NOT the real context nor a pass-through proxy over it — it is a façade exposing only what a mount legitimately needs (`tools.register` marker-guarded, a read-only `tools.get`/`schemas`, `on`/`once`, `provide`, the timer helpers, and the services the plugin DECLARED in `inject`), with every framework-plumbing member (`root`, `parent`, `fiber`, `reflect`, `registry`, `extend`, `isolate`, `intercept`, `plugin`, `set`, `mixin`, …) denied with a teaching error. This closes an escape *class* rather than a single hole: a proxy that merely special-cased `ctx.tools` still handed back the raw context through `ctx.root`, `ctx.extend()`, or a service instance's `.ctx`, and mount code could then `ctx.root.tools.register({…})` to bypass the marker check and realm normalization — a raw vm-realm result then errors a real agent turn at the plainness check. The façade has no context-valued member to reach, and the one indirect leak (an injected-service method returning a `Context`) is rejected on the way back to sandbox code. Two narrower rules complete the surface. First, **service access requires an `inject` declaration**: reaching a service the mount did not declare is refused even when a global provider is live — otherwise a mount could depend on a provider cordis never sees, and unmounting that provider would neither park the consumer nor unwind the tools it registered, leaving a model-visible tool that fails only at execution time. Because the read is gated on the declaration, cross-mount `provide`/`inject` keeps its lifecycle guarantees (the plugin's own `inject` and the fiber's pending/active gating drive activation and unload); only the `apply`-time `ctx` surface is narrowed. Second, **`ctx.tools.get` returns a read-only schema view** (name/description/parameters), never the live `ToolDefinition` — handing back the definition would expose its `execute`, letting mount code call another tool directly and bypass `ToolRegistry.execute` and its pre/post-execute hooks and accounting; a mount that wants to invoke a tool must go through the registry, and one that wants to introspect gets the same view `schemas()` returns. +Three boundary mechanisms make model-written code behave correctly across the realm seam. **Dual-realm `instanceof`**: most objects sandbox code touches are host-realm (tool `args`, event payloads, service returns), so a plain `x instanceof Array` in the vm would silently be false — a per-sandbox prelude gives the vm realm's own constructors a `Symbol.hasInstance` that checks both the vm constructor and its host counterpart, patching only vm-realm globals. **Realm normalization of tool results**: objects built inside the vm carry the vm realm's `Object.prototype`, which the session log's append-time plainness check (`isJsonValue` in `dsh-session`, a prototype-identity comparison) rejects, so the sandbox's `harness.defineTool` JSON round-trips every `execute` return into the host realm — which also projects it onto exactly what the log durably stores — and then shape-checks it against the two `ToolExecuteReturn` forms, so a JSON-valid but wrong-shape return (a bare string, `{ content: 'ok' }`) fails that one call with a teaching error instead of entering the log as corrupt tool-result content. **A whitelist context façade**: the `ctx` a mounted plugin's `apply` receives is NOT the real context nor a pass-through proxy over it — it is a façade exposing only what a mount legitimately needs (`tools.register` marker-guarded, a read-only `tools.get`/`schemas`, `on`/`once`, `provide`, the timer helpers, and the services the plugin DECLARED in `inject`), with every framework-plumbing member (`root`, `parent`, `fiber`, `reflect`, `registry`, `extend`, `isolate`, `intercept`, `plugin`, `set`, `mixin`, …) denied with a teaching error. This closes an escape *class* rather than a single hole: a proxy that merely special-cased `ctx.tools` still handed back the raw context through `ctx.root`, `ctx.extend()`, or a service instance's `.ctx`, and mount code could then `ctx.root.tools.register({…})` to bypass the marker check and realm normalization — a raw vm-realm result then errors a real agent turn at the plainness check. The façade has no context-valued member to reach, and the one indirect leak (an injected-service method returning a `Context`) is rejected on the way back to sandbox code. Two narrower rules complete the surface. First, **service access requires an `inject` declaration**: reaching a service the mount did not declare is refused even when a global provider is live — otherwise a mount could depend on a provider cordis never sees, and unmounting that provider would neither park the consumer nor unwind the tools it registered, leaving a model-visible tool that fails only at execution time. Because the read is gated on the declaration, cross-mount `provide`/`inject` keeps its lifecycle guarantees (the plugin's own `inject` and the fiber's pending/active gating drive activation and unload); only the `apply`-time `ctx` surface is narrowed. Second, **`ctx.tools.get` returns a read-only schema view** (name/description/parameters), never the live `ToolDefinition` — handing back the definition would expose its `execute`, letting mount code call another tool directly and bypass `ToolRegistry.execute` and its pre/post-execute hooks and accounting; a mount that wants to invoke a tool must go through the registry, and one that wants to introspect gets the same view `schemas()` returns. Boundary errors are written around the mistakes models actually make (see [Consequences](#consequences) for how each was found), and the boundary normalizes rather than lectures wherever the input has exactly one meaning: schema `parameters` accept the JSON-Schema dialect models write by strong prior — the `{ type: 'object', properties, required: […] }` wrapper unwraps to the SchemaSpec DSL (the `required` array becoming per-property flags, at any nesting level), `type: 'integer'` maps to `number`, and `required: false` reads as optional — while genuinely meaningless input is rejected with the vocabulary enumerated (an unknown type lists the five valid ones; a non-boolean `required` names the rule). The remaining teaching errors: an unbalanced `});` closing gets the vm's offending source line plus a "code is a function body" reminder; TypeScript syntax gets the remove-annotations fix (detected on the failing line only, so an ` as ` inside a description string does not misfire); a forgotten `return` gets the two valid plugin forms; a Node built-in call gets the redirect to its cordis service; a tool-name collision on re-mount gets the unmount-first-then-remount recipe. diff --git a/packages/cordis/tool-cordis/src/guard.ts b/packages/cordis/tool-cordis/src/guard.ts index 90eaaec41b..f51faeb42e 100644 --- a/packages/cordis/tool-cordis/src/guard.ts +++ b/packages/cordis/tool-cordis/src/guard.ts @@ -27,8 +27,12 @@ * realm's `Object.prototype`, and the session log's append-time plainness check * (`dsh-session`'s `isJsonValue`, a prototype-identity comparison) rejects * foreign-realm data — so every dynamic tool's `execute` return is JSON - * round-tripped into the host realm before it reaches the registry, and the - * schema itself is rebuilt as fresh host-realm objects. And a malformed tool + * round-tripped into the host realm and shape-checked against the two + * `ToolExecuteReturn` forms before it reaches the registry (the registry + * trusts the shape blindly — it spreads `result.content`, so an unvalidated + * `{ content: 'ok' }` would enter the session log as `['o','k']` and silently + * corrupt the next model request), and the schema itself is rebuilt as fresh + * host-realm objects. And a malformed tool * schema must fail at REGISTRATION, not when a later request assembles it — so * dynamic tool registration accepts only definitions produced by the sandbox's * `harness.defineTool`, which normalizes `parameters` up front. @@ -137,14 +141,67 @@ function assertDynamicTool(tool: unknown): asserts tool is DynamicToolDefinition } } +/** + * Structurally a content block, checked AFTER the JSON round-trip: a plain + * object carrying a string `type` tag. Deliberately nothing deeper — the + * ContentBlock union is merge-extensible (an unknown tag must pass), and every + * downstream consumer dispatches on `type` and falls through unknowns. + */ +function isContentBlockShape(value: unknown): boolean { + return isPlainRecord(value) && typeof value.type === 'string' +} + +/** + * How much of an invalid execute return the teaching error echoes back — a + * huge blob would burn the model turn the error is trying to save. + */ +const RETURN_PREVIEW_LIMIT = 120 + +/** + * Compact JSON preview of an invalid execute return for the teaching error + * (`String(…)` for the un-stringifiable undefined case), truncated to + * {@link RETURN_PREVIEW_LIMIT}. + */ +function describeReturn(value: unknown): string { + // JSON.stringify is TYPED as always returning string, but it yields + // undefined for an undefined input (the routed forgot-return case) — the + // assertion widens the type back to the runtime truth. + const json = JSON.stringify(value) as string | undefined + if (json === undefined) return String(value) + return json.length > RETURN_PREVIEW_LIMIT ? `${json.slice(0, RETURN_PREVIEW_LIMIT)}…` : json +} + +/** + * Validate a round-tripped `execute` return against the two shapes + * {@link ToolExecuteReturn} allows: an ARRAY of content blocks, or + * `{ content: blocks, meta? }`. The registry trusts the shape blindly — it + * spreads `result.content`, so an unvalidated `{ content: 'ok' }` would enter + * the session log as `['o','k']` and silently corrupt the next model request — + * so a wrong shape fails THIS call with a teaching error instead. + */ +function assertExecuteReturn(value: unknown): ToolExecuteReturn { + if (Array.isArray(value) && value.every(isContentBlockShape)) { + return value as ToolExecuteReturn + } + if (isPlainRecord(value) && Array.isArray(value.content) && value.content.every(isContentBlockShape)) { + return value as ToolExecuteReturn + } + throw new Error( + `execute returned ${describeReturn(value)} — a tool's execute must return an ARRAY of content blocks, never a bare string:\n` + + ' ✓ return [{ type: \'text\', text: someString }]\n' + + ' ✓ return { content: [{ type: \'text\', text: someString }], meta: anyJsonValue }', + ) +} + /** * The `harness.defineTool` handed into the sandbox: the real DSL, with * `parameters` normalized into a fresh host-realm SchemaSpec (JSON-Schema * wrapper unwrapped, `integer` mapped, `required: false` dropped) and the * tool's `execute` return normalized into the host realm via a JSON round-trip - * (see the module doc). The round-trip also projects the return onto exactly - * what the log would durably store, so a non-JSON-serializable return surfaces - * as that one call's error instead of poisoning the turn. + * (see the module doc). The round-trip projects the return onto exactly what + * the log would durably store, and {@link assertExecuteReturn} then vets that + * projection — so a non-JSON-serializable OR wrong-shape return surfaces as + * that one call's teaching error instead of poisoning the turn. * @param options - the standard `defineTool` options; `parameters` may be the SchemaSpec DSL or a JSON-Schema-style wrapper. * @returns the marker-tagged definition `harness.registerTool` (and the guarded `ctx.tools.register`) accepts. */ @@ -155,7 +212,12 @@ export function sandboxDefineTool(options: Parameters[0]): To return markDynamicTool({ ...tool, async execute(args, exec) { - return JSON.parse(JSON.stringify(await execute(args, exec))) as ToolExecuteReturn + // JSON.stringify yields NO JSON for an undefined (or function/symbol) + // return despite its string-typed signature — route that into + // assertExecuteReturn's teaching error rather than letting JSON.parse + // throw its cryptic '"undefined" is not valid JSON'. + const json = JSON.stringify(await execute(args, exec)) as string | undefined + return assertExecuteReturn(json === undefined ? undefined : JSON.parse(json) as unknown) }, }) } diff --git a/packages/cordis/tool-cordis/tests/mount.spec.ts b/packages/cordis/tool-cordis/tests/mount.spec.ts index 7ee266ce30..fc29eeb2a0 100644 --- a/packages/cordis/tool-cordis/tests/mount.spec.ts +++ b/packages/cordis/tool-cordis/tests/mount.spec.ts @@ -60,6 +60,95 @@ describe('cordis_mount', () => { expect(isJsonValue({ content: reversed.content, isError: reversed.isError })).toBe(true) }) + it('threads the { content, meta } object return form through to the registry result', async () => { + const ctx = await setup() + await call(ctx, 'cordis_mount', { + code: ` + return { + name: 'meta-return', + inject: ['tools'], + apply(ctx) { + harness.registerTool(ctx, harness.defineTool({ + name: 'meta_tool', + description: 'attaches a private presentation payload', + parameters: {}, + async execute() { + return { content: [{ type: 'text', text: 'ok' }], meta: { kind: 'demo' } } + }, + })) + }, + } + `, + }) + const result = await call(ctx, 'meta_tool', {}) + expect(result.isError).toBe(false) + expect(text(result)).toBe('ok') + expect(result.meta).toEqual({ kind: 'demo' }) + }) + + it.each([ + ['a bare string', 'return \'ok\'', '"ok"'], + ['an object whose content is a string', 'return { content: \'ok\' }', '{"content":"ok"}'], + ['an array of non-objects', 'return [\'ok\']', '["ok"]'], + ['blocks missing the type tag', 'return [{ text: \'hi\' }]', '[{"text":"hi"}]'], + ['object-form blocks missing the type tag', 'return { content: [{ text: \'hi\' }] }', '{"content":[{"text":"hi"}]}'], + ['undefined — a forgotten return', 'return undefined', 'undefined'], + ])('rejects an execute return of %s as that one call\'s teaching error', async (_label, returnStatement, preview) => { + // The failure this prevents: the registry trusts the return shape + // (postExecute spreads result.content), so an unvalidated { content: 'ok' } + // would enter the session log as ['o','k'] and silently corrupt the next + // model request. The shape check turns it into THIS call's error instead — + // one well-formed text block the log and the model can digest. + const ctx = await setup() + await call(ctx, 'cordis_mount', { + code: ` + return { + name: 'bad-return', + inject: ['tools'], + apply(ctx) { + harness.registerTool(ctx, harness.defineTool({ + name: 'bad_return_tool', + description: 'returns a wrong shape', + parameters: {}, + async execute() { ${returnStatement} }, + })) + }, + } + `, + }) + const result = await call(ctx, 'bad_return_tool', {}) + expect(result.isError).toBe(true) + expect(result.content).toHaveLength(1) + expect(result.content[0]!.type).toBe('text') + expect(text(result)).toContain(`execute returned ${preview}`) + expect(text(result)).toContain('must return an ARRAY of content blocks') + expect(text(result)).toContain('✓ return { content: [{ type: \'text\', text: someString }], meta: anyJsonValue }') + }) + + it('truncates a huge invalid execute return in the teaching error', async () => { + const ctx = await setup() + await call(ctx, 'cordis_mount', { + code: ` + return { + name: 'huge-return', + inject: ['tools'], + apply(ctx) { + harness.registerTool(ctx, harness.defineTool({ + name: 'huge_return_tool', + description: 'returns a huge wrong shape', + parameters: {}, + async execute() { return 'x'.repeat(500) }, + })) + }, + } + `, + }) + const result = await call(ctx, 'huge_return_tool', {}) + expect(result.isError).toBe(true) + expect(text(result)).toContain('…') + expect(text(result)).not.toContain('x'.repeat(200)) + }) + it('accepts a JSON-Schema-style parameters wrapper and normalizes it to the DSL', async () => { // The dialect models write by strong prior: the { type:'object', // properties, required: […] } wrapper, `type: 'integer'`, and