Merge pull request #72 from deepseek-ai/codex/rfc-simplify-candidates
docs: propose simplification RFCs
This commit is contained in:
Tianyi Cui
GitHub
@@ -0,0 +1,106 @@
|
||||
---
|
||||
name: dsh-find-simplifications
|
||||
description: 'Use when working in the deepseek-harness repo to find non-obvious simplification candidates and write proposed RFCs or inline TODO/FIXME/XXX notes for dead, duplicated, speculative, or over-built code surfaces; especially for requests like "find simplification RFCs", "look for unnecessary complexity", "audit for removal-style cleanups", or "fold worthwhile simplification ideas from another PR".'
|
||||
---
|
||||
|
||||
# Finding DeepSeek Harness Simplifications
|
||||
|
||||
This skill helps turn a broad "find things to simplify" request into evidence-backed RFCs that remove or collapse existing harness surface area. It is guidance, not a checklist: follow the code, keep judgment active, and prefer a few well-proven candidates over a pile of thin guesses.
|
||||
|
||||
## Start With Repo Context
|
||||
|
||||
- Read `AGENTS.md`, especially the pre-release stance, tests-document-behavior section, conventions, defensive patterns, and Type Safety and Documentation section.
|
||||
- Skim [docs/architecture.md](../../../docs/architecture.md) before judging anything under `packages/`; simplifications that fight the service map or event taxonomy need extra evidence.
|
||||
- Use the RFC index ([docs/rfc/README.md](../../../docs/rfc/README.md)) to understand intentional architecture. The most relevant implemented examples are [drop mutable session summary](../../../docs/rfc/implemented/2026-06-19-drop-mutable-session-summary.md), [shared persistence write coordinator](../../../docs/rfc/implemented/2026-06-18-shared-persistence-write-coordinator.md), [capability seams](../../../docs/rfc/implemented/2026-06-13-capability-seams.md), and the twin adapter / dual persistence backend RFCs.
|
||||
- Treat dual LLM adapters and dual persistence backends as intentional by default. Do not propose deleting either twin/backend as "low effort" unless the user explicitly overrides that constraint. Removing an unused method or hook inside a protected seam can still be valid if it does not collapse the protected design.
|
||||
|
||||
## What Counts As A Strong Candidate
|
||||
|
||||
A strong simplification removes, folds, or demotes something real and has clear evidence that the current shape costs more than it buys:
|
||||
|
||||
- A public method, event, config knob, registry notification, helper, package, durable event, or test artifact has no production consumer.
|
||||
- Tests or docs are the only consumers, and the behavior they pin is not load-bearing.
|
||||
- Two representations mirror the same fact, especially across durable session events and transient `agent/*` events.
|
||||
- A seam has methods every implementation must support but no consumer uses.
|
||||
- A package boundary exists only for test/demo/support code and adds publish or dependency overhead.
|
||||
- A feature implements speculative product generality: multi-session/session-load, background task rosters, live registry invalidation, mid-turn steering, tool-owned UI rendering, and similar shapes with no product owner.
|
||||
- An invariant, rollback path, goldens set, or special-case test exists only to protect an unused surface.
|
||||
- The simplified behavior may differ slightly, but the new behavior is still reasonable and easier to explain.
|
||||
|
||||
Thin candidates are usually not enough for an RFC: deleting one typo, running `knip` once, removing an intentionally documented backend/adapter, or flagging "this looks complex" without call-site proof.
|
||||
|
||||
## Survey Broadly
|
||||
|
||||
Use parallel subagents when the user asks for breadth or many candidates. Give each agent a domain and require evidence, not guesses. Useful domains:
|
||||
|
||||
- Agent loop and session log: turn/step boundaries, steering, abort/cancel, durable events, replay, load/resume.
|
||||
- ACP and UI surfaces: `session/*` methods, terminal `_meta`, transcript rendering, single vs multi-session state.
|
||||
- LLM/tools/system prompt: stream/generate surfaces, assemblers, registries, tool schema defaults, presentation hooks.
|
||||
- Bash and tool execution: foreground/background split, task ownership, output spill files, executor methods.
|
||||
- Packages/examples/scripts/tests: package boundaries, static inventories, redundant snapshot goldens, support packages.
|
||||
|
||||
If subagents are unavailable, simulate the same breadth yourself. Do not let the first good candidate stop the survey.
|
||||
|
||||
## Prove Or Reject Each Candidate
|
||||
|
||||
For every symbol or behavior, classify consumers before writing:
|
||||
|
||||
- Production corpus: `packages/*/src`, `examples/*/src`, `examples/**/*.yml`, runtime scripts, and loader/config paths.
|
||||
- Non-production corpus: tests, README/docs, RFCs, snapshots, generated goldens, and comments.
|
||||
- Ambiguous corpus: examples and scripts that may be product smoke paths. Inspect usage before classifying.
|
||||
|
||||
Use `rg` first. Good searches include the exact symbol, event name, package name, config key, method name with both `.name(` and `name(`, and any wire strings. Then read the call sites. `knip` can help, but it is not a substitute for understanding public interfaces, dynamic event names, tests, docs, and Cordis loader paths.
|
||||
|
||||
Reject or downgrade a candidate when:
|
||||
|
||||
- A production caller exists and the simplification would be a feature decision rather than a cleanup.
|
||||
- The surface is explicitly justified by an implemented RFC or a hard-won defensive pattern, and the new evidence does not beat that reason.
|
||||
- The removal would force unrelated churn without actually making the contract smaller.
|
||||
- The idea is correct but tiny. Add a targeted TODO/FIXME/XXX instead, using the urgency semantics in [docs/development.md](../../../docs/development.md).
|
||||
|
||||
## Write The RFC
|
||||
|
||||
Create one file per durable proposal under `docs/rfc/proposed/yyyy-mm-dd-topic.md` and add it to the Proposed table in `docs/rfc/README.md`. Keep prose paragraphs on one physical line and use relative Markdown links.
|
||||
|
||||
Prefer this shape, adjusting when the idea needs it:
|
||||
|
||||
- `# RFC: <action-oriented title>`
|
||||
- `Status: proposed`
|
||||
- `## Problem`: name the current surface, cite the relevant files, and state the consumer evidence. Separate production callers from tests/docs.
|
||||
- `## Proposal`: say exactly what to remove, fold, demote, or rehome. Include tests, docs, READMEs, JSDoc, event-taxonomy, snapshot, and generated-file cleanup when relevant.
|
||||
- `## Why not keep it?` or `## What we give up`: make the strongest counterargument legible.
|
||||
- `## Acceptance criteria`: observable end state and gates.
|
||||
- `## Risks`: public API changes, behavior changes, future product wants, and why the tradeoff is still reasonable.
|
||||
|
||||
Be concrete enough that an implementing PR can follow the trail. Avoid vague "simplify this package" RFCs. When a proposal overlaps an existing RFC, consolidate the useful details into the existing one rather than creating a duplicate.
|
||||
|
||||
## Inline TODO Notes
|
||||
|
||||
Use inline TODO/FIXME/XXX only for small, local cleanups that are clearly useful but not durable design decisions. Keep them short and actionable:
|
||||
|
||||
- Name the smell with a stable tag, e.g. `TODO(double-default)` or `XXX(unused-default)`.
|
||||
- Explain why it is safe to revisit and what action would simplify it.
|
||||
- Do not add TODOs for speculative complaints or for behavior that needs an RFC-level decision.
|
||||
|
||||
## When Folding Another PR Or Branch
|
||||
|
||||
Diff the sibling branch against `origin/master`, not against the current PR branch, so you see its independent contribution. For each item:
|
||||
|
||||
- Port non-overlapping RFCs or TODOs that meet the quality bar.
|
||||
- Consolidate overlapping material into the existing RFC that owns the topic.
|
||||
- Do not port duplicate or lower-confidence proposals just to preserve the count.
|
||||
- Update the PR body so reviewers see the true candidate count and scope.
|
||||
- Close the duplicate PR only when the user asked you to, or when you clearly own that housekeeping.
|
||||
|
||||
## Validation And PR Hygiene
|
||||
|
||||
For docs-only RFC work, run at least `pnpm run doc-sync`, `pnpm run lint`, and `git diff --check`. For code comments or skill changes, also run the relevant validator when one exists. Before pushing, expect the pre-push hook to run module graph freshness, unit tests, snapshots, doc-sync, and hygiene.
|
||||
|
||||
When opening or updating a PR, summarize:
|
||||
|
||||
- How many RFCs and inline notes were added.
|
||||
- The main areas surveyed.
|
||||
- What was intentionally excluded.
|
||||
- Which checks passed.
|
||||
|
||||
Use a draft PR while the survey is still expanding; mark ready only when the candidate set, review responses, and validation are settled.
|
||||
@@ -32,6 +32,17 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r
|
||||
| [Optional Code Mode — model writes TypeScript against an SDK of all tools](proposed/2026-06-15-optional-code-mode.md) | 2026-06-15 |
|
||||
| [Runtime schemas for the event vocabulary (Zod vs the merge-extensible-map pattern)](proposed/2026-06-16-typed-event-schemas.md) | 2026-06-16 |
|
||||
| [Unify the agent id and the session id](proposed/2026-06-20-unify-agent-and-session-id.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 unconsumed assembled LLM convenience surfaces](proposed/2026-06-20-drop-unconsumed-llm-assembled-surfaces.md) | 2026-06-20 |
|
||||
| [Drop the unconsumed `llm/adapter-change` event](proposed/2026-06-20-drop-unconsumed-llm-adapter-change-event.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 |
|
||||
| [Fold trace-only session facts into load-bearing events](proposed/2026-06-20-collapse-trace-only-session-events.md) | 2026-06-20 |
|
||||
| [Extract a generic long-running tool runtime](proposed/2026-06-20-generic-long-running-tool-runtime.md) | 2026-06-20 |
|
||||
| [Make the shared example base providerless](proposed/2026-06-20-providerless-example-base.md) | 2026-06-20 |
|
||||
| [Use `session.jsonl` as the only snapshot session-log artifact](proposed/2026-06-20-remove-redundant-snapshot-log-goldens.md) | 2026-06-20 |
|
||||
| [Reorganize packages into a modular hierarchy](proposed/2026-06-20-package-hierarchy.md) | 2026-06-20 |
|
||||
| [Discover package inventories instead of maintaining static lists](proposed/2026-06-20-discover-package-inventory.md) | 2026-06-20 |
|
||||
|
||||
## Implemented
|
||||
|
||||
@@ -70,3 +81,14 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r
|
||||
| Title | First proposed |
|
||||
|---|---|
|
||||
| [Deep-readonly public surfaces](rejected/2026-06-11-immutable-public-surfaces.md) | 2026-06-11 |
|
||||
| [Persist assembled assistant messages, not stream chunks](rejected/2026-06-20-assembled-assistant-messages-only.md) | 2026-06-20 |
|
||||
| [Drop ACP session/load until resume has a product shape](rejected/2026-06-20-drop-acp-session-load.md) | 2026-06-20 |
|
||||
| [Drop ACP terminal `_meta` rendering](rejected/2026-06-20-drop-acp-terminal-meta.md) | 2026-06-20 |
|
||||
| [Drop bash full-output spill files](rejected/2026-06-20-drop-bash-output-spill-files.md) | 2026-06-20 |
|
||||
| [Drop durable step boundary events](rejected/2026-06-20-drop-durable-step-boundaries.md) | 2026-06-20 |
|
||||
| [Drop unused session lineage metadata](rejected/2026-06-20-drop-unused-session-lineage.md) | 2026-06-20 |
|
||||
| [Fold the persistence interface into dsh-session](rejected/2026-06-20-fold-session-persistence-interface.md) | 2026-06-20 |
|
||||
| [Collapse tool-owned UI presentation](rejected/2026-06-20-generic-tool-rendering.md) | 2026-06-20 |
|
||||
| [Retire mid-turn steering](rejected/2026-06-20-retire-mid-turn-steering.md) | 2026-06-20 |
|
||||
| [Return the ACP bridge to one live session per connection](rejected/2026-06-20-single-session-acp-bridge.md) | 2026-06-20 |
|
||||
| [Truncate interrupted final turns on load](rejected/2026-06-20-truncate-interrupted-turns.md) | 2026-06-20 |
|
||||
@@ -5,6 +5,8 @@ Status: proposed
|
||||
<!-- XXX: legacy ADR/RFC body format, not yet normalized to a unified RFC template. -->
|
||||
> **Implementation status:** the multi-session bridge (steps 1, 3, 4) and the bash task-ownership isolation are implemented in `packages/acp` + `packages/tool-bash`. **Per-session *permission* ownership is deferred** — it depends on [the ACP support permission gate](2026-06-14-acp-agent-client-protocol.md) (`TODO(rfc010-permission-gate)`), which is itself deferred; the `agent→sessionId` reverse map the gate will route through is in place. Step 2's per-session disposer scope is now implemented (see [agent lifecycle & ownership seams](../implemented/2026-06-18-agent-lifecycle-and-ownership-seams.md)): the factory returns a per-agent `AgentHandle` whose `dispose()` stops the loop, awaits quiescence, unregisters the agent, and removes its session, so a bare client disconnect leaves no registered agent or session-store entry. Status stays `proposed` until per-session permission ownership lands.
|
||||
|
||||
> **Target-client note:** Zed is the current target ACP client, and its ACP client maintains a `HashMap<SessionId, AcpSession>` plus `pending_sessions` for concurrent `session/load` calls. The competing simplification to return to one live session per connection was rejected after checking that target-client shape; this RFC remains the path for finishing multiplexing and per-session permission ownership. See [the rejected simplification](../rejected/2026-06-20-single-session-acp-bridge.md).
|
||||
|
||||
## Problem
|
||||
|
||||
[ACP support](2026-06-14-acp-agent-client-protocol.md) ships with a single active session per connection: a second `session/new` is rejected. Editors expect to run several conversations over one agent subprocess — a user opens multiple threads, or a client pre-warms sessions. The single-session guard is a deliberate MVP scope cut, not an architectural limit; this RFC lifts it.
|
||||
|
||||
@@ -0,0 +1,33 @@
|
||||
# RFC: Fold trace-only session facts into load-bearing events
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
The session event vocabulary includes first-class events that are not part of replayable conversation history and have little or no production consumption. `usage` is already present as a model stream chunk before the loop also appends a separate `usage` event. `error` duplicates the `turn/end { kind: 'error', message, code }` reason for loop failures; ACP settlement reads the turn-end reason, ACP rendering ignores the `error` event, and `deriveMessages()` skips it.
|
||||
|
||||
These events make the canonical transcript look more useful as telemetry than it currently is. They add event variants, invariants, tests, snapshots, and persistence cases, but they are not load-bearing as separate records. The facts they carry can still be useful: token usage should remain available for accounting, and an error's step number should not silently disappear. The simplification is to fold those facts into nearby events consumers already must understand, not to record less information.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove standalone trace-only events only where their information can be preserved without a parallel record:
|
||||
|
||||
- Fold successful-step usage into the matching `assistant/message`, e.g. `assistant/message { turn, step, content, usage? }`, so the assembled model output and its accounting travel together.
|
||||
- For a failed or aborted step that has usage but no `assistant/message`, carry the usage on the terminal turn reason or another load-bearing failure record in the same turn. The implementing design must prove no usage chunk that is currently persisted becomes unrepresented.
|
||||
- Fold the step number from the standalone `error` event into `turn/end.reason` for `kind: 'error'`, e.g. `{ kind: 'error', step, message, code? }`. `turn/end` is the durable turn outcome ACP and resume already consume.
|
||||
- Keep `agent/error` and logging for live diagnostics; do not add a second session-log error record after `turn/end`.
|
||||
|
||||
If analytics become real, add a projection helper or a dedicated telemetry store with its own retention policy. The user conversation log should contain what is needed to render, resume, audit, and account for the interaction without requiring consumers to reconcile duplicate trace rows.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `SessionEventMap` drops standalone `usage` and `error` only after their fields are represented on load-bearing session events.
|
||||
- The loop no longer appends a separate `usage` event for a usage chunk.
|
||||
- The loop records durable failures through `turn/end { kind: 'error', step, message, code? }` or an equivalent no-information-loss shape and reports live diagnostics through `agent/error`.
|
||||
- ACP snapshots and persistence tests stop asserting trace-only lines.
|
||||
- Documentation explains exactly where token usage and operational errors are observed.
|
||||
- The session format version and recorded fixtures are refreshed; non-current stored logs are rejected per the pre-release format policy.
|
||||
|
||||
## What we give up
|
||||
|
||||
A consumer can no longer filter the canonical log for standalone `usage` or step-level `error` rows. It must read those facts from the assistant/failure events that carry them. That is a reasonable simplification only if the implementing PR proves the same facts remain present; otherwise the standalone events should stay.
|
||||
@@ -0,0 +1,26 @@
|
||||
# RFC: Discover package inventories instead of maintaining static lists
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
Package and gate inventories are repeated by hand. [scripts/publint-all.ts](../../../scripts/publint-all.ts) has a static list of publishable packages. The [package cookbook](../../cookbook/adding-a-package.md) tells authors to update several files. The [package README](../../../packages/README.md) carries a hand-written dependency graph. [CI](../../../.github/workflows/ci.yml) and [development docs](../../development.md) can drift from the actual `doc-sync` subcommands when new gates are added. These lists are small today, but every new package or gate creates another manual synchronization point.
|
||||
|
||||
Static lists are appropriate when they encode policy; they are needless friction when they duplicate manifest data or layout facts that already exist in `package.json`, workspace globs, or the package hierarchy.
|
||||
|
||||
## Proposal
|
||||
|
||||
Make package/gate inventories discoverable. Publishability should come from the deliberate [package hierarchy](2026-06-20-package-hierarchy.md) plus package manifests, not from a static array in a script or the npm `private` flag. Module graph generation should read package manifests. `doc-sync` should be the one command that defines and prints its sub-gates, with docs linking to that command rather than restating a second list.
|
||||
|
||||
The hierarchy does not need to encode every fact about a package, but it should encode the broad maintenance policy: core/product packages, integrations, capability seams, and support/test/example packages should not all require a hand-maintained exception list before scripts can tell them apart.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `publint-all` discovers publishable packages from the hierarchy plus manifests instead of a hard-coded array.
|
||||
- Adding a package does not require editing a static package list for every gate.
|
||||
- Docs describe the source of truth rather than repeating generated inventories.
|
||||
- CI invokes the aggregate commands and lets those commands own their sub-gate lists.
|
||||
|
||||
## What we give up
|
||||
|
||||
Discovery scripts can become too clever. The implementation should stay boring: read manifests, filter on explicit fields, print the resolved list, and fail loud. The payoff is removing manual inventory drift, not inventing a build system.
|
||||
@@ -0,0 +1,43 @@
|
||||
# RFC: Drop the unconsumed `llm/adapter-change` event
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`LlmService.registerAdapter()` emits `llm/adapter-change` on registration and disposal ([packages/llm/src/index.ts](../../../packages/llm/src/index.ts)). Grepping `llm/adapter-change` across `packages/*/src` and `examples/*/src` finds only the declaration, emit sites, docs, and tests; no production listener subscribes to it.
|
||||
|
||||
This differs from `tools/change` and `system-prompt/change`. Those two events are also unconsumed today, but they are plausible registry-change signals for future live tool/prompt UIs. LLM adapter registration is more of a boot-time implementation detail: adapters are not a user-visible palette and the real model-call interception seam is `llm/stream`. Keeping an adapter-change event with no listener repeats the [drop-the-dead-summary](../implemented/2026-06-19-drop-mutable-session-summary.md) pattern at a smaller scale.
|
||||
|
||||
The event is not free. `registerAdapter()` yields its rollback disposer before emitting `llm/adapter-change` so a throwing listener unwinds the mutation instead of leaking an adapter entry, and the package carries tests for that listener-throw path. That defensive ordering protects a failure mode only tests can trigger.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove only `llm/adapter-change`:
|
||||
|
||||
- Delete the `llm/adapter-change` declaration from `dsh-llm`'s `interface Events`.
|
||||
- Delete the `ctx.emit('llm/adapter-change')` calls.
|
||||
- Simplify `registerAdapter()`'s effect generator: keep the mutation and rollback disposer for HMR/disposal, but drop the listener-throw rollback ordering that exists only for the removed event.
|
||||
- Remove the "Emits `llm/adapter-change` on registration and disposal" sentence from `LlmService.registerAdapter`'s JSDoc.
|
||||
- Rewrite the adapter-disposer test to assert the returned disposer removes the adapter without subscribing to `llm/adapter-change`; delete the listener-throw rollback test that exists solely for the removed event.
|
||||
- Update the event taxonomy table in [docs/architecture.md](../../../docs/architecture.md) and [packages/llm/README.md](../../../packages/llm/README.md). The [doc-sync-enforcement RFC](../implemented/2026-06-11-doc-sync-enforcement.md) should avoid using `llm/adapter-change` as an example once the event is gone.
|
||||
|
||||
## Why not remove every registry change event?
|
||||
|
||||
A microkernel where registries announce mutations is a coherent convention. `tools/change` and `system-prompt/change` may become useful when a UI can live-refresh available tools or prompt sections. This RFC leaves that convention intact where it has a plausible user-facing consumer and cuts only the adapter-change event whose current and likely future consumer is unclear.
|
||||
|
||||
If an LLM adapter browser or dynamic model-picker needs this signal later, reintroduce it with that consumer and a clearer payload than "something changed."
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `llm/adapter-change` and its 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 adapter.
|
||||
- `tools/change` and `system-prompt/change` remain documented and tested.
|
||||
- `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.
|
||||
- **The registry-change convention becomes uneven.** That is acceptable because LLM adapter registration is not the same user-facing concept as tools or prompt sections. Uneven but honest beats uniform but dead.
|
||||
|
||||
This is a small cut, but it retires a standing correctness invariant that guards a consumer that does not exist.
|
||||
@@ -0,0 +1,45 @@
|
||||
# RFC: Drop unconsumed assembled LLM convenience surfaces
|
||||
|
||||
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 raw chunks through its own `BlockAssembler` so it can log 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` and `ctx.llm.generate` across `packages/*/src` and `examples/*/src` finds no production callers. The references are the service methods, docs, and tests; adapter tests use `generate()` as a convenient driver, but they can hand-drain `stream()` through the same assembler helper without preserving a public production API.
|
||||
|
||||
This is the [drop-mutable-session-summary](../implemented/2026-06-19-drop-mutable-session-summary.md) pattern: assembled-view APIs with tested contracts, consumed by tests rather than production. They were built speculatively for consumers that do not care about token-level deltas, but the one real consumer cares about deltas precisely so it can persist high-fidelity replay data.
|
||||
|
||||
`streamBlocks()` 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 incremental in-order yield. `generate()` drags `GenerateResult`, `BlockAssembler.result()`, and the `llm/generate` waterfall as a second interception surface over the same underlying stream. The loop's assembler usage is `push()` / `message()` / `usage` / `finish` — not streaming flush or one-shot service assembly.
|
||||
|
||||
## Proposal
|
||||
|
||||
Make `stream()` the only public LLM call surface:
|
||||
|
||||
- Remove `LlmService.streamBlocks()` and its JSDoc.
|
||||
- Remove `LlmService.generate()`, the `llm/generate` waterfall event, and `GenerateResult` if no surviving API needs that named result shape.
|
||||
- Remove `BlockAssembler.flushReady()`, `BlockAssembler.flushRemaining()`, and the `flushed` cursor field.
|
||||
- Remove `BlockAssembler.result()` if it is only a helper for the deleted `generate()` service path and tests.
|
||||
- Replace adapter-test use of `ctx.llm.generate()` with a small test helper that calls `ctx.llm.stream()`, pushes chunks into `BlockAssembler`, and returns the assembled message, usage, and finish reason needed by that test. That keeps the [twin-adapter design](../implemented/2026-06-13-twin-llm-adapters.md) intact while avoiding a public method whose only callers are tests.
|
||||
- Remove or rework the `flushReady`/`flushRemaining`-dependent tests. Keep assembler invariants that still apply to `push()` / `blocks()` / `message()`; delete behavior that only pins the removed flush API.
|
||||
- Update every doc/comment reference to `streamBlocks`, `generate`, `GenerateResult`, and `llm/generate` across `docs/`, package READMEs, and source comments. The `ctx.llm` service-map row in [docs/architecture.md](../../../docs/architecture.md) becomes `stream()` only, the event taxonomy drops `llm/generate`, and the [property-based-testing RFC](../implemented/2026-06-11-property-based-testing.md) names block-assembly invariants without referring to removed convenience methods.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `streamBlocks`, `generate`, `llm/generate`, and the assembler helpers they alone require 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).
|
||||
- Adapter tests still exercise both real adapters through `stream()` and the shared assembler, not through a test-only public shortcut.
|
||||
- The loop behaves identically — verified by unchanged ACP snapshot goldens.
|
||||
- `packages/llm/README.md`, [docs/architecture.md](../../../docs/architecture.md), and module docs no longer mention the removed convenience surfaces.
|
||||
|
||||
## Risks
|
||||
|
||||
- **It removes public methods from a core vocabulary package.** A future plugin that wants assembled blocks without deltas would need to call `stream()` and use `BlockAssembler` directly or reintroduce a focused helper with a real consumer. Given the pre-release "foundation over speculative future" stance ([AGENTS.md](../../../AGENTS.md)), this is the right time to cut test-only public shape.
|
||||
- **Adapter tests get a little more explicit.** They lose the ergonomic `generate()` wrapper, but that is useful pressure: tests exercise the same streaming path production uses.
|
||||
- **Waterfall users lose `llm/generate`.** No production listener exists. Any future caching/retry/logging plugin should wrap `llm/stream`, which remains the single provider call path.
|
||||
|
||||
The size is modest, but it is a clean removal of speculative surface area from the LLM package, leaving one model-call contract for both production and tests.
|
||||
@@ -0,0 +1,35 @@
|
||||
# RFC: Extract a generic long-running tool runtime
|
||||
|
||||
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`, `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`.
|
||||
|
||||
## Proposal
|
||||
|
||||
Move long-running task semantics above bash into a tool-agnostic runtime. Bash remains able to run background commands, but it stops owning the general concepts of task ids, ownership tokens, polling, cancellation, completion notifications, and model-facing "read/kill this task" commands.
|
||||
|
||||
The runtime should own:
|
||||
|
||||
- Stable task ids and owner tokens keyed to the calling session/agent.
|
||||
- Registration of a long-running task with a producer for incremental output and a completion promise.
|
||||
- Generic read/cancel/list operations with the same cross-session authorization rule for every tool.
|
||||
- Completion notification injection into the owning session.
|
||||
- Presentation hooks for pending/running/completed task state, with bash supplying only command-specific labels and output formatting.
|
||||
|
||||
`dsh-bash` then keeps the bash-specific execution contract: resolve a request into a command spec, run a foreground command, or start a process and hand its streams/process handle to the generic runtime. `dsh-tool-bash` keeps the model-facing command tool, but the follow-up operations become generic long-running-tool operations or a shared utility that bash registers with, rather than bespoke `bash_output`/`bash_kill` plumbing.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- The bash-specific packages no longer define the generic task registry, owner-token authorization, polling, cancellation, or completion-notification machinery.
|
||||
- A shared long-running-task service or tool layer owns those semantics and is documented as the path for any future background-capable tool.
|
||||
- Bash background behavior remains available through the shared layer, with tests proving cross-session isolation still holds.
|
||||
- ACP and snapshot fixtures render background bash through the shared task vocabulary, not through bash-only lifecycle semantics.
|
||||
- The [tool cookbook](../../cookbook/adding-a-tool.md) points long-running tools at the shared runtime instead of telling each tool to invent its own task protocol.
|
||||
|
||||
## What we give up
|
||||
|
||||
The bash package loses local ownership of an already-working background-task implementation, and the implementing PR may temporarily churn model-facing tool names or transcript presentation. That churn is worthwhile if it leaves one background-task contract instead of making every future long-running tool clone bash's private protocol.
|
||||
@@ -0,0 +1,59 @@
|
||||
# RFC: Reorganize packages into a modular hierarchy
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`packages/` is flat. Core product packages, provider integrations, capability seams, example UI support, and snapshot-only replay support all sit at the same level and look equally foundational. The [package README](../../../packages/README.md) already has a `FIXME(package-hierarchy)` noting that `ui-stdio` and `llm-replay` were extracted from examples mostly for reuse and coverage. The flat layout makes support packages appear more product-shaped than they are and forces publish/lint/doc scripts to encode intent through comments or static lists.
|
||||
|
||||
This is not just cosmetic. A package's location currently says little about whether it is core API, a swappable capability, an adapter integration, an example harness helper, or test infrastructure. That makes future removal harder because every top-level package looks like part of the same public surface.
|
||||
|
||||
## Proposal
|
||||
|
||||
Move packages into a deliberate hierarchy under `packages/`. The exact layout is deferred to the implementing PR, but it should group packages by modular role rather than keep every package at one flat level.
|
||||
|
||||
One plausible shape:
|
||||
|
||||
```text
|
||||
packages/
|
||||
core/
|
||||
session/
|
||||
system-prompt/
|
||||
tools/
|
||||
agent/
|
||||
agent-loop/
|
||||
invariants/
|
||||
llm/
|
||||
llm/
|
||||
adapters/
|
||||
llm-deepseek/
|
||||
llm-pi-ai/
|
||||
bash/
|
||||
bash/
|
||||
bash-local/
|
||||
tool-bash/
|
||||
session-persistence/
|
||||
session-persistence/
|
||||
session-persistence-jsonl/
|
||||
session-persistence-sqlite/
|
||||
acp/
|
||||
support/
|
||||
ui-stdio/
|
||||
llm-replay/
|
||||
```
|
||||
|
||||
The final implementation may choose different names or groupings, but it should keep the same intent: core APIs, package families such as LLM/bash/session persistence, standalone integrations such as ACP, and support/test/example packages are distinguishable from the filesystem alone. Npm package names can stay `@deepseek-ai/dsh-*`; the hierarchy is about repo structure and maintenance policy, not public package renaming.
|
||||
|
||||
This proposal does not delete `llm-replay` or `ui-stdio` by itself. It makes their status honest: either they graduate into product packages with documented consumers, or they live under a support/testing/example classification where release and compatibility expectations are lower.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- Packages move from the flat `packages/<name>/` layout into a documented modular hierarchy.
|
||||
- The implementing PR chooses the exact hierarchy and updates workspace globs, TypeScript paths, package docs, generated module graphs, `cordis.yml` package paths, build scripts, and publish/lint scripts in one coordinated move.
|
||||
- Scripts that publish, lint publishability, or generate package inventories use the hierarchy instead of an ad hoc static list where the hierarchy is enough to express the policy.
|
||||
- Docs explain which package groups are part of the product API and which groups are support/test/example infrastructure.
|
||||
- New package guidance tells authors where to place a package and discourages new one-off top-level groups.
|
||||
|
||||
## What we give up
|
||||
|
||||
The restructure churns imports, workspace globs, docs links, and package paths. That churn is acceptable pre-release if it prevents the flat layout from fossilizing support packages as product contracts.
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Make the shared example base providerless
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
The examples have two shared base files: [examples/base-core.yml](../../../examples/base-core.yml) is providerless, while [examples/base.yml](../../../examples/base.yml) includes that core plus the real `llm-deepseek` adapter. Snapshot replay needs the providerless core with `llm-replay`, because loading the real adapter without a key throws. The normal demos need the real adapter. The result is a naming inversion: the file named `base.yml` is not the reusable base for all examples, while the true base is `base-core.yml`.
|
||||
|
||||
The split is understandable, but it makes every config explanation longer. It also leads to awkward test setup like a keyless smoke test carrying a dummy API key so an adapter can boot even though the model is not called.
|
||||
|
||||
## Proposal
|
||||
|
||||
Rename the providerless core to [examples/base.yml](../../../examples/base.yml) and make adapter selection explicit in each concrete example. The coding and ACP real configs add a tiny `llm-deepseek` include or local block; snapshot config adds `llm-replay`. Delete [examples/base-core.yml](../../../examples/base-core.yml).
|
||||
|
||||
The shared base should contain only provider-neutral services and tools: `llm`, sessions, system prompt, tools, agents, invariants, bash executor, and bash tool schemas. Anything that chooses a model provider belongs at the leaf config.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [examples/base.yml](../../../examples/base.yml) is providerless.
|
||||
- [examples/base-core.yml](../../../examples/base-core.yml) is deleted.
|
||||
- Real demo configs explicitly add the DeepSeek adapter.
|
||||
- Snapshot replay config includes the same providerless base and its replay adapter.
|
||||
- The [examples README](../../../examples/README.md), example-specific READMEs, and RFC references stop explaining "base = base-core plus adapter".
|
||||
|
||||
## What we give up
|
||||
|
||||
Real demos lose one layer of convenience: each must opt into the adapter. That is the right default for examples, because adapter choice is the variable part and providerless wiring is the shared product core.
|
||||
@@ -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."
|
||||
@@ -0,0 +1,32 @@
|
||||
# RFC: Keep one public stop primitive
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
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. 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
|
||||
|
||||
- `Agent` exposes no public `abort()` or `whenIdle()`; `steer()` remains part of the message surface.
|
||||
- ACP cancellation continues to call `cancel()`.
|
||||
- Agent teardown continues to await quiescence through handle disposal.
|
||||
- Tests cover cancellation and disposal as the two supported stop paths.
|
||||
|
||||
## What we give up
|
||||
|
||||
A future plugin cannot abort only the current model/tool step while preserving queued prompts through the public interface. If that use case becomes real, it should return with a named consumer and a narrower contract. Today it is latent generality that keeps private loop mechanics public.
|
||||
|
||||
## Related
|
||||
|
||||
This RFC only removes the stop/quiescence methods. Mid-turn steering remains an intentional message path; the resulting public surface is `send()`, `steer()`, `inject()`, `cancel()`, status, options, session, and identity.
|
||||
@@ -0,0 +1,31 @@
|
||||
# RFC: Stop mirroring durable boundaries as agent events
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
The loop records the canonical transcript in `SessionEvent` and also emits a parallel set of live `agent/*` mirror events: `agent/turn-start`, `agent/turn-end`, `agent/step-start`, `agent/step-end`, `agent/stream-chunk`, and `agent/steering`. The mirrors make consumers choose between two sources of truth. ACP already chose the session log for the editor-facing transcript because a throwing peer listener can prevent later `agent/*` listeners from observing a boundary, while the session event was already appended. The stdio UI is the only production consumer that still renders turn boundaries and the token stream from the mirror events; it already renders tool calls and results from `session/event`.
|
||||
|
||||
This duplication is not free. Every lifecycle change has to update the session event, the mirror event, docs, invariants, tests, and snapshot expectations. The duplicate boundary events also make failure ordering subtle: a turn can be durably closed before a live `agent/turn-end` listener runs, so a post-boundary listener failure has no valid in-log position left and must be reported out of band.
|
||||
|
||||
## Proposal
|
||||
|
||||
Make `session/event` the live transcript stream. Consumers that render turns, tool calls, tool results, assistant messages, and durable boundaries subscribe to `session/event` and derive their UI from the same event vocabulary persistence uses. Keep agent lifecycle/control events that are not transcript data: `agent/created`, `agent/disposed`, `agent/status`, `agent/error`, and `agent/queued`. `agent/queued` is an inbox acknowledgement rather than a transcript mirror: it fires before any durable event exists, and cancelled queued work may never enter the log.
|
||||
|
||||
Remove the duplicate durable-boundary mirrors from the agent event taxonomy. If a UI wants an agent handle from a session event, it can keep a small map from session id to agent built from `agent/created`/`agent/disposed`, or the registry can offer an explicit lookup. The canonical record remains the event-sourced session log.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- ACP and stdio render transcript content from `session/event`.
|
||||
- `agent/turn-start`, `agent/turn-end`, `agent/step-start`, `agent/step-end`, and `agent/steering` are removed or reduced to private implementation details.
|
||||
- `agent/queued` is either retained and documented as live-only inbox/control state, or deleted in a separate proposal that names the queue-acknowledgement capability loss.
|
||||
- Tests assert the persisted event stream, not a second mirror stream, for turn and step ordering.
|
||||
- Documentation presents `SessionEvent` as both the durable source and the live transcript feed.
|
||||
|
||||
## What we give up
|
||||
|
||||
A plugin can no longer observe turn/step boundaries from a convenient `Agent`-first event. It must either subscribe to `session/event` or maintain a session-to-agent association. That is an acceptable trade: transcript consumers should not depend on a second event feed that can drift from the durable log.
|
||||
|
||||
## Related
|
||||
|
||||
Because high-fidelity `assistant/chunk` persistence remains load-bearing, `agent/stream-chunk` can be evaluated as another mirror of durable session data rather than as the only token stream. If a future proposal moves chunks out of the canonical log, `agent/stream-chunk` would need a fresh decision as a deliberately live-only UI signal.
|
||||
@@ -0,0 +1,31 @@
|
||||
# RFC: Use `session.jsonl` as the only snapshot session-log artifact
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
Model-driving ACP snapshot scenarios ship both `session.jsonl` and `session.golden.jsonl`. For normal recorded scenarios, `session.jsonl` is the replay fixture harvested from a real run, and the replay test normalizes the newly persisted log and compares it to `session.golden.jsonl`. In the current fixtures, the normalized recorded log and normalized golden are identical for the ordinary recorded scenarios.
|
||||
|
||||
Authored override scenarios (`error-finish`, `cancel`) currently use `replay.override.json` to drive model behavior and keep `session.jsonl` as a minimal dummy fixture, while `session.golden.jsonl` holds the expected persisted log. The override file is a JSON array of `ReplayEntry` objects: `{ "kind": "chunks", "chunks": StreamChunk[] }`, `{ "kind": "throw", "chunks": StreamChunk[], "message": string, "code": string, "status"?: number }`, or `{ "kind": "hang" }`. That split is also unnecessary: when an override sidecar exists, `llm-replay` replaces the derived script and does not need `session.jsonl` for model chunks, so `session.jsonl` can still be the expected session-log artifact for the scenario.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove the `session.golden.jsonl` concept entirely. Every scenario has at most one committed session-log artifact, `session.jsonl`:
|
||||
|
||||
- For recorded scenarios, `session.jsonl` remains the raw harvested log. Replay still derives model chunks from it, and the snapshot test compares the replay run's normalized persisted log against normalized `session.jsonl`.
|
||||
- For authored override scenarios, `replay.override.json` drives model behavior and `session.jsonl` holds the expected produced session log. The replay adapter ignores the fixture for model chunks when the override exists, so the same file can be the expected log without affecting replay behavior.
|
||||
- For no-model scenarios, `session.jsonl` can stay as the minimal fixture needed to boot `llm-replay`; no session-log comparison is needed unless the scenario creates a persisted session.
|
||||
|
||||
Stdout goldens remain unchanged; they are the editor-facing projection and are not redundant with the session fixture.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `session.golden.jsonl` disappears from the snapshot harness, fixtures, orphan guards, and docs.
|
||||
- The snapshot test derives the expected session log from `session.jsonl` for every model scenario.
|
||||
- Authored sidecar scenarios commit their expected produced log in `session.jsonl`; `replay.override.json` remains the model-behavior override.
|
||||
- Orphan-fixture guards understand which files are required by scenario kind.
|
||||
- The [ACP snapshot tests RFC](../implemented/2026-06-19-acp-snapshot-tests.md) is updated to describe the reduced fixture set.
|
||||
|
||||
## What we give up
|
||||
|
||||
Reviewers lose one artifact name that made the expected persisted log visually separate from the replay fixture. The stdout golden still protects the editor transcript, and comparing replay output to `session.jsonl` preserves the loop/persistence regression check without duplicating files.
|
||||
@@ -0,0 +1,32 @@
|
||||
# RFC: Persist assembled assistant messages, not stream chunks
|
||||
|
||||
Status: rejected — high-fidelity chunk replay, partial failed streams, and snapshot replay currently depend on persisted `assistant/chunk` events. Dropping chunks is only viable with a no-information-loss replay/artifact replacement.
|
||||
|
||||
## Problem
|
||||
|
||||
The canonical session log currently persists every `assistant/chunk` exactly as streamed by the model. The [session persistence RFC](../implemented/2026-06-14-session-persistence.md) chose this for token-level replay fidelity and contiguous `seq`, but the cost has grown: JSONL fixtures are dominated by tiny delta records, snapshot scenarios replay the model by grouping chunk events, ACP load reconstructs prior assistant output from chunks, and any future log reader must distinguish durable message history from token-level trace.
|
||||
|
||||
For successful steps that assemble completed content, the loop already appends an `assistant/message`. That is the event `deriveMessages()` uses for the next model request. In other words, the normal resumable conversation state is already present without the chunks; chunks are a live rendering and deterministic-test artifact, not required conversation history. Failed or aborted streams are different: partial assistant output may exist only as chunks, and empty max-token steps may produce no `assistant/message` at all.
|
||||
|
||||
## Proposal
|
||||
|
||||
Stop storing `assistant/chunk` in the canonical session log. The durable log keeps `assistant/message`, `tool/call`, `tool/result`, `usage` if retained, and turn boundaries. Live UIs can still receive token deltas through a deliberately transient stream event. Snapshot replay should move its model script into an explicit fixture sidecar or derive it from a recorded adapter artifact, rather than treating the canonical user session as a token tape. Scenarios that need partial failed-stream output must record that output in the replay fixture.
|
||||
|
||||
ACP `session/load` can replay prior assistant messages as complete content blocks instead of simulating the original token stream. A loaded transcript need not reproduce every historical delta; it must show the same completed assistant content and resume with a valid provider history.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `SessionEventMap` drops `assistant/chunk`, or marks it as non-persisted if a transitional live event is needed.
|
||||
- [Session persistence docs](../../../packages/session-persistence/README.md) no longer require every stream chunk to be stored verbatim.
|
||||
- `llm-replay` and ACP snapshots use an explicit replay fixture format or sidecar for model chunks.
|
||||
- `session/load` renders completed assistant messages from `assistant/message`.
|
||||
- Stored logs get much smaller and remain `seq`-contiguous without chunk holes.
|
||||
- The session format version and recorded fixtures are refreshed; non-current stored logs are rejected per the pre-release format policy.
|
||||
|
||||
## What we give up
|
||||
|
||||
The canonical user session no longer reconstructs the exact token stream of an old turn. It also loses partial assistant output from failed or aborted streams unless another event or fixture records it. That is too much information loss for the current resume, load, and snapshot contracts. Tests that need exact deterministic streams should own that fixture directly only if the production session log keeps enough fidelity for user-visible recovery.
|
||||
|
||||
## Related
|
||||
|
||||
This supersedes the chunk-persistence choice in [session persistence](../implemented/2026-06-14-session-persistence.md) and affects [ACP snapshot tests](../implemented/2026-06-19-acp-snapshot-tests.md), whose current replay plugin derives its script from `assistant/chunk` events.
|
||||
@@ -0,0 +1,25 @@
|
||||
# RFC: Drop ACP session/load until resume has a product shape
|
||||
|
||||
Status: rejected — Zed is the current target ACP client, advertises and exercises load-capable sessions, and keeps pending-load state for concurrent `session/load`. The bridge should keep `session/load` and make the resume contract solid.
|
||||
|
||||
## Problem
|
||||
|
||||
ACP advertises `loadSession: true` and implements `session/load` by injecting persistence into the bridge, validating cwd against stored metadata, reconstructing an agent from the persisted log, and replaying prior transcript updates to the client. That path has its own race handling, loading-id guard, replay presenter logic, and tests. It also depends on the canonical log retaining enough UI data to reconstruct old chunks and tool presentations.
|
||||
|
||||
Durable persistence remains foundational, but editor-visible resume is not yet a designed product flow. There is no session picker, no title/preview metadata, and no clear UX for failed or partial loads. The bridge is paying complexity for a feature that is exercised by tests, documentation, and the current target client's session model.
|
||||
|
||||
## Proposal
|
||||
|
||||
For now, ACP starts fresh sessions only. `initialize` advertises `loadSession: false` or omits the capability, and `session/load` is unsupported. Persistence remains available to the agent loop and tests; resume can still exist as a lower-level factory if another consumer needs it. The editor bridge should reintroduce `session/load` alongside a real session-selection UX and a stable load transcript contract.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- ACP no longer injects `sessionPersistence` solely for `session/load`.
|
||||
- `initialize` does not advertise load support.
|
||||
- The `session/load` handler, loading-id tracking, cwd preflight for loaded sessions, and load replay tests are removed.
|
||||
- Snapshot fixtures no longer rely on load replay presentation.
|
||||
- [ACP docs](../../../packages/acp/README.md) describe fresh-session support only.
|
||||
|
||||
## What we give up
|
||||
|
||||
An editor cannot reopen a prior persisted session through ACP. That is a real product feature, but the current implementation is ahead of the UX and ties the bridge to token-level log replay. Keeping persistence while dropping editor load narrows the bridge to the workflow it can currently present cleanly.
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Drop ACP terminal `_meta` rendering
|
||||
|
||||
Status: rejected — Zed is the current target client, and the terminal `_meta` convention is intentional Zed UX with a plain ACP fallback for other clients.
|
||||
|
||||
## Problem
|
||||
|
||||
The ACP bridge implements a Zed-specific terminal-card convention through `_meta.terminal_info`, `_meta.terminal_output`, and `_meta.terminal_exit`. The implemented [rich ACP bash rendering RFC](../implemented/2026-06-18-acp-terminal-and-tool-rendering.md) deliberately avoided ACP's client-side `terminal/create` because bash execution belongs in the harness, but still adopted the reference agents' display-only `_meta` convention. That gives a nicer Zed card at the cost of bridge state, capability negotiation, terminal ids, special update mapping, text fallback tests, and exit-pill parsing in `dsh-tool-bash`.
|
||||
|
||||
The fallback path already exists: render the tool call and completed output as normal ACP content blocks. Non-Zed clients rely on that path anyway, but the Zed terminal card is a current target-client feature rather than speculative decoration.
|
||||
|
||||
## Proposal
|
||||
|
||||
Ignore `clientCapabilities._meta.terminal_output` and render bash results through the plain ACP content path. Keep execution agent-side through `dsh-bash`; only the display-specific terminal metadata is removed. A terminal card can return later if ACP standardizes agent-executed terminals or if the product decides Zed-specific display is worth the maintenance cost.
|
||||
|
||||
This proposal is narrower than [collapsing tool-owned UI presentation](2026-06-20-generic-tool-rendering.md): it keeps generic `presentCall`/`presentResult` if those survive, but removes the terminal sub-shape and `_meta` mapping.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- ACP no longer reads or stores `_meta.terminal_output` capability state.
|
||||
- `TerminalRendering`, terminal ids, terminal cwd resolution, and `_meta.terminal_*` update mapping disappear from `@deepseek-ai/dsh-acp`.
|
||||
- `ToolTerminal` disappears from `@deepseek-ai/dsh-tools`, or is unused and deleted with the presentation cleanup.
|
||||
- Bash result presentation no longer parses exit status for terminal pills.
|
||||
- The implemented [rich ACP bash rendering RFC](../implemented/2026-06-18-acp-terminal-and-tool-rendering.md) stays in `implemented/` as shipped history and is cross-linked from this proposal if superseded.
|
||||
|
||||
## What we give up
|
||||
|
||||
Zed users lose the dedicated terminal card: no cwd header, terminal display, or exit pill. They still see the command and output as plain content. That is a reasonable simplification while the ACP bridge is still unreleased and the `_meta` keys are a convention rather than a standard.
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Drop bash full-output spill files
|
||||
|
||||
Status: rejected — full-output recovery is a real bash behavior. A future artifact/blob service may generalize it, but dropping spill files before that replacement would lose useful command output.
|
||||
|
||||
## Problem
|
||||
|
||||
`dsh-bash-local` keeps bounded in-memory output and spills large stdout/stderr streams into private temp files. That requires a private directory, random owner-only file creation, close-failure handling, byte-offset incremental reads, lossy read reporting, path rendering in model-facing text, and cleanup discipline. The tool then tells the model to read a local spill path when output was truncated.
|
||||
|
||||
This solves a real problem, but in a narrow and leaky way. A spill path is a process-local filesystem artifact exposed to model output, not a durable harness artifact with scoped access, retention, or UI affordances. It also complicates background-task reads because a lossy incremental read has to point at one or two spill files.
|
||||
|
||||
## Proposal
|
||||
|
||||
Keep tail truncation, drop full-output spill files. A bash result contains the bounded tail plus a clear truncation marker; no path is emitted. If users need full-output recovery, add a generic artifact/blob service with explicit ownership, cleanup, and UI rendering, then let bash attach large outputs to that service.
|
||||
|
||||
This proposal can land independently of [a generic long-running tool runtime](../proposed/2026-06-20-generic-long-running-tool-runtime.md). If background tasks stay, `bash_output` should still report that output was dropped, but without advertising a spill path.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `CollectedOutput` no longer carries spill paths.
|
||||
- `OutputCollector` keeps bounded buffers only and deletes the temp-file machinery.
|
||||
- `renderResult()` reports truncation without a filesystem path.
|
||||
- Tests cover tail truncation and no longer assert full-output file contents.
|
||||
- Security guidance in [root AGENTS.md](../../../AGENTS.md) stops treating private spill files as a model-visible interface.
|
||||
|
||||
## What we give up
|
||||
|
||||
A model or user cannot recover the omitted prefix of a huge command output from a temp file. That is acceptable until there is a real artifact service. The current spill path is too much bespoke machinery for a feature whose lifecycle and permissions are not designed.
|
||||
@@ -0,0 +1,28 @@
|
||||
# RFC: Drop durable step boundary events
|
||||
|
||||
Status: rejected — `step/end` is the durable indication that a model step finished, and keeping the symmetric `step/start` / `step/end` pair makes crash repair, invariants, and transcript inspection clearer than inferring completion from adjacent step-scoped events.
|
||||
|
||||
## Problem
|
||||
|
||||
The session log stores `step/start` and `step/end` events even though every step-scoped event already carries `{ turn, step }`: assistant chunks, assistant messages, tool calls, tool results, usage, and errors. `deriveMessages()` ignores step boundaries, ACP ignores them for UI, and the main consumers are invariants, tests, snapshot goldens, and crash repair.
|
||||
|
||||
The rejected argument was that boundary events make the log more ceremonial than informative. In practice, `step/end` is concrete information: a reader can tell whether a model request finished, crashed, or is being repaired without deriving that state from the next event. A bare `step/start` is likewise useful for a model request that began but produced no chunks before failing.
|
||||
|
||||
## Proposal
|
||||
|
||||
Make the turn the only durable boundary. Remove `step/start` and `step/end` from `SessionEventMap`; keep the numeric `step` field on events that need grouping. The loop increments the step counter and records step-scoped events with that number, but it no longer appends open/close boundary events. Consumers infer step groups from contiguous events sharing `(turn, step)`.
|
||||
|
||||
The invariants plugin should enforce that step-scoped events have valid positive step numbers within an open turn, not that separate boundary records surround them. Crash repair should not synthesize `step/end`; if an interrupted turn is preserved, the repair path can still close the turn without inventing step boundary records.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `SessionEventMap` no longer includes `step/start` or `step/end`.
|
||||
- The loop has no `closeStep()` finalization path.
|
||||
- ACP snapshots and persistence contract fixtures stop expecting step-boundary lines.
|
||||
- `deriveMessages()` and replay derive the same message history from step-scoped events.
|
||||
- The [event taxonomy docs](../../architecture.md) describe turns as the durable boundary and steps as a field on step-scoped records.
|
||||
- The session format version and recorded fixtures are refreshed; non-current stored logs are rejected per the pre-release format policy.
|
||||
|
||||
## What we give up
|
||||
|
||||
The log no longer records "a model request started but produced no event before the process died" as a durable fact, and no longer has an explicit "this step completed" marker. That loss is not acceptable while the session log is the durable replay and audit surface.
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Drop unused session lineage metadata
|
||||
|
||||
Status: rejected — `parentSession` is part of the documented fork/sub-agent seam and is already preserved by the agent/session resume path. The field is future-facing, but it is not accidental dead state.
|
||||
|
||||
## Problem
|
||||
|
||||
`SessionHeader.parentSession` records the session a new session was forked from. It is defined in `dsh-session`, preserved by persistence backends, copied through resume, documented as lineage metadata, and covered by round-trip tests. The repo has no production fork UI or sub-agent flow that reads it. The planned sub-agent/fork seam is still a TODO, so the field is currently stored future shape.
|
||||
|
||||
The cost is small per file but broad across the format: every backend schema and metadata serializer preserves a value that no completed feature reads yet. Because the header is an on-disk contract, even a placeholder field becomes something future refactors must either maintain, migrate, or deliberately break.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove `parentSession` from `SessionHeader` until a real fork/resume feature needs lineage. Forking can still seed a new session with prior events if such an API exists, but the durable parent pointer should be introduced alongside the feature that reads it and the UX that explains it.
|
||||
|
||||
If lineage returns, decide then whether it belongs in the immutable header, a session graph index, or a first-class event. The current field should not pre-commit that design.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `SessionHeader` contains version, id, createdAt, and optional cwd only.
|
||||
- JSONL and SQLite metadata schemas stop storing parent-session ids.
|
||||
- Resume and list APIs no longer round-trip `parentSession`.
|
||||
- Docs and tests remove fork-lineage claims that are not backed by a production consumer.
|
||||
- The session format version, backend schema versions, and recorded fixtures are refreshed as needed; non-current stored data is rejected per the pre-release format policy, with no migration path.
|
||||
|
||||
## What we give up
|
||||
|
||||
The codebase loses a ready-made lineage hook for future fork/sub-agent UX. That is intentional. The field is easy to reintroduce when the feature exists, and the unreleased stance lets the format change without migrations.
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Fold the persistence interface into dsh-session
|
||||
|
||||
Status: rejected — the separate persistence interface package is the intended modular capability seam for durable backends. Folding it into `dsh-session` would reduce package count at the cost of a cleaner backend boundary.
|
||||
|
||||
## Problem
|
||||
|
||||
`dsh-session-persistence` is an interface package whose main concepts are already owned by `dsh-session`: `SessionHeader`, `SessionEvent`, `SessionId`, `session/event`, and `session/flush`. The package adds the abstract `SessionPersistence` service, the shared write coordinator, and contract helpers. Backend packages depend on it, and `agent-loop` has to optionally find a sibling service for resume.
|
||||
|
||||
The capability-seam split made sense when persistence was a new swappable backend design. After the mutable summary was removed, the interface package mostly wraps the session log's own storage concern. Keeping it separate may be more ceremony than clarity.
|
||||
|
||||
## Proposal
|
||||
|
||||
Move the abstract `SessionPersistence` service, the coordinator, and persistence contract helpers into `dsh-session`. Keep JSONL and SQLite as separate backend packages that register the session-owned service. This preserves backend swappability while deleting one support package and one cross-package seam.
|
||||
|
||||
The implementing PR should update the [capability seams](../implemented/2026-06-13-capability-seams.md) guidance with the exception: persistence is not like bash or LLM because its vocabulary and lifecycle events are already the session package's core domain.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `@deepseek-ai/dsh-session-persistence` is removed as a package.
|
||||
- `dsh-session` exports the persistence service type, coordinator, and contract helpers.
|
||||
- JSONL and SQLite backend packages depend on `dsh-session` directly.
|
||||
- `agent-loop` resume uses the session-owned service key.
|
||||
- [Session persistence](../implemented/2026-06-14-session-persistence.md), [shared persistence write coordinator](../implemented/2026-06-18-shared-persistence-write-coordinator.md), and [package docs](../../../packages/session-persistence/README.md) explain why backend implementations remain separate.
|
||||
|
||||
## What we give up
|
||||
|
||||
`dsh-session` becomes heavier: it owns both the in-memory log and the persistence interface. That is the trade. If third-party persistence backends were already a public ecosystem, the separate interface package would be a cleaner SDK boundary; pre-release, the extra package looks like abstraction before there is an external consumer.
|
||||
@@ -0,0 +1,31 @@
|
||||
# RFC: Collapse tool-owned UI presentation
|
||||
|
||||
Status: rejected — tool-owned presentation should wait for more real tools before being generalized or deleted. Bash and ACP currently need the existing richer presentation path.
|
||||
|
||||
## Problem
|
||||
|
||||
Tools can define `presentCall()` and `presentResult()` callbacks that return `ToolCallPresentation`, `ToolResultPresentation`, and optional `ToolTerminal` fields. The code itself flags the design as muddy: title, kind, raw input, content, terminal cwd, terminal output, exit code, and signal grew incrementally into a bag of optional fields. ACP then maintains pending call state to pair a result with the original args, creates replay-only presenters on `session/load`, and maps terminal subfields into Zed-specific `_meta`. `dsh-tool-bash` even parses exit status back out of rendered text because the pure replay-safe presenter no longer has the structured `BashRunResult`.
|
||||
|
||||
The real first-party use is bash presentation for ACP. That is too little evidence to freeze a cross-package UI presentation API.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove tool-owned UI presentation callbacks for now. The canonical tool events already carry the tool name, raw argument string, result content, and error state. UIs render a generic tool card from those fields. Tool-specific rich rendering can return later as a tagged render-intent union after there are at least two real tools and two real consumers to validate the vocabulary.
|
||||
|
||||
As a smaller alternative, replace the current optional-field bag with one explicit union in a single PR; but if the goal is simplification, the stronger move is to delete the callbacks and keep the generic path.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `ToolDefinition` drops `presentCall` and `presentResult`.
|
||||
- `ToolCallPresentation`, `ToolResultPresentation`, `ToolTerminal`, and `ToolCallKind` disappear unless a minimal generic UI type still needs one.
|
||||
- ACP no longer keeps presenter pending state or calls tool callbacks during live streaming/load replay.
|
||||
- `dsh-tool-bash` no longer parses rendered text to recover exit status for a UI pill.
|
||||
- Snapshot goldens show generic tool cards and text results.
|
||||
|
||||
## What we give up
|
||||
|
||||
Bash loses its custom terminal-looking card and model-written description placement. The fallback remains reasonable: the command appears as tool input, and the output appears as text. Rich rendering should be designed when the product has enough UI/tool variety to justify a stable presentation contract.
|
||||
|
||||
## Related
|
||||
|
||||
This is the broad version of [dropping ACP terminal metadata](2026-06-20-drop-acp-terminal-meta.md). If this RFC is accepted, that narrower RFC becomes unnecessary.
|
||||
@@ -0,0 +1,33 @@
|
||||
# RFC: Retire mid-turn steering
|
||||
|
||||
Status: rejected — mid-turn steering is an intentional agent capability for between-step user/plugin input and future goal/loop workflows. It is complexity with a product direction, not an accidental duplicate of `send()`.
|
||||
|
||||
## Problem
|
||||
|
||||
The agent exposes two user-message paths that look close but have different lifecycle semantics: `send()` queues a normal user turn, while `steer()` injects a message between steps of the currently running turn and falls back to `send()` when idle. That distinction leaks through the whole stack: `Agent.steer()` is public API, the session log has a durable `steering/message` event, the agent event taxonomy has `agent/steering`, the loop maintains a steering FIFO beside the queued-message FIFO, cancellation clears both queues, and `deriveMessages()` has to render steering as a tagged synthetic user message rather than a normal prompt.
|
||||
|
||||
The continuation seam amplifies the cost. `agent/turn-continuation` defaults to `hadToolCalls || steeringInjected`, so a same-turn steering message can force the loop to call the model again even if the model did not ask for tools. The comments name future `/goal`, `/loop`, and budget-guard uses, but the current repo has no production listener; only tests register the waterfall. Separately, the only production UI that calls `steer()` is the stdio demo. ACP already sends prompts through the ordinary queue while a turn is running.
|
||||
|
||||
## Proposal
|
||||
|
||||
Delete mid-turn user steering for now. `Agent.send()` becomes the single public way to submit user content; when the agent is running, the content waits for the next turn. The loop continues within a turn only for tool calls, not because a user typed while a step was running. A caller that wants to interrupt the current turn uses `cancel()` and then `send()`.
|
||||
|
||||
Remove `Agent.steer()`, the steering FIFO, `steering/message`, `agent/steering`, steering-derived continuation, and the cancellation logic that distinguishes queued messages from steering messages. Remove `agent/turn-continuation` in the same change unless the implementing PR discovers a production listener; without steering, the current repo has no concrete continuation consumer left. If a real budget or goal plugin later needs forced continuation, it should reintroduce a narrower seam with that plugin as the concrete consumer.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `Agent` exposes one user-message entry point, `send()`.
|
||||
- The durable session event vocabulary no longer contains `steering/message`.
|
||||
- `deriveMessages()` renders normal user messages and context injections, with no steering tag path.
|
||||
- The loop has one queued-message FIFO and no same-turn user-message continuation path.
|
||||
- `agent/turn-continuation` is removed or narrowed to a named production consumer.
|
||||
- The stdio UI and docs describe input while running as queued next-turn input.
|
||||
- The session format version and recorded fixtures are refreshed; non-current stored logs are rejected per the pre-release format policy.
|
||||
|
||||
## What we give up
|
||||
|
||||
A user cannot add same-turn steering content while a model is between tool steps. That behavior is useful in theory for "while you are already working, also consider X", but it is not the behavior ACP exposes today and it makes the turn boundary much harder to reason about. The simpler behavior is reasonable: user input becomes the next prompt, and cancellation remains the explicit tool for replacing in-flight work.
|
||||
|
||||
## Related
|
||||
|
||||
This pairs naturally with [dropping durable step boundaries](2026-06-20-drop-durable-step-boundaries.md), because removing same-turn steering and `agent/turn-continuation` leaves tool calls as the only reason a turn contains multiple model steps.
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Return the ACP bridge to one live session per connection
|
||||
|
||||
Status: rejected — Zed is the current target ACP client and its ACP implementation is explicitly multi-session: it stores live sessions in a `HashMap<SessionId, AcpSession>`, tracks `pending_sessions`, joins concurrent loads for the same id, and tests close-during-load behavior.
|
||||
|
||||
## Problem
|
||||
|
||||
The ACP bridge now supports multiple live sessions on one JSON-RPC connection. That capability brings multi-entry session maps, reverse session/agent lookups, per-session prompt state, loading ids, demux for every event, cross-session teardown, and isolation concerns for future permission prompts and background tasks. The older [multi-session ACP proposal](../proposed/2026-06-14-acp-multi-session.md) still tracks the unfinished permission-ownership piece; this RFC is the competing simplification path.
|
||||
|
||||
The product target has proven it needs concurrent editor conversations over one harness process: Zed's ACP connection owns multiple sessions and load states. The snapshot replay tier still avoids concurrent model streams because its replay entries are positional; that is a test-fixture limitation, not a reason to remove bridge multiplexing.
|
||||
|
||||
## Proposal
|
||||
|
||||
Scope ACP back to one live session per connection. `session/new` or `session/load` creates the only session record; a second live session request is rejected until the existing session is disposed or the connection closes. If editors need multiple chat tabs, they can launch multiple agent subprocesses until the bridge has a concrete multi-session UX and permission model.
|
||||
|
||||
Remove the multi-session maps and demux where a single `SessionRecord | undefined` is enough. The bridge can still keep the agent/session lifecycle seams that make disposal correct; the simplification is only about multiplexing more than one active session through the same transport.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- ACP has one active session record per connection.
|
||||
- `session/new` and `session/load` reject while that record exists.
|
||||
- Event handlers no longer demux across a `Map<sessionId, record>`.
|
||||
- Multi-session tests are removed or moved under the proposal that continues to defend multiplexing.
|
||||
- The existing [multi-session ACP proposal](../proposed/2026-06-14-acp-multi-session.md) is updated to link this RFC and remains the live direction.
|
||||
|
||||
## What we give up
|
||||
|
||||
An ACP client cannot host several concurrent conversations on one server process. That is a meaningful capability cut. The simpler model is still reasonable for an unreleased harness: one editor conversation maps to one agent process, and cross-session permission/background-task isolation stops being a live correctness burden.
|
||||
@@ -0,0 +1,32 @@
|
||||
# RFC: Truncate interrupted final turns on load
|
||||
|
||||
Status: rejected — a single turn can contain substantial real work, including many steps and large tool output. Preserving interrupted turns is preferable to silently dropping that tail on load.
|
||||
|
||||
## Problem
|
||||
|
||||
The current persistence contract preserves a final turn that was durably written but never closed. On load, `interruptedTurnClosers()` scans the tail, synthesizes error `tool/result` events for unanswered tool calls, appends a `step/end` when a step is open, appends `turn/end { kind: 'interrupted' }`, and asks the backend to durably commit that repair. The coordinator, JSONL backend, SQLite backend, session event vocabulary, invariants, docs, and tests all model this synthetic close path.
|
||||
|
||||
This is a lot of machinery to preserve partial work from the last crashed turn. It also invents events that never happened. A synthetic tool result is useful because it makes provider history valid, but it also means the resumed log contains model-visible text that no tool produced. The current design optimizes for maximum tail preservation before there is a released product or a real resume UX that proves partial-turn recovery matters.
|
||||
|
||||
## Proposal
|
||||
|
||||
On load, keep only the last completed turn. A backend still tolerates and truncates a torn final record, but if the parsed durable prefix ends after an open `turn/start`, the canonical repair is to drop every event after the previous `turn/end`. No synthetic `tool/result`, no synthetic `step/end`, no `turn/end { interrupted }`, and no `interrupted` turn-end reason.
|
||||
|
||||
This makes the persisted turn boundary simple: a completed `turn/end` is the checkpoint. Anything after the last checkpoint is crash tail. The next prompt resumes from the last known-valid provider transcript, not from a partially reconstructed final turn.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `TurnEndReasonMap` drops the `interrupted` variant.
|
||||
- `interruptedTurnClosers()` and its tests disappear.
|
||||
- The persistence coordinator's repair hook truncates backend-specific torn/open tail state without appending closers.
|
||||
- [Session persistence docs](../../../packages/session-persistence/README.md) say load returns the last completed turn, plus no partial final turn.
|
||||
- Snapshot and contract tests update together with the behavior they pin.
|
||||
- The session format version and recorded fixtures are refreshed; non-current stored logs are rejected per the pre-release format policy, with no migration path.
|
||||
|
||||
## What we give up
|
||||
|
||||
A crash can lose real work from the final turn: assistant text, tool calls, and tool output appended after the previous `turn/end`. That is the deliberate simplification. The product is unreleased, the final-turn recovery semantics are not user-proven, and a clean completed-turn checkpoint is much easier to explain, test, and implement. A future "recover partial crashed work" feature should be designed as an explicit user-facing recovery view, not as synthetic events silently inserted into the canonical transcript.
|
||||
|
||||
## Related
|
||||
|
||||
This is a direct simplification of [session persistence](../implemented/2026-06-14-session-persistence.md) and [turn enclosure](../implemented/2026-06-15-turn-enclosure-invariant.md). It also removes much of the motivation for durable step boundary events, making [drop durable step boundary events](2026-06-20-drop-durable-step-boundaries.md) smaller.
|
||||
@@ -1,6 +1,6 @@
|
||||
# @deepseek-ai/dsh-acp
|
||||
|
||||
The **Agent Client Protocol (ACP)** bridge: exposes the DeepSeek Harness coding agent as an ACP server over JSON-RPC stdio, so editors (Zed and other ACP clients) can drive it — streaming render, tool-call display, and resumable sessions. **N concurrent sessions per connection** (see [ACP multi-session](../../docs/rfc/proposed/2026-06-14-acp-multi-session.md)): each maps to its own `ReactLoopAgent`, and every event is demuxed strictly by session id so two sessions streaming at once never interleave.
|
||||
The **Agent Client Protocol (ACP)** bridge: exposes the DeepSeek Harness coding agent as an ACP server over JSON-RPC stdio, so editors (Zed and other ACP clients) can drive it — streaming render, tool-call display, and resumable sessions. Zed is the current target client: baseline ACP behavior should remain reasonable for other clients, but bridge capabilities and compatibility decisions are evaluated against Zed first. **N concurrent sessions per connection** (see [ACP multi-session](../../docs/rfc/proposed/2026-06-14-acp-multi-session.md)): each maps to its own `ReactLoopAgent`, and every event is demuxed strictly by session id so two sessions streaming at once never interleave.
|
||||
|
||||
It is a **client-driver / UI plugin**, the structured analogue of the readline `stdio-chat` plugin — NOT a loop change and NOT a [capability seam](../../docs/rfc/implemented/2026-06-13-capability-seams.md). It consumes the existing `agent/*` event taxonomy, the `dsh-agent` create/resume factory, and `dsh-session-persistence`.
|
||||
|
||||
|
||||
@@ -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'
|
||||
|
||||
|
||||
@@ -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'),
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -3,6 +3,7 @@ import { resolve } from 'node:path'
|
||||
|
||||
// publint every publishable package (vendor/ is private upstream code and
|
||||
// examples/ are not packages; both are out of scope).
|
||||
// TODO(package-inventory): derive this from the deliberate package hierarchy.
|
||||
const packages = [
|
||||
'packages/llm',
|
||||
'packages/session',
|
||||
|
||||
Reference in New Issue
Block a user