From 5f9d10c58793de43dcc208d80d04a3fd0bc622ea Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 02:45:21 +0800 Subject: [PATCH] fix review findings: stale seam docs + race-free doneFor test helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of the PR2 diff caught doc/comment sites doc-sync does not gate (core-data-structures prose) and a latent test-helper race: - docs/core-data-structures/persistence.md + bash.md, sqlite README, and two source comments (coordinator.ts, jsonl.spec.ts) still listed the removed has/delete/get/list methods — updated to the surviving four-method persistence surface and the get/list-free bash seam. - doneFor(ctx, id) attached its onTaskDone listener lazily, after the task could already have closed (e.g. `true`), so it could miss the completion and hang. Replaced with trackCompletions(ctx): one eagerly-installed listener (mounted in setup() before any task starts) records every completion, and doneFor resolves immediately for an already-finished task or on completion otherwise. Race-free, and there is no get-by-id seam left to poll instead. --- docs/core-data-structures/bash.md | 2 +- docs/core-data-structures/persistence.md | 4 +- packages/bash/tool-bash/tests/tools.spec.ts | 46 ++++++++++++++----- .../tests/jsonl.spec.ts | 4 +- .../session-persistence-sqlite/README.md | 2 +- .../session-persistence/src/coordinator.ts | 2 +- 6 files changed, 41 insertions(+), 19 deletions(-) diff --git a/docs/core-data-structures/bash.md b/docs/core-data-structures/bash.md index c601d8cd74..dfbdec1d2f 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), `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)). +`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)). diff --git a/docs/core-data-structures/persistence.md b/docs/core-data-structures/persistence.md index 630d38480f..f1ee857998 100644 --- a/docs/core-data-structures/persistence.md +++ b/docs/core-data-structures/persistence.md @@ -2,7 +2,7 @@ The **durability seam** for the event log. [session.md](session.md) describes the in-memory `Session` — the append-only `SessionEvent` log that is the source of truth. This page describes how that log is made durable: the abstract `SessionPersistence` service, its backends, the flush checkpoint, crash recovery, and the metadata header that travels alongside the log. -The seam is a textbook [capability seam](../rfc/implemented/architecture/2026-06-13-capability-seams.md): one abstract service ([dsh-session-persistence](../../packages/session-persistence/session-persistence), `ctx.sessionPersistence`) defining create/append/load/list/has/delete over the existing `SessionEvent` — **no parallel persisted type** — and two interchangeable backends that pass the same `runPersistenceContract` suite. See the [session-persistence RFC](../rfc/implemented/architecture/2026-06-14-session-persistence.md). +The seam is a textbook [capability seam](../rfc/implemented/architecture/2026-06-13-capability-seams.md): one abstract service ([dsh-session-persistence](../../packages/session-persistence/session-persistence), `ctx.sessionPersistence`) defining create/append/load/list over the existing `SessionEvent` — **no parallel persisted type** — and two interchangeable backends that pass the same `runPersistenceContract` suite. See the [session-persistence RFC](../rfc/implemented/architecture/2026-06-14-session-persistence.md). ## The flush checkpoint @@ -55,7 +55,7 @@ Replay/fork is therefore `ctx.sessions.create(id, { seed: seedEvents })`; resumi ## The backends -Both implement the same abstract `SessionPersistence` (create/append/load/list/has/delete over `SessionEvent`) and pass `runPersistenceContract`, proving the seam is genuinely backend-agnostic: +Both implement the same abstract `SessionPersistence` (create/append/load/list over `SessionEvent`) and pass `runPersistenceContract`, proving the seam is genuinely backend-agnostic: - **[dsh-session-persistence-jsonl](../../packages/session-persistence/session-persistence-jsonl)** — an append-only JSONL log per session with crash-safe atomic writes, the interrupted-turn crash recovery above, and a read/replay path. - **[dsh-session-persistence-sqlite](../../packages/session-persistence/session-persistence-sqlite)** — `node:sqlite`, one row per `SessionEvent`. The row shape `(session_id, seq, type, time, data)` maps 1:1 onto the event, so there is no parallel persisted schema to keep in sync. diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index d8d166ea5b..b7429ff9bb 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -24,6 +24,7 @@ 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,22 +68,42 @@ function text(result: { content: { type: string; text?: string }[] }): string { } /** - * 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. + * 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). */ -function doneFor(ctx: Context, id: string): Promise { - return new Promise((resolve) => { - const dispose = ctx.bash.onTaskDone((task) => { - if (task.id === id) { - dispose() - resolve(task) - } - }) +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) => { + const waiter = state.waiters.get(task.id) + if (waiter) { + state.waiters.delete(task.id) + waiter(task) + } else { + state.done.set(task.id, 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', @@ -312,6 +333,7 @@ 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]! diff --git a/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts b/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts index 8acc578521..86c4a9d08b 100644 --- a/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts +++ b/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts @@ -629,8 +629,8 @@ describe('SessionPersistenceJsonl: edge cases', () => { const a = meta('dup-id', '/projA') await ctx.sessionPersistence.create(a) await ctx.sessionPersistence.append(a.id, oneTurnLog()) - // A fresh backend creating the SAME id under cwd B must still refuse: load/ - // has identify by id across all buckets, so a second log would make resume + // A fresh backend creating the SAME id under cwd B must still refuse: load + // identifies by id across all buckets, so a second log would make resume // nondeterministic. create scans every bucket, not just meta.cwd's. const ctx2 = new Context() await ctx2.plugin(SessionStore) diff --git a/packages/session-persistence/session-persistence-sqlite/README.md b/packages/session-persistence/session-persistence-sqlite/README.md index 02255c024f..f397f80445 100644 --- a/packages/session-persistence/session-persistence-sqlite/README.md +++ b/packages/session-persistence/session-persistence-sqlite/README.md @@ -6,7 +6,7 @@ A SQLite durable session-persistence backend — a second `SessionPersistence` i ## Storage model -Each `SessionEvent` maps 1:1 onto a row in an `events` table `(session_id, seq, type, time, data)` — `data` is the event payload as JSON text, so the row shape is the event verbatim (including `assistant/chunk`, keeping `seq` contiguous). Out-of-log metadata (`SessionHeader`) lives in a `sessions` row. A `sessions` row is written only by the first `append` — its existence is the lazy-materialization signal (`has`/`list` report exactly the sessions that have a row), so no separate column is needed. +Each `SessionEvent` maps 1:1 onto a row in an `events` table `(session_id, seq, type, time, data)` — `data` is the event payload as JSON text, so the row shape is the event verbatim (including `assistant/chunk`, keeping `seq` contiguous). Out-of-log metadata (`SessionHeader`) lives in a `sessions` row. A `sessions` row is written only by the first `append` — its existence is the lazy-materialization signal (`list` reports exactly the sessions that have a row), so no separate column is needed. The repo targets Node ≥ 24 (the root `engines` field), which includes the stable `node:sqlite` module. The database opens with `foreign_keys = ON` (so `ON DELETE CASCADE` drops a session's events with its row) and `journal_mode = WAL`. The table-layout version is stored in `PRAGMA user_version` and checked on open: a fresh database is stamped with the current `SCHEMA_VERSION`; a database written by any other, incompatible build (a non-current `user_version`, older or newer) is rejected rather than opened against an unknown layout — there is no migration (unreleased software). diff --git a/packages/session-persistence/session-persistence/src/coordinator.ts b/packages/session-persistence/session-persistence/src/coordinator.ts index d2f5712502..58f1246763 100644 --- a/packages/session-persistence/session-persistence/src/coordinator.ts +++ b/packages/session-persistence/session-persistence/src/coordinator.ts @@ -202,7 +202,7 @@ export class PersistenceCoordinator { throw new Error(`session "${meta.id}" already exists in this backend`) } // A persisted artifact under this id (in ANY scope) blocks creation: load/ - // has/resume identify a session by id alone, so a second artifact would make + // resume identify a session by id alone, so a second artifact would make // resume nondeterministic. if (await this.backend.loadStored(meta.id) !== undefined) { throw new Error(`session "${meta.id}" already has a persisted log on disk; load/resume it instead of creating`)