From 24168aee70ac77d3dbaaecf0a8224561f7002bfd Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 06:17:11 +0800 Subject: [PATCH] =?UTF-8?q?revert=20bash=20get()/list()=20removal=20?= =?UTF-8?q?=E2=80=94=20keep=20persistence-only=20prune?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original prune removed BashExecutor.get()/.list() too, but each is a one-line accessor over the executor's already-tracked tasks map, and removing them forced dsh-tool-bash's tests onto a ~35-line onTaskDone completion-tracking harness just to replace the one-line ctx.bash.get(id) lookup. Per the AGENTS.md "RFCs are proposals, not golden truth" principle, that disproportionate migration cost is evidence the methods earn their keep — a test harness IS a consumer programming against the seam. Restore get()/list() (seam + LocalBashExecutor impl + the bash tests that used them, dropping the doneFor/trackCompletions scaffolding). The persistence has()/delete()/deleteStored removal stands — it had only contract-test callers and no test-ergonomics cost. The RFC is retitled persistence-only with an implementation note recording the bash revert. --- docs/cordis-catalog/events-and-services.md | 2 + docs/core-data-structures/bash.md | 2 +- .../2026-06-20-prune-dead-seam-methods.md | 12 +-- .../architecture/2026-06-20-branded-ids.md | 4 +- ...06-20-generic-long-running-tool-runtime.md | 2 +- packages/bash/bash-local/src/index.ts | 8 ++ .../bash/bash-local/tests/executor.spec.ts | 6 +- packages/bash/bash/README.md | 1 + packages/bash/bash/src/index.ts | 6 ++ packages/bash/bash/tests/service.spec.ts | 10 +++ .../bash/tool-bash/tests/integration.spec.ts | 12 +-- packages/bash/tool-bash/tests/tools.spec.ts | 77 ++++++------------- 12 files changed, 68 insertions(+), 74 deletions(-) diff --git a/docs/cordis-catalog/events-and-services.md b/docs/cordis-catalog/events-and-services.md index e8445b4786..0422555ec2 100644 --- a/docs/cordis-catalog/events-and-services.md +++ b/docs/cordis-catalog/events-and-services.md @@ -327,7 +327,9 @@ Semantics every implementation must honor: abstract resolve(request: BashExecRequest): BashExecSpec abstract run(spec: BashExecSpec): Promise abstract start(spec: BashExecSpec): BashTask +abstract get(id: string): BashTask | undefined abstract ownerOf(id: string): string | undefined +abstract list(): BashTask[] abstract readOutput(id: string): BashTaskRead abstract kill(id: string): boolean onTaskDone(listener: BashTaskListener): () => void diff --git a/docs/core-data-structures/bash.md b/docs/core-data-structures/bash.md index dfbdec1d2f..c601d8cd74 100644 --- a/docs/core-data-structures/bash.md +++ b/docs/core-data-structures/bash.md @@ -120,4 +120,4 @@ interface BashTaskRead { ## The service -`BashExecutor` (`ctx.bash`, abstract — defined in [`packages/bash/bash/src/index.ts`](../../packages/bash/bash/src/index.ts)) mirrors the `LlmService`/`LlmAdapter` split: `resolve` (request → spec), `run` (foreground), `start` (background), `ownerOf`/`readOutput`/`kill`, and `onTaskDone` (a `BashTaskListener` completion callback). Spawned commands get a **scrubbed env** (dropping `*KEY*`/`*SECRET*`/`*TOKEN*`) and spill files use a private 0700 dir with random names and owner-only opens — model output never gets the ambient environment or a predictable path. The implementation that provides all this is `dsh-bash-local`; the model-facing `bash`/`bash_output`/`bash_kill` schemas that call it are in `dsh-tool-bash` (and present as terminals via the [tool-presentation vocabulary](tools.md#tool-presentation-ui-vocabulary)). +`BashExecutor` (`ctx.bash`, abstract — defined in [`packages/bash/bash/src/index.ts`](../../packages/bash/bash/src/index.ts)) mirrors the `LlmService`/`LlmAdapter` split: `resolve` (request → spec), `run` (foreground), `start` (background), `get`/`ownerOf`/`list`/`readOutput`/`kill`, and `onTaskDone` (a `BashTaskListener` completion callback). Spawned commands get a **scrubbed env** (dropping `*KEY*`/`*SECRET*`/`*TOKEN*`) and spill files use a private 0700 dir with random names and owner-only opens — model output never gets the ambient environment or a predictable path. The implementation that provides all this is `dsh-bash-local`; the model-facing `bash`/`bash_output`/`bash_kill` schemas that call it are in `dsh-tool-bash` (and present as terminals via the [tool-presentation vocabulary](tools.md#tool-presentation-ui-vocabulary)). diff --git a/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md b/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md index ec9dc45223..3cf99e47ef 100644 --- a/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md +++ b/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md @@ -1,7 +1,9 @@ -# RFC: Prune dead methods from the persistence and bash capability seams +# RFC: Prune dead methods from the persistence seam Status: implemented (proposed and accepted 2026-06-20) +> **Implementation note (scope narrowed from the original proposal).** This RFC proposed pruning dead methods from BOTH the persistence seam (`SessionPersistence.has()`/`.delete()`) and the bash seam (`BashExecutor.get()`/`.list()`). Only the **persistence** removal shipped. The bash `get()`/`.list()` removal was reverted before merge: each is a one-line accessor over the executor's already-tracked `tasks` map, and removing them forced `dsh-tool-bash`'s tests onto a ~35-line `onTaskDone`-based completion-tracking harness to replace the one-line `ctx.bash.get(id)` lookup — the migration cost dwarfed the surface removed. Per the [AGENTS.md "RFCs are proposals, not golden truth"](../../../../AGENTS.md) principle, that friction is evidence the method earns its keep (a test harness IS a consumer that programs against the seam), so `get()`/`list()` stay. The bash-seam analysis below is retained for the record but was NOT acted on; `BashTaskId`-branding those methods lands in the [branded-ids RFC](../../proposed/architecture/2026-06-20-branded-ids.md) instead. The persistence removal stands: `has()`/`delete()` had only contract-test callers and no test-ergonomics cost to remove. + ## Problem Two capability seams ([interface / implementation / consumer](../../implemented/architecture/2026-06-13-capability-seams.md)) carry abstract methods that no consumer calls. The seam exists to let implementations and consumers evolve independently — but a method no consumer programs against is not a seam, it is speculative surface every implementation must still implement and test. @@ -35,10 +37,10 @@ Re-adding a seam method with a live consumer is cheap and better-designed than t ## Acceptance criteria -- `has`/`delete`/`deleteStored` and `get`/`list` are gone from their seams, impls, and contract suites; `pnpm run knip` reports no new dead exports. -- The remaining seam operations (`create`/`append`/`load`/`list` for persistence; `run`/`start`/`ownerOf`/`onTaskDone`/`readOutput`/`kill`/`resolve` for bash) are untouched; ACP `session/list`, bash tool flows, and crash-recovery behave identically. -- `pnpm run test:coverage` stays 100% per-file (the contract/spec rows for the removed methods are deleted with them). -- Seam READMEs and `docs/architecture.md` no longer list the removed methods. +- `has`/`delete`/`deleteStored` are gone from the persistence seam, impl, and contract suites; `pnpm run knip` reports no new dead exports. (The bash `get`/`list` removal was reverted — see the implementation note above; those methods remain.) +- The remaining seam operations (`create`/`append`/`load`/`list` for persistence; `run`/`start`/`get`/`ownerOf`/`list`/`onTaskDone`/`readOutput`/`kill`/`resolve` for bash) are untouched; ACP `session/list`, bash tool flows, and crash-recovery behave identically. +- `pnpm run test:coverage` stays 100% per-file (the contract/spec rows for the removed persistence methods are deleted with them). +- Persistence seam READMEs and `docs/architecture.md` no longer list the removed `has`/`delete` methods. ## Risks diff --git a/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md b/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md index d69eb8f8c6..93a4bf6cda 100644 --- a/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md +++ b/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md @@ -6,7 +6,7 @@ Status: proposed The harness already brands three identifiers — `CallId` (`packages/llm/llm/src/brand.ts`), `SessionId` (`packages/core/session/src/types.ts`), and `AgentId` (`packages/core/agent/src/types.ts`) — using the `Branded = string & { readonly [BRAND]: B }` machinery and a zero-cost cast factory per type. `brand.ts` also states the governing policy: *"Branding is for IDs that cross package boundaries and could plausibly be confused; not every string needs a brand."* That policy is right; the problem is that it is only half-applied. Two gaps let a structurally-identical-but-semantically-wrong string slip through the type checker today. -**Gap 1 — unbranded cross-boundary IDs in the bash seam.** The background-task id is a plain `string`: `BashTask.id: string` (`packages/bash/bash/src/types.ts`), carried as `string` through the whole executor seam (`BashExecutor.ownerOf`/`readOutput`/`kill(id: string)` in `packages/bash/bash/src/index.ts`) and validated/passed as `string` by the model-facing tools (`validateTaskId`, `assertTaskAccess`, the `task_id` schema arg in `packages/bash/tool-bash/src/index.ts`). It is generated by a per-executor counter — `` `bash-${this.nextTaskId++}` `` in `packages/bash/bash-local/src/index.ts` — which gives it **exactly the same `name-N` shape as `SessionId`'s default** (`` `session-${++counter}` `` in `packages/core/session/src/index.ts`). A bash task id and a session id are trivially swappable at a call site and the compiler says nothing. This is the headline case the user asked about, and it is a model-facing id (the model passes `task_id` back to `bash_output`/`bash_kill`), so a confusion here is reachable from untrusted input. +**Gap 1 — unbranded cross-boundary IDs in the bash seam.** The background-task id is a plain `string`: `BashTask.id: string` (`packages/bash/bash/src/types.ts`), carried as `string` through the whole executor seam (`BashExecutor.get`/`ownerOf`/`readOutput`/`kill(id: string)` in `packages/bash/bash/src/index.ts`) and validated/passed as `string` by the model-facing tools (`validateTaskId`, `assertTaskAccess`, the `task_id` schema arg in `packages/bash/tool-bash/src/index.ts`). It is generated by a per-executor counter — `` `bash-${this.nextTaskId++}` `` in `packages/bash/bash-local/src/index.ts` — which gives it **exactly the same `name-N` shape as `SessionId`'s default** (`` `session-${++counter}` `` in `packages/core/session/src/index.ts`). A bash task id and a session id are trivially swappable at a call site and the compiler says nothing. This is the headline case the user asked about, and it is a model-facing id (the model passes `task_id` back to `bash_output`/`bash_kill`), so a confusion here is reachable from untrusted input. The bash **owner token** is the related sub-case: `BashExecRequest.owner?: string` and `BashExecSpec.owner: string | undefined` (`packages/bash/bash/src/types.ts`) are documented as a deliberately *opaque* isolation key, but in every live caller the value IS the owning agent's `session.header.id` (`callerToken = (exec) => exec.agent?.session.header.id` in `packages/bash/tool-bash/src/index.ts`) — i.e. a `SessionId` wearing a `string` disguise. It is compared for access control (`owner !== callerToken(exec)`), so a mismatched-but-well-typed string here is a cross-session isolation bug the type system currently cannot catch. This is the same `session.header.id`-as-owner alias that the [unify-the-agent-id-and-the-session-id](../simplification/2026-06-20-unify-agent-and-session-id.md) proposal calls the "bash owner-token alias hole". @@ -16,7 +16,7 @@ The bash **owner token** is the related sub-case: `BashExecRequest.owner?: strin A type-only change. Brands are zero-cost casts; nothing about runtime behavior, serialization, comparison, or the wire format changes. The work is in three parts, all honoring the existing "not every string" policy. -- **Brand the bash task id.** Add `BashTaskId = Branded<'BashTaskId'>` plus its same-named factory in `packages/bash/bash/src/types.ts` (the package that *owns* the id), importing `Branded` from `@deepseek-ai/dsh-llm` exactly as `SessionId`/`AgentId` already do. Thread it through `BashTask.id`, the `BashExecutor` seam methods (`ownerOf`/`readOutput`/`kill`), the generation site in `dsh-bash-local` (brand the counter output once, at creation), and the `dsh-tool-bash` validate/access surface (`validateTaskId` returns a `BashTaskId`; `task_id` is branded at the tool boundary where the model's string arrives). +- **Brand the bash task id.** Add `BashTaskId = Branded<'BashTaskId'>` plus its same-named factory in `packages/bash/bash/src/types.ts` (the package that *owns* the id), importing `Branded` from `@deepseek-ai/dsh-llm` exactly as `SessionId`/`AgentId` already do. Thread it through `BashTask.id`, the `BashExecutor` seam methods (`get`/`ownerOf`/`readOutput`/`kill`), the generation site in `dsh-bash-local` (brand the counter output once, at creation), and the `dsh-tool-bash` validate/access surface (`validateTaskId` returns a `BashTaskId`; `task_id` is branded at the tool boundary where the model's string arrives). - **Mint a distinct `OwnerToken` brand.** Add `OwnerToken = Branded<'OwnerToken'>` in `packages/bash/bash/src/types.ts`; type `BashExecRequest.owner` / `BashExecSpec.owner` / `BashExecutor.ownerOf` as `OwnerToken | undefined`. The `dsh-tool-bash` consumer casts the agent's `session.header.id` (a `SessionId`) into an `OwnerToken` at the boundary — the one place the two vocabularies meet. The bash seam never imports `dsh-session`. (Rationale in the next section.) diff --git a/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md b/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md index d17224c46f..4f034e3020 100644 --- a/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md +++ b/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md @@ -4,7 +4,7 @@ Status: proposed ## Problem -The bash capability seam supports both foreground commands and long-running background tasks. Background support is large: the abstract executor exposes `start`, `ownerOf`, `readOutput`, `kill`, and `onTaskDone`; the local executor tracks tasks, incremental reads, owner tokens, process cleanup, and completion listeners; the model sees three tools (`bash`, `bash_output`, `bash_kill`); the tool plugin injects completion notices back into the owning agent's session. The local executor fences task access behind owner tokens because predictable global task ids are a cross-session read/kill hazard. +The bash capability seam supports both foreground commands and long-running background tasks. Background support is large: the abstract executor exposes `start`, `get`, `ownerOf`, `list`, `readOutput`, `kill`, and `onTaskDone`; the local executor tracks tasks, incremental reads, owner tokens, process cleanup, and completion listeners; the model sees three tools (`bash`, `bash_output`, `bash_kill`); the tool plugin injects completion notices back into the owning agent's session. The local executor fences task access behind owner tokens because predictable global task ids are a cross-session read/kill hazard. The [tool cookbook](../../../cookbook/adding-a-tool.md) already points at the real design smell: background bash is really generic long-running-tool infrastructure living inside one tool. If future tools need background execution, polling, kill, ownership, and completion notices, those semantics should not be hidden in `dsh-bash`. diff --git a/packages/bash/bash-local/src/index.ts b/packages/bash/bash-local/src/index.ts index 7b55200cf8..df6e2285a9 100644 --- a/packages/bash/bash-local/src/index.ts +++ b/packages/bash/bash-local/src/index.ts @@ -176,12 +176,20 @@ export class LocalBashExecutor extends BashExecutor { return task } + get(id: string): BashTask | undefined { + 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()] + } + readOutput(id: string): BashTaskRead { const task = this.tasks.get(id) if (!task) throw new Error(`unknown bash task "${id}"`) diff --git a/packages/bash/bash-local/tests/executor.spec.ts b/packages/bash/bash-local/tests/executor.spec.ts index ec90aeb77e..3dd7f7983a 100644 --- a/packages/bash/bash-local/tests/executor.spec.ts +++ b/packages/bash/bash-local/tests/executor.spec.ts @@ -98,6 +98,8 @@ describe('LocalBashExecutor background tasks', () => { const task = bash.start(bash.resolve({ command: 'sleep 0.2; echo done' })) expect(Date.now() - before).toBeLessThan(150) expect(task.status).toBe('running') + expect(bash.get(task.id)).toBe(task) + expect(bash.list()).toContain(task) await task.done expect(task.status).toBe('completed') expect(task.exitCode).toBe(0) @@ -235,6 +237,7 @@ describe('LocalBashExecutor background tasks', () => { await running.done expect(finished.status).toBe('completed') expect(running.signal).toBe('SIGTERM') + expect(bash.list()).toEqual([]) }) it('disposing the executor fiber kills running tasks (no orphans)', async () => { @@ -246,13 +249,14 @@ describe('LocalBashExecutor background tasks', () => { bash.onTaskDone(listener) const task = bash.start(bash.resolve({ command: 'sleep 60' })) - const running = task + const running = bash.get(task.id)! await new Promise(resolve => setTimeout(resolve, 50)) // Grab the pid before dispose clears the registry. const pid = (running as unknown as { running: { pid: number } }).running.pid await fiber.dispose() await waitGone(pid) + expect(bash.list()).toEqual([]) // Listener silenced by base-class teardown — no late notifications. expect(listener).not.toHaveBeenCalled() }) diff --git a/packages/bash/bash/README.md b/packages/bash/bash/README.md index c982a217c7..ce8816dee7 100644 --- a/packages/bash/bash/README.md +++ b/packages/bash/bash/README.md @@ -18,6 +18,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. | diff --git a/packages/bash/bash/src/index.ts b/packages/bash/bash/src/index.ts index 9df8c720aa..f4e2d964fe 100644 --- a/packages/bash/bash/src/index.ts +++ b/packages/bash/bash/src/index.ts @@ -85,6 +85,9 @@ export abstract class BashExecutor extends Service { /** Start a background task and return its handle immediately. */ abstract start(spec: BashExecSpec): BashTask + /** 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 @@ -100,6 +103,9 @@ export abstract class BashExecutor extends Service { */ abstract ownerOf(id: string): string | undefined + /** All tracked background tasks (insertion order). */ + abstract list(): BashTask[] + /** Read output produced since the previous read. Throws for unknown ids. */ abstract readOutput(id: string): BashTaskRead diff --git a/packages/bash/bash/tests/service.spec.ts b/packages/bash/bash/tests/service.spec.ts index 2646715df9..4b28bb72be 100644 --- a/packages/bash/bash/tests/service.spec.ts +++ b/packages/bash/bash/tests/service.spec.ts @@ -44,10 +44,18 @@ class StubExecutor extends BashExecutor { return task } + get(id: string): BashTask | undefined { + return this.tasks.get(id) + } + ownerOf(id: string): string | undefined { return this.owners.get(id) } + list(): BashTask[] { + return [...this.tasks.values()] + } + readOutput(id: string): BashTaskRead { const task = this.tasks.get(id) if (!task) throw new Error(`unknown bash task "${id}"`) @@ -80,6 +88,8 @@ describe('BashExecutor service seam', () => { it('registers as ctx.bash and serves the abstract API', async () => { const { bash } = await setup() const task = bash.start(bash.resolve({ command: 'sleep 1' })) + expect(bash.get(task.id)).toBe(task) + expect(bash.list()).toEqual([task]) expect(bash.kill(task.id)).toBe(true) expect(bash.kill(task.id)).toBe(false) const result = await bash.run(bash.resolve({ command: 'true' })) diff --git a/packages/bash/tool-bash/tests/integration.spec.ts b/packages/bash/tool-bash/tests/integration.spec.ts index fadcbf741d..0ab786ca85 100644 --- a/packages/bash/tool-bash/tests/integration.spec.ts +++ b/packages/bash/tool-bash/tests/integration.spec.ts @@ -8,7 +8,6 @@ import ToolRegistry from '@deepseek-ai/dsh-tools' import AgentRegistry from '@deepseek-ai/dsh-agent' import AgentLoop, { ReactLoopAgent } from '@deepseek-ai/dsh-agent-loop' import { LocalBashExecutor } from '@deepseek-ai/dsh-bash-local' -import type { BashTask } from '@deepseek-ai/dsh-bash' import * as ToolBash from '@deepseek-ai/dsh-tool-bash' import { MockAdapter, textResponse, toolCallResponse } from '../../../core/agent-loop/tests/mock-adapter.ts' @@ -144,18 +143,13 @@ describe('bash tool through the agent loop', () => { return next() }) - // Capture the single background task's completion. Registered BEFORE send so - // a fast task (echo) can't finish before the listener is attached; onTaskDone - // delivers the task object once it completes (completion may race turn end). - const taskDone = new Promise((resolve) => { - const dispose = ctx.bash.onTaskDone((task) => { dispose(); resolve(task) }) - }) - agent.send([{ type: 'text', text: 'run echo bg-ok in the background' }]) await waitForIdle(ctx, agent) // Wait for the background task itself (completion may race turn end). - await taskDone + const task = ctx.bash.get(taskId) + if (!task) throw new Error(`task ${taskId} not registered`) + await task.done const log = events(agent) const firstResult = findEvent(log, 'tool/result') diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index e0cc1165e3..c49410a9b2 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -24,7 +24,6 @@ async function setup() { await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 }) ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } await ctx.plugin(ToolBash) - trackCompletions(ctx) return ctx } @@ -67,44 +66,6 @@ function text(result: { content: { type: string; text?: string }[] }): string { return result.content.filter(block => block.type === 'text').map(block => block.text).join('') } -/** - * Per-context background-completion tracker. The task is started indirectly - * (via `ctx.tools.execute`), so `start()`'s return is not accessible here, and - * there is no get-by-id seam to poll current state — the only surviving way to - * await a task by id is the executor's `onTaskDone` listener. Registering that - * listener lazily (after the task may have already closed) would miss the - * completion and hang; so {@link trackCompletions} installs ONE listener - * EAGERLY (before any task starts) that records every completion, and - * {@link doneFor} resolves from that record — immediately if the task already - * finished, otherwise when it does. Call `trackCompletions(ctx)` right after - * the executor is mounted (`setup()` does this for you). - */ -const completions = new WeakMap; waiters: Map void> }>() - -function trackCompletions(ctx: Context): void { - const state = { done: new Map(), waiters: new Map void>() } - completions.set(ctx, state) - ctx.bash.onTaskDone((task) => { - // Always record the completion so a later doneFor(id) still resolves; also - // wake any waiter already parked on this id. - state.done.set(task.id, task) - const waiter = state.waiters.get(task.id) - if (waiter) { - state.waiters.delete(task.id) - waiter(task) - } - }) -} - -/** Resolve (with the task object) once the background task `id` has completed. */ -function doneFor(ctx: Context, id: string): Promise { - const state = completions.get(ctx) - if (!state) throw new Error('trackCompletions(ctx) must be called before doneFor(ctx, …)') - const already = state.done.get(id) - if (already) return Promise.resolve(already) - return new Promise(resolve => state.waiters.set(id, resolve)) -} - class LossyReadBashExecutor extends BashExecutor { private readonly task: BashTask = { id: 'bash-lossy', @@ -133,10 +94,18 @@ class LossyReadBashExecutor extends BashExecutor { return this.task } + get(id: string): BashTask | undefined { + return id === this.task.id ? this.task : undefined + } + ownerOf(): string | undefined { return undefined } + list(): BashTask[] { + return [this.task] + } + readOutput(id: string): BashTaskRead { if (id !== this.task.id) throw new Error(`unknown bash task "${id}"`) return { task: this.task, delta: 'tail', lossy: true } @@ -317,7 +286,7 @@ describe('background tools', () => { expect(text(first)).toContain('first') expect(text(first)).toContain('[status: running]') - await doneFor(ctx, id) + await ctx.bash.get(id)!.done const second = await call(ctx, 'bash_output', { task_id: id }) expect(text(second)).toContain('second') expect(text(second)).not.toContain('first') @@ -334,11 +303,10 @@ describe('background tools', () => { await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100 }) ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } await ctx.plugin(ToolBash) - trackCompletions(ctx) const started = await call(ctx, 'bash', { command: 'for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await doneFor(ctx, id) + await ctx.bash.get(id)!.done const read = await call(ctx, 'bash_output', { task_id: id }) expect(text(read)).toContain('[some output was dropped from memory; full output: ') }) @@ -361,7 +329,7 @@ describe('background tools', () => { const killed = await call(ctx, 'bash_kill', { task_id: id }) expect(text(killed)).toBe(`killed background task ${id}`) - await doneFor(ctx, id) + await ctx.bash.get(id)!.done const again = await call(ctx, 'bash_kill', { task_id: id }) expect(text(again)).toBe(`task ${id} had already finished`) @@ -405,7 +373,7 @@ describe('background tools', () => { agent, }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await doneFor(ctx, id) + await ctx.bash.get(id)!.done expect(inject).toHaveBeenCalledTimes(1) const [content, options] = inject.mock.calls[0] as [ @@ -428,7 +396,7 @@ describe('background tools', () => { agent, }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await expect(doneFor(ctx, id)).resolves.toBeDefined() + await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined() }) it('rethrows a non-disposed inject failure (not blindly swallowed)', async () => { @@ -447,7 +415,7 @@ describe('background tools', () => { agent, }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await doneFor(ctx, id) + await ctx.bash.get(id)!.done // notifyTaskDone caught and logged the rethrown error. expect(errorSpy).toHaveBeenCalled() const logged = errorSpy.mock.calls.flat().some(arg => arg instanceof Error && arg.message === 'unexpected inject bug') @@ -475,7 +443,7 @@ describe('background tools', () => { const id = /task (bash-\d+)/.exec(text(started))![1]! // Unregister the agent BEFORE the task completes (simulate disconnect). unregisterFakeAgents(ctx) - await expect(doneFor(ctx, id)).resolves.toBeDefined() + await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined() expect(inject).not.toHaveBeenCalled() }) @@ -483,7 +451,7 @@ describe('background tools', () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'true', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await expect(doneFor(ctx, id)).resolves.toBeDefined() + await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined() }) }) @@ -566,7 +534,7 @@ describe('background task ownership (cross-session isolation)', () => { const b = fakeAgent('sess-b') const started = await callAs(ctx, a, 'bash', { command: 'echo done', description: 'bg', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await doneFor(ctx, id) + await ctx.bash.get(id)!.done // Completion does NOT clear ownership: B is still rejected, A still allowed. const readByB = await callAs(ctx, b, 'bash_output', { task_id: id }) expect(readByB.isError).toBe(true) @@ -599,9 +567,7 @@ describe('background task ownership (cross-session isolation)', () => { // token) survive. await fiber.dispose() await ctx.plugin(ToolBash) - // The task survived the reload, still running and still owned by A — proven - // via A's own bash_output (reports running status) and the surviving owner token. - expect(text(await callAs(ctx, a, 'bash_output', { task_id: id }))).toContain('[status: running]') + expect(ctx.bash.get(id)?.status).toBe('running') expect(ctx.bash.ownerOf(id)).toBe('sess-a') // After reload, ownership is INTACT → B is STILL rejected. @@ -709,10 +675,10 @@ describe('status lines', () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'sleep 60', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - const done = doneFor(ctx, id) + const task = ctx.bash.get(id)! await call(ctx, 'bash_kill', { task_id: id }) - const task = await done + await task.done // Simulate the variant where the close event carried no signal. task.signal = null const read = await call(ctx, 'bash_output', { task_id: id }) @@ -723,7 +689,8 @@ describe('status lines', () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'true', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - const task = await doneFor(ctx, id) + const task = ctx.bash.get(id)! + await task.done // Defensive: completed tasks always carry an exit code in practice; the // ?? 0 fallback covers task shapes from other executor implementations. task.exitCode = null