Merge remote-tracking branch 'origin/master' into codex/app-attribution-rfc
# Conflicts: # docs/rfc/README.md
This commit is contained in:
Tianyi Cui
@@ -1,68 +0,0 @@
|
||||
# RFC: Branded IDs everywhere they belong
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
The harness already brands three identifiers — `CallId` (`packages/llm/llm/src/brand.ts`), `SessionId` (`packages/core/session/src/types.ts`), and `AgentId` (`packages/core/agent/src/types.ts`) — using the `Branded<B> = string & { readonly [BRAND]: B }` machinery and a zero-cost cast factory per type. `brand.ts` also states the governing policy: *"Branding is for IDs that cross package boundaries and could plausibly be confused; not every string needs a brand."* That policy is right; the problem is that it is only half-applied. Two gaps let a structurally-identical-but-semantically-wrong string slip through the type checker today.
|
||||
|
||||
**Gap 1 — unbranded cross-boundary IDs in the bash seam.** The background-task id is a plain `string`: `BashTask.id: string` (`packages/bash/bash/src/types.ts`), carried as `string` through the whole executor seam (`BashExecutor.get`/`ownerOf`/`readOutput`/`kill(id: string)` in `packages/bash/bash/src/index.ts`) and validated/passed as `string` by the model-facing tools (`validateTaskId`, `assertTaskAccess`, the `task_id` schema arg in `packages/bash/tool-bash/src/index.ts`). It is generated by a per-executor counter — `` `bash-${this.nextTaskId++}` `` in `packages/bash/bash-local/src/index.ts` — which gives it **exactly the same `name-N` shape as `SessionId`'s default** (`` `session-${++counter}` `` in `packages/core/session/src/index.ts`). A bash task id and a session id are trivially swappable at a call site and the compiler says nothing. This is the headline case the user asked about, and it is a model-facing id (the model passes `task_id` back to `bash_output`/`bash_kill`), so a confusion here is reachable from untrusted input.
|
||||
|
||||
The bash **owner token** is the related sub-case: `BashExecRequest.owner?: string` and `BashExecSpec.owner: string | undefined` (`packages/bash/bash/src/types.ts`) are documented as a deliberately *opaque* isolation key, but in every live caller the value IS the owning agent's `session.header.id` (`callerToken = (exec) => exec.agent?.session.header.id` in `packages/bash/tool-bash/src/index.ts`) — i.e. a `SessionId` wearing a `string` disguise. It is compared for access control (`owner !== callerToken(exec)`), so a mismatched-but-well-typed string here is a cross-session isolation bug the type system currently cannot catch. This is the same `session.header.id`-as-owner alias that the [unify-the-agent-id-and-the-session-id](../simplification/2026-06-20-unify-agent-and-session-id.md) proposal calls the "bash owner-token alias hole".
|
||||
|
||||
**Gap 2 — brand erosion at the seams of the *already-branded* IDs.** Even `CallId`/`SessionId`/`AgentId` decay back to bare `string` at exactly the places confusion is most likely: the registry/store `Map` key types and most public method params. Representative sites: `SessionStore.store = new Map<string, Session>()` and `create`/`prepare(id?: string)`/`get(id: string)` (`packages/core/session/src/index.ts`); `AgentRegistry.store = new Map<string, Agent>()` and `register`/`get(id: string)` (`packages/core/agent/src/index.ts`); `ToolPresenter.pending = new Map<string, …>()` keyed by call id and `call(callId: string)`/`result(callId: string)` (`packages/ui/acp/src/index.ts`); the ACP session-id surface beyond the store map — `SessionRecord.sessionId: string`, `bySession = new WeakMap<Agent, string>()`, `loadingIds = new Set<string>()`, `requireSession(sessionId: string)`, and the exported `streamSessionEventUpdate(sessionId: string, …)` (`packages/ui/acp/src/index.ts`); and the persistence coordinator's `Map<string, …>` keyed by session id (`packages/session-persistence/session-persistence/src/coordinator.ts`). A brand that is dropped at the `Map` key buys nothing on lookups — the value of the existing brands is partly unrealized.
|
||||
|
||||
## Proposal
|
||||
|
||||
A type-only change. Brands are zero-cost casts; nothing about runtime behavior, serialization, comparison, or the wire format changes. The work is in three parts, all honoring the existing "not every string" policy.
|
||||
|
||||
- **Brand the bash task id.** Add `BashTaskId = Branded<'BashTaskId'>` plus its same-named factory in `packages/bash/bash/src/types.ts` (the package that *owns* the id), importing `Branded` from `@deepseek-ai/dsh-llm` exactly as `SessionId`/`AgentId` already do. Thread it through `BashTask.id`, the `BashExecutor` seam methods (`get`/`ownerOf`/`readOutput`/`kill`), the generation site in `dsh-bash-local` (brand the counter output once, at creation), and the `dsh-tool-bash` validate/access surface (`validateTaskId` returns a `BashTaskId`; `task_id` is branded at the tool boundary where the model's string arrives).
|
||||
|
||||
- **Mint a distinct `OwnerToken` brand.** Add `OwnerToken = Branded<'OwnerToken'>` in `packages/bash/bash/src/types.ts`; type `BashExecRequest.owner` / `BashExecSpec.owner` / `BashExecutor.ownerOf` as `OwnerToken | undefined`. The `dsh-tool-bash` consumer casts the agent's `session.header.id` (a `SessionId`) into an `OwnerToken` at the boundary — the one place the two vocabularies meet. The bash seam never imports `dsh-session`. (Rationale in the next section.)
|
||||
|
||||
- **Stop the brand erosion.** Propagate the existing brands to the `Map` key types and public method params listed under Gap 2 — `Map<SessionId, Session>`, `get(id: SessionId)`, `Map<AgentId, Agent>`, `Map<CallId, …>`, the ACP `SessionRecord.sessionId: SessionId` surface, the coordinator's `Map<SessionId, …>`. This is the larger mechanical share of the diff and the part that makes the *existing* brands actually load-bearing on lookups, not just on the struct fields.
|
||||
|
||||
Illustrative shape (the factory pattern is identical to the three existing brands):
|
||||
|
||||
```ts ignore-check
|
||||
import type { Branded } from '@deepseek-ai/dsh-llm'
|
||||
|
||||
/** A background bash task handle (generated `bash-N` by the local executor). */
|
||||
export type BashTaskId = Branded<'BashTaskId'>
|
||||
export function BashTaskId(id: string): BashTaskId {
|
||||
return id as BashTaskId
|
||||
}
|
||||
|
||||
/** A bash task's opaque isolation key — the consumer's owner identity, NOT the bash seam's. */
|
||||
export type OwnerToken = Branded<'OwnerToken'>
|
||||
export function OwnerToken(id: string): OwnerToken {
|
||||
return id as OwnerToken
|
||||
}
|
||||
```
|
||||
|
||||
## Why a distinct OwnerToken brand (not SessionId)
|
||||
|
||||
The obvious shortcut is to type `owner` as `SessionId` directly — it always *is* one. We reject that. The bash executor seam is a capability seam (interface `dsh-bash`, implementation `dsh-bash-local`, consumer `dsh-tool-bash`) and its owner token is *documented as deliberately opaque*: the executor "never interprets it (no access policy lives in the seam — that is the consumer's job)" (`packages/bash/bash/src/types.ts`). Typing the seam's field as `SessionId` would import `dsh-session`'s vocabulary into a package that must not know what an owner token *means* — it would couple a generic execution backend to the session model and contradict the opaque-token design. A sandboxed or remote executor that replaces `dsh-bash-local` should not inherit a session dependency. The distinct `OwnerToken` brand keeps the seam decoupled: `dsh-bash` knows only "an owner is some opaque branded token," and the `dsh-tool-bash` consumer — which already decides the access policy — is the single boundary that casts its `SessionId` into an `OwnerToken`. The brand still delivers the safety win (you cannot pass a `BashTaskId` or a raw string where an owner is expected) without the coupling.
|
||||
|
||||
## Out of scope / possible extensions
|
||||
|
||||
Kept deliberately narrow per the "not every string needs a brand" policy. Each of these is a plausible future brand, deferred with a reason, not a commitment:
|
||||
|
||||
- **`ModelId`** (`GenerateOptions.model`, the `LlmService` adapter-registry key) — a real cross-package lookup key (config → agent → llm → adapter); a reasonable next brand, left out only to keep this RFC's blast radius focused.
|
||||
- **`ToolName`** (the `ToolRegistry` key) — author-defined, human-readable, and rarely confused with another id; the weakest candidate, likely not worth a brand.
|
||||
- **`ErrorCode`** (`HarnessError.code`) — a closed vocabulary (`ABORTED`, `NO_ADAPTER`, …), not a per-instance id; better served by a string-literal union than a brand, if anything.
|
||||
- **Numeric ordinals** — turn number, step number, and the event `seq` are `number`, not `string`, so `Branded<string>` does not apply; a parallel `number & { readonly [BRAND]: B }` variant could brand them, but they are positional ordinals rarely passed across boundaries, so the payoff is low.
|
||||
- **Validated construction** — the brand factories are pure casts with no runtime check, and every boundary (ACP `sessionId`, provider-issued `call.id`, the empty-string fallback in `dsh-llm-deepseek`) trusts the raw string today. A `SessionId.parse()` / `isValid()` companion that throws on malformed input at boundaries is a genuine gap, but it is a *runtime-behavior* change with its own design (what is "malformed"? what do we do on failure?) and belongs in its own RFC, not bundled into this type-only pass.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `BashTaskId` and `OwnerToken` are defined in `dsh-bash` and threaded end-to-end: the executor seam, the `dsh-bash-local` generation site, and the `dsh-tool-bash` model-facing surface all speak the brands; `dsh-bash` gains no dependency on `dsh-session`.
|
||||
- No collection keyed by an in-scope branded id (`CallId`/`SessionId`/`AgentId`/`BashTaskId`) is keyed by bare `string` — this covers `Map`, `WeakMap` value slots, and `Set` membership (e.g. the ACP `bySession`/`loadingIds`), not just `Map<string, …>`; the corresponding public method params and exported function signatures (e.g. `streamSessionEventUpdate`) take the brand, not `string`.
|
||||
- Brands are constructed via the cast factory at each boundary where a raw string enters (provider call id, ACP session id, model-supplied `task_id`); no `as` casts scattered at call sites.
|
||||
- `pnpm run typecheck` and `pnpm run doc-sync` are green; the change is observably type-only (no snapshot, no e2e behavioral diff).
|
||||
|
||||
## Risks / what we give up
|
||||
|
||||
- **Mechanical churn across two surfaces.** Propagating brands touches the bash seam (interface + impl + consumer) and the ACP session-id surface plus the persistence coordinator. The risk is broad but low-severity: a missed site is a compile error, not a silent bug. It ships as its own PR, converged with Codex, and stacks naturally near the [unify-the-agent-id-and-the-session-id](../simplification/2026-06-20-unify-agent-and-session-id.md) work (both touch the session-id / owner-token boundary; if that proposal lands first, `OwnerToken` still stays distinct from the unified id for the decoupling reason above).
|
||||
- **Brands do not validate.** A brand is a confusability guard, not a correctness proof: a *wrong* session id that is still a well-formed string passes the type checker exactly as before. This RFC does not close that gap (see Out of scope) — it only stops the *category* error of passing the wrong *kind* of id.
|
||||
- **The "where to stop" line stays a judgment call.** Branding `BashTaskId` but not `ToolName`, `OwnerToken` but not `ModelId`, is a taste call about which strings "could plausibly be confused." Reasonable reviewers may want more or fewer; the policy in `brand.ts` is the tie-breaker, and this RFC errs toward the ids that are model-facing or used for access control.
|
||||
@@ -1,44 +0,0 @@
|
||||
# RFC: Extract example apps into packages
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
An example folder is supposed to be *thin* — the variable wiring of a demo, not the demo's machinery. Today it is thick. Each example carries a hand-rolled `start.ts` boot bootstrap, an infra preamble (`timer`, and — for the stdio demos — `logger` + `hmr`), nested includes of three shared YAML fragments, and per-example `agent-loop`/persistence/system-prompt config. The actual app — the spine of services every agent needs — is spread across the leaf and the [base.yml](../../../../examples/base.yml) / [base-core.yml](../../../../examples/base-core.yml) / [acp-tail.yml](../../../../examples/acp-agent/acp-tail.yml) includes.
|
||||
|
||||
The deeper problem is a **coupled front-door cluster** that lives at the leaf with nothing enforcing it. Choosing the ACP bridge over `ui-stdio` is not one swappable line: an ACP server must **drop the stdout console logger** (stdout is the JSON-RPC channel — a stray log corrupts the frames), omit `hmr` (the editor owns the subprocess), and pre-create **no** agents (ACP `session/new` creates them on demand), whereas the stdio app needs a console logger, `hmr`, and a pre-created `main`. (`timer` is the one infra plugin common to both — it writes nothing to stdout — so it belongs in the shared spine, not the cluster.) Today that coupling is enforced only by prose warnings in [acp-agent/cordis.yml](../../../../examples/acp-agent/cordis.yml) and [base-core.yml](../../../../examples/base-core.yml). A leaf that wires a console logger into the ACP config is a one-line, comment-only mistake away — exactly the [stdout-purity footgun](../../implemented/feature/2026-06-18-acp-terminal-and-tool-rendering.md) the examples guard by hand. The three `start.ts` files also duplicate the Loader-boot tail, the `.env` loader, and (for ACP) snapshot-mode branching and the stdin-dispose lifecycle.
|
||||
|
||||
## Proposal
|
||||
|
||||
Make each example **mostly an invocation of an app package**, splitting the wiring along the existing [interface / implementation / consumer seam](../../implemented/architecture/2026-06-13-capability-seams.md): the **app package owns the composition**, the leaf `cordis.yml` owns only the **swappable choices** (which LLM adapter, which bash executor, model, prompt, persistence root).
|
||||
|
||||
- **`@deepseek-ai/dsh-agent-core`** — a Cordis bundle plugin for the providerless, executor-less, UI-less spine: `timer` + `llm` + sessions + system-prompt + tools + agents + invariants + `tool-bash` + `agent-loop`. This is today's [base-core.yml](../../../../examples/base-core.yml) **minus** `bash-local`, **plus** `timer` and the loop, as code instead of a YAML include. The bundle **forwards** `agent-loop`'s `agents` list as its own config (default `[]`, exactly the existing `AgentLoop.Config` shape in [packages/core/agent-loop/src/index.ts](../../../../packages/core/agent-loop/src/index.ts)) — so each app supplies its own pre-created agents. This is precisely the reason [base-core.yml](../../../../examples/base-core.yml) gives today for keeping `agent-loop` *out* of the shared core ("the examples disagree — stdio needs a pre-created `main`, acp needs none"); forwarding the config dissolves that objection — the loop is shared, the agents list is per-app.
|
||||
- **`@deepseek-ai/dsh-stdio-agent`** and **`@deepseek-ai/dsh-acp-agent`** — app packages, each consuming `dsh-agent-core` and **baking in its coupled front-door cluster**: stdio = `ui-stdio` + console logger + `hmr` + a pre-created `main`; acp = the `acp` bridge + **no stdout logger** + no `hmr` + no pre-created agents. The coupling becomes structurally unreachable from the leaf.
|
||||
- **Drop `start.ts`.** Each app package exposes a `bin`; the `demo:*` scripts invoke it (e.g. `dsh-stdio-agent ./cordis.yml`). The Loader-boot tail, `.env` loading, snapshot-mode selection, and stdin-dispose lifecycle move into that bin, owned by the app.
|
||||
- **Collapse each leaf `cordis.yml`** to backends + config: the LLM adapter (`llm-deepseek` with apiKey/models, or `llm-replay`), the bash executor (`bash-local`), and one app-bundle entry carrying the app's config (model, system prompt, persistence root — surfaced as the app package's own `Config`, which routes each value to wherever the app wires it: stdio onto its pre-created agent, acp onto the bridge plugin). A handful of entries, no infra preamble.
|
||||
- **Fold echo-agent onto `dsh-stdio-agent`**, swapping the LLM backend to the local `mock-llm` and adding the local `echo-tool` at the leaf — the clean demonstration of "swap the backend, keep the app". `mock-llm.ts` / `echo-tool.ts` stay as example-local teaching plugins.
|
||||
- **Retire** [base.yml](../../../../examples/base.yml), [base-core.yml](../../../../examples/base-core.yml), and [acp-tail.yml](../../../../examples/acp-agent/acp-tail.yml) — the spine they shared now lives in `dsh-agent-core`.
|
||||
|
||||
`bash-local` and the LLM adapter stay **leaf choices**: the bundle ships `tool-bash` (the consumer schema), the leaf picks the executor implementation, so a sandboxed executor or replay adapter swaps in without touching the app.
|
||||
|
||||
## Why not keep the wiring in shared YAML includes?
|
||||
|
||||
The `base*.yml`/`acp-tail.yml` includes already dedupe the *config*, but a YAML include cannot **encapsulate** the front-door coupling — it can only describe it in a comment and trust every leaf to obey. It also cannot own a `bin`, so the boot glue stays copied across three `start.ts` files. A package turns "the ACP app never logs to stdout" from a prose warning into a property of the artifact: there is no logger entry in the leaf to get wrong.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- Each example directory is `cordis.yml` + `README.md` + tests only — no `start.ts`, no infra preamble; `base.yml`/`base-core.yml`/`acp-tail.yml` are gone.
|
||||
- `demo:echo` / `demo:coding` / `demo:acp` run via the app-package `bin`s.
|
||||
- `pnpm run test`, `pnpm run test:snapshot` (re-recorded), `pnpm run typecheck`, `pnpm run knip`, `pnpm run publint`, and `pnpm run doc-sync` are green; the new packages carry the per-file 100% coverage gate and a README like every `@deepseek-ai/dsh-*`.
|
||||
|
||||
## What we give up
|
||||
|
||||
- **The bare-plugin-tree pedagogy.** echo-agent's inlined `cordis.yml` showed every plugin at once; the spine now lives behind a bundle, so seeing the whole tree means opening `dsh-agent-core`. The app package's README must carry that teaching weight.
|
||||
- **A layer of indirection.** "What does this demo load?" becomes a package read, not a single YAML scan.
|
||||
- **Migration cost** (the implementing PR, not this one): three new packages, three leaf rewrites, the boot glue moved into bins, re-recorded ACP snapshots, and rewritten example READMEs + [examples/AGENTS.md](../../../../examples/AGENTS.md).
|
||||
|
||||
## Related
|
||||
|
||||
- Supersedes [Make the shared example base providerless](../../rejected/architecture/2026-06-20-providerless-example-base.md): renaming `base.yml` to the providerless core is moot once the spine moves into `dsh-agent-core` and the `base*.yml` files are deleted.
|
||||
- Builds on the [capability-seams](../../implemented/architecture/2026-06-13-capability-seams.md) interface/implementation/consumer split — backends and presentation stay leaf choices; the spine is the shared bundle.
|
||||
- Complements [Reorganize packages into a modular hierarchy](../../implemented/architecture/2026-06-20-package-hierarchy.md): the new app/core packages slot into a group under that hierarchy (a product group for the reusable core bundle, or alongside the examples for app-specific wiring).
|
||||
@@ -22,6 +22,10 @@ The runtime should own:
|
||||
|
||||
`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.
|
||||
|
||||
## Current seam consumption
|
||||
|
||||
A consumer census of the surface the runtime would carve up. Production has two seam consumers: `packages/bash/tool-bash/src/index.ts` consumes `resolve`, `run`, `start`, `ownerOf`, `readOutput`, `kill`, and `onTaskDone`; and the hook bridges — via `dsh-hook-protocol`'s `runHook` (`packages/hooks/hook-protocol/src/runner.ts`) — consume `resolve` + `run` only, a foreground-only trusted-plugin caller that sets the seam's `stdin`/`env` fields, so the background machinery stays single-consumer (which sharpens the extraction premise). `get()`/`list()` have test-harness consumers only — they were removed once and reverted on the merits (the implementation note in [prune dead methods from the persistence seam](../../implemented/simplification/2026-06-20-prune-dead-seam-methods.md) records the test-migration cost dwarfing the surface removed). The per-task `BashTask.done` promise has no consumer through the public seam either (`dsh-tool-bash` completes via `onTaskDone`), but it is production-load-bearing INSIDE the implementation: `dsh-bash-local`'s disposal awaits it to reach quiescence. The seam therefore exposes two public completion representations — the per-task promise and the global `onTaskDone` listener registry — and the shipped consumers use only the latter: the runtime should pick exactly one public completion surface and record which. Two shape facts for the split to dissolve or preserve deliberately: `BashExecSpec.timeoutMs` is required but ignored by `start()` (documented in the seam JSDoc itself), and `stdin`/`env` ride the shared spec for the foreground trusted-plugin path — the carve-up must keep a plain in-process foreground `resolve`+`run` path carrying them, so hook execution is never forced through the long-running runtime. Adjacent blast radius: the credential scrub is duplicated between the two production spawn sites (`packages/bash/bash-local/src/run.ts` and `packages/subagent/subagent-acp/src/run.ts`); if the runtime absorbs spawn-env policy, collapsing that duplication is its work too.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- The bash-specific packages no longer define the generic task registry, owner-token authorization, polling, cancellation, or completion-notification machinery.
|
||||
|
||||
@@ -7,7 +7,7 @@ Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
The coding agent is reachable only through the readline `stdio-chat` plugin: it reads lines from stdin, calls `agent.send()`, and prints `agent/stream-chunk` to stdout. There is no structured protocol, so the agent cannot be embedded in an editor — no streaming render, no tool-call display, no permission UI, no resumable sessions.
|
||||
The coding agent is reachable only through the readline `stdio-chat` plugin: it reads lines from stdin, calls `agent.send()`, and prints the assistant token stream (`session/event` `assistant/chunk`) to stdout. There is no structured protocol, so the agent cannot be embedded in an editor — no streaming render, no tool-call display, no permission UI, no resumable sessions.
|
||||
|
||||
Editors are converging on the Agent Client Protocol (ACP), which Zed and others speak: JSON-RPC 2.0 over newline-delimited stdio, modeled on the Language Server Protocol. An editor boots the agent as a subprocess and exchanges `initialize` / `session/new` / `session/prompt`, rendering streamed `session/update` notifications and `session/request_permission` prompts. The goal is for the agent to be a drop-in ACP server — implement the protocol once and run in any ACP client, with no per-editor glue.
|
||||
|
||||
@@ -15,7 +15,7 @@ This RFC has a hard prerequisite on [session persistence](../../implemented/arch
|
||||
|
||||
## Proposal
|
||||
|
||||
A new plugin package `@deepseek-ai/dsh-acp` — a client-driver / UI plugin, the structured analogue of `stdio-chat`. It is NOT a change to the loop and NOT an [capability seams](../../implemented/architecture/2026-06-13-capability-seams.md) interface/implementation/consumer capability split; it consumes the existing `agent/*` event taxonomy and the `tools/execute` waterfall.
|
||||
A new plugin package `@deepseek-ai/dsh-acp` — a client-driver / UI plugin, the structured analogue of `stdio-chat`. It is NOT a change to the loop and NOT an [capability seams](../../implemented/architecture/2026-06-13-capability-seams.md) interface/implementation/consumer capability split; it consumes the existing `agent/*` event taxonomy and the `tools/pre-execute`/`tools/post-execute` waterfalls.
|
||||
|
||||
It depends on the official `@agentclientprotocol/sdk` (the `AgentSideConnection` class) — Apache-2.0, actively versioned. The SDK declares a `zod` peer dependency and imports `zod/v4` at runtime, so `packages/ui/acp` must declare `zod` itself (per the workspace dependency constraints). This is the renamed successor to `@zed-industries/agent-client-protocol`, which is now deprecated on npm.
|
||||
|
||||
@@ -27,9 +27,9 @@ The mapping between ACP and existing harness seams — each row names the seam a
|
||||
| `session/new {cwd, mcpServers, additionalDirectories}` → `{sessionId}` | the `dsh-agent` create factory (see Dependency note + Plan) | the seam must accept `{ sessionId, meta }` so the ACP-generated `sessionId` becomes the live/persisted session id and the validated `cwd` is attached as the `SessionHeader` (today `AgentLoop.create(id)` hardcodes `${id}-session` and takes no metadata); reject a 2nd session (single-session MVP, see [ACP multi-session](2026-06-14-acp-multi-session.md)); `cwd` validated (require absolute) — any absolute cwd is honored: it becomes the session's `SessionHeader.cwd` and the default bash workdir (per-session cwd, see § Deferred → RESOLVED), so the server need not launch in the workspace; non-empty `mcpServers` and `additionalDirectories` are rejected for the MVP because silently ignoring requested servers/roots would desync the client's tool and filesystem-scope UI |
|
||||
| `session/load {sessionId, cwd, mcpServers, additionalDirectories}` | the `dsh-agent` resume factory ([session persistence](../../implemented/architecture/2026-06-14-session-persistence.md) + Dependency note) | load `{ meta, events }`, seed the session, re-derive history via `deriveMessages()`, replay prior turns to the client as `session/update` per the ACP load contract; `mcpServers` and `additionalDirectories` rejected as in `session/new` |
|
||||
| `session/prompt {prompt}` | `agent.send()` (idle) | text blocks → `TextBlock`; reject image/audio per advertised capabilities; one in-flight prompt per session |
|
||||
| resolve `session/prompt` → `{stopReason}` | `agent/turn-end` (extended, see Plan) | map the harness kebab `TurnEndReason` to the ACP snake_case `StopReason` wire enum: `completed`→`end_turn`, `max-tokens`→`max_tokens`, `aborted`(cancel)→`cancelled`, plus `refusal`/`max_turn_requests` when applicable; honor the batch-into-one-turn and send-not-synchronously-running settle semantics |
|
||||
| `session/update: agent_message_chunk` | `agent/stream-chunk` `text-delta` only | do NOT also emit on `block-end(TextBlock)` — it carries the fully-assembled block and would duplicate the streamed text |
|
||||
| `session/update: agent_thought_chunk` | `agent/stream-chunk` `reasoning-delta` | |
|
||||
| resolve `session/prompt` → `{stopReason}` | the `turn/end` `session/event` (its `reason`) | map the harness kebab `TurnEndReason` to the ACP snake_case `StopReason` wire enum: `completed`→`end_turn`, `max-tokens`→`max_tokens`, `aborted`(cancel)→`cancelled`, plus `refusal`/`max_turn_requests` when applicable; honor the batch-into-one-turn and send-not-synchronously-running settle semantics |
|
||||
| `session/update: agent_message_chunk` | `session/event` `assistant/chunk` `text-delta` only | do NOT also emit on `block-end(TextBlock)` — it carries the fully-assembled block and would duplicate the streamed text |
|
||||
| `session/update: agent_thought_chunk` | `session/event` `assistant/chunk` `reasoning-delta` | |
|
||||
| `session/update: tool_call` (pending→in_progress) | `session/event` `tool/call` | demux via a Session→sessionId map; `kind` inferred from the tool name |
|
||||
| `session/update: tool_call_update` (completed/failed) | `session/event` `tool/result` | a throwing `tools/execute` yields NO `tool/result` → fail the pending tool UI from `agent/error`/turn-end |
|
||||
| `session/request_permission {sessionId, toolCall, options}` | prepended `tools/execute` listener | no-op unless `exec.agent` is ACP-owned; await the outcome; `selected/allow_*` → `next()`; `reject_*`/`cancelled` → veto `ToolExecutionResult{isError}` |
|
||||
@@ -37,7 +37,7 @@ The mapping between ACP and existing harness seams — each row names the seam a
|
||||
|
||||
The permission gate is the first real consumer of the `tools/execute` veto seam (the documented "single veto/sandbox/permission seam" plus the deferred "Permission system" TODO in [docs/architecture.md](../../../architecture.md)). It is a single global listener registered with `prepend: true` so it runs before any other tool wrapper. `ToolExecution.agent` is optional and the `Agent` interface carries no origin marker, so the bridge tracks ownership itself: it records each agent it creates in a `WeakMap<Agent, sessionId>` and the gate no-ops (calls `next()` immediately) for any `exec.agent` it does not own — non-ACP agents and the no-agent case pass straight through. For an owned agent it resolves the session, issues `session/request_permission`, and stores the pending resolver on that session's record so the outcome — or a `session/cancel`/connection-close — settles it exactly once.
|
||||
|
||||
Lifecycle and disposal: the connection, listeners, and in-flight permission promises register via `ctx.effect`/`ctx.on`; teardown is async and awaits quiescence — close the connection, settle/reject pending permissions, `agent.abort()`, and wait for the agent to settle. The disposal-settle signal must come from the `dsh-agent` interface, not the loop: `agent.done` exists only on the concrete `ReactLoopAgent`, so the bridge instead observes `agent/status` reaching `idle`/`disposed` (or the RFC lifts a quiescence promise onto the `Agent` interface). Every listener contains its `send()` exceptions (log, never reject the turn) because stream chunks are emitted inside the model step, so a throwing listener would corrupt the turn.
|
||||
Lifecycle and disposal: the connection, listeners, and in-flight permission promises register via `ctx.effect`/`ctx.on`; teardown is async and must *reach* quiescence, not just request it — close the connection, settle/reject pending permissions, and dispose each owned agent through its `AgentHandle.dispose()` (which stops the loop, `await`s its exit, and unregisters). Owner teardown goes through that handle seam, not the loop's concrete `agent.done` (which exists only on `ReactLoopAgent`); a non-owner that merely wants to *observe* the current work settling without tearing the agent down awaits the interface-level `agent.whenIdle()`. Every listener contains its `send()` exceptions (log, never reject the turn) because stream chunks are emitted inside the model step, so a throwing listener would corrupt the turn.
|
||||
|
||||
**Dependency note (architecture rule).** [docs/architecture.md](../../../architecture.md) states "plugins depend on interface packages, never on `dsh-agent-loop`." Creating and resuming agents is currently only on the concrete `AgentLoop` (`ctx.agentLoop`), so this RFC proposes adding an **abstract create/resume factory** to the `dsh-agent` interface (registry-level `create({ sessionId, meta })` / `resume(...)`), implemented by the loop, so `dsh-acp` injects only `agents` (the interface) and the dependency rule holds. The alternative — injecting the concrete `agentLoop` and recording a documented exception in the architecture doc — is explicitly the non-preferred fallback.
|
||||
|
||||
@@ -46,7 +46,7 @@ Lifecycle and disposal: the connection, listeners, and in-flight permission prom
|
||||
1. Package scaffold `packages/ui/acp/` per [the cookbook](../../../cookbook/adding-a-package.md); add `@agentclientprotocol/sdk` and `zod`. Add the abstract create/resume factory to `dsh-agent` (the interface) so the bridge can `inject: ['agents', 'sessions', 'tools', 'sessionPersistence']` without depending on the concrete loop; `sessionPersistence` is required because `session/load` advertises `loadSession: true`. (Fallback only if the factory is judged not worth it: inject `agentLoop` directly and record the architecture-rule exception in `docs/architecture.md`.)
|
||||
2. Connection plus `initialize`/`session/new`: wire `AgentSideConnection` to stdin/stdout; protocolVersion negotiation; the single-session guard; create the live session through the new `{ sessionId, meta }` factory seam (so the ACP `sessionId` and validated `cwd` become the session's id and header); the `sessionId↔agent` and `Session↔sessionId` maps.
|
||||
3. Internal edit — turn-end reason fidelity (sanctioned: edit internals to fit ACP). Extend `TurnEndReasonMap` in the proper places: (a) declaration-merge a `max-tokens` variant in the owning package (`packages/core/session/src/types.ts`, alongside `completed|aborted|error|disposed`) — add `max-tokens` because `FinishReasonMap` produces it (DeepSeek maps `length` → `max-tokens`); do not add `refusal`, since no current adapter produces it (unknown DeepSeek finish reasons collapse to `error`), but leave a comment in `TurnEndReasonMap` noting `refusal` should be added when an adapter first emits it (`FinishReasonMap` is merge-extensible); (b) make `agent-loop`'s `loop.ts` populate the reason from the model `finish` chunk — `assembler.finish` lives inside `runStep`, so `runStep` must return it up to `runTurn`, and the rule is "the last step's finish reason wins, but any `max-tokens` in the turn surfaces as `max-tokens`"; (c) no consumer exhaustively switches over `TurnEndReason` today (the invariants plugin switches on `SessionEventType`, and `deriveMessages` ignores `turn/end`), so adding `max-tokens` is a non-breaking extension — but recheck before landing; (d) update [docs/architecture.md](../../../architecture.md) (the CI-verified loop-lifecycle/event-taxonomy doc) and the affected package READMEs/JSDoc (`dsh-session`, `dsh-agent`, `dsh-agent-loop`) per the repo doc-sync policy. This replaces a fragile "observe the finish chunk in the bridge" hack with a real, documented contract.
|
||||
4. Prompt-turn streaming plus load: translate `agent/stream-chunk` and `session/event` into `session/update`; resolve `session/prompt` on settle, mapping the harness `TurnEndReason` to the ACP `StopReason` wire enum (`completed`→`end_turn`, `max-tokens`→`max_tokens`, `aborted`→`cancelled`) — a small total function with a test asserting the exact wire strings, since the SDK rejects an unknown `stopReason`. Concrete correlation, since the loop batches queued messages into one turn and `send()` does not synchronously flip to running: install listeners before `send()`; gate on an observed `agent/turn-start` (confirms work was accepted) then resolve on the next `agent/turn-end`; reject an empty/whitespace prompt up front rather than calling `send()` (no turn would ever start, so the RPC would hang). Implement `session/load` on the session-persistence resume seam.
|
||||
4. Prompt-turn streaming plus load: translate `session/event` (the `assistant/chunk` token stream plus boundaries and tool activity) into `session/update`; resolve `session/prompt` on settle, mapping the harness `TurnEndReason` to the ACP `StopReason` wire enum (`completed`→`end_turn`, `max-tokens`→`max_tokens`, `aborted`→`cancelled`) — a small total function with a test asserting the exact wire strings, since the SDK rejects an unknown `stopReason`. Concrete correlation, since the loop batches queued messages into one turn and `send()` does not synchronously flip to running: install the `session/event` listener before `send()`; capture the prompt's owning turn from its `turn/start` record, then resolve on that turn's `turn/end` (with `agent/status` idle/disposed as a fallback); reject an empty/whitespace prompt up front rather than calling `send()` (no turn would ever start, so the RPC would hang). Implement `session/load` on the session-persistence resume seam.
|
||||
5. Permission gate: a single `tools/execute` listener registered with `prepend: true`, owning a `WeakMap<Agent, sessionId>` of bridge-created agents; no-op (`next()`) for unowned/no-agent calls; for owned calls → `session/request_permission` → allow (`next()`) / veto; settle the stored resolver exactly once on outcome, cancel, or connection close.
|
||||
6. Example wiring (extract a shared base). `@cordisjs/plugin-include` is itself a plugin entry that resets `ctx.baseUrl` and loads a path, so a child `cordis.yml` can nest-include a shared base; the extraction is safe because every dependent plugin declares `inject` (loader groups initialize via `Promise.all`, so YAML order is NOT the dependency mechanism — never rely on it). Extract the provider/tool core (`llm, sessions, system-prompt, tools, agents, invariants, llm-deepseek, bash-local, tool-bash`) into `examples/base.yml`; have both `coding-agent` and a new `examples/acp-agent/` include it and add their own UI plugin plus logger. Keep `agent-loop` per-example (NOT in the base): `AgentLoop` creates its configured agents in its constructor, and the two examples disagree — `coding-agent` needs a pre-created `main` (its `stdio-chat` calls `ctx.agents.get('main')`), while `acp-agent` must pre-create none (ACP `session/new` creates agents). So `coding-agent` declares `agent-loop` with `agents: [{ id: main, … }]` and `acp-agent` with `agents: []`. `acp-agent` loads `dsh-session-persistence-jsonl` (from [session persistence](../../implemented/architecture/2026-06-14-session-persistence.md) — required for `session/load`), omits the stdout logger (see Risks), and adds `pnpm run demo:acp` plus the Zed `agent_servers` snippet.
|
||||
7. Tests (the repo cares a lot here): a property-based test for the protocol shape (precedent: [property-based testing](../../implemented/testing/2026-06-11-property-based-testing.md)) — fuzz arbitrary harness event sequences and assert ACP-stream invariants (never a `tool_call_update` before its `tool_call`; exactly one `session/prompt` resolution per prompt; monotonic, well-formed ordering; `stopReason` in the legal set); codec unit tests over an in-memory `Duplex` pair (drive `AgentSideConnection` without a subprocess; assert exact frames for `initialize`, `session/new`, a full prompt turn); the mandatory HMR-safety test (dispose the fiber; assert the connection closed, all `ctx.on` listeners gone, any in-flight `request_permission` settled); failure-path tests (connection closes mid-stream; closes with a permission pending; a notification `send()` rejects but the turn survives; `finish{kind:'error'|'aborted'}`; a `tools/execute` throw with no `tool/result`; a second `session/new` rejected; a `session/prompt` while one is in flight; an empty prompt rejected without hanging; a `session/load` re-derives identical history and replays it); and an e2e (`*.e2e.ts`, self-skips without `DEEPSEEK_API_KEY`) that boots `examples/acp-agent`, connects a `ClientSideConnection`, sends a real prompt, owns and disposes the harness in `afterEach`, and verifies the world (files on disk), not the agent's self-report.
|
||||
@@ -67,7 +67,7 @@ New third-party runtime dependency plus protocol drift: `@agentclientprotocol/sd
|
||||
|
||||
Turn-settle and prompt-correlation hazards: honor "queued messages batch into one turn" and "`send()` does not synchronously flip to running" (see `stdio-chat.ts` and the defensive-patterns section of [docs/architecture.md](../../../architecture.md)); gate resolution on an observed running→idle transition and handle the empty-prompt / no-work branch so an RPC can't hang.
|
||||
|
||||
Permission-await and disposal hangs: a pending `request_permission` whose connection closes or whose turn aborts must settle exactly once; disposal must reach quiescence (observe the interface-level settle signal — `agent/status` reaching `idle`/`disposed`, since `agent.done` is `ReactLoopAgent`-only), not orphan awaits on a closed pipe.
|
||||
Permission-await and disposal hangs: a pending `request_permission` whose connection closes or whose turn aborts must settle exactly once; disposal must reach quiescence — tear each owned agent down through `AgentHandle.dispose()` (which stops the loop and awaits its exit), rather than orphaning awaits on a closed pipe.
|
||||
|
||||
The 100% per-file coverage gate (repo policy) makes a branch-heavy protocol bridge real work. Accepted deliberately, surfaced so it isn't a surprise at PR time.
|
||||
|
||||
|
||||
@@ -55,7 +55,7 @@ These are illustrations of the seam's reach, **not commitments** — the MVP shi
|
||||
|
||||
**3c. The single tool — `run_code`.** Registered normally in `ctx.tools` with one parameter `{ code: string (required) }`. Because it is an ordinary tool, the unchanged loop dispatches it through the normal path — this is the crux of "zero loop changes." Its `execute(args, exec)`:
|
||||
|
||||
1. Builds the SDK bindings. For each real tool, an async `invoke(callArgs)` that **checks `exec.signal?.aborted` (throwing if set) before and after** calling `ctx.tools.execute({ callId: <deterministic sub-id>, name, arguments: callArgs, agent: exec.agent, signal: exec.signal })`, then maps the resulting `ContentBlock[]` to a simplified `{ output, isError }` (text blocks for the MVP), and emits an observability event. The explicit abort check matters because `ctx.tools.execute()` *catches* thrown tool errors and converts them to `isError` results — without the check, an aborted sub-call would look like ordinary error data and the program would keep running instead of stopping. Sub-dispatch still flows through the `tools/execute` waterfall, so permission/sandbox/hook plugins apply to code-mode calls exactly as to native ones.
|
||||
1. Builds the SDK bindings. For each real tool, an async `invoke(callArgs)` that **checks `exec.signal?.aborted` (throwing if set) before and after** calling `ctx.tools.execute({ callId: <deterministic sub-id>, name, arguments: callArgs, agent: exec.agent, signal: exec.signal })`, then maps the resulting `ContentBlock[]` to a simplified `{ output, isError }` (text blocks for the MVP), and emits an observability event. The explicit abort check matters because `ctx.tools.execute()` *catches* thrown tool errors and converts them to `isError` results — without the check, an aborted sub-call would look like ordinary error data and the program would keep running instead of stopping. Sub-dispatch still flows through the `tools/pre-execute`/`tools/post-execute` waterfalls, so permission/sandbox/hook plugins apply to code-mode calls exactly as to native ones.
|
||||
2. Calls `ctx.codeRuntime.run({ code: args.code, sdk: bindings, signal: exec.signal })`.
|
||||
3. Surfaces the outcome. A *successful* run returns `[{ type: 'text', text: <console logs + return value> }]`. A *runtime-error* result cannot be reported by returning content, because a normal `ToolDefinition.execute()` returns only `Promise<ContentBlock[]>` and `ToolRegistry.execute()` hardcodes `isError: false` on any successful return — `isError: true` arises only from the registry's catch path. So on an error result the tool **throws a `CodeRunError extends HarnessError`** (`HarnessError` is exported from `dsh-llm`; the registry catch turns any throw into `isError: true` with the message as text, and a `HarnessError` additionally carries structured `{ name, code }`). An alternative — registering `run_code` handling as a `tools/execute` listener that returns a full `ToolExecutionResult` and can set `isError` directly — is noted; the throw is simpler and preferred.
|
||||
|
||||
|
||||
@@ -0,0 +1,39 @@
|
||||
# RFC: Pre-tool input rewrite — a consistent design (proposed)
|
||||
|
||||
Status: proposed (2026-06-30)
|
||||
|
||||
<!-- XXX: legacy ADR/RFC body format, not yet normalized to a unified RFC template. -->
|
||||
|
||||
## Context
|
||||
|
||||
The [interception-seams RFC](../../implemented/feature/2026-06-30-interception-seams.md) added `tools/pre-execute` returning a `PreToolDecision` (allow/deny/ask) — but deliberately NOT input rewrite (a hook changing a tool call's `arguments` before it runs). Claude Code's `PreToolUse` hook offers an `updatedInput`, so a faithful CC bridge wants the same. This RFC designs that, separately, because doing it consistently is a real problem — not a field to bolt onto the allow decision.
|
||||
|
||||
## The problem: three readers of pre-execution arguments
|
||||
|
||||
In the loop, a tool call's arguments are committed to the log and read by live consumers BEFORE the tool executes:
|
||||
|
||||
1. **`assistant/message`** is appended before tool dispatch — it is the model-history source `deriveMessages()` replays, so it carries the tool-call arguments the model itself emitted.
|
||||
2. **`tool/call`** is the durable AUDIT record, appended before `ctx.tools.execute()`.
|
||||
3. **Live presentation reads `tool/call.arguments`**: the ACP bridge remembers them and passes them to `presentResult`; `dsh-tool-bash` derives the card title, the rawInput, the cwd, and the terminal-vs-background treatment from them.
|
||||
|
||||
So an "input rewrite" that changes ONLY what executes would make the UI show one command while another RAN, and render result state against the wrong arguments — a real inconsistency, not a documentable gap. (The existing low-level capability to mutate `exec.arguments` in a listener has exactly this latent inconsistency; it is unadvertised precisely because of this — yet not unused: a tool-bash integration test rewrites a scripted call's arguments through it (`packages/bash/tool-bash/tests/integration.spec.ts`), so this design must either sanction that path with the consistency unit below or seal it — `readonly` arguments at the seam, with the test shim moved onto a behavior-level helper.)
|
||||
|
||||
## Proposed design (sketch — to validate against the code when built)
|
||||
|
||||
Treat input rewrite as a consistency unit: when a `pre-execute` hook supplies `updatedInput`, the rewrite must be reflected in ALL three readers, atomically, before execution:
|
||||
|
||||
- The `tool/call` audit event records the REWRITTEN arguments (with the original retained in a sidecar field for the audit trail — a hook changed the call, and both the original and the effective arguments are facts worth keeping).
|
||||
- The `assistant/message` in derived history must agree with what executed — options to evaluate: rewrite the assistant message's tool-call block in place (changes what the model "sees it said"), or record a separate correction the next request carries. The CC model is that the model sees the rewrite took effect.
|
||||
- Presentation (`presentCall`/`presentResult`) reads the rewritten arguments, so the UI shows what actually ran.
|
||||
|
||||
The shape would extend `PreToolDecision` with an allow-variant `arguments` (or a dedicated `{kind:'rewrite', arguments}`), and the loop would thread the rewrite through the three readers above rather than only into `ctx.tools.execute()`.
|
||||
|
||||
## Why not now
|
||||
|
||||
The interception-seams RFC notes input rewrite "fought the code across two review rounds" — the signal AGENTS.md names for an over-reaching change. Shipping allow/deny/ask first keeps the seam honest (no advertised contract that silently desyncs the UI), and a CC/Codex bridge that receives an `updatedInput` logs it and surfaces a faithful-but-degraded warning (like `ask`→deny) until this lands. This RFC is the home for the consistent design; `TODO(pre-tool-input-rewrite)` in the loop's pre-execute call site anchors it.
|
||||
|
||||
## Open questions
|
||||
|
||||
- Does rewriting the `assistant/message` tool-call block corrupt any provider's expectation on replay, or is a separate correction safer?
|
||||
- Should the original arguments be preserved on the `tool/call` event (audit) and, if so, under what field?
|
||||
- How does this interact with a future permission `ask` flow (a user approving a rewritten call)?
|
||||
@@ -4,7 +4,7 @@ Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
Package and gate inventories are repeated by hand. 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. `tsconfig.build.json` lists all 18 packages as explicit project `references`. These lists are small today, but every new package or gate creates another manual synchronization point.
|
||||
Package and gate inventories are repeated by hand. 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. `tsconfig.build.json` and the root `tsconfig.json` each hand-list every package as explicit project `references` — two identical sets that grow in lockstep, so a single generator can emit both — and `tsconfig.base.json`'s paths map hand-lists the per-group glob fan-out. `knip.json` restates a per-package `entry` stanza for each package that gains an `*.e2e.ts` suite — byte-identical overrides that exist only because the shared `packages/*/*` stanza omits the e2e glob (an entry glob matching no files is inert, so the default stanza could carry it for every package). The ACP snapshot suite's scenario table (`examples/acp-agent/tests/acp.snapshot.ts`) hand-maintains a `childSessions` count per scenario that duplicates the number of `session.<n>.jsonl` fixture siblings on disk. These lists are small today, but every new package or scenario class creates another manual synchronization point.
|
||||
|
||||
The [package hierarchy](../../implemented/architecture/2026-06-20-package-hierarchy.md) already removed several of these by hand: `scripts/publint-all.ts` now derives its list from the `packages/<group>/<pkg>` layout, and the two `tsconfig` `paths` maps collapsed to one `@deepseek-ai/dsh-*` wildcard. What remains is the inventory that cannot be globbed away — chiefly `tsconfig.build.json`'s project `references`, which TypeScript requires as an explicit array (no wildcard form).
|
||||
|
||||
@@ -16,12 +16,16 @@ Make the remaining package/gate inventories discoverable. A single canonical sou
|
||||
|
||||
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.
|
||||
|
||||
Two of the cataloged items need no generator at all: folding the e2e entry glob into knip's default stanza deletes the per-package restatements outright, and `childSessions` can be discovered from each scenario's fixture directory, leaving the scenario table to declare only policy (`recorded`, `hasModelTurn`, `comparesLog`) — and even those track fixture-derivable facts today (`comparesLog` ⟺ the committed log has entries beyond its header line; `recorded` ⟺ `hasModelTurn` with no `replay.override.json` sibling), so each new scenario class keeps adding knobs the fixture directory already answers.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `tsconfig.build.json` project `references` are generated from the hierarchy (a generator emits them; a `--check` gate fails when the committed copy is stale), rather than hand-maintained.
|
||||
- Adding a package does not require editing a static package list for any 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.
|
||||
- `knip.json` carries a per-package override only where it encodes real information (an extra entry file, an ignored dependency), never a restatement of the default stanza.
|
||||
- Snapshot scenarios declare policy, not facts discoverable from their fixture directories.
|
||||
|
||||
## What we give up
|
||||
|
||||
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Generate the RFC index tables
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`docs/rfc/README.md`'s per-lifecycle/per-class tables are hand-maintained even though every fact in them is derivable: an RFC's path encodes lifecycle and class, its filename encodes the first-proposed date, and its H1 carries the title. `scripts/verify-rfc-classification.ts` already walks the tree and cross-checks the index — the expensive parsing exists; it reports instead of writing.
|
||||
|
||||
The tables are also the repo's highest-contention docs hotspot: every proposal wave appends rows to the same few lines, so concurrent RFC branches conflict precisely there while agreeing everywhere else, and each conflict is resolved by hand-merging rows whose content the filesystem already knows. [The classification RFC](../../implemented/process/2026-06-20-rfc-classification.md) records rejecting auto-generation to keep the file curated — but the curated part of the README is the prose, and the prose never conflicts; only the mechanical tables do.
|
||||
|
||||
## Proposal
|
||||
|
||||
Keep the curated prose; generate the tables. Add a `gen-rfc-index` mode (a `--write` flag on `verify-rfc-classification.ts`, or a sibling script sharing its walker) that scans the RFC tree, reads each H1, derives the date from the filename, and rewrites the table rows under stable generated markers per `## {Lifecycle}` / `### {Class}` section; `verify-rfc-classification` asserts freshness — the `gen-cordis-catalog`/`verify-cordis-catalog` pattern. The class and lifecycle sets stay closed in the script. The implementing PR amends the classification RFC's rejected-alternatives record per [implemented/AGENTS.md](../../implemented/AGENTS.md), since this supersedes that recorded choice.
|
||||
|
||||
## Why not keep the verifier-only model?
|
||||
|
||||
It catches mistakes but still makes every proposal edit a shared hotspot, and a failed verifier is strictly more annoying than a generator for a purely mechanical row: the author has already named and placed the file; the index copy adds no information. This is the same hand-list-versus-derivation judgment the [package-inventory proposal](2026-06-20-discover-package-inventory.md) applies to tsconfig references and knip stanzas — applied to the one list that demonstrably conflicts.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `pnpm run gen-rfc-index` (or the chosen spelling) rewrites only the generated table regions; `verify-rfc-classification` fails when they are stale and passes after regeneration.
|
||||
- Adding, moving, or deleting an RFC requires editing only the RFC file itself; the rows are produced from path + H1 + filename date.
|
||||
- The prose outside the generated markers is untouched by the generator; `pnpm run doc-sync` passes.
|
||||
|
||||
## Risks
|
||||
|
||||
Generated regions inside a curated file need explicit markers so ownership is obvious to reviewers. Reading H1s makes a malformed header a generator error — useful pressure, and it should fail clearly. This supersedes an implemented process decision; amending that RFC's record is part of the change, not optional.
|
||||
@@ -1,33 +0,0 @@
|
||||
# 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.
|
||||
@@ -1,43 +0,0 @@
|
||||
# RFC: Drop the unconsumed `llm/adapter-change` event
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`LlmService.registerAdapter()` emits `llm/adapter-change` on registration and disposal ([packages/llm/llm/src/index.ts](../../../../packages/llm/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/simplification/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](../../../architecture.md) and [packages/llm/llm/README.md](../../../../packages/llm/llm/README.md). The [doc-sync-enforcement RFC](../../implemented/process/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.
|
||||
@@ -1,45 +0,0 @@
|
||||
# RFC: Drop unconsumed assembled LLM convenience surfaces
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`LlmService` ([packages/llm/llm/src/index.ts](../../../../packages/llm/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/llm/src/index.ts)).
|
||||
- `generate()` — one fully-assembled `GenerateResult`, dispatched through a second `llm/generate` waterfall ([index.ts:151-157](../../../../packages/llm/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/core/agent-loop/src/loop.ts](../../../../packages/core/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/simplification/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/llm/src/assembler.ts:138-168](../../../../packages/llm/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/architecture/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](../../../architecture.md) becomes `stream()` only, the event taxonomy drops `llm/generate`, and the [property-based-testing RFC](../../implemented/testing/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/llm/README.md`, [docs/architecture.md](../../../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.
|
||||
@@ -1,49 +0,0 @@
|
||||
# RFC: Prune dead methods from the persistence and bash capability seams
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
Two capability seams ([interface / implementation / consumer](../../implemented/architecture/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/session-persistence/src/index.ts:142-151](../../../../packages/session-persistence/session-persistence/src/index.ts)). Production consumers of `ctx.sessionPersistence` use only two of them: the agent-loop resume path calls `load()` ([packages/core/agent-loop/src/index.ts:176-194](../../../../packages/core/agent-loop/src/index.ts)), and the ACP bridge calls `list()` for `session/list` ([packages/ui/acp/src/index.ts](../../../../packages/ui/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/ui/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/session-persistence/src/coordinator.ts:298-310](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)). `delete()` drags the `deleteStored` backend hook ([coordinator.ts:99](../../../../packages/session-persistence/session-persistence/src/coordinator.ts), [coordinator.ts:313-319](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)) that every backend must implement. This is the [drop-mutable-session-summary](../../implemented/simplification/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/bash/src/index.ts:88-107](../../../../packages/bash/bash/src/index.ts)), both implemented by `LocalBashExecutor` ([packages/bash/bash-local/src/index.ts:179-191](../../../../packages/bash/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/bash/tests/service.spec.ts](../../../../packages/bash/bash/tests/service.spec.ts), [packages/bash/bash-local/tests/executor.spec.ts](../../../../packages/bash/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/bash/tool-bash/tests/tools.spec.ts](../../../../packages/bash/tool-bash/tests/tools.spec.ts), [packages/bash/tool-bash/tests/integration.spec.ts](../../../../packages/bash/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/architecture/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/session-persistence/README.md](../../../../packages/session-persistence/session-persistence/README.md)'s `has(id)`/`delete(id)` API row and its "delegates its six public service methods" prose → four, [packages/bash/bash/README.md](../../../../packages/bash/bash/README.md)'s `get(id)`/`list()` row), the backend READMEs that describe `has`/`list` semantics ([packages/session-persistence/session-persistence-sqlite/README.md](../../../../packages/session-persistence/session-persistence-sqlite/README.md), [packages/session-persistence/session-persistence-jsonl/README.md](../../../../packages/session-persistence/session-persistence-jsonl/README.md) — reword "absent from `has()`/`list()`" to just `list()`), the service-map / seam docs in [docs/architecture.md](../../../architecture.md), and the persistence prose in the [session-persistence RFC](../../implemented/architecture/2026-06-14-session-persistence.md) and [shared write-coordinator RFC](../../implemented/architecture/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/session-persistence/src/index.ts](../../../../packages/session-persistence/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/session-persistence/src/coordinator.ts](../../../../packages/session-persistence/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."
|
||||
@@ -1,32 +0,0 @@
|
||||
# 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.
|
||||
@@ -1,31 +0,0 @@
|
||||
# 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.
|
||||
@@ -9,12 +9,13 @@ The agent factory carries TWO ids for what is, in every live consumer, one thing
|
||||
- `agentId` — the `AgentRegistry` handle (the actor identity; the registry rejects a duplicate).
|
||||
- `sessionId` — the event-sourced session / persisted-log identity (`session.header.id`).
|
||||
|
||||
`CreateAgentOptions` takes both separately; `ResumeAgentOptions` takes an `agentId` plus a `resumeSessionId`. They diverge in exactly two places:
|
||||
`CreateAgentOptions` takes both separately; `ResumeAgentOptions` takes an `agentId` plus a `resumeSessionId`. They diverge in exactly three places:
|
||||
|
||||
- **Config-driven create** (`AgentLoop.create`): a stable `agentId` (e.g. `"echo"`) with a fresh per-run `sessionId` (`${id}-session-<uuid>`).
|
||||
- **Resume**: a caller-supplied `agentId` (e.g. `"main"`) on a persisted `resumeSessionId`.
|
||||
- **In-process subagent children**: the backend mints the child's `agentId` and `sessionId` as two independent UUIDs (`packages/subagent/subagent-inprocess/src/index.ts`) that nothing distinguishes — `parentSession` records lineage independently.
|
||||
|
||||
Everywhere a live consumer actually looks an agent up — the **ACP bridge, the only production path** — the two are already unified: `agentId === sessionId === <uuid>`.
|
||||
Where a live consumer looks an agent up, no lookup needs an id translation: the ACP bridge — the primary production path — already unifies the two (`agentId === sessionId === <uuid>`; both factory call sites brand `AgentId(sessionId)` directly, and its reverse lookup keys on the `Agent` object itself), and the CC hooks bridge resolves subagent children directly by the `agentId` its lifecycle event carries. The one production population whose two ids actually DIVERGE is the in-process subagent children — the same cosmetic separation as the config path, and the same one-field simplification under unification. One consumer already pays the two-id tax: ui-stdio keeps a `labelBySession` map (seeded from the registry, maintained by `agent/created`/`agent/disposed` listeners) solely to translate `session.header.id` back to an agent id for its turn labels — machinery that deletes outright when the ids unify. And the CC hooks bridge stamps `session_id: agent.session.header.id` into every hook payload, so under unification a subagent hook's `session_id` and `agent_id` become the same string — one less identity for a hook author to reconcile.
|
||||
|
||||
The separation is **latent generality no consumer exercises**: nothing reads a *stable* `agentId` back across runs (each process starts fresh, and persistence keys off the session id, never the agent id). The config path's "stable agentId, fresh sessionId" buys nothing concrete — it is cosmetic. And the `agentId !== sessionId` case is precisely what opens the bash owner-token alias hole: the bash completion-notice routes by `session.header.id`, but the registry enforces uniqueness only on `agentId`, so a programmatic caller registering two agents with different agent ids but the SAME session id can mis-route a notice (see [agent lifecycle and ownership seams](../../implemented/architecture/2026-06-18-agent-lifecycle-and-ownership-seams.md) § Seam precondition). The current code documents this as a precondition rather than guaranteeing it.
|
||||
|
||||
@@ -40,13 +41,13 @@ That was the review's first suggestion. It would couple the generic registry to
|
||||
|
||||
## Risks
|
||||
|
||||
This touches public factory interfaces (`CreateAgentOptions`, `ResumeAgentOptions`, `AgentFactory`) and the config-agent id scheme, so it is a deliberate cross-package change, not a local patch — it ships as its own PR (converged with Codex), stacked on the bash owner-token work that surfaced the precondition.
|
||||
This touches public factory interfaces (`CreateAgentOptions`, `ResumeAgentOptions`, `AgentFactory`) and the config-agent id scheme, so it is a deliberate cross-package change, not a local patch — it ships as its own PR (converged with Codex); the bash owner-token precondition it closes is documented in [agent lifecycle and ownership seams](../../implemented/architecture/2026-06-18-agent-lifecycle-and-ownership-seams.md).
|
||||
|
||||
The genuine risks of collapsing the two ids into one (the case AGAINST this proposal — to be weighed honestly before implementing):
|
||||
|
||||
- **It forecloses a one-agent-resumes-many-sessions / one-session-driven-by-many-agents future.** Today the separate ids leave room for an agent (a stable actor) to detach from one session and attach to another, or for a handoff where a new agent process adopts an existing session under a new actor handle. Unifying makes "agent" and "session" the same lifetime, so any such future needs a NEW seam (e.g. an explicit `actorId` distinct from the session) — re-introducing the very separation we removed. We judge this generality currently unused, but it is a door this change closes.
|
||||
|
||||
- **Sub-agents / fork / spawn (an explicitly deferred seam) may WANT a stable actor id across forked sessions.** `AgentLoop.create`'s `TODO(sub-agents)` envisions a child agent seeded from a parent's event log. If the design wants "the same agent identity across a fork" (parent and child share an actor but have distinct session logs), a unified id blocks it. The implementing PR must check the intended fork/spawn model BEFORE unifying, or accept that fork always mints a fresh combined id.
|
||||
- **Subagents / fork / spawn may WANT a stable actor id across forked sessions.** The [subagent seam](../../implemented/feature/2026-06-21-subagent-capability-seam.md) runs a child agent seeded from a parent's event log (fork). If a future design wants "the same agent identity across a fork" (parent and child share an actor but have distinct session logs), a unified id blocks it. The implementing PR must check the intended fork/spawn model BEFORE unifying, or accept that fork always mints a fresh combined id. (As shipped, each subagent child mints its own distinct agent id — `parentSession` records lineage — so the seam does not currently rely on a shared actor id, but unifying would foreclose adding one.)
|
||||
|
||||
- **The config-driven resume-or-create policy becomes load-bearing, not cosmetic.** Today the per-run-uuid session id quietly sidesteps the "a fixed id collides with its own on-disk log on the second run" problem. Once the id is unified and stable, a config agent restarting MUST decide resume-vs-fresh deliberately — there is no longer a throwaway session id to hide behind. Getting this wrong reintroduces the create-collision the uuid was avoiding (a durable backend refuses to re-create an id whose log exists). This is the one real design decision the implementing PR owns, and it is easy to get subtly wrong.
|
||||
|
||||
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Drop the `image` content block until a path can honor it
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`ImageBlock` (`packages/llm/llm/src/types.ts`) has no production producer, and every consumer on every path DROPS it: the deepseek adapter's serializer skips image blocks (a documented MVP limitation), the pi-ai converter skips them as unrepresentable, the ACP codec neither advertises image prompt capability nor forwards image blocks outbound and REJECTS image prompt content inbound, and the compaction estimator charges a flat token constant and renders `[image]`. An `ImageBlock` constructed today would silently vanish from the wire — the vocabulary advertises a capability no path honors, which is the silent-data-loss shape AGENTS.md's defensive patterns warn against. The only constructors anywhere are tests pinning the skip/drop/estimate branches.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove `ImageBlock`, its `ContentBlockMap` entry, the explicit `image` estimate/placeholder arms in compact-basic, and the image-naming comments in the deepseek serializer's, pi-ai converter's, and ACP codec's default arms — those default arms already absorb the case the way they absorb any unknown block type. Update the vocabulary line in [architecture.md](../../../architecture.md), the block list in `packages/llm/llm/README.md`, the deepseek README's image-skip row, the pi-ai README's images-not-representable row, the compact-basic README's image-estimation and `[image]`-placeholder rows, the pastes in [core.md](../../../core-data-structures/core.md) and [llm-streaming.md](../../../core-data-structures/llm-streaming.md), and the type-equiv manifest; amend the [content-block vocabulary RFC](../../implemented/architecture/2026-06-11-content-block-vocabulary.md)'s block list and multimodal-home consequence per [implemented/AGENTS.md](../../implemented/AGENTS.md); drop or retarget the tests that construct image blocks to exercise the removed branches. The ACP codec's inbound rejection of image PROMPT content is unaffected — that guard is about protocol content a client can send regardless of our vocabulary, and it stays.
|
||||
|
||||
## Why not keep it?
|
||||
|
||||
This is the most contested cut in the batch. Multimodal input (screenshots) is a plausible near-term coding-agent feature, and the [content-block vocabulary RFC](../../implemented/architecture/2026-06-11-content-block-vocabulary.md) reserved the slot deliberately. Two responses. First, `ContentBlockMap` is merge-extensible by design: a real multimodal feature reintroduces `image` in core in the same coordinated change that maps it in the adapters, advertises and renders it in ACP, and prices it in compaction — the producer and its consumers arrive together, which is how the map is meant to grow. Second, the middle option — keep the type but make adapters throw UNSUPPORTED instead of silently dropping — converts this into exactly the shape the [request-knobs RFC](2026-07-04-drop-inert-request-knobs.md) argues against: surface whose only implementation is rejection. Absence (a compile error at the would-be producer) is strictly clearer than either silent loss or universal throw.
|
||||
|
||||
If review lands on keeping the slot, the fallback this RFC records is: keep `ImageBlock` but replace every silent skip with a loud rejection, and document that policy in the vocabulary — the current silent drop is the one state with no defender.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- No `ImageBlock` / harness `type: 'image'` block construction outside this RFC; the codec's inbound ACP-image rejection still passes its tests.
|
||||
- Adapter/codec/compaction switches handle the case through their unknown-block default arms (pinned by the existing plugin-added-block tests where present).
|
||||
- Doc pastes, the manifest, and the architecture vocabulary list updated; `pnpm run doc-sync` green.
|
||||
|
||||
## Risks
|
||||
|
||||
Re-adding a core vocabulary type later touches several packages at once — but that coordinated change is the shape a real multimodal feature needs anyway (adapter mapping, ACP advertisement, compaction pricing), and none of it exists today to preserve.
|
||||
@@ -0,0 +1,33 @@
|
||||
# RFC: Drop `GenerateOptions.prefill` and `ToolSchema.strict` — request knobs with no working end-to-end path
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
Two request-contract knobs ride the whole request pipeline, yet neither can do anything today:
|
||||
|
||||
- **`prefill`** (`packages/llm/llm/src/types.ts`) has no production setter — the loop assembles `model`/`system`/`tools`/`messages` plus `sessionId`/`signal`, and the compaction backend adds only `maxTokens` — and BOTH adapters reject it: `packages/llm/llm-deepseek/src/serialize.ts` and `packages/llm/llm-pi-ai/src/adapter.ts` each throw `LlmError('UNSUPPORTED')` on a non-undefined `prefill`. The field's entire observable behavior is two throws, each pinned by one adapter test. DeepSeek's chat-prefix completion is a Beta feature on a base URL neither adapter targets.
|
||||
- **`strict`** (`ToolSchema`, same file) is threaded through `DefineToolOptions`/`defineTool` (`packages/core/tools/src/schema.ts`), the registry's `schemas()` allowlist (`packages/core/tools/src/index.ts`), the deepseek wire mapping (`packages/llm/llm-deepseek/src/serialize.ts`, whose wire-type note records that strict mode requires the `/beta` base URL the adapter does not use), and a per-tool payload-patching pass in `packages/llm/llm-pi-ai/src/adapter.ts`. No shipped tool sets it — `rg` across every `tool-*` package src and `examples/` finds zero `strict:` producers; the only setters are dsh-tools unit tests.
|
||||
|
||||
Both knobs are adapter-symmetric, so removal sheds them from both twins together — the [twin-adapter design](../../implemented/architecture/2026-06-13-twin-llm-adapters.md) is untouched.
|
||||
|
||||
## Proposal
|
||||
|
||||
- Remove `prefill` from `GenerateOptions`, both adapters' UNSUPPORTED guards, the tests pinning the throws, the paste lines in [core.md](../../../core-data-structures/core.md), the adapter README rows documenting the rejection, and the cookbook line using prefill as the UNSUPPORTED example ([adding-an-llm-adapter.md](../../../cookbook/adding-an-llm-adapter.md)); amend the [content-block vocabulary RFC](../../implemented/architecture/2026-06-11-content-block-vocabulary.md)'s consequence line naming prefill as having a home, per [implemented/AGENTS.md](../../implemented/AGENTS.md).
|
||||
- Remove `strict` from `ToolSchema`, `DefineToolOptions`, `defineTool`, and the `schemas()` allowlist; drop the deepseek serializer branch; simplify the pi-ai payload fixup to the unconditional scrub of pi-ai's own strict default (that half exists for wire parity with the hand-rolled twin and survives); drop the setter tests and the core.md paste line.
|
||||
|
||||
This RFC deliberately does NOT touch `temperature`, `stop`, or `maxTokens`: those are honored end-to-end by both adapters and are the natural first targets of a request-mutating hook plugin on `agent/request`.
|
||||
|
||||
## Why not keep them?
|
||||
|
||||
"An explicit UNSUPPORTED throw is honest contract behavior" — but a knob whose only implementation across both twins is rejection promises nothing, and deleting it upgrades the failure mode: an accidental setter becomes a compile error instead of a runtime throw. "Strict schema adherence is an officially documented provider feature with complete plumbing" — but a knob is not product surface until a shipped tool sets it AND an endpoint honors it; today neither is true. Each returns with its first real producer: `prefill` together with an adapter that implements chat-prefix completion (and a stated policy for adapters that do not), `strict` together with a tool that wants it and a beta-endpoint story.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `rg prefill` and a tool-schema-scoped `rg strict` return only this RFC (and unrelated prose such as `strictEqual`).
|
||||
- Both adapters compile and their contract tests pass without the guards; the pi-ai fixup still scrubs the library's strict default (wire parity pinned by its serializer tests).
|
||||
- Doc pastes and the type-equiv manifest in sync; `pnpm run doc-sync` green.
|
||||
|
||||
## Risks
|
||||
|
||||
The shipped hook bridges set no request fields at all, and a request-mutating plugin (an `agent/request` waterfall listener) would reach for `temperature`/`stop` (kept, working), not a field adapters reject. If chat-prefix completion or strict mode become product features, the re-add lands with the adapter/endpoint work, where the contract can say what actually happens rather than "everyone throws".
|
||||
@@ -0,0 +1,32 @@
|
||||
# RFC: Drop the unconsumed web observation surface — the `providers-change` event and the status methods
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`WebService` exposes an observation surface no production code observes:
|
||||
|
||||
- **`web/providers-change`** (`packages/web/web/src/index.ts`) is declared and emitted on every provider registration and disposal, and each registration effect's rollback yield is ordered BEFORE the emit solely so a throwing change listener unwinds the registration. No listener exists outside the package's own two unit tests (one of which exists to pin that rollback ordering).
|
||||
- **`searchStatus()` / `fetchStatus()` and the `WebCapabilityStatus` union** (same package) have zero production callers: `dsh-tool-web` executes directly through `ctx.web.search()`/`fetch()` and surfaces unavailability as the structured `WebError` codes the seam throws at execution time (`packages/web/tool-web/src/search.ts`, `packages/web/tool-web/src/fetch.ts`); the only status callers are the web packages' own tests. The prose in `packages/web/tool-web/README.md` and [architecture.md](../../../architecture.md) still claims the tool "reads only the aggregated `searchStatus()`/`fetchStatus()`" — drift that survives only because nothing checks prose against call sites.
|
||||
|
||||
The seam's own design starves both surfaces of consumers: tool registration follows product ENABLEMENT, not provider availability (`packages/web/tool-web/src/index.ts`), and provider selection resolves at execution time, never cached — so there is no cache to invalidate, no registration set to recompute, and no caller that needs an availability probe distinct from executing and routing the structured error. HMR cleanup is carried by the effect disposers themselves.
|
||||
|
||||
This mirrors [drop the unconsumed `llm/adapter-change` event](../../implemented/simplification/2026-06-20-drop-unconsumed-llm-adapter-change-event.md), which removed the same notification shape, the same rollback-before-emit machinery, and the same listener-throw test from `LlmService`. That RFC's keep/cut criterion — keep `tools/change` for its plausible user-facing tool-list consumer, cut the boot-time backend-registry signal — puts a web-provider registry squarely on the cut side; the status methods are the same judgment applied to a pull surface instead of a push one.
|
||||
|
||||
## Proposal
|
||||
|
||||
Delete the event declaration, both emits, and the rollback-before-emit ordering (the plain `ctx.effect` disposer keeps HMR cleanup). Delete `searchStatus()`/`fetchStatus()`/`WebCapabilityStatus` — the provider-private `status()` stays, since it feeds execution-time selection. Delete the two event tests and rewrite the status-based test assertions onto the behavior a real caller observes (a successful `search()`/`fetch()`, or the structured `WebError` codes for unavailable/ambiguous/misconfigured provider sets). Run `pnpm run gen-cordis-catalog`; update `packages/web/web/README.md`, `packages/web/tool-web/README.md` (the drifted reads-status sentence), [web.md](../../../core-data-structures/web.md), and the web paragraph in [architecture.md](../../../architecture.md). The implementing PR amends the [web capability seam RFC](../../implemented/architecture/2026-06-24-web-capability-seam.md)'s facts (it specifies the event and the status aggregation) per [implemented/AGENTS.md](../../implemented/AGENTS.md).
|
||||
|
||||
## Why not keep it?
|
||||
|
||||
The web seam RFC specified both deliberately — the event as a minimal HMR-visibility signal, the status methods as the tool's aggregated diagnostics — and a future provider-status panel is imaginable. But the same RFC's other choices starved them: derived-on-call selection and enablement-based registration leave no consumer that CAN need either, the shipped tool demonstrates the real pattern (execute and route the structured error), and the drifted README sentence shows the promised consumer never materialized. Per AGENTS.md "RFCs are proposals, not golden truth", these are the parts of that proposal the code has since shown to over-reach; a future observer reintroduces the smallest signal or query it actually consumes, shaped by that consumer.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- No `providers-change`, `searchStatus`, `fetchStatus`, or `WebCapabilityStatus` spelling outside RFC history; the catalog is regenerated and fresh (`verify-cordis-catalog` green).
|
||||
- Registration/disposal HMR-safety tests prove cleanup through execution behavior rather than the removed surfaces.
|
||||
- `packages/web/tool-web/README.md` and the architecture paragraph describe the execution-time error-routing contract the tool actually has.
|
||||
|
||||
## Risks
|
||||
|
||||
A future provider-picker UI or diagnostics panel wants change notifications or a status query — it re-adds the smallest surface it consumes; the identical judgment, and its reversal condition, is already recorded on the llm precedent.
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Fold the stdio UI helper into the stdio app
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`@deepseek-ai/dsh-ui-stdio` is a whole package whose only runtime importer is the app package `@deepseek-ai/dsh-stdio-agent` (`packages/ui/stdio-agent/src/index.ts`). The examples reach the readline UI by loading the app, never by composing the helper themselves; every other repo reference is mechanical or descriptive surface that exists BECAUSE the package boundary exists — manifest and tsconfig entries, generated module-graph rows, dependency-graph and README rows, and doc comments naming the package. [The ui group README](../../../../packages/ui/README.md) records the placement rationale — the helper "exists chiefly for the examples and the coverage gate — `ui/` is reserved for surfaces shipped as product" — which leaves a standing tension: a shipped product app depends on a support package documented as NOT product surface.
|
||||
|
||||
The boundary buys package metadata, workspace and tsconfig references, module-graph rows, README entries, and publint surface for a helper that is not independently swappable: the stdio app's front-door cluster always includes the readline UI, and nothing else can meaningfully consume it.
|
||||
|
||||
## Proposal
|
||||
|
||||
Fold the helper into `@deepseek-ai/dsh-stdio-agent`: move `createStdioChat`, its `StdioRuntime` test seam, and its unit tests into `packages/ui/stdio-agent`; delete the `packages/support/ui-stdio` package with its manifest, references, module-graph rows, and README rows; update every reference that names the package (the example e2e module docs, `packages/README.md`, the support and todo README rows, the stdio-agent README, the ui group README, tsconfig references, the generated module graph). Keep the runtime seam so EOF handling, rendering, disposal, and piped-vs-TTY behavior stay unit-covered without hijacking process globals; the keyless Loader-path smokes keep guarding the export shape end-to-end.
|
||||
|
||||
## Why not promote it to `ui/` instead?
|
||||
|
||||
Promotion would resolve the support-vs-product mismatch while keeping the boundary — the right call only if the readline UI were an independently swappable integration or had a second composer, and the consumer census says neither. The structured ACP bridge stays its own package because it is the product protocol surface with its own contract and snapshot tiers; the readline helper is scaffolding for one app's front door. Re-extraction stays cheap pre-release: if a second product app wants the readline UI, split it back out then, with that consumer shaping the package contract.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `packages/support/ui-stdio` no longer exists; the helper and its tests live in `packages/ui/stdio-agent`; no reference to the deleted package remains outside RFC history.
|
||||
- The stdio app still renders transcript events, handles stdin lines and EOF, renders todo checklists, and disposes readline listeners under HMR; the echo/coding keyless smokes still boot through the real Loader path and guard the export shape.
|
||||
- Manifests, tsconfig references, the generated module graph, and docs are updated; `pnpm run test:coverage`, `pnpm run test:snapshot`, `pnpm run doc-sync`, `pnpm run build`, and `pnpm run hygiene` pass.
|
||||
|
||||
## Risks
|
||||
|
||||
A future standalone terminal UI may want the helper as a package again — reintroduce it with that second consumer rather than keeping the boundary for hypothetical reuse. Moving tests risks blurring app-composition tests with UI-rendering tests; keeping the runtime seam and the colocated unit tests avoids that.
|
||||
@@ -0,0 +1,30 @@
|
||||
# RFC: Prune dead core-spine surface — `SurfaceManager.invalidate()`, the loop-internal exports, `ToolExecutionResult.callId`
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
Three pieces of public spine surface share one defect class: their only possible role is to be ignored, or their trigger is unreachable.
|
||||
|
||||
1. **`SurfaceManager.invalidate()`** (`packages/core/session/src/surface.ts`). Its documented trigger — "the log has been replaced wholesale (e.g. after Session seed)" — is structurally unreachable: seeding happens inside the `Session` constructor, `_surface` is created lazily on first access, and the log reference is never reassigned afterward, so no constructed `SurfaceManager` ever observes a wholesale replacement. Sole caller: its own unit test. A rollback primitive protecting a scenario the implementation cannot produce.
|
||||
2. **The `runLoop`, `Inbox`, and `InboxMessage` exports** (`packages/core/agent-loop/src/index.ts`). `runLoop` has no importer outside the package — the only callers are the package's own internals (the agent constructs its loop with it), so the public re-export has zero consumers; `Inbox`/`InboxMessage` likewise reach outside code only through the package's own inbox spec (switchable to the source module). The exports contradict the package's own docs — the inbox module doc says the public surface is `Agent.send()`/`Agent.steer()` — and the [architecture dependency rule](../../../architecture.md): nothing programs against `dsh-agent-loop`; a replacement loop is a different bundle built on `dsh-agent`, not a consumer of this package's internals. `ReactLoopAgent` stays exported (cross-package tests construct it by package name).
|
||||
3. **`ToolExecutionResult.callId`** (`packages/core/tools/src/index.ts`; the *input* `ToolExecution.callId` stays). Zero readers — and no listener can even construct a result: `tools/pre-execute`/`tools/post-execute` listeners return Decisions, the registry builds every result itself and always sets `callId` to the input `exec.callId`, and the post-execute dispatch snapshots the outcome before the waterfall precisely so a listener mutating the shared result reference cannot corrupt the id. The loop independently ignores `result.callId` in favor of its own `call.id`, and two regression tests exist solely to prove the field cannot matter (the loop's ignores-result-callId test and the registry's mutation guard). A field that is by construction a copy of its input, defended by snapshot machinery, and pinned by tests proving it is ignored is pure liability surface; the ACP bridge correlates via the session event's `data.callId`, never via the execution result.
|
||||
|
||||
## Proposal
|
||||
|
||||
Delete the method and its test; delete the three export lines and their `packages/core/agent-loop/README.md` rows, pointing the inbox spec at the source module; drop the result field from the type, the registry's construction sites (the deny result, the dispatch result, `toolErrorResult`, and the post-execute snapshot's `callId` leg), the loop's ignore-comment, the proves-ignored regression test, and the mutation guard's `callId` assertions — the hazard they all pin disappears with the field, while the result's `additionalContext` ferry (a consumed post-execute channel) stays untouched. Update the `ToolExecutionResult` paste in [tools.md](../../../core-data-structures/tools.md) (and its `scripts/type-equiv.manifest.json` row) and the result-shape row in `packages/core/tools/README.md`; for the `invalidate()` removal, amend the [session-surface RFC](../../implemented/architecture/2026-06-18-session-surface.md)'s full-rebuild-after-wholesale-replacement sentence per [implemented/AGENTS.md](../../implemented/AGENTS.md).
|
||||
|
||||
Sequencing: the in-flight surface-cache work (tool-pairing balance caching) neither uses nor touches `invalidate`, so that removal lands after or alongside it mechanically. The execute pipeline is `tools/pre-execute` → dispatch → `tools/post-execute`, and post-execute listeners receive the execution object alongside the result — nothing needs the result's own id.
|
||||
|
||||
## Why not keep them?
|
||||
|
||||
A future consumer that swaps a session's log in place would want a reset primitive — it re-adds `invalidate` with itself. A replacement-loop author might want to reuse the inbox or the driver — the architecture already answers that a replacement loop is a different bundle. An isolated result-logging listener might want self-contained correlation on the result — the execution object is in scope at every listener, and a field that exists only to be ignored is worse than absent: it invites exactly the orphaned-pairing bug the loop comment warns about.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `invalidate()` and the result `callId` appear only in this RFC; `runLoop`/`Inbox`/`InboxMessage` remain package-internal only — no re-export from the package index and no outside-package importer; the agent-loop README lists only the consumed public surface; the inbox spec imports the source module.
|
||||
- The pre-/post-execute pipeline contract tests pass with the shrunk result type; the mutation-guard and proves-ignored tests shed their `callId` legs with the hazard they pin.
|
||||
|
||||
## Risks
|
||||
|
||||
All three are compile-visible removals with no runtime behavior change on any shipped path.
|
||||
@@ -0,0 +1,31 @@
|
||||
# RFC: Prune producer-less vocabulary variants (block cache hints, the `agent` message source, the `continuation` turn trigger)
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
The merge-extensible vocabulary maps are designed to grow by declaration merging, and the codebase already states the admission policy on `TurnEndReasonMap` (`packages/core/session/src/types.ts`): a variant like `refusal` is "deliberately omitted until" an adapter or loop first emits it. Three declared vocabulary items violate that policy — each has no producer and no consumer, and two have not even a test:
|
||||
|
||||
- **`CacheHint` and the three `cache?: CacheHint` fields** on `TextBlock`/`ToolResultBlock`/`ImageBlock` (`packages/llm/llm/src/types.ts`). Nothing constructs a block with `cache:` anywhere — src, tests, and doc pastes all come up empty — and neither adapter reads `.cache`: DeepSeek prompt caching is automatic, so the adapters map `prompt_cache_hit_tokens` OUT of responses without ever sending a hint IN. This is Anthropic-style `cache_control` surface with no provider that can honor it.
|
||||
- **`MessageSourceMap.agent`** (`{ kind: 'agent'; agentId: string }`, same file). Zero constructors, tests included. Its intended producer shipped without it: the subagent backends send the parent's prompt to the child with no `source`, so it logs as `{ kind: 'user' }`, and the generic envelope renderer interpolates `source.kind` without ever routing on it. The variant is pasted into [core.md](../../../core-data-structures/core.md).
|
||||
- **`TurnTriggerMap.continuation`** (`packages/core/session/src/types.ts`). The loop structurally cannot emit it — continuation happens *within* a turn as further steps, never as a new turn — and it constructs only `message` and `injection` triggers. The only writer is one hand-built test fixture that needs an arbitrary non-message trigger (`packages/support/llm-replay/tests/llm-replay.spec.ts`); the only production trigger reader, the ACP bridge, filters on `kind === 'message'`. The variant is pasted into [session.md](../../../core-data-structures/session.md).
|
||||
|
||||
## Proposal
|
||||
|
||||
Delete `CacheHint` with its three `cache?` fields, the `agent` message-source variant, and the `continuation` turn-trigger variant. Switch the llm-replay fixture to an `injection` trigger (any non-`message` trigger serves its purpose). Update the type-equiv pastes in [core.md](../../../core-data-structures/core.md) and [session.md](../../../core-data-structures/session.md) (and `scripts/type-equiv.manifest.json` where block identity shifts) in the same change, and amend the [content-block vocabulary RFC](../../implemented/architecture/2026-06-11-content-block-vocabulary.md)'s consequence line naming cache hints as having a home, per [implemented/AGENTS.md](../../implemented/AGENTS.md).
|
||||
|
||||
Each variant returns the day it gains a real producer, exactly as the maps are designed to grow: a caching feature re-adds `cache` together with the adapter that transmits it; subagent attribution re-adds `agent` together with the backend that stamps it and a consumer that routes on it; an auto-continue feature that genuinely starts new turns re-adds `continuation` with the plugin that emits it.
|
||||
|
||||
## Why not keep them?
|
||||
|
||||
The [content-block vocabulary RFC](../../implemented/architecture/2026-06-11-content-block-vocabulary.md) lists "cache hints … have a home" as a design consequence, and reserved slots do advertise intent. But an empty slot is contract surface every implementation and consumer must consider (must my adapter honor `cache`? must my renderer route `agent` sources?), and the sibling map's own JSDoc already rejects reservation-without-emitter — `refusal` and `max_turn_requests` are named as variants to add *when something first emits them*, not declared in advance. Holding already-declared dead variants to the same standard makes the vocabulary mean something: if it is in the map, something produces it.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `rg` for `CacheHint`, the `agent` message-source spelling, and the `continuation` trigger spelling returns only this RFC.
|
||||
- The core-data-structures pastes and the type-equiv manifest are in sync (`pnpm run doc-sync` green).
|
||||
- The fixture asserts the same replay behavior with an `injection` trigger; the suite is green.
|
||||
|
||||
## Risks
|
||||
|
||||
None operational — nothing can construct these values today. The mirror-event removals (recorded in [the boundary-mirror RFC](../../implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) and [the stream-chunk RFC](../../implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md)) touch only transient `agent/*` events, never the durable vocabulary, so there is no collision. Elsewhere in the vocabulary the admission policy already holds: `rejected`, `prompt/blocked`, and `hook/invoked`/`hook/result` each have live producers — this RFC extends the same bar to the three variants that lack one. If the [image-block RFC](2026-07-04-drop-image-content-block.md) ships first, one of the three `cache?` fields leaves with it; the two proposals are independent and compose in either order.
|
||||
@@ -0,0 +1,29 @@
|
||||
# RFC: Prune write-only fields and a dead routing knob from the fs seam
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
The [fs seam split](../../implemented/simplification/2026-06-26-fsspec-style-fs-seam.md) moved read routing and policy out of the backend into `dsh-tool-fs` and `dsh-fs-policy`. Four pieces of surface kept the pre-split shape — populated on every call, read by nobody:
|
||||
|
||||
1. **`STREAM_MIN_SIZE` + `FsIoInternals.streamMinSize` in `dsh-fs-local`** (`packages/fs/fs-local/src/fsio.ts`, re-exported from `packages/fs/fs-local/src/index.ts`): zero readers anywhere, including fs-local's own source and tests. The backend has no read routing — `readWholeText`/`streamWholeText` are separate primitives the caller chooses between — and the real routing constant lives in the consumer (`packages/fs/tool-fs/src/read.ts`, compared against `info.size`). Two mirrors of the 10 MiB fact; the backend's is dead, and the knob's JSDoc claims a "read routing" override that does not exist.
|
||||
2. **`FsTarget.inputPath`** (`packages/fs/fs/src/types.ts`): every backend and every test fake must fabricate a "diagnostics only" value with zero production readers — the policy plugin and every error message use `targetKey`/`displayPath`. The `listDir` producer exposes the semantic wobble: directory children get the bare entry name, which was nobody's "input".
|
||||
3. **`FsEditOutcome.replacements` + `.replaceAll`** (`packages/fs/fs/src/types.ts`): `replacements` has zero production readers (the single-match policy itself stays — it is enforced by the `FS_AMBIGUOUS_EDIT`/`FS_EDIT_NOT_FOUND` throws inside the backend, whose error message keeps the internal count); `replaceAll` is read only by `formatEditOutput` in `packages/fs/tool-fs/src/edit.ts` — as an echo of the `replace_all` argument the tool already holds. Shrunk, `FsEditOutcome` becomes `{ version, before, after }`, parallel to `FsWriteOutcome`'s genuinely backend-discovered fields.
|
||||
4. **`FileReadOutcome.limit` + `.version`** (`packages/fs/tool-fs/src/read-render.ts`): populated by the read tool, but `formatReadOutput` renders `offset`/`lines`/`totalLines`/`truncatedByBytes` only, and the `fs/observed` emit uses `info.version` directly rather than the outcome copy.
|
||||
|
||||
## Proposal
|
||||
|
||||
Delete the fs-local constant, its re-export, and the `streamMinSize` knob (the remaining `FsIoInternals` knobs are genuinely used by the atomic-write tests); drop `inputPath` from `FsTarget`; shrink `FsEditOutcome` to `{ version, before, after }` and pass `replaceAll` to `formatEditOutput` from the parsed args; drop `limit`/`version` from `FileReadOutcome`. Update the [filesystem.md](../../../core-data-structures/filesystem.md) pastes, the type-equiv manifest, `packages/fs/fs/README.md`, and the test fakes that currently must fabricate the removed fields.
|
||||
|
||||
## Why not keep them?
|
||||
|
||||
A future permission/containment layer might want the pre-resolution path for error text — but it would want the *request*, which every call site still holds. "N occurrences replaced" might become model-facing text — a behavior change to design when wanted, and the backend-internal count survives for its error message. A read footer might display `limit` — everything the footer shows already derives from `lines`/`totalLines`. Meanwhile every current and future backend (remote, native) must fabricate wire fields nobody consumes, and every test fake must satisfy them.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- The removed surfaces are gone — `STREAM_MIN_SIZE`/`streamMinSize` in `dsh-fs-local`, `FsTarget.inputPath`, `FsEditOutcome.replacements`/`.replaceAll`, and `FileReadOutcome.limit`/`.version` — while the request-side `replaceAll` (`FsEditSpec`) and the version fields on the other outcome types are untouched; doc pastes and the manifest in sync; the suite is green with the shrunk fakes.
|
||||
- `formatEditOutput`'s emitted text is unchanged for both `replace_all` branches, so no snapshot golden churns.
|
||||
|
||||
## Risks
|
||||
|
||||
The in-flight fs discovery work (glob/grep tools) touches the same `dsh-fs` type files — a textual, not design, conflict; land in either order and reconcile mechanically. Backends gain no new obligations; they shed four.
|
||||
@@ -0,0 +1,28 @@
|
||||
# RFC: Remove the `agent/steering` mirror emit
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`agent/steering` is the last remaining transient mirror of a durable session event. The loop's steering drain appends the durable `steering/message { turn, content, source }` and, on the very next line, emits `agent/steering(agent, turn, content, source)` — the identical fact as a fire-and-forget event (`packages/core/agent-loop/src/loop.ts`, `drainSteering`). It has zero production listeners: the only subscriber anywhere is a loop regression test asserting the emit carries `source` — the same fact the durable event already records one line above.
|
||||
|
||||
Both mirror-removal RFCs retained it while explicitly deferring the decision this RFC now makes. The [boundary-mirror removal](../../implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) kept it as "a live control signal, not a boundary"; the [stream-chunk removal](../../implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md) kept it as "a live control signal with no durable twin, retained (its fate is a separate future decision)". The second rationale does not survive the code: the durable twin is `steering/message`, appended immediately before the emit with the same payload. The mirrored-vs-live-only line the taxonomy actually draws puts it on the mirror side: `agent/queued` is genuinely live-only (it fires at enqueue time, before any durable event exists, and already carries a `steering: boolean` flag — cancelled queued work never enters the log), while `agent/steering` fires at the exact moment its durable twin lands, carrying nothing the log does not.
|
||||
|
||||
Steering carries real production traffic — the hook bridges' turn-continuation decisions inject their reasons through `inbox.steer()`, landing as durable `steering/message` events that the hook-matrix goldens pin — and every one of those consumers observes the durable event. Nothing observes the mirror.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove the `agent/steering` declaration from `packages/core/agent/src/types.ts` (and its mention in the live-events JSDoc list there), the emit in `drainSteering` (whose `ctx` parameter becomes unused and goes too), the row in `packages/core/agent/README.md`, and the emit line in the loop-pseudocode blocks (`packages/core/agent-loop/src/loop.ts` module doc and [architecture.md](../../../architecture.md)); run `pnpm run gen-cordis-catalog`. Retarget the one regression test at the durable `steering/message` event — the source-preservation fact it pins lives on the log. The implementing PR amends the two retaining RFCs' scope lines per [implemented/AGENTS.md](../../implemented/AGENTS.md): the boundary RFC's retained-list entry and the stream-chunk RFC's "no durable twin" clause.
|
||||
|
||||
## Why not keep it?
|
||||
|
||||
"It is a control signal, not a boundary" — but the taxonomy's operative distinction is mirrored-vs-live-only, not control-vs-boundary, and this event mirrors. A consumer that wants enqueue-time notification has `agent/queued` (with its steering flag); a consumer that wants drain-time notification is by definition asking for the moment `steering/message` is appended, which `session/event` delivers with the same payload plus durability. The rejected [retire-mid-turn-steering RFC](../../rejected/simplification/2026-06-20-retire-mid-turn-steering.md) defended the steering *capability* — `steer()`, the durable event, continuation forcing — all of which this removal keeps untouched.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- No `agent/steering` spelling outside this RFC and the two amended RFCs; the catalog is regenerated and fresh.
|
||||
- The retargeted test pins source preservation on `steering/message`; the suite is green.
|
||||
|
||||
## Risks
|
||||
|
||||
None known: zero production listeners exist to migrate, and both live-notification needs (enqueue, drain) have surviving homes (`agent/queued`, `session/event`).
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Share the app bins' boot glue instead of maintaining twin copies
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`packages/ui/stdio-agent/src/bin.ts` and `packages/ui/acp-agent/src/bin.ts` carry four near-twin helpers — `loadEnv`, `installFailLoud`, `assertEntriesLoaded`, `boot` — whose bodies differ essentially in the diagnostic prefix, plus two copies of the hardest-won boot lore in the repo: the `Promise.allSettled` swallow inside `loader.await()`, the silent-exit-0 import-failure guard, and the `--expose-internals` resolution note (the failure classes behind AGENTS.md's "real entry path means the published artifact" pattern). Drift has already begun: `boot(configPath)` resolves the path internally in one bin but requires a pre-resolved absolute path in the other, and the twin JSDoc prose has forked.
|
||||
|
||||
The duplication is aggravated by a coverage hole: all of this logic sits OUTSIDE the per-file 100% gate — `vitest.config.ts` excludes `packages/*/*/src/bin.ts` because importing a self-executing bin (top-level `await main()`) runs it — which also makes the `export` keywords on these helpers decorative: no spec can import them, so the only exercisers are the subprocess smokes, and the two `built-bin.e2e.ts` suites duplicate their temp-node_modules scaffolding as well. The genuinely per-app pieces are small and real: the ACP bin owns snapshot-mode config selection (`resolveConfigPath`), replay-mode env skipping, the stdin-EOF dispose lifecycle, and stdout purity; the stdio bin owns nothing extra.
|
||||
|
||||
## Proposal
|
||||
|
||||
Extract the four helpers, parameterized by the bin's diagnostic prefix, into an importable non-bin module shared by both apps — a small published package in the `ui` group (the bins are published artifacts, so their runtime dependency must be published too, not `support/`). Each `bin.ts` becomes a thin self-executing `main()` plus its app-specific glue. The shared module gains unit tests and falls under the coverage gate; the loader-failure lore gets one home; the subprocess smokes remain the artifact-level guard — the published-bin smoke is NOT replaced by unit tests, per the "real entry path" defensive pattern. The implementing PR amends the [extract example app packages RFC](../../implemented/architecture/2026-06-20-extract-example-app-packages.md)'s facts ("boot glue moved into that bin, owned by the app" is the sentence that changes).
|
||||
|
||||
## Why not keep the duplication?
|
||||
|
||||
The bins were framed as independently-owned published artifacts, and a new package carries fixed overhead (manifest, README, tsconfig reference, publint surface) that rivals the deduplicated line count. But app-vs-app sharing was never weighed by that RFC — it consolidated three example `start.ts` copies INTO the bins and stopped there; the drift is now observed fact rather than speculation; and the coverage-gap argument is independent of the dedup argument: this is the only nontrivial runtime logic in the repo exempt from the per-file 100% gate. The alternative of a copy-by-convention shared source file is the current state with extra steps.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- The four helpers exist once, unit-tested, under the coverage gate; both bins are thin mains plus app-specific glue.
|
||||
- Both built-bin smokes still pass under plain node in the node_modules-shaped temp dir, including the missing-config non-zero exit.
|
||||
- The app-packages RFC's facts are amended in the same change.
|
||||
|
||||
## Risks
|
||||
|
||||
Churn in two published bins and one new package boundary; the shared module must stay dependency-light (cordis plus the loader). If the implementing PR finds the package overhead genuinely exceeds the dedup — the honest failure mode of this proposal — the fallback that still pays is extracting only the coverage-exempt pure logic (`assertEntriesLoaded`, `resolveConfigPath`) into an importable module within each app package, ending the coverage exemption without a new package.
|
||||
@@ -0,0 +1,32 @@
|
||||
# RFC: Tighten the hook-protocol contract — dialect, discarded fields, double defaults, and lib-owned `hook/result` semantics
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
Five pieces of the `dsh-hook-protocol`/bridge contract miss the discipline the [subagent-observe-enrich RFC](../../implemented/feature/2026-06-30-subagent-observe-enrich.md) records — it dropped an `agentType` lifecycle field for lacking a consumer, and these fail the same test:
|
||||
|
||||
1. **`HookDialect`'s `'native'` variant** (`packages/hooks/hook-protocol/src/types.ts`) has zero producers — the bridges stamp `'claude'` and `'codex'`; the only `'native'` constructor anywhere is the lib's own unit test. The field's own JSDoc defines `dialect` as "the bridge that ran it", and native is not a bridge: the [interception-seams RFC](../../implemented/feature/2026-06-30-interception-seams.md) records that native hooks are not a package and that "a native plugin can already use the typed Decisions" without the durable hook log, and the flagship native-plugin worked example asserts exactly that (no `hook/*` events at all).
|
||||
2. **`HookOutput.suppressOutput`** (same file) is parsed by the codec and discarded on every path: no bridge branch, no merge fold, no warn, no deferred-list row — uniquely among its parsed-but-unhonored siblings, each of which carries a stated deferral (`updatedInput` → a logged warn plus the [pre-tool-input-rewrite proposal](../feature/2026-06-30-pre-tool-input-rewrite.md); `systemMessage` → a logged warn plus a README deferred row; `continue`/`stopReason` → a `TODO(hook-continue-false)` anchor plus the `'stop'` decision record). Structurally there is nothing to suppress: hook stdout never enters any transcript (context flows only via `additionalContext`; the log records only `decision`/`stderrSummary`), so a hook author setting `suppressOutput: true` gets silent nothing with no warn.
|
||||
3. **`hook/result.durationMs`** is durable timing telemetry with no reader. Both bridges write it, and the ACP snapshot normalizer scrubs it to `0` because wall-clock hook runtime is replay noise (`examples/acp-agent/tests/snapshot-normalize.ts`); the remaining consumers are tests and the goldens that exist because the field exists. Deterministic provenance fields (`point`, `matcher`, `turn`, `handlerId`) earn their durability as audit facts; a nondeterministic field that replay must erase and nothing reads earns neither its bytes nor its special-case scrub.
|
||||
4. **`defaultTimeoutMs` is double-defaulted in both bridge configs** — a schema `.default(600_000)` AND a `?? 600_000` fallback (`packages/hooks/hooks-claude/src/index.ts`, `packages/hooks/hooks-codex/src/index.ts`) — the same two-homes-for-one-literal shape the ACP bridge's `TODO(double-default)` flags, for a knob no shipped config sets; the per-hook `timeoutSec` is the real timeout surface.
|
||||
5. **The `hook/result` semantics live in the bridges, twice, not in the lib that owns the event.** `summarize()` — the 500-character stderr truncation rule — is byte-identical in `packages/hooks/hooks-claude/src/index.ts` and `packages/hooks/hooks-codex/src/index.ts`, and so is the decision-string rule `output.decision ?? (output.continue === false ? 'stop' : 'pass')`; yet `dsh-hook-protocol` declares `hook/result`, documents `stderrSummary` as "truncated" without owning the truncation, and documents the decision values without owning the mapping. If one bridge drifts (a different cap, a different fallback), the shared durable event's semantics fork silently.
|
||||
|
||||
## Proposal
|
||||
|
||||
Narrow `HookDialect` to `'claude' | 'codex'` and fix its JSDoc; retarget the lib's one `'native'` test. Drop `suppressOutput` from `HookOutput`, the codec's parse lines, its codec-test assertions, and the parsed-superset lists in the lib README and [hook-protocol-lib RFC](../../implemented/feature/2026-06-30-hook-protocol-lib.md) (amended per [implemented/AGENTS.md](../../implemented/AGENTS.md)). Drop `durationMs` from `HookResultRecord`, `RunHookResult`, the `hook/result` event, the bridge appends, the docs/catalog, and the snapshot normalizer's special-case scrub (retiring `runHook`'s injected clock if nothing else needs it); the hook goldens refresh mechanically as the scrubbed field disappears. Replace the bridges' `defaultTimeoutMs` config knob with one shared reference-default constant in `dsh-hook-protocol` (per-hook `timeoutSec` stays the override surface). Move the `hook/result` semantics into the lib: `appendHookResult` (or a helper it exposes) derives `stderrSummary` and the decision string from the `HookOutput` + exit outcome, and both bridges delete their private copies. Rider: un-export `BLOCKING_EXIT_CODE` (zero importers; even the codec tests spell the literal `2`).
|
||||
|
||||
## Why not keep them?
|
||||
|
||||
The [hook-protocol-lib RFC](../../implemented/feature/2026-06-30-hook-protocol-lib.md) deliberately records "parses the full CC superset" — the strongest counterargument is that this proposal re-litigates decisions that RFC records. But parsing a field whose value can never influence anything is not protocol faithfulness, it is a reader trap; a dialect variant that the design's own thesis says will never be stamped is vocabulary without an interpreter; and durable telemetry that replay must scrub is a cost with no buyer. Each returns trivially with its first real consumer (a transcript surface with hook stdout to suppress; a native-provenance feature that logs hook events; a trace viewer that reads timings — as live diagnostics or a deliberately durable telemetry event designed for it). On item 5, the lib RFC chose per-bridge explicitness over a parameterized engine — but that choice governed payload construction and Decision mapping; the semantics of the SHARED durable event are precisely the "primitives where duplication would actually be dangerous" that the same RFC assigns to the lib.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `HookDialect` is two-valued; `rg "'native'"` in the hooks packages returns only this RFC's amended references.
|
||||
- `suppressOutput` and `durationMs` appear nowhere in source, parsed-field doc lists, the catalog, or the normalizer; the hook goldens are re-recorded or refreshed without the field.
|
||||
- Both bridge configs lose `defaultTimeoutMs`; the reference default lives once, in the lib; per-hook `timeoutSec` still overrides it.
|
||||
- One definition each of the truncation rule and the decision-string rule, in `dsh-hook-protocol`, exercised by both bridges' suites.
|
||||
|
||||
## Risks
|
||||
|
||||
The `dialect`, `suppressOutput`, `defaultTimeoutMs`, and semantics changes are invisible on the wire and in the goldens; the `durationMs` removal churns the hook goldens once (a mechanical refresh — the field was already normalized to a constant). The cost is churn in `dsh-hook-protocol` and both bridges — cheap under the pre-release stance, and cheaper than letting two copies of a durable event's semantics age apart.
|
||||
@@ -0,0 +1,27 @@
|
||||
# RFC: Trim unreachable ACP bridge surface — the branding knobs and the kind-sniffing fallback
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
Two pieces of `dsh-acp` surface are unreachable from any shipped configuration:
|
||||
|
||||
1. **`AcpConfig.agentName` / `agentVersion`** (`packages/ui/acp/src/index.ts`). The shipped app package hands the bridge only `{ model, systemPrompt }` (`packages/ui/acp-agent/src/index.ts`), so no leaf `cordis.yml` — the only production config surface — can set the knobs at all; they are settable solely by direct-mounting the bridge, which only a unit test does. Every snapshot golden — the hook-matrix scenarios included — pins the schema defaults (`deepseek-harness-acp` / `0.0.1`). The pair also carries a live `TODO(double-default)`: the literals exist twice (schema `.default(...)` plus `??` fallbacks), with the TODO asking to pick one home.
|
||||
2. **The `toolKindFor` name heuristic** (same file) special-cases `bash*`/`read*`/`write`/`edit*` tool names in the generic-fallback path. Since the [render-intent union](../../implemented/architecture/2026-07-02-tool-render-intent-union.md), every first-party tool those arms match ships its own `presentCall` carrying its kind, and the presenter-less production tools (`subagent`, `subagent_fork`) fall through to `other` anyway. The arms are production-reachable only when a tool's `presentCall` THROWS (the containment fallback) — and the bridge's own module doc states the design rule the heuristic violates: "the bridge never special-cases tool names".
|
||||
|
||||
## Proposal
|
||||
|
||||
Hardcode `agentInfo` at the `initialize` site (`{ name: 'deepseek-harness-acp', version: '0.0.1' }`), deleting the two config fields, their schema defaults, the `??` fallbacks, and the `TODO(double-default)` whose subject vanishes; drop the knob half of the direct-mount config test, the two rows in `packages/ui/acp/README.md`, and the `packages/ui/acp/acp-feature-support.md` cell that cites the knobs. Zero golden churn — the emitted wire value is unchanged. Replace `toolKindFor` with the constant `'other'` in both fallback sites (the presenter fallback and `nullToolPresenter`) and delete the heuristic with its test rows.
|
||||
|
||||
## Why not keep them?
|
||||
|
||||
`agentInfo` is client-visible branding a deployment will eventually want configurable — but a knob no shipped config can reach is not configurability, it is drift surface (the double-default TODO is its symptom), and the honest re-add must include the `dsh-acp-agent` plumb-through that does not exist today either; both arrive together with the deployment that needs them. For the heuristic: a hypothetical third-party presenter-less tool named `read_docs` would lose its inferred `read` icon — but inferring kinds from unknown plugins' names is exactly the special-casing the render-intent design rejected. The behavior delta on shipped paths is confined to the presenter-throw fallback, where rendering kind `other` makes the client show the raw input instead of a masquerading first-party card — strictly better diagnostics for a broken presenter.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `agentName`/`agentVersion` and `toolKindFor` appear only in this RFC; snapshot goldens are byte-identical; bridge tests are green with the constant fallback.
|
||||
- The `initialize` handshake continues to report `deepseek-harness-acp`/`0.0.1` (pinned by the handshake snapshot).
|
||||
|
||||
## Risks
|
||||
|
||||
None beyond the presenter-throw rendering delta described above — an error path whose new behavior is more diagnosable than the old.
|
||||
@@ -1,31 +0,0 @@
|
||||
# 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/testing/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,26 @@
|
||||
# RFC: Single-source the acp-agent replay config
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`examples/acp-agent` ships two hand-maintained configs: `cordis.yml` (the live tree) and `cordis.snapshot.yml` (the keyless replay tree). Stripped of comments and blanks, their entire difference is ONE plugin entry — the eight-line `llm-deepseek` stanza (with its `!!js` env keys and model list) versus the two-line `llm-replay` stanza. Every other entry is byte-identical, including the multi-line system prompt and both hook-bridge stanzas. Every app-shape change must therefore be made twice, and the [hook-snapshot-matrix RFC](../../implemented/testing/2026-07-04-hook-snapshot-matrix.md) records paying exactly that tax: "hence the symmetric edit to both configs".
|
||||
|
||||
Nothing gates the symmetry. If the copies drift, the snapshot tier silently exercises a different app than the one that ships — the ["green units, broken product" class of gap](../../../postmortem/0001-acp-default-export-drops-inject.md) the snapshot tier exists to close, reintroduced one level up, with reviewer vigilance as the only defense.
|
||||
|
||||
## Proposal
|
||||
|
||||
Make the replay tree derive from the live tree instead of mirroring it. Preferred endpoint: a single source — either `cordis.snapshot.yml` becomes a thin overlay that includes `cordis.yml` and swaps only the llm entry (if the vendored loader/include config supports entry-level override), or the acp-agent bin's existing `DSH_SNAPSHOT=replay` branch performs the one-entry swap on the parsed config and `cordis.snapshot.yml` is deleted. Fallback endpoint, if single-sourcing is judged too magical for a teaching example: keep both files and add a boring verify gate (in the `doc-sync`/`hygiene` family) asserting the two configs' entry sets are equal modulo the llm entry. The implementing PR picks after checking the loader's include/override capability, updates the recording docs, and amends the snapshot RFCs' facts per [implemented/AGENTS.md](../../implemented/AGENTS.md).
|
||||
|
||||
## Why not keep the twin?
|
||||
|
||||
An explicit replay file is transparently readable and teaches replay semantics — the strongest counterargument, and the reason the fallback keeps the file and adds only the gate. YAML surgery inside the published bin is real complexity in a shipping artifact, and an include-overlay depends on loader capability that may not exist. But the status quo — a 125-line hand-maintained near-copy of a 141-line file whose one meaningful difference is two lines, defended by nothing — is the one option with a silent failure mode, and it grows with every plugin the app gains (the hook-bridge stanzas are twins in both files).
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- Either one config file plus a mechanical llm-entry swap exercised by the snapshot suite itself, or two files plus a symmetry gate that fails CI on any non-llm divergence.
|
||||
- All snapshot scenarios (hook matrix included) pass unchanged; `pnpm run test:snapshot:record` still boots the live tree.
|
||||
|
||||
## Risks
|
||||
|
||||
The include-overlay shape may be unsupported by the vendored loader — then the bin-side swap or the gate. `echo-agent`/`coding-agent` are unaffected (no snapshot twin). If the gate route is chosen, it is one more bespoke verify script — the cost the repo's gate-friendly policy explicitly accepts for encoding an invariant no human reliably remembers.
|
||||
Reference in New Issue
Block a user