From 351a2e42b3d70a2514d78b5a9b7348e71a523ec5 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Mon, 13 Jul 2026 16:29:50 +0800 Subject: [PATCH] fix(tasks): close background task review findings --- docs/cordis-catalog/services.md | 2 +- docs/core-data-structures/tasks.md | 2 +- packages/bash/bash-local/src/index.ts | 6 ++-- packages/bash/bash-sandbox/README.md | 2 +- packages/bash/bash-sandbox/src/index.ts | 8 +++--- .../bash/bash-sandbox/tests/sandbox.spec.ts | 14 ++++++++++ packages/core/agent-loop/README.md | 2 +- packages/tasks/tasks/README.md | 2 +- packages/tasks/tasks/src/index.ts | 23 +++++++++++---- packages/tasks/tasks/src/types.ts | 2 +- packages/tasks/tasks/tests/tasks.spec.ts | 28 +++++++++++++++++++ packages/ui/acp/acp-feature-support.md | 2 +- packages/util/brand/src/index.ts | 8 +++--- scripts/gen-cordis-catalog.ts | 2 -- 14 files changed, 78 insertions(+), 25 deletions(-) diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index 7e0e663f37..c4711ce37e 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -270,7 +270,7 @@ attachSurface(name: string): () => void Types: [Agent](../core-data-structures/core.md) -Source: [`packages/tasks/tasks/src/index.ts:97`](../../packages/tasks/tasks/src/index.ts) +Source: [`packages/tasks/tasks/src/index.ts:99`](../../packages/tasks/tasks/src/index.ts) ## `ctx.tools` — `ToolRegistry` diff --git a/docs/core-data-structures/tasks.md b/docs/core-data-structures/tasks.md index 3753c3cf6d..83351f0651 100644 --- a/docs/core-data-structures/tasks.md +++ b/docs/core-data-structures/tasks.md @@ -20,7 +20,7 @@ interface TaskStart { label: string /** * The spawning agent. Its `session.header.id` becomes the task's owner - * token (read/kill/wait/list are fenced to that session), and its `ctx` scope + * identity (read/kill/wait/list are fenced to that session), and its `ctx` scope * owns an async cleanup that cancels and awaits the task during disposal. It * must be the exact live instance currently registered under its agent id; * a stale object whose id has been reused is rejected before work starts. diff --git a/packages/bash/bash-local/src/index.ts b/packages/bash/bash-local/src/index.ts index 065516da6a..198142f822 100644 --- a/packages/bash/bash-local/src/index.ts +++ b/packages/bash/bash-local/src/index.ts @@ -194,6 +194,7 @@ export class LocalBashExecutor extends BashExecutor { // suffices — runBash only rejects with Error instances. proc.status = 'killed' running.stderr.push(Buffer.from(`spawn failed: ${String(error)}`)) + this.onProcessDone(proc, running) this.live.delete(proc) }), readOutput: (): BashProcessRead => { @@ -227,8 +228,9 @@ export class LocalBashExecutor extends BashExecutor { /** * Settlement hook for subclasses that attach execution facts to a process. - * Called after exit facts are stamped and before {@link BashProcess.done} - * resolves. The base implementation is intentionally empty. + * Called after exit facts or spawn-failure output are stamped and before + * {@link BashProcess.done} resolves. The base implementation is intentionally + * empty. * @param _proc - the settled process handle. * @param _running - the process collectors, including full in-memory stderr. */ diff --git a/packages/bash/bash-sandbox/README.md b/packages/bash/bash-sandbox/README.md index 5d462fe1f3..4f27ec878d 100644 --- a/packages/bash/bash-sandbox/README.md +++ b/packages/bash/bash-sandbox/README.md @@ -13,7 +13,7 @@ Every command is confined by handing the provider the exact `['bash', '-c', comm Semantics: - **Denials are result facts.** A failed run whose stderr carries the selected backend's own denial dialect — the signatures the provider stamps on every wrap (EROFS text under bwrap, EACCES under Landlock, EPERM under Seatbelt) — is reported as `BashRunResult.sandbox.denied: true` (conservative classification, read from the collected stderr tail); every CONFINED run also carries the mode it executed under (`result.sandbox.mode`) and the provider's enforcement completeness (`result.sandbox.enforcement`: `full`, or `partial` on an older Landlock ABI). -- **Runner failures are sandbox failures, never command failures.** Foreground execution throws `SANDBOX_UNAVAILABLE`; a settled background process stamps `process.sandbox.runnerFailed`, which the bash producer renders through generic `task_output`. +- **Runner failures are sandbox failures, never command failures.** Foreground execution throws `SANDBOX_UNAVAILABLE`; a settled background process stamps `process.sandbox.runnerFailed`, which the bash producer renders through generic `task_output`. Spawn failures also pass through settlement, so confined background handles retain their mode/enforcement facts and release per-process accounting. - **Config-time default, per-call policy.** The DEFAULT mode is fixed by this entry's config for the executor's lifetime; `resolve()` stamps it onto every spec, and an explicit request-level `sandboxMode` override — set by the tool layer only for a call whose wider mode a human granted through `ctx.approval` ([the sandbox RFC § Escalation](../../../docs/rfc/implemented/feature/2026-07-06-sandbox.md)) — makes THAT call run, classify, and report under its own mode while every neighbor keeps the default (background facts are stamped per task at settle). The capability fact `ctx.bash.sandboxMode` reports the configured default so the tool layer advertises escalation only when this executor is mounted. The model learns of the sandbox only through result facts — the static bash tool description explains the denial marker; there is no current-mode statement in the system prompt. - **File effects only.** Network and process visibility are deliberately not restricted — the mode vocabulary does not pretend to cover what the backend does not enforce. - Process mechanics (spawn, process-group kills, output collection/spill, background handles, credential scrub) are inherited from [`dsh-bash-local`](../bash-local/); runner selection lives in [`dsh-sandbox-local`](../../sandbox/sandbox-local/). diff --git a/packages/bash/bash-sandbox/src/index.ts b/packages/bash/bash-sandbox/src/index.ts index 3cc2a84aaf..5a7e102d6d 100644 --- a/packages/bash/bash-sandbox/src/index.ts +++ b/packages/bash/bash-sandbox/src/index.ts @@ -164,8 +164,8 @@ export class SandboxBashExecutor extends LocalBashExecutor { private readonly mode: SandboxMode private readonly workspaceRoot: string /** - * Per-task facts, keyed by task id from `start()` until the settle stamp - * consumes them: the mode the task runs under (per-call — an escalated task + * Per-process facts, keyed by handle from `start()` until the settle stamp + * consumes them: the mode the process runs under (per-call — an escalated task * differs from its neighbors) plus its wrap facts. The seam returns facts * PER WRAP — a provider may legally vary enforcement or dialect between * calls — so overlapping background tasks must each classify against their @@ -234,8 +234,8 @@ export class SandboxBashExecutor extends LocalBashExecutor { // Sandbox facts are stamped at settle time by onProcessDone() // (denial classification runs against the settled task's collected // stderr). The map entry lands synchronously after spawn, strictly - // before the earliest possible settle (a process exit reaches us no - // sooner than the next tick). + // before the earliest possible settle (including a spawn rejection, whose + // promise reaction cannot run until this synchronous start call returns). const confined = this.confine(spec.command, mode) const proc = super.start({ ...spec, command: confined.command }) const { enforcement, denialSignatures, runnerFailureSignatures } = confined diff --git a/packages/bash/bash-sandbox/tests/sandbox.spec.ts b/packages/bash/bash-sandbox/tests/sandbox.spec.ts index f9e5a7ad10..eddbe5c164 100644 --- a/packages/bash/bash-sandbox/tests/sandbox.spec.ts +++ b/packages/bash/bash-sandbox/tests/sandbox.spec.ts @@ -246,6 +246,20 @@ describe('result facts', () => { }) describe('background sandbox facts', () => { + it('stamps facts and releases accounting when background spawn fails', async () => { + const { bash } = await setup() + const missingWorkdir = join(mkdtempSync(join(tmpdir(), 'dsh-sandbox-missing-cwd-')), 'missing') + const task = bash.start(bash.resolve({ command: 'true', workdir: missingWorkdir })) + + await task.done + + expect(task.status).toBe('killed') + expect(task.readOutput().delta).toContain('spawn failed:') + expect(task.sandbox).toEqual({ mode: 'read-only', denied: false, enforcement: 'full' }) + const accounting = (bash as unknown as { processFacts: Map }).processFacts + expect(accounting.size).toBe(0) + }) + it('stamps a settled denial: nonzero exit + permission stderr under a confined mode', async () => { const { bash } = await setup() const task = bash.start(bash.resolve({ command: 'echo "x: Permission denied" >&2; exit 1' })) diff --git a/packages/core/agent-loop/README.md b/packages/core/agent-loop/README.md index 6832800d2e..932a0f2638 100644 --- a/packages/core/agent-loop/README.md +++ b/packages/core/agent-loop/README.md @@ -104,6 +104,6 @@ Everything that goes beyond "call the model, run the tools, repeat" belongs to p - Hooks and policy: the relevant `agent/*` checkpoints plus the guarded `tools/pre-execute` → `tools/execute` → `tools/post-execute` → `tools/result` pipeline; exact signatures and modes live in the [generated event catalog](../../../docs/cordis-catalog/events.md) - Compaction: `agent/pre-step` - Sandbox, permission, plan mode: `tools/pre-execute` for extensible deny/ask, `tools.guard()` for monotonic owner policy, `tools/post-execute` for result decisions, and `tools/result` for final observation -- Sub-agents: implemented outside the loop as `ctx.subagents` providers; in-process providers use `ctx.agents.create()` and owned `AgentHandle` teardown, while child streaming/progress and background/poll collection remain deferred. +- Sub-agents: implemented outside the loop as `ctx.subagents` providers; in-process providers use `ctx.agents.create()` and owned `AgentHandle` teardown, while generic [`ctx.tasks`](../../tasks/tasks/) plus [`dsh-tool-subagent`](../../subagent/tool-subagent/) own background collection. - Persistence: `session/event` + `session/flush` - UI: `session/event` (assistant token stream, boundaries, tool activity) + `agent/*` control events (`agent/status`, `agent/created`/`agent/disposed`) diff --git a/packages/tasks/tasks/README.md b/packages/tasks/tasks/README.md index 854880d180..cbf3a8a308 100644 --- a/packages/tasks/tasks/README.md +++ b/packages/tasks/tasks/README.md @@ -8,7 +8,7 @@ The background task registry (`ctx.tasks`): a runtime-global, CONCRETE service ( - `get(id, caller?)` / `list(caller?)` — non-consuming snapshots; `list` returns only caller-owned plus unowned tasks (a global listing would leak foreign labels). - `read(id, caller?): TaskRead` — stream kinds consume the per-task cursor (v1's single intended reader is the owning model — a non-consuming multi-reader surface would be a cursor/snapshot API extension, not a `read` change); final kinds read the terminal output idempotently. - `kill(id, caller?, reason?)` — `'requested'` (live task: producer `cancel` runs first — a throw fails the kill loud and leaves the task untouched — then `stopping`) or `'already-terminal'`. Every successful kill marks the task `reported` (the killer saw the end → completion notice suppressed). -- `wait(id, timeoutMs, caller?, signal?)` — resolves with the terminal snapshot (marked `reported`), or the live snapshot at timeout; an aborted signal rejects the WAIT only — unless the task already settled, in which case the wait still resolves and delivers the terminal snapshot (settlement suppressed the completion notice on this waiter's behalf, so rejecting would leave the finish both unreported and un-noticed). Timing is a [`dsh-timeout`](../../util/timeout/README.md) `deadline()` scoped to the `TASK_WAIT_TIMEOUT` code, so a nested foreign deadline never misreads as a wait timeout. +- `wait(id, timeoutMs, caller?, signal?)` — resolves with the terminal snapshot (marked `reported`), or the live snapshot at timeout; an aborted signal rejects the WAIT only — unless the task already settled, in which case the wait still resolves and delivers the terminal snapshot (settlement suppressed the completion notice on this waiter's behalf, so rejecting would leave the finish both unreported and un-noticed). Timing is a [`dsh-timeout`](../../util/timeout/README.md) `deadline()` scoped to the `TASK_WAIT_TIMEOUT` code, so a nested foreign deadline never misreads as a wait timeout; timeout and abort detach their settlement resolver immediately, keeping retention bounded while the task remains live. - `onTaskDone(listener)` — exactly once per terminal task record; effect-scoped, per-listener containment, silent after service disposal. - `attachSurface(name)` — declares a control surface exists (the model tools, or a deployment's custom surface); effect-scoped. diff --git a/packages/tasks/tasks/src/index.ts b/packages/tasks/tasks/src/index.ts index 4f7b00a864..f7218d773f 100644 --- a/packages/tasks/tasks/src/index.ts +++ b/packages/tasks/tasks/src/index.ts @@ -83,6 +83,8 @@ interface TrackedTask { markSettled: () => void /** Live {@link TaskService.wait} calls — a settlement with waiters marks the task reported. */ waiters: number + /** Removable resolvers for live waits; timeout/abort unregister before the task settles. */ + waitResolvers: Set<() => void> } /** True for the three terminal {@link TaskStatus} values. */ @@ -169,6 +171,7 @@ export class TaskService extends Service { settled, markSettled, waiters: 0, + waitResolvers: new Set(), } this.store.set(id, task) @@ -276,8 +279,10 @@ export class TaskService extends Service { * has already settled: settlement saw this live waiter and suppressed the * completion notice on its behalf, so the wait still resolves and delivers * the terminal snapshot it owes (an abort must never leave a finished task - * both unreported and notice-suppressed). Throws for an unknown id, a task - * owned by another session, or a non-positive timeout. + * both unreported and notice-suppressed). Each live wait uses a removable + * resolver that timeout/abort detaches, so a long-running task does not + * retain expired waits. Throws for an unknown id, a task owned by another + * session, or a non-positive timeout. * @param id - the task to wait for. * @param timeoutMs - max wait in milliseconds (positive, finite; the surface caps it). * @param caller - the waiting agent, checked against the task's owner. @@ -313,7 +318,13 @@ export class TaskService extends Service { // the wait. `using` clears the timer on every exit path. using d = deadline(signal, timeoutMs, TASK_WAIT_TIMEOUT) await new Promise((resolve, reject) => { + const onSettled = (): void => { + task.waitResolvers.delete(onSettled) + d.signal.removeEventListener('abort', onAbort) + resolve() + } const onAbort = (): void => { + task.waitResolvers.delete(onSettled) if (timeoutOf(d.signal, TASK_WAIT_TIMEOUT) !== undefined) { resolve() } else if (isTerminal(task.status)) { @@ -325,11 +336,8 @@ export class TaskService extends Service { reject(new Error('wait aborted')) } } + task.waitResolvers.add(onSettled) d.signal.addEventListener('abort', onAbort, { once: true }) - void task.settled.then(() => { - d.signal.removeEventListener('abort', onAbort) - resolve() - }) }) } finally { uncount() @@ -437,6 +445,9 @@ export class TaskService extends Service { } } } + const waitResolvers = [...task.waitResolvers] + task.waitResolvers.clear() + for (const resolveWait of waitResolvers) resolveWait() task.markSettled() } diff --git a/packages/tasks/tasks/src/types.ts b/packages/tasks/tasks/src/types.ts index 7414eb216a..0345269645 100644 --- a/packages/tasks/tasks/src/types.ts +++ b/packages/tasks/tasks/src/types.ts @@ -71,7 +71,7 @@ export interface TaskStart { label: string /** * The spawning agent. Its `session.header.id` becomes the task's owner - * token (read/kill/wait/list are fenced to that session), and its `ctx` scope + * identity (read/kill/wait/list are fenced to that session), and its `ctx` scope * owns an async cleanup that cancels and awaits the task during disposal. It * must be the exact live instance currently registered under its agent id; * a stale object whose id has been reused is rejected before work starts. diff --git a/packages/tasks/tasks/tests/tasks.spec.ts b/packages/tasks/tasks/tests/tasks.spec.ts index a38503a2f8..1ae4567dce 100644 --- a/packages/tasks/tasks/tests/tasks.spec.ts +++ b/packages/tasks/tasks/tests/tasks.spec.ts @@ -59,6 +59,14 @@ async function harness() { /** Let the settlement continuation (a `done.then`) run. */ const tick = () => new Promise(r => setTimeout(r, 0)) +/** Inspect the internal resolver registry to pin bounded retention while a task stays live. */ +function waitResolverCount(ctx: Context, id: TaskId): number { + const service = ctx.tasks as unknown as { store: Map void> }> } + const task = service.store.get(id) + if (task === undefined) throw new Error(`missing test task ${id}`) + return task.waitResolvers.size +} + describe('TaskService.start', () => { it('preserves the SessionId brand on public owner snapshots', () => { expectTypeOf().toEqualTypeOf() @@ -254,6 +262,26 @@ describe('TaskService.wait', () => { expect(await ctx.tasks.wait(id, 5)).toMatchObject({ status: 'running', reported: false }) }) + it('unregisters timed-out and aborted wait resolvers while the task remains live', async () => { + const ctx = await harness() + const id = ctx.tasks.start(producer().spec) + + for (let index = 0; index < 3; index += 1) { + const wait = ctx.tasks.wait(id, 5) + expect(waitResolverCount(ctx, id)).toBe(1) + await expect(wait).resolves.toMatchObject({ status: 'running' }) + expect(waitResolverCount(ctx, id)).toBe(0) + } + + const controller = new AbortController() + const wait = ctx.tasks.wait(id, 5_000, undefined, controller.signal) + expect(waitResolverCount(ctx, id)).toBe(1) + controller.abort() + await expect(wait).rejects.toThrow('wait aborted') + expect(waitResolverCount(ctx, id)).toBe(0) + expect(ctx.tasks.get(id).status).toBe('running') + }) + it('returns immediately for an already-terminal task', async () => { const ctx = await harness() const p = producer() diff --git a/packages/ui/acp/acp-feature-support.md b/packages/ui/acp/acp-feature-support.md index be14fe6237..69ba9f33bb 100644 --- a/packages/ui/acp/acp-feature-support.md +++ b/packages/ui/acp/acp-feature-support.md @@ -133,7 +133,7 @@ The bridge rejects unsupported prompt blocks rather than silently dropping them | Multi-session (N per connection) | S | ✅ | Strict per-session demux; concurrent streams never interleave. See the [multi-session RFC](../../../docs/rfc/implemented/feature/2026-06-14-acp-multi-session.md). | | Disconnect / disposal teardown | S | ✅ | Quiesces every live session on client disconnect or Cordis disposal. | | `_meta` extensibility | S | ⚠️ | Consumed (Zed terminal cap) and emitted (terminal `_meta`); no other custom extensions. | -| Background-task ownership isolation | — | ✅ | `bash_output`/`bash_kill` reject another session's task via an opaque owner token. | +| Background-task ownership isolation | — | ✅ | Generic `task_output`/`task_kill` reject tasks whose branded owner `SessionId` belongs to another session. | | stdout-is-the-protocol guarantee | S | ✅ | The bridge runs in an example with no stdout logger. | ## Gap summary diff --git a/packages/util/brand/src/index.ts b/packages/util/brand/src/index.ts index 051ced94b7..0c669e2416 100644 --- a/packages/util/brand/src/index.ts +++ b/packages/util/brand/src/index.ts @@ -10,13 +10,13 @@ * comparison, logging, and serialization all behave as ordinary strings. * * Policy: a package brands the ids it owns — `CallId` in dsh-llm (tool-call - * correlation), `SessionId` in dsh-session, `AgentId` in dsh-agent, - * `BashTaskId`/`OwnerToken` in dsh-bash. Branding is for ids that cross package - * boundaries and could plausibly be confused; not every string needs a brand. + * correlation), `SessionId` in dsh-session, `AgentId` in dsh-agent, and + * `TaskId` in dsh-tasks. Branding is for ids that cross package boundaries and + * could plausibly be confused; not every string needs a brand. * This package owns ONLY the primitive — no concrete id, no runtime code beyond * the (erased) type — so the brand vocabulary stays dependency-free and a * package can brand its ids without depending on an unrelated capability - * package (e.g. dsh-bash brands its ids without pulling in dsh-llm). + * package. * * @module @deepseek-ai/dsh-brand */ diff --git a/scripts/gen-cordis-catalog.ts b/scripts/gen-cordis-catalog.ts index 68139e9ff8..2db6f871ff 100644 --- a/scripts/gen-cordis-catalog.ts +++ b/scripts/gen-cordis-catalog.ts @@ -101,8 +101,6 @@ export const LINK_MAP: Record = { BashExecRequest: 'bash.md', BashExecSpec: 'bash.md', BashRunResult: 'bash.md', - BashTask: 'bash.md', - BashTaskRead: 'bash.md', ConfinedArgv: 'sandbox.md', SandboxMode: 'sandbox.md', SandboxPolicy: 'sandbox.md',