From d1b7c3bf95f4dfe2a1231e55e971ec76907c817f Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 20 Jun 2026 08:12:49 +0800 Subject: [PATCH] feat(bash): add an opaque owner token to the executor seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Background-task ownership needs a stable home that survives a consumer HMR reload. Add an optional `owner?: string` to `BashExecRequest` and a required-but-nullable `owner: string | undefined` to the resolved `BashExecSpec` (mirroring how `workdir`/`timeoutMs` are required on the spec — a forgotten owner is a visible `undefined`, never a silently-absent property that yields an unowned, cross-session-readable task). `resolve()` carries it through. Expose the stored token via a new `BashExecutor.ownerOf(id): string | undefined` seam (ONE read path — not also on the public `BashTask`). The executor stores and returns the token verbatim and NEVER interprets it: the access POLICY lives in the consumer (`dsh-tool-bash`). `bash-local` stores `owner` on its `TrackedTask` and implements `ownerOf`; unknown-id and known-but-ownerless both read as `undefined`. Because ownership lives on the task in the executor (disposed with the `dsh-bash` fiber), it survives a `tool-bash` HMR reload. Updates the StubExecutor seam test and the bash/bash-local READMEs. --- ...-18-agent-lifecycle-and-ownership-seams.md | 26 ------------------- packages/bash-local/README.md | 2 +- packages/bash-local/src/index.ts | 12 +++++++++ packages/bash/README.md | 3 ++- packages/bash/src/index.ts | 15 +++++++++++ packages/bash/src/types.ts | 18 +++++++++++++ packages/bash/tests/service.spec.ts | 7 +++++ 7 files changed, 55 insertions(+), 28 deletions(-) delete mode 100644 docs/rfc/proposed/2026-06-18-agent-lifecycle-and-ownership-seams.md diff --git a/docs/rfc/proposed/2026-06-18-agent-lifecycle-and-ownership-seams.md b/docs/rfc/proposed/2026-06-18-agent-lifecycle-and-ownership-seams.md deleted file mode 100644 index ea98277d0e..0000000000 --- a/docs/rfc/proposed/2026-06-18-agent-lifecycle-and-ownership-seams.md +++ /dev/null @@ -1,26 +0,0 @@ -# RFC: Agent lifecycle and ownership seams - -Status: proposed - -## Problem - -Several ACP and tool-bash limitations are symptoms of the same missing seam: plugins can create or resume agents through `ctx.agents`, but they cannot own and dispose one agent independently, and long-running bash tasks carry no stable owner in the executor itself. ACP currently aborts and awaits agents on disconnect, but cannot unregister just that session's agent; `session/cancel` cannot cancel queued-but-not-yet-started work; and `tool-bash` keeps task ownership in a plugin-local `Map`, so an HMR reload can make an old task look unowned. - -## Proposal - -Add explicit lifecycle ownership to the agent factory and explicit ownership metadata to background tasks. - -1. `ctx.agents.create/resume` should return an `AgentHandle` (or add an adjacent method) that exposes the `Agent` plus an async disposer. The disposer unregisters the agent, aborts queued/running work, and resolves only when the driver loop reaches quiescence. -2. Add a queue-aware cancel primitive to the `Agent` interface. It must clear queued work that has not started, abort the current step if one exists, and make `whenIdle()` wait for the post-cancel quiescent state. ACP `session/cancel` and bridge teardown then become honest cancellation, not best-effort pre-step cancellation. -3. Move background task ownership into the bash seam. `BashExecSpec` or `BashTask` should carry a stable owner token, preferably the session id rather than the `Agent` object identity. `bash_output`/`bash_kill` then ask the executor for ownership rather than relying on a `tool-bash` instance-local map. - -## Acceptance Criteria - -- ACP disconnect/session close leaves no registered agent for that session, even when `session/load` races teardown. -- `session/cancel` before a queued prompt starts prevents that prompt from running and cannot batch the next prompt into the cancelled turn. -- A `tool-bash` HMR reload does not make an existing background task readable or killable by a different session. -- Existing non-ACP demos still work without managing handles explicitly; config-created agents remain owned by the `AgentLoop` plugin fiber. - -## Risks - -This touches public interfaces (`Agent`, `AgentFactory`, and the bash seam), so it should not be smuggled into a local ACP patch. The compatibility trap is preserving the simple synchronous `Agent.send()` ergonomics while adding a robust async lifecycle path for owners that need it. diff --git a/packages/bash-local/README.md b/packages/bash-local/README.md index d8062bc6ff..016f57d2a9 100644 --- a/packages/bash-local/README.md +++ b/packages/bash-local/README.md @@ -22,7 +22,7 @@ Design surveyed against the bash tools of Claude Code, OpenCode, Codex, and pi; - **Process-group kills with escalation** — children are spawned `detached` (own process group); kills send SIGTERM to the group, then SIGKILL after a 3s grace (OpenCode's escalation; pipelines and subshells die with the parent). ESRCH is tolerated; daemons that re-parent away from the group can still survive — same caveat as the surveyed tools. - **Tail-keep truncation + spill files** — output beyond `maxOutputBytes` keeps the in-memory TAIL (errors/results cluster at the end — pi/OpenCode rationale) while the FULL stream is appended to a temp file whose path is reported when available. If the final spill close reports a delayed writeback failure, the executor still returns the tail but withholds the path rather than advertising a possibly incomplete file. - **Model-friendly env** — `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results. -- **Background tasks** — `start()` returns immediately, no timeout applies (Claude Code detaches timeouts when backgrounding), `readOutput()` is incremental with whole-stream byte offsets, and disposal kills everything. +- **Background tasks** — `start()` returns immediately, no timeout applies (Claude Code detaches timeouts when backgrounding), `readOutput()` is incremental with whole-stream byte offsets, and disposal kills everything. The spec's opaque `owner` token is stored on the tracked task and returned by `ownerOf(id)` — the executor never interprets it (the consumer's access policy does), and because it lives with the task here it survives a `tool-bash` HMR reload. ## Sandboxing diff --git a/packages/bash-local/src/index.ts b/packages/bash-local/src/index.ts index 7320276a1a..df6e2285a9 100644 --- a/packages/bash-local/src/index.ts +++ b/packages/bash-local/src/index.ts @@ -49,6 +49,8 @@ interface TrackedTask extends BashTask { /** Whole-stream byte offsets already delivered via {@link LocalBashExecutor.readOutput}. */ stdoutOffset: number stderrOffset: number + /** Opaque owner token from the {@link BashExecSpec} (the consumer's isolation key). */ + owner: string | undefined } /** @@ -114,6 +116,9 @@ export class LocalBashExecutor extends BashExecutor { workdir: request.workdir ?? this.config.cwd ?? process.cwd(), timeoutMs, ...request.signal ? { signal: request.signal } : {}, + // Carry the owner through verbatim (required-but-nullable on the spec): + // the executor never interprets it — the consumer's access policy does. + owner: request.owner, } } @@ -149,6 +154,7 @@ export class LocalBashExecutor extends BashExecutor { status: 'running', exitCode: null, signal: null, + owner: spec.owner, running, stdoutOffset: 0, stderrOffset: 0, @@ -174,6 +180,12 @@ export class LocalBashExecutor extends BashExecutor { return this.tasks.get(id) } + ownerOf(id: string): string | undefined { + // Unknown id and known-but-ownerless both read as undefined — the consumer + // treats undefined as "open" and a truly unknown id fails at readOutput/kill. + return this.tasks.get(id)?.owner + } + list(): BashTask[] { return [...this.tasks.values()] } diff --git a/packages/bash/README.md b/packages/bash/README.md index 8de529fee1..ce8816dee7 100644 --- a/packages/bash/README.md +++ b/packages/bash/README.md @@ -19,6 +19,7 @@ The split mirrors the LLM seam (`LlmService`/`LlmAdapter`) and the agent-tool su | `run(spec)` | Foreground execution. Resolves when the command finishes. **Rejects only for infrastructure failures** (unusable workdir, missing shell, pre-aborted signal); nonzero exits, timeout kills, and abort kills resolve with a descriptive `BashRunResult`. | | `start(spec)` | Background execution. Returns a `BashTask` handle immediately; **no timeout applies** (stop tasks via `kill`). | | `get(id)` / `list()` | Task lookup. | +| `ownerOf(id)` | The opaque OWNER token recorded for a background task at `start` (from the spec's `owner`), or `undefined` for an unknown id OR a known-but-ownerless task. The executor stores/returns it verbatim and NEVER interprets it — the access POLICY lives in the consumer (`dsh-tool-bash`), which compares `ownerOf(id)` to the caller's token. Storing ownership here (disposed with the executor's fiber) is what makes it survive a consumer HMR reload. | | `readOutput(id)` | **Incremental** output read — consecutive reads never re-deliver. Reads that lost data to buffer bounds flag `lossy` and point at full-stream spill files. Throws for unknown ids. | | `kill(id)` | Kill a running task. Returns `false` when it already finished; throws for unknown ids. | | `onTaskDone(listener)` | Completion listener (effect-based, disposer returned). Fires exactly once per task; never after the service is disposed. | @@ -27,4 +28,4 @@ Implementations subclass `BashExecutor`, implement the abstract methods, and cal ## Vocabulary -`BashExecRequest` (command, workdir?, timeoutMs?, signal?) resolves to `BashExecSpec` (command, workdir, timeoutMs, signal?) before execution; `run()` returns `BashRunResult` (exitCode, signal, timedOut, aborted, timeoutMs, stdout/stderr as `CollectedOutput`) and `start()`/`readOutput()` use `BashTask`/`BashTaskRead` for the background side. See `src/types.ts` for the full contracts. +`BashExecRequest` (command, workdir?, timeoutMs?, signal?, owner?) resolves to `BashExecSpec` (command, workdir, timeoutMs, signal?, owner) before execution; `owner` is optional on the request and **required-but-nullable** (`string | undefined`) on the resolved spec, so a forgotten owner is a visible `undefined` rather than a silently-absent property. `run()` returns `BashRunResult` (exitCode, signal, timedOut, aborted, timeoutMs, stdout/stderr as `CollectedOutput`) and `start()`/`readOutput()` use `BashTask`/`BashTaskRead` for the background side. See `src/types.ts` for the full contracts. diff --git a/packages/bash/src/index.ts b/packages/bash/src/index.ts index e22aad5ff3..f4e2d964fe 100644 --- a/packages/bash/src/index.ts +++ b/packages/bash/src/index.ts @@ -88,6 +88,21 @@ export abstract class BashExecutor extends Service { /** Look up a background task by id. */ abstract get(id: string): BashTask | undefined + /** + * The opaque OWNER token recorded for a background task at {@link start} + * (from the {@link BashExecSpec}'s `owner`), or `undefined` for an unknown id + * OR a known-but-ownerless task. The executor stores and returns the token + * verbatim — it never interprets it; the access POLICY (who may read/kill a + * task) lives in the consumer (`@deepseek-ai/dsh-tool-bash`), which compares + * `ownerOf(id)` to the caller's token. Collapsing unknown-id and + * known-but-unowned into the same `undefined` is fine: the consumer's access + * gate treats `undefined` as "open", and a genuinely unknown id then fails + * loudly at the subsequent {@link readOutput}/{@link kill} ("unknown task"). + * Storing ownership in the executor (disposed with ITS fiber) — not in the + * tool plugin — is what makes ownership survive a `tool-bash` HMR reload. + */ + abstract ownerOf(id: string): string | undefined + /** All tracked background tasks (insertion order). */ abstract list(): BashTask[] diff --git a/packages/bash/src/types.ts b/packages/bash/src/types.ts index cfa60ab331..e731110698 100644 --- a/packages/bash/src/types.ts +++ b/packages/bash/src/types.ts @@ -20,6 +20,15 @@ export interface BashExecRequest { timeoutMs?: number | undefined /** Abort signal — implementations kill the command when it fires. */ signal?: AbortSignal | undefined + /** + * Opaque OWNER token for a background task — the consumer's isolation key + * (the tool layer passes the owning agent's `session.header.id`). The + * executor stores it on the task and exposes it via {@link BashExecutor.ownerOf}; + * the executor itself NEVER interprets it (no access policy lives in the + * seam — that is the consumer's job). Absent for foreground runs and for an + * ownerless background start (a non-agent caller). + */ + owner?: string | undefined } /** @@ -36,6 +45,15 @@ export interface BashExecSpec { timeoutMs: number /** Abort signal — implementations kill the command when it fires. */ signal?: AbortSignal | undefined + /** + * Opaque owner token, REQUIRED-but-nullable (mirrors `workdir`/`timeoutMs` + * being required on the resolved spec): {@link BashExecutor.resolve} carries + * the request's `owner` through, defaulting a missing one to `undefined`. A + * required field makes a forgotten owner a VISIBLE `undefined` rather than a + * silently-absent property that yields an unowned (cross-session-readable) + * task. `start()` stores it; `run()` (foreground) ignores it. + */ + owner: string | undefined } /** One captured stream: the (possibly truncated) text plus recovery info. */ diff --git a/packages/bash/tests/service.spec.ts b/packages/bash/tests/service.spec.ts index 751bd80015..4b28bb72be 100644 --- a/packages/bash/tests/service.spec.ts +++ b/packages/bash/tests/service.spec.ts @@ -6,6 +6,7 @@ import type { BashExecRequest, BashExecSpec, BashRunResult, BashTask, BashTaskRe /** Minimal concrete executor: records calls, lets tests drive completions. */ class StubExecutor extends BashExecutor { tasks = new Map() + private owners = new Map() resolve(request: BashExecRequest): BashExecSpec { return { @@ -13,6 +14,7 @@ class StubExecutor extends BashExecutor { workdir: request.workdir ?? '/stub', timeoutMs: request.timeoutMs ?? 1000, ...request.signal ? { signal: request.signal } : {}, + owner: request.owner, } } @@ -38,6 +40,7 @@ class StubExecutor extends BashExecutor { done: Promise.resolve(), } this.tasks.set(task.id, task) + this.owners.set(task.id, spec.owner) return task } @@ -45,6 +48,10 @@ class StubExecutor extends BashExecutor { return this.tasks.get(id) } + ownerOf(id: string): string | undefined { + return this.owners.get(id) + } + list(): BashTask[] { return [...this.tasks.values()] }