feat(bash): add an opaque owner token to the executor seam
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.
This commit is contained in:
@@ -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.
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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()]
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
@@ -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[]
|
||||
|
||||
|
||||
@@ -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. */
|
||||
|
||||
@@ -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<string, BashTask>()
|
||||
private owners = new Map<string, string | undefined>()
|
||||
|
||||
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()]
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user