docs: fold useful simplification RFCs from pr 74
This commit is contained in:
@@ -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 |
|
||||
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
49
docs/rfc/proposed/2026-06-20-prune-dead-seam-methods.md
Normal file
49
docs/rfc/proposed/2026-06-20-prune-dead-seam-methods.md
Normal file
@@ -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."
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user