Coder/deepseek-harness
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

simplify(seams): prune dead methods from the persistence and bash seams

Browse Source 
Two capability seams carried abstract methods no production consumer calls.
A method no consumer programs against is not a seam — it is speculative
surface every implementation must still provide and test.

- SessionPersistence: remove has() and delete(), the coordinator's
  has/delete/deleteCore, and the PersistenceBackend.deleteStored hook (with its
  jsonl + sqlite + in-spec memory-stub impls). Surviving service surface:
  create/append/load/list. Production uses only load() (resume) and list()
  (ACP session/list).
- BashExecutor: remove get(id) and list(), the abstract decls and the
  LocalBashExecutor impls. The internal tasks map survives (it backs
  ownerOf/readOutput/kill); get/list were pure public accessors over it with no
  shipping caller and no bash_list tool.
- Migrate tests that reached through ctx.bash.get(id) to the public completion
  seam: a doneFor(id) helper over onTaskDone awaits a task by id, and the
  HMR-reload ownership test now proves task survival through A's own bash_output
  ([status: running]) plus ownerOf + B-rejection — a stronger through-the-tool
  assertion than the removed lookup peek.
- Update seam READMEs (six -> four service methods, drop the deleteStored hook
  and the get/list row) and the two implemented persistence RFCs in place.

Implements docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md
This commit is contained in:
Tianyi Cui
2026-06-21 02:17:27 +08:00
parent 584349f881
commit 7792347c4f
25 changed files with 92 additions and 224 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
packages/bash/bash-local/src/index.ts
View File

@@ -176,20 +176,12 @@ 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}"`)

6
packages/bash/bash-local/tests/executor.spec.ts
View File

@@ -98,8 +98,6 @@ 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)
@@ -237,7 +235,6 @@ 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 () => {
@@ -249,14 +246,13 @@ describe('LocalBashExecutor background tasks', () => {
bash.onTaskDone(listener)
const task = bash.start(bash.resolve({ command: 'sleep 60' }))
const running = bash.get(task.id)!
const running = task
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()
})

1
packages/bash/bash/README.md
View File

@@ -18,7 +18,6 @@ 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. |

6
packages/bash/bash/src/index.ts
View File

@@ -85,9 +85,6 @@ 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
@@ -103,9 +100,6 @@ 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

10
packages/bash/bash/tests/service.spec.ts
View File

@@ -44,18 +44,10 @@ 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}"`)
@@ -88,8 +80,6 @@ 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' }))

12
packages/bash/tool-bash/tests/integration.spec.ts
View File

@@ -8,6 +8,7 @@ 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'
@@ -143,13 +144,18 @@ 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).
const task = ctx.bash.get(taskId)
if (!task) throw new Error(`task ${taskId} not registered`)
await task.done
await taskDone
const log = events(agent)
const firstResult = findEvent(log, 'tool/result')

54
packages/bash/tool-bash/tests/tools.spec.ts
View File

@@ -66,6 +66,23 @@ function text(result: { content: { type: string; text?: string }[] }): string {
return result.content.filter(block => block.type === 'text').map(block => block.text).join('')
}
/**
* Resolve once the background task with `id` completes. The task is started
* indirectly (via `ctx.tools.execute`), so `start()`'s return is not accessible
* here; the executor's `onTaskDone` listener delivers the SAME task object on
* completion, which is the surviving seam for awaiting a task by id.
*/
function doneFor(ctx: Context, id: string): Promise<BashTask> {
return new Promise<BashTask>((resolve) => {
const dispose = ctx.bash.onTaskDone((task) => {
if (task.id === id) {
dispose()
resolve(task)
}
})
})
}
class LossyReadBashExecutor extends BashExecutor {
private readonly task: BashTask = {
id: 'bash-lossy',
@@ -94,18 +111,10 @@ 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 }
@@ -286,7 +295,7 @@ describe('background tools', () => {
expect(text(first)).toContain('first')
expect(text(first)).toContain('[status: running]')
await ctx.bash.get(id)!.done
await doneFor(ctx, id)
const second = await call(ctx, 'bash_output', { task_id: id })
expect(text(second)).toContain('second')
expect(text(second)).not.toContain('first')
@@ -306,7 +315,7 @@ describe('background tools', () => {
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 ctx.bash.get(id)!.done
await doneFor(ctx, id)
const read = await call(ctx, 'bash_output', { task_id: id })
expect(text(read)).toContain('[some output was dropped from memory; full output: ')
})
@@ -329,7 +338,7 @@ describe('background tools', () => {
const killed = await call(ctx, 'bash_kill', { task_id: id })
expect(text(killed)).toBe(`killed background task ${id}`)
await ctx.bash.get(id)!.done
await doneFor(ctx, id)
const again = await call(ctx, 'bash_kill', { task_id: id })
expect(text(again)).toBe(`task ${id} had already finished`)
@@ -373,7 +382,7 @@ describe('background tools', () => {
agent,
})
const id = /task (bash-\d+)/.exec(text(started))![1]!
await ctx.bash.get(id)!.done
await doneFor(ctx, id)
expect(inject).toHaveBeenCalledTimes(1)
const [content, options] = inject.mock.calls[0] as [
@@ -396,7 +405,7 @@ describe('background tools', () => {
agent,
})
const id = /task (bash-\d+)/.exec(text(started))![1]!
await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined()
await expect(doneFor(ctx, id)).resolves.toBeDefined()
})
it('rethrows a non-disposed inject failure (not blindly swallowed)', async () => {
@@ -415,7 +424,7 @@ describe('background tools', () => {
agent,
})
const id = /task (bash-\d+)/.exec(text(started))![1]!
await ctx.bash.get(id)!.done
await doneFor(ctx, id)
// 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')
@@ -443,7 +452,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(ctx.bash.get(id)!.done).resolves.toBeUndefined()
await expect(doneFor(ctx, id)).resolves.toBeDefined()
expect(inject).not.toHaveBeenCalled()
})
@@ -451,7 +460,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(ctx.bash.get(id)!.done).resolves.toBeUndefined()
await expect(doneFor(ctx, id)).resolves.toBeDefined()
})
})
@@ -534,7 +543,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 ctx.bash.get(id)!.done
await doneFor(ctx, id)
// 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)
@@ -567,7 +576,9 @@ describe('background task ownership (cross-session isolation)', () => {
// token) survive.
await fiber.dispose()
await ctx.plugin(ToolBash)
expect(ctx.bash.get(id)?.status).toBe('running')
// 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.ownerOf(id)).toBe('sess-a')
// After reload, ownership is INTACT → B is STILL rejected.
@@ -675,10 +686,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 task = ctx.bash.get(id)!
const done = doneFor(ctx, id)
await call(ctx, 'bash_kill', { task_id: id })
await task.done
const task = await done
// Simulate the variant where the close event carried no signal.
task.signal = null
const read = await call(ctx, 'bash_output', { task_id: id })
@@ -689,8 +700,7 @@ 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 = ctx.bash.get(id)!
await task.done
const task = await doneFor(ctx, id)
// Defensive: completed tasks always carry an exit code in practice; the
// ?? 0 fallback covers task shapes from other executor implementations.
task.exitCode = null