revert bash get()/list() removal — keep persistence-only prune
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.
This commit is contained in:
Tianyi Cui
@@ -327,7 +327,9 @@ Semantics every implementation must honor:
|
||||
abstract resolve(request: BashExecRequest): BashExecSpec
|
||||
abstract run(spec: BashExecSpec): Promise<BashRunResult>
|
||||
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
|
||||
|
||||
@@ -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)).
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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<B> = 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.)
|
||||
|
||||
|
||||
@@ -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`.
|
||||
|
||||
|
||||
@@ -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}"`)
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
|
||||
@@ -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. |
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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' }))
|
||||
|
||||
@@ -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<BashTask>((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')
|
||||
|
||||
@@ -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<Context, { done: Map<string, BashTask>; waiters: Map<string, (task: BashTask) => void> }>()
|
||||
|
||||
function trackCompletions(ctx: Context): void {
|
||||
const state = { done: new Map<string, BashTask>(), waiters: new Map<string, (task: BashTask) => 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<BashTask> {
|
||||
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<BashTask>(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
|
||||
|
||||
Reference in New Issue
Block a user