diff --git a/docs/rfc/README.md b/docs/rfc/README.md index 5f2e25ae31..b75f95d0bf 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -35,6 +35,9 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Retire mid-turn steering](proposed/2026-06-20-retire-mid-turn-steering.md) | 2026-06-20 | | [Stop mirroring durable boundaries as agent events](proposed/2026-06-20-remove-agent-boundary-mirror-events.md) | 2026-06-20 | | [Keep one public stop primitive](proposed/2026-06-20-public-agent-stop-surface.md) | 2026-06-20 | +| [Drop the unconsumed `streamBlocks()` assembled-view surface](proposed/2026-06-20-drop-unconsumed-llm-block-views.md) | 2026-06-20 | +| [Drop the unconsumed registry `*/change` events](proposed/2026-06-20-drop-unconsumed-registry-change-events.md) | 2026-06-20 | +| [Prune dead methods from the persistence and bash seams](proposed/2026-06-20-prune-dead-seam-methods.md) | 2026-06-20 | | [Drop durable step boundary events](proposed/2026-06-20-drop-durable-step-boundaries.md) | 2026-06-20 | | [Truncate interrupted final turns on load](proposed/2026-06-20-truncate-interrupted-turns.md) | 2026-06-20 | | [Persist assembled assistant messages, not stream chunks](proposed/2026-06-20-assembled-assistant-messages-only.md) | 2026-06-20 | diff --git a/docs/rfc/proposed/2026-06-20-drop-unconsumed-llm-block-views.md b/docs/rfc/proposed/2026-06-20-drop-unconsumed-llm-block-views.md new file mode 100644 index 0000000000..a0fb71d40b --- /dev/null +++ b/docs/rfc/proposed/2026-06-20-drop-unconsumed-llm-block-views.md @@ -0,0 +1,44 @@ +# RFC: Drop the unconsumed `streamBlocks()` assembled-view surface on `dsh-llm` + +Status: proposed + +## Problem + +`LlmService` ([packages/llm/src/index.ts](../../../packages/llm/src/index.ts)) exposes three call surfaces over a model: + +- `stream()` — raw `StreamChunk`s, dispatched through the `llm/stream` waterfall. +- `streamBlocks()` — a "convenience view" that runs the chunks through a `BlockAssembler` and yields completed `ContentBlock`s in stream order ([index.ts:137-144](../../../packages/llm/src/index.ts)). +- `generate()` — one fully-assembled `GenerateResult`, dispatched through a second `llm/generate` waterfall ([index.ts:151-157](../../../packages/llm/src/index.ts)). + +The only production consumer of the LLM service is the agent loop, and it uses `stream()` exclusively — feeding the raw chunks through its own `BlockAssembler` so it can log raw chunks for replay fidelity while assembling in parallel ([packages/agent-loop/src/loop.ts](../../../packages/agent-loop/src/loop.ts), the `ctx.llm.stream(req)` step). Grepping `streamBlocks` across `packages/*/src` and `examples/*/src` finds zero callers; the only references are the method itself, two doc comments, and two test files (`llm/tests/properties.spec.ts`, `agent-loop/tests/review-fixes.spec.ts`). + +This is the [drop-mutable-session-summary](../implemented/2026-06-19-drop-mutable-session-summary.md) pattern: an entire assembled-view API with a property-tested contract, consumed by nothing but its own tests. It was built speculatively for "consumers that don't care about token-level deltas" that never materialized — the one real consumer cares about deltas precisely so it can log them. + +`streamBlocks()` also drags a dedicated slice of `BlockAssembler` behind it: `flushReady()` and `flushRemaining()` ([packages/llm/src/assembler.ts:138-168](../../../packages/llm/src/assembler.ts)) plus the `flushed` cursor field exist only to support the incremental in-order yield. The loop's assembler usage is `push()` / `message()` / `usage` / `finish` — never the streaming flush. With `streamBlocks()` gone, `flushReady`/`flushRemaining`/`flushed` are dead too. + +## Proposal + +Delete `streamBlocks()` and the assembler's streaming-flush machinery it alone drives: + +- Remove `LlmService.streamBlocks()` and its JSDoc. +- Remove `BlockAssembler.flushReady()`, `BlockAssembler.flushRemaining()`, and the `flushed` cursor field. +- Remove or rework the `flushReady`/`flushRemaining`-dependent tests: in `llm/tests/properties.spec.ts` the `flushReady() ++ flushRemaining() === blocks()` property, the strict-order property, and the "streaming and one-shot assembly agree on usage and finish" property (which pushes-then-flushes incrementally) all exercise the streaming-flush path; the `flushRemaining` cases in `llm/tests/assembler.spec.ts` and the three `streamBlocks` edge-case tests in `agent-loop/tests/review-fixes.spec.ts` likewise. Each is either deleted or, where it also asserts a non-flush invariant worth keeping (e.g. streaming vs one-shot agreeing on usage/finish), rewritten to use `push()` + `message()`/`result()` without the removed flush methods — the behavior pinned to the deleted methods goes, per AGENTS.md "tests document behavior, not golden truth". +- Update every doc/comment reference to `streamBlocks` — grep it across `docs/`, `packages/llm/README.md`, and source comments. `packages/llm/README.md` mentions it twice (the API-list row and the `BlockAssembler` "used by `streamBlocks()`/`generate()`" line); the `assembler.ts` module doc references it; and the retained `generate()` JSDoc currently reads "Same completion guarantees as `streamBlocks()`" — reword it to state the guarantee directly. The `ctx.llm` service-map row in [docs/architecture.md](../../../docs/architecture.md) (`stream()` / `streamBlocks()` / `generate()`) drops `streamBlocks()` too. The [property-based-testing RFC](../implemented/2026-06-11-property-based-testing.md) needs two edits: its motivating anecdote ("a `streamBlocks` ordering bug") is reworded to name the bug class (a block-assembly ordering bug) rather than a removed method, and its dsh-llm invariant list — which names `flushReady()+flushRemaining() ≡ blocks()` as a checked property — is updated to drop the removed-method invariant and keep only the ones the surviving assembler API (`push`/`blocks`/`message`/`result`) still supports. + +## Scope: why `generate()` and `llm/generate` stay + +`generate()` is not dead the same way: the twin-adapter e2e/unit suites (`llm-deepseek`, `llm-pi-ai`) use `ctx.llm.generate({...})` as a convenient one-shot driver to assert provider behavior, and `GenerateResult` / `assembler.result()` back it. Those adapter tests are the [twin-adapter design](../implemented/2026-06-13-twin-llm-adapters.md), explicitly out of scope for a simplification pass. Removing `generate()` would force adapter-test call sites to hand-drain `stream()`, which is churn in protected territory for a method that is at least a legitimate ergonomic driver. So this RFC deliberately stops at the surface that nothing — not even an out-of-scope test — consumes. If a later pass wants to also collapse `generate()`/`llm/generate`/`result()`, that is a separate decision with a real caller to migrate. + +## Acceptance criteria + +- `streamBlocks` and the assembler streaming-flush methods are gone; `pnpm run knip` reports no new dead exports. +- `pnpm run test:coverage` stays at 100% per-file (the deleted methods take their dedicated tests with them; no remaining line goes uncovered). +- `generate()`, `stream()`, `result()`, `blocks()`, `message()` are untouched and the loop behaves identically — verified by the unchanged ACP snapshot goldens. +- `packages/llm/README.md` and the module docs no longer mention `streamBlocks`. + +## Risks + +- **It removes a public method from a core vocabulary package.** A future plugin that wants "assembled blocks without the deltas" would have to re-add it (or call `generate()` and read `.message.content`). Given the pre-release "foundation over speculative future" stance ([AGENTS.md](../../../AGENTS.md)) and that the obvious assembled-view need is already served by `generate()`, this is the right time to cut — re-adding a thin assembler wrapper later is trivial if a real consumer appears. +- **Low blast radius.** The change is confined to `dsh-llm`; no other package imports `streamBlocks` or the flush methods, so there is no cross-package ripple. + +The size is modest, but it is a clean, zero-production-impact removal of a speculative surface — the cheapest kind of correctness. diff --git a/docs/rfc/proposed/2026-06-20-drop-unconsumed-registry-change-events.md b/docs/rfc/proposed/2026-06-20-drop-unconsumed-registry-change-events.md new file mode 100644 index 0000000000..16c1b8faf2 --- /dev/null +++ b/docs/rfc/proposed/2026-06-20-drop-unconsumed-registry-change-events.md @@ -0,0 +1,50 @@ +# RFC: Drop the unconsumed registry `*/change` notification events + +Status: proposed + +## Problem + +Three registries each emit a "something changed" notification event that no production listener subscribes to: + +- `tools/change` — emitted by `ToolRegistry.register()` on register and disposal ([packages/tools/src/index.ts:302-304](../../../packages/tools/src/index.ts)). +- `system-prompt/change` — emitted by `SystemPrompt.section()` and `.tools()` ([packages/system-prompt/src/index.ts:86-110](../../../packages/system-prompt/src/index.ts)). +- `llm/adapter-change` — emitted by `LlmService.registerAdapter()` ([packages/llm/src/index.ts:98-100](../../../packages/llm/src/index.ts)). + +Grepping the three event names across `packages/*/src` and `examples/*/src` finds only the emit sites and their declarations — zero `ctx.on('.../change')` listeners in production. The only subscribers are each package's own spec file, and they subscribe purely to test that the emit fires. They are listed in the event taxonomy table ([docs/architecture.md](../../../docs/architecture.md)) as `emit` events, but nothing reacts to them. + +These events are speculative generality for a hypothetical reactive consumer (a UI that live-refreshes its tool palette, say) that does not exist. That alone would be a mild [drop-the-dead-summary](../implemented/2026-06-19-drop-mutable-session-summary.md)-style cut. What makes it worth an RFC is the machinery the events drag along: to emit `.../change` safely, each registry orders its generator effect so the rollback disposer is `yield`ed before the change-emit, specifically so a throwing change-listener unwinds the mutation instead of leaking a registry entry. Every one of the three carries a multi-line comment justifying this ordering, plus a dedicated "rollback when a change listener throws" test. That is a non-trivial correctness burden guarding a failure mode that only the tests' own injected listeners can trigger, because there are no real listeners. + +## Proposal + +Remove the three `*/change` events and the defensive machinery that exists only to make them safe: + +- Delete the `tools/change`, `system-prompt/change`, `llm/adapter-change` declarations from each package's `interface Events`. +- Delete the `ctx.emit('.../change')` calls. +- Simplify each `ctx.effect` generator: the mutation and its rollback disposer remain (HMR/disposal still need them), but the "yield rollback before the emit so a throwing listener rolls back" ordering comment and any emit-after-yield collapse to a plain `set`/`push` plus a `yield () => delete`/`splice`. No behavior an external observer can see changes, because nothing observes the events. +- Remove the "Emits `.../change` on register/unregister" sentence from the surviving registration-method JSDocs — these sit on methods that stay, so they go stale rather than vanish with the deleted code: `LlmService.registerAdapter` ([packages/llm/src/index.ts](../../../packages/llm/src/index.ts)), `ToolRegistry.register` ([packages/tools/src/index.ts](../../../packages/tools/src/index.ts)), and both `SystemPrompt.section` and `SystemPrompt.tools` ([packages/system-prompt/src/index.ts](../../../packages/system-prompt/src/index.ts)). +- Delete or rewrite the tests that exist to exercise the events. The change-listener-rollback tests are deleted outright (the rollback behavior goes with the event). The positive emission-subscriber tests are handled case by case: `system-prompt/tests/system-prompt.spec.ts`'s "emits system-prompt/change ..." is deleted (its disposal coverage is duplicated by the separate "cleans up tool providers on fiber dispose" / "removes section when returned disposer is called directly" tests), but `llm/tests/service.spec.ts`'s "disposes adapter registration on adapter-change event emission" is the only test that calls the `registerAdapter()` returned disposer and asserts the adapter is removed (the HMR test at "unregisters adapters when the owning fiber is disposed" covers fiber disposal, a different path) — so it is rewritten to drop the event subscription while keeping the returned-disposer assertion, not deleted. Per AGENTS.md "tests document behavior, not golden truth". +- Update the event taxonomy table in [docs/architecture.md](../../../docs/architecture.md) (remove the three rows) and re-run `pnpm run verify-event-taxonomy`, which mechanically checks the table against source. Also remove the per-package README event rows that list them: [packages/tools/README.md](../../../packages/tools/README.md) (`tools/change`), [packages/system-prompt/README.md](../../../packages/system-prompt/README.md) (`system-prompt/change`), and [packages/llm/README.md](../../../packages/llm/README.md) (`llm/adapter-change`). The [doc-sync-enforcement RFC](../implemented/2026-06-11-doc-sync-enforcement.md), whose `verify-event-taxonomy` description names these three as the events that surfaced when the check landed, is reworded so its example does not point at removed events. + +## Why not keep them as a "registries announce changes" convention? + +That is the honest counter-argument: a microkernel where every registry announces its mutations is a clean, uniform reactive substrate, and a future live UI would want exactly this. Three considerations push the other way: + +1. **The harness already has a finer-grained feed for the one realistic consumer.** A UI live-renders from `session/event` and `agent/*`, not from registry mutations — tools/sections/adapters are registered at plugin-load time and effectively static during a session. The `.../change` events fire almost exclusively during boot and HMR, when nothing is watching. +2. **Pre-release stance.** [AGENTS.md](../../../AGENTS.md) says optimize for the correct foundation, not a speculative future; add the seam when a real consumer needs it. Re-adding an emit is one line; the cost today is the standing rollback-ordering burden on three hot registration paths. +3. **The events are not free — they shape the registration code.** Keeping them means keeping the throwing-change-listener invariant and its tests forever, for a listener that cannot exist until someone adds one. + +If a reactive consumer is later built, it should be reintroduced deliberately, as one coherent decision about which registries announce what (and possibly a single `registry/change` shape), not as three independently-grown emits nothing reads. + +## Acceptance criteria + +- The three events and their emits are gone; `pnpm run verify-event-taxonomy` passes against the updated table. +- HMR-safety tests still pass: disposing a contributing fiber still removes the tool/section/adapter (the rollback disposer is retained; only the change-emit and its throwing-listener guard are removed). +- `pnpm run test:coverage` stays 100% per-file. +- No production code path changes observable behavior (verified by unchanged ACP snapshot goldens and the echo-agent smoke test). + +## Risks + +- **Removing a documented emit event is a public-surface change.** It is in the taxonomy table, so it reads as deliberate API. But "declared and emitted" is not "consumed" — the same distinction that justified dropping the mutable summary. The taxonomy table is updated in the same change, so the docs do not drift. +- **A registry that genuinely wants change-notification later pays a small reintroduction cost.** Judged acceptable per the pre-release stance; the reintroduction is mechanical. + +This is a small-to-medium cut across three packages and, more valuably, it retires a standing correctness invariant that guards a consumer that does not exist. diff --git a/docs/rfc/proposed/2026-06-20-prune-dead-seam-methods.md b/docs/rfc/proposed/2026-06-20-prune-dead-seam-methods.md new file mode 100644 index 0000000000..d38583c60b --- /dev/null +++ b/docs/rfc/proposed/2026-06-20-prune-dead-seam-methods.md @@ -0,0 +1,49 @@ +# RFC: Prune dead methods from the persistence and bash capability seams + +Status: proposed + +## Problem + +Two capability seams ([interface / implementation / consumer](../implemented/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. + +### `SessionPersistence.has()` and `.delete()` + +The abstract service declares four operations beyond create/append: `load`, `list`, `has`, `delete` ([packages/session-persistence/src/index.ts:142-151](../../../packages/session-persistence/src/index.ts)). Production consumers of `ctx.sessionPersistence` use only two of them: the agent-loop resume path calls `load()` ([packages/agent-loop/src/index.ts:176-194](../../../packages/agent-loop/src/index.ts)), and the ACP bridge calls `list()` for `session/list` ([packages/acp/src/index.ts](../../../packages/acp/src/index.ts)). Grepping every `sessionPersistence.*` / `persistence.*` use across `packages/*/src` and `examples/` finds no `has(` and no `delete(` on the service. The `.has(`/`.delete(` calls in `packages/acp/src/index.ts` are on the in-memory `SessionStore` and a local `Set` of loading ids, not persistence. The only callers of `has`/`delete` are the contract suites and per-backend specs. + +`has()` is not just unused — it is the most intricate branch in the shared coordinator: a tracked-vs-untracked dual-probe (`loadLive(id, cwd)` for a live-tracked session vs `loadStored(id)` for an untracked one) with a multi-line rationale ([packages/session-persistence/src/coordinator.ts:298-310](../../../packages/session-persistence/src/coordinator.ts)). `delete()` drags the `deleteStored` backend hook ([coordinator.ts:99](../../../packages/session-persistence/src/coordinator.ts), [coordinator.ts:313-319](../../../packages/session-persistence/src/coordinator.ts)) that every backend must implement. This is the [drop-mutable-session-summary](../implemented/2026-06-19-drop-mutable-session-summary.md) pattern: a contract test exercises both, but no shipping code asks "is this session persisted?" or removes one. + +### `BashExecutor.get()` and `.list()` + +The bash seam declares `get(id)` ("look up a background task by id") and `list()` ("all tracked background tasks") ([packages/bash/src/index.ts:88-107](../../../packages/bash/src/index.ts)), both implemented by `LocalBashExecutor` ([packages/bash-local/src/index.ts:179-191](../../../packages/bash-local/src/index.ts)). The sole production consumer — `dsh-tool-bash` — drives tasks via `ownerOf`, `onTaskDone`, `start`, `readOutput`, `kill`, `resolve`, `run`; it never calls `get`/`list` in shipping code, and there is no `bash_list` tool exposing a task roster to the model. So both are dead production seam surface. They are used by tests, more broadly than a single idiom: the bash seam/executor specs assert them directly ([packages/bash/tests/service.spec.ts](../../../packages/bash/tests/service.spec.ts), [packages/bash-local/tests/executor.spec.ts](../../../packages/bash-local/tests/executor.spec.ts) both call `get()`/`list()`), and several `dsh-tool-bash` tests reach through `ctx.bash.get(id)` to await a task's `done`, read its `status`, or inspect task fields ([packages/tool-bash/tests/tools.spec.ts](../../../packages/tool-bash/tests/tools.spec.ts), [packages/tool-bash/tests/integration.spec.ts](../../../packages/tool-bash/tests/integration.spec.ts)). These are test-harness conveniences, not shipping consumers — but they are real test code an implementing PR must migrate or delete. + +## Proposal + +Remove the methods nothing consumes, from the abstract seam, the implementation, and the contract/spec suites that exist only to exercise them: + +- `SessionPersistence.has()` / `.delete()`: delete the abstract declarations, the coordinator's `has`/`delete`/`deleteCore`, and the `PersistenceBackend.deleteStored` hook. Remove the `has`/`delete` rows from the contract suite and the per-backend specs (jsonl + sqlite each implement `deleteStored` only to satisfy the hook — that implementation goes too). The backends are the [dual-backend](../implemented/2026-06-14-session-persistence.md) design and otherwise out of scope, but removing a hook they implement for no consumer is part of removing the hook, not a backend redesign. +- `BashExecutor.get()` / `.list()`: delete the abstract declarations and the `LocalBashExecutor` impls. The seam/executor specs that assert `get()`/`list()` directly (`bash/tests/service.spec.ts`, `bash-local/tests/executor.spec.ts`) lose those assertions (the behavior is being removed). The `dsh-tool-bash` tests that reach through `ctx.bash.get(id)` to await `done`, read `status`, or inspect task fields switch to the public completion/status seam they should use — `onTaskDone` (or the `done` promise and status the `start()` return already exposes) — keeping their coverage without the removed lookup method. +- Update every doc and source-comment reference to the removed methods — not only literal `has(`/`delete(`/`get(`/`list(`/`deleteStored` call spellings, but also `{@link has}`/`{@link delete}` JSDoc links and prose that counts the methods (removing 2 of the persistence service's 6 public methods makes any "six public methods" phrasing wrong). The implementing PR greps `has`/`delete`/`get`/`list`/`deleteStored`/`{@link `/`six ` across `docs/`, `packages/*/README.md`, and source comments, and fixes each. The known doc sites: the seam READMEs ([packages/session-persistence/README.md](../../../packages/session-persistence/README.md)'s `has(id)`/`delete(id)` API row and its "delegates its six public service methods" prose → four, [packages/bash/README.md](../../../packages/bash/README.md)'s `get(id)`/`list()` row), the backend READMEs that describe `has`/`list` semantics ([packages/session-persistence-sqlite/README.md](../../../packages/session-persistence-sqlite/README.md), [packages/session-persistence-jsonl/README.md](../../../packages/session-persistence-jsonl/README.md) — reword "absent from `has()`/`list()`" to just `list()`), the service-map / seam docs in [docs/architecture.md](../../../docs/architecture.md), and the persistence prose in the [session-persistence RFC](../implemented/2026-06-14-session-persistence.md) and [shared write-coordinator RFC](../implemented/2026-06-18-shared-persistence-write-coordinator.md). The known source-comment sites: the abstract `create()` JSDoc's `{@link has}/{@link list}` link ([packages/session-persistence/src/index.ts](../../../packages/session-persistence/src/index.ts) — drop the `has` link), the coordinator's "six public methods"/"six public service methods" module + class JSDoc and its lazy-materialization JSDoc justifying the `materialized` flag by "the signal `has`/`list` rely on" ([packages/session-persistence/src/coordinator.ts](../../../packages/session-persistence/src/coordinator.ts)), the JSONL backend's `loadStored`/`deleteStored` comment, and the SQLite backend's `schema.ts` and `index.ts` comments that mention "absent from `has`/`list`" — all reworded to the surviving four-method, `list()`-only contract. + +## Why not keep them as "the seam should be complete"? + +The instinct that a persistence seam "should" offer delete, or a task executor "should" offer enumeration, is real — and it is exactly the speculative-completeness the pre-release stance warns against ([AGENTS.md](../../../AGENTS.md): optimize for the correct foundation, not for hypothetical callers you do not have). Each of these is one method to re-add the day a consumer needs it: + +- A session-management UI that deletes old sessions will want `delete()` — add it then, designed against that UI's real needs (soft-delete? cascade? confirmation?), not guessed now. +- A `bash_list` tool that shows the model its running tasks will want `list()` — add it with the tool. + +Re-adding a seam method with a live consumer is cheap and better-designed than the speculative version, because the consumer pins the contract. Carrying it unused means every implementation (and every future backend) must implement and test a method that does nothing. + +## 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. + +## Risks + +- **`delete()` is the kind of operation a product eventually wants.** True — but "eventually" is the point. Deleting it now and re-adding it against a real consumer is strictly better than shipping a guessed contract. The dual backends each shed a `deleteStored` impl, which is a bounded edit in otherwise-out-of-scope packages. +- **`list()` on the bash seam is the natural seed for a future `bash_list`.** Acknowledged in the [pre-release foundation stance](../../../AGENTS.md): add the seed when the tool lands. The executor still tracks tasks internally (the `tasks` map backs `ownerOf`/`readOutput`/`kill`); exposing an enumeration is a one-line re-add. +- **Low coupling.** Both removals are confined to their seam + impl + tests; no cross-package consumer references the removed methods, so there is no ripple beyond the docs. + +Modest size, but it converts two seams from "what an implementation must provide for nobody" back to "exactly what a consumer uses." diff --git a/docs/rfc/proposed/2026-06-20-public-agent-stop-surface.md b/docs/rfc/proposed/2026-06-20-public-agent-stop-surface.md index e793b7bde6..8ff4ebb46b 100644 --- a/docs/rfc/proposed/2026-06-20-public-agent-stop-surface.md +++ b/docs/rfc/proposed/2026-06-20-public-agent-stop-surface.md @@ -6,13 +6,15 @@ Status: proposed The public `Agent` handle exposes three ways to reason about stopping work: `abort(reason?)`, `cancel(reason?)`, and `whenIdle()`. `abort()` kills only the in-flight step and leaves queued work alone; `cancel()` clears queued and steering work, aborts the running step, and handles the pre-step race; `whenIdle()` exposes the loop's private quiescence waiter to any consumer. In production, ACP uses `cancel()` for `session/cancel`, while lifecycle owners tear down agents through `AgentHandle.dispose()`. No production caller needs bare `abort()` or `whenIdle()`. +The `abort()`/`cancel()` distinction is real — `abort()` preserves queued prompts and steering while `cancel()` drops them — but no shipping code calls the public `abort()` verb. The loop's own stop paths (`cancel()` and disposal) abort the current `AbortController` directly rather than routing through `Agent.abort()`. Most tests that call `abort()` interrupt an empty queue and can switch to `cancel(reason)`; the one steering re-delivery test that deliberately depends on queue preservation should drive the in-flight `AbortController` directly, because `cancel()` would drop the queued steering it is trying to prove survives a step abort. The no-argument `abort()` default reason (`'aborted'`) is also deleted with the verb rather than preserved by accident; `cancel()` keeps its own `'cancelled'` default. + The extra surface area makes the loop carry public semantics that are mostly teardown internals. `whenIdle()` needs waiter state, special disposed-agent behavior, and a loop-exit promise so it resolves after quiescence rather than merely after a status flip. `abort()` has to be documented as distinct from queue-aware cancellation even though a UI cancellation almost always wants the broader operation. ## Proposal Keep `cancel()` as the only public stop primitive on `Agent`. Lifecycle owners use `AgentHandle.dispose()` to stop and unregister an agent; non-owners use `cancel()` to abandon current and queued work. The implementation can keep private abort controllers and quiescence promises, but they are not part of the plugin-facing `Agent` contract. -Delete public `abort()` and `whenIdle()`, the tests that exercise them as standalone API, and the docs that describe step-only abort as an embedding feature. The disposer remains async and still waits for the loop to stop; that guarantee moves entirely onto `AgentHandle.dispose()`. +Delete public `abort()` and `whenIdle()`, the tests that exercise them as standalone API, and the docs that describe step-only abort as an embedding feature. Empty-queue abort tests migrate to `cancel(reason)` where they still prove cancellation behavior; tests whose subject is the loop's internal `AbortController` behavior drive that controller directly; tests that only pin the removed no-arg `abort()` default go away with the method. The disposer remains async and still waits for the loop to stop; that guarantee moves entirely onto `AgentHandle.dispose()`. ## Acceptance criteria diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index d107c2c0cb..089379e83a 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -203,6 +203,10 @@ interface SessionRecord { * others are no-ops (settle-exactly-once). */ export function apply(ctx: Context, config: AcpConfig): void { + // TODO(double-default): these literals duplicate the Config schema defaults + // (`agentName`/`agentVersion` `.default(...)` above). The Loader applies the + // schema before apply() runs, so the `??` only fires for direct-apply unit + // tests. Pick one home for the default to avoid drift. const agentName = config.agentName ?? 'deepseek-harness-acp' const agentVersion = config.agentVersion ?? '0.0.1' diff --git a/packages/bash-local/src/run.ts b/packages/bash-local/src/run.ts index 46d48cf5fd..8a8d2065d2 100644 --- a/packages/bash-local/src/run.ts +++ b/packages/bash-local/src/run.ts @@ -159,7 +159,11 @@ export class OutputCollector { writeSync(this.spillFd, chunk) } - /** Read the collected tail without finalizing (used by background polling). */ + // TODO(snapshot-scope): `snapshot()` has one internal caller (`finalize()` at + // the bottom of this file) and `totalBytes` is read only by a test. The live + // background-poll path goes through `readFrom()`, so inline snapshot() into + // finalize() and drop or privatize the totalBytes getter. + /** Read the collected tail without finalizing (the final-result snapshot). */ snapshot(): CollectedOutput { return { text: Buffer.concat(this.chunks).toString('utf8'), diff --git a/packages/tools/src/schema.ts b/packages/tools/src/schema.ts index 5e8887f11b..b717eabf9a 100644 --- a/packages/tools/src/schema.ts +++ b/packages/tools/src/schema.ts @@ -39,7 +39,14 @@ export interface SchemaProp { description?: string /** Enum of allowed values (strings only). */ enum?: string[] - /** Default value. */ + /** + * Default value, emitted into the JSON Schema only (validation never applies + * it — see the validator note below). + * + * XXX(unused-default): no tool definition in the repo sets `default`; it rides + * into the wire schema for a model that no tool surfaces it to. Drop the field + * and its converter line unless a real tool needs a model-visible default. + */ default?: unknown /** Nested properties for type: 'object'. */ properties?: SchemaSpec