From 35acd34fd96c5ebbfdd2bd0a263e9700dad62035 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Thu, 9 Jul 2026 23:37:51 +0800 Subject: [PATCH] fix(tools): enforce a disabled run_in_background at execution time (review findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit enableRunInBackground: false removed the parameter from the advertised schema only — the arg validator deliberately allows undeclared keys, so a caller (or a model that has seen the parameter elsewhere) could still force run_in_background: true and start background work past the deployment's opt-out, in both tool-bash and tool-subagent. Both producers now refuse the forced key loud in execute(); tests pin the refusal (and that nothing spawns) alongside the untouched foreground path; the schema-omission-is-advertising rule is recorded in the runtime RFC and both READMEs. --- ...026-06-20-generic-long-running-tool-runtime.md | 2 +- packages/bash/tool-bash/README.md | 2 +- packages/bash/tool-bash/src/index.ts | 7 +++++++ packages/bash/tool-bash/tests/tools.spec.ts | 9 +++++++++ packages/subagent/tool-subagent/README.md | 2 +- packages/subagent/tool-subagent/src/index.ts | 7 +++++++ .../tool-subagent/tests/tool-subagent.spec.ts | 15 +++++++++++++++ 7 files changed, 41 insertions(+), 3 deletions(-) diff --git a/docs/rfc/implemented/architecture/2026-06-20-generic-long-running-tool-runtime.md b/docs/rfc/implemented/architecture/2026-06-20-generic-long-running-tool-runtime.md index 8393de41c2..39f2e70dae 100644 --- a/docs/rfc/implemented/architecture/2026-06-20-generic-long-running-tool-runtime.md +++ b/docs/rfc/implemented/architecture/2026-06-20-generic-long-running-tool-runtime.md @@ -100,7 +100,7 @@ Completion notices stay durable context, not a wake-up (`agent.inject()` appends ## Producer opt-in and schema exposure -Whether a producer tool offers `run_in_background` is that producer's own defaulted config: `enableRunInBackground?: boolean` on `dsh-tool-bash` and on each `dsh-tool-subagent` instance (both default `true` — bash keeps its always-exposed behavior, and a deployment disables either per instance from cordis.yml, no code edit). A disabled producer omits the parameter from its schema entirely, so schema and capability can never disagree. `ctx.tasks` plays no part in schema shaping — it never rewrites or decorates a producer's tool schema (Kimi Code regex-rewrites its bash description when background is disabled; config-owns-the-schema makes that trick unnecessary) — it only provides runtime registration. The two halves compose fail-loud: the producer's config decides what the model sees, and a background call that still reaches `start()` without a control surface throws the load-this-package error. `start()` preflights every failable check (the fence, validation, the owner-cleanup attach) BEFORE invoking the producer's `run()` and commits atomically after — background work started without a collectable id is structurally impossible, not a producer rollback obligation. +Whether a producer tool offers `run_in_background` is that producer's own defaulted config: `enableRunInBackground?: boolean` on `dsh-tool-bash` and on each `dsh-tool-subagent` instance (both default `true` — bash keeps its always-exposed behavior, and a deployment disables either per instance from cordis.yml, no code edit). A disabled producer omits the parameter from its schema entirely — and, because the arg validator deliberately allows undeclared keys, its `execute` ALSO refuses a forced `run_in_background: true` loud (the omission is advertising; the execution-time check is the enforcement). `ctx.tasks` plays no part in schema shaping — it never rewrites or decorates a producer's tool schema (Kimi Code regex-rewrites its bash description when background is disabled; config-owns-the-schema makes that trick unnecessary) — it only provides runtime registration. The two halves compose fail-loud: the producer's config decides what the model sees, and a background call that still reaches `start()` without a control surface throws the load-this-package error. `start()` preflights every failable check (the fence, validation, the owner-cleanup attach) BEFORE invoking the producer's `run()` and commits atomically after — background work started without a collectable id is structurally impossible, not a producer rollback obligation. ## The awaited owner-cleanup seam diff --git a/packages/bash/tool-bash/README.md b/packages/bash/tool-bash/README.md index 44bc2f1b35..eb59289dd0 100644 --- a/packages/bash/tool-bash/README.md +++ b/packages/bash/tool-bash/README.md @@ -10,7 +10,7 @@ The plugin also contributes the `tool:bash` prompt section (order 105) — the c | key | default | meaning | |---|---|---| -| `enableRunInBackground` | `true` | Expose `run_in_background` in the schema. Disabled, the parameter is absent entirely (schema and capability never disagree) and the description says background execution is unavailable. | +| `enableRunInBackground` | `true` | Expose `run_in_background` in the schema. Disabled, the parameter is absent entirely (schema and capability never disagree), the description says background execution is unavailable, and a caller that forces the key anyway is refused at execution time (the arg validator allows undeclared keys, so the schema omission alone is not enforcement). | ## The `bash` tool diff --git a/packages/bash/tool-bash/src/index.ts b/packages/bash/tool-bash/src/index.ts index 6bc0d334f3..98a5f6bfd4 100644 --- a/packages/bash/tool-bash/src/index.ts +++ b/packages/bash/tool-bash/src/index.ts @@ -350,6 +350,13 @@ export function apply(ctx: Context, config: Config): void { ...args.timeoutMs !== undefined ? { timeoutMs: args.timeoutMs } : {}, } if (args.run_in_background === true) { + // The schema omission is advertising, not enforcement — the arg + // validator deliberately allows undeclared keys, so a caller (or a + // model that has seen the parameter elsewhere) can still send it. + // A disabled deployment must refuse at execution time, loud. + if (!backgroundEnabled) { + throw new Error('run_in_background is disabled for this deployment (enableRunInBackground: false)') + } // The generic runtime owns everything task-shaped; without it a task // id would be uncollectable — fail loud with the fix, not a dangle. const tasks = ctx.get('tasks') diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index 71edc21eb5..23e2dac95a 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -408,6 +408,15 @@ describe('background execution through the task runtime', () => { // The registry-held definition agrees (schema and capability never disagree). const parameters = ctx.tools.get('bash')!.parameters as { properties: Record } expect('run_in_background' in parameters.properties).toBe(false) + + // Schema omission is advertising, not enforcement: the arg validator + // allows undeclared keys, so a forced run_in_background must be REFUSED + // at execution time (review finding) — while foreground still works. + const forced = await call(ctx, 'bash', { command: 'echo hi', description: 'test command', run_in_background: true }) + expect(forced.isError).toBe(true) + expect(text(forced)).toContain('run_in_background is disabled for this deployment') + const foreground = await call(ctx, 'bash', { command: 'echo hi', description: 'test command' }) + expect(foreground.isError).toBe(false) }) }) diff --git a/packages/subagent/tool-subagent/README.md b/packages/subagent/tool-subagent/README.md index 78c502bdd3..1e7d95a5a7 100644 --- a/packages/subagent/tool-subagent/README.md +++ b/packages/subagent/tool-subagent/README.md @@ -14,7 +14,7 @@ The tool description and the `prompt` parameter description are DERIVED from the |---|---| | `provider` (required) | The `ctx.subagents` provider name to start runs on (`spawn`, `fork`, `acp`, …). | | `toolName` | The model-facing tool name to register (default `subagent`). Set a distinct value per load when exposing multiple providers, e.g. `subagent` + `subagent_acp`. | -| `enableRunInBackground` | Expose `run_in_background` in this instance's schema (default `true`). Disabled, the parameter is absent entirely — delegation through this instance stays strictly synchronous. | +| `enableRunInBackground` | Expose `run_in_background` in this instance's schema (default `true`). Disabled, the parameter is absent entirely AND a caller that forces the key anyway is refused at execution time (the arg validator allows undeclared keys) — delegation through this instance stays strictly synchronous. | | `agentOptions` | Default per-child `{ model? }` applied to every spawned child. (No per-child persona: the deployment persona is a context-wide section every agent shares.) | ## Foreground lifecycle (synchronous collect) diff --git a/packages/subagent/tool-subagent/src/index.ts b/packages/subagent/tool-subagent/src/index.ts index b32d6e4d4c..fbe2b1cdc2 100644 --- a/packages/subagent/tool-subagent/src/index.ts +++ b/packages/subagent/tool-subagent/src/index.ts @@ -253,6 +253,13 @@ export function apply(ctx: Context, config: Config): void { } if (args.run_in_background === true) { + // The schema omission is advertising, not enforcement — the arg + // validator deliberately allows undeclared keys, so a caller (or a + // model that has seen the parameter elsewhere) can still send it. + // A disabled instance must refuse at execution time, loud. + if (!backgroundEnabled) { + throw new Error('run_in_background is disabled for this tool instance (enableRunInBackground: false)') + } // The generic runtime owns everything task-shaped; without it a task // id would be uncollectable — fail loud with the fix, not a dangle. const tasks = ctx.get('tasks') diff --git a/packages/subagent/tool-subagent/tests/tool-subagent.spec.ts b/packages/subagent/tool-subagent/tests/tool-subagent.spec.ts index 70e5e82c0b..89affcf163 100644 --- a/packages/subagent/tool-subagent/tests/tool-subagent.spec.ts +++ b/packages/subagent/tool-subagent/tests/tool-subagent.spec.ts @@ -81,6 +81,21 @@ describe('dsh-tool-subagent', () => { expect(schema!.description).not.toContain('task_output') }) + it('refuses a forced run_in_background at execution time when the instance disables it (review finding)', async () => { + // Schema omission is advertising, not enforcement: the arg validator + // allows undeclared keys, so the opt-out must also hold in execute(). + const ctx = await setup({ provider: 'mock', enableRunInBackground: false }) + const parent = { id: AgentId('agent-sess-off'), inject: () => {}, session: { header: { version: 0, id: 'sess-off', createdAt: 0 } } } as unknown as Agent + + const forced = await callSubagent(ctx, { description: 'd', prompt: 'p', run_in_background: true }, { agent: parent }) + expect(forced.isError).toBe(true) + expect(text(forced)).toContain('run_in_background is disabled for this tool instance') + // The provider was never asked to start a child. + expect(ctx.subagents.getProvider('mock')).toBeDefined() + const foreground = await callSubagent(ctx, { description: 'd', prompt: 'p' }, { agent: parent }) + expect(foreground.isError).toBe(false) + }) + it.each([ { stopReason: 'aborted' as const, fragment: 'cancelled' }, { stopReason: 'error' as const, fragment: 'failed' },