mirror of
https://github.com/deepseek-ai/deepseek-harness
synced 2026-08-15 21:04:50 +00:00
docs(rfc): propose nine simplification RFCs from a five-domain survey
Survey of master for surface area whose consumers are tests/docs only, classified per candidate (production vs non-production corpus, rg + call-site reads). New proposed/simplification RFCs: - prune-producerless-vocabulary-variants: CacheHint/cache? fields, MessageSource 'agent', TurnTrigger 'continuation' (the TurnEndReasonMap omitted-until-emitted policy, applied) - drop-inert-request-knobs: GenerateOptions.prefill (both adapters throw UNSUPPORTED), ToolSchema.strict (zero setters; beta-URL-only feature) - drop-web-providers-change-event: the llm/adapter-change precedent replayed - drop-image-content-block: no producer; every consumer silently drops it - prune-write-only-fs-surface: fs-local STREAM_MIN_SIZE/streamMinSize, FsTarget.inputPath, FsEditOutcome.replacements/replaceAll, FileReadOutcome.limit/version - prune-unimplemented-subagent-vocabulary: outputSchema/structured, toolFilter, sendMessage/resume (depthLimit stays) - trim-acp-bridge-unreachable-surface: agentName/agentVersion knobs (resolves TODO(double-default)), toolKindFor name-sniffing - prune-dead-core-spine-surface: SurfaceManager.invalidate(), runLoop/Inbox exports, ToolExecutionResult.callId - share-app-bin-boot-glue: the twin coverage-exempt bin helpers Also supplements three existing proposed RFCs with survey evidence: the bash seam consumption census (generic-long-running-tool-runtime), three more static inventories (discover-package-inventory), and the bridge's already-1:1 id usage (unify-agent-and-session-id).
This commit is contained in:
Tianyi Cui
@@ -51,6 +51,15 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r
|
||||
|---|---|
|
||||
| [Unify the agent id and the session id](proposed/simplification/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 |
|
||||
| [Stop mirroring durable boundaries as agent events](proposed/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) | 2026-06-20 |
|
||||
| [Prune producer-less vocabulary variants (block cache hints, the `agent` message source, the `continuation` turn trigger)](proposed/simplification/2026-07-04-prune-producerless-vocabulary-variants.md) | 2026-07-04 |
|
||||
| [Drop `GenerateOptions.prefill` and `ToolSchema.strict` — request knobs with no working end-to-end path](proposed/simplification/2026-07-04-drop-inert-request-knobs.md) | 2026-07-04 |
|
||||
| [Drop the unconsumed `web/providers-change` event](proposed/simplification/2026-07-04-drop-web-providers-change-event.md) | 2026-07-04 |
|
||||
| [Drop the `image` content block until a path can honor it](proposed/simplification/2026-07-04-drop-image-content-block.md) | 2026-07-04 |
|
||||
| [Prune write-only fields and a dead routing knob from the fs seam](proposed/simplification/2026-07-04-prune-write-only-fs-surface.md) | 2026-07-04 |
|
||||
| [Prune the unimplemented subagent seam vocabulary](proposed/simplification/2026-07-04-prune-unimplemented-subagent-vocabulary.md) | 2026-07-04 |
|
||||
| [Trim unreachable ACP bridge surface — the branding knobs and the kind-sniffing fallback](proposed/simplification/2026-07-04-trim-acp-bridge-unreachable-surface.md) | 2026-07-04 |
|
||||
| [Prune dead core-spine surface — `SurfaceManager.invalidate()`, the loop-internal exports, `ToolExecutionResult.callId`](proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md) | 2026-07-04 |
|
||||
| [Share the app bins' boot glue instead of maintaining twin copies](proposed/simplification/2026-07-04-share-app-bin-boot-glue.md) | 2026-07-04 |
|
||||
|
||||
### Architecture
|
||||
|
||||
|
||||
@@ -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 (`packages/bash/tool-bash/src/index.ts`) consumes `resolve`, `run`, `start`, `ownerOf`, `readOutput`, `kill`, and `onTaskDone`. `get()`/`list()` and the per-task `BashTask.done` promise have test-harness consumers only — `get()`/`list()` 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), and `done` doubles as `dsh-bash-local`'s dispose-to-quiescence primitive. The seam therefore carries two public completion representations — the per-task promise and the global `onTaskDone` listener registry — of which production consumes one: the runtime should pick exactly one public completion surface and record which. One shape wart for the split to dissolve: `BashExecSpec.timeoutMs` is required but ignored by `start()`, an artifact of sharing one spec type between foreground and background execution.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- The bash-specific packages no longer define the generic task registry, owner-token authorization, polling, cancellation, or completion-notification machinery.
|
||||
|
||||
@@ -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`. `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 gate 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`).
|
||||
|
||||
## 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
|
||||
|
||||
|
||||
@@ -14,7 +14,7 @@ The agent factory carries TWO ids for what is, in every live consumer, one thing
|
||||
- **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`.
|
||||
|
||||
Everywhere a live consumer actually looks an agent up — the **ACP bridge, the only production path** — the two are already unified: `agentId === sessionId === <uuid>`.
|
||||
Everywhere a live consumer actually looks an agent up — the **ACP bridge, the only production path** — the two are already unified: `agentId === sessionId === <uuid>`. Concretely, both bridge factory call sites brand `AgentId(sessionId)` directly, and the bridge's reverse lookup keys on the `Agent` object itself — there is no id translation anywhere in the bridge to migrate.
|
||||
|
||||
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.
|
||||
|
||||
|
||||
@@ -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, and the explicit skip/estimate branches in the deepseek serializer, the pi-ai converter, the ACP codec's outbound mapping, and compact-basic — the default arms those switches already carry for plugin-added block types absorb the cases. Update the vocabulary line in [architecture.md](../../../architecture.md), 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; 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), and the adapter README rows documenting the rejection.
|
||||
- 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
|
||||
|
||||
A hooks/config plugin arriving via the interception seams may want to set request fields — it will 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,28 @@
|
||||
# RFC: Drop the unconsumed `web/providers-change` event
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
`WebService` declares and emits `web/providers-change` (`packages/web/web/src/index.ts`) on every provider registration and disposal, and orders each registration effect's rollback yield 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). The remaining references are the generated catalog and README/doc prose.
|
||||
|
||||
The seam's own design removed the natural consumer. `dsh-tool-web` registers tools by product ENABLEMENT, deliberately not by provider availability (`packages/web/tool-web/src/index.ts`), and `searchStatus()`/`fetchStatus()` are derived per call, never cached — so there is no cache to invalidate and no registration set to recompute when providers come and go. HMR cleanup is already carried by the effect disposers themselves.
|
||||
|
||||
This is shape-for-shape the surface the repo already cut once: [drop the unconsumed `llm/adapter-change` event](../../implemented/simplification/2026-06-20-drop-unconsumed-llm-adapter-change-event.md) removed the same notification, 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.
|
||||
|
||||
## Proposal
|
||||
|
||||
Delete the event declaration, both emits, and the rollback-before-emit ordering (the plain `ctx.effect` disposer keeps HMR cleanup); delete the two event tests; run `pnpm run gen-cordis-catalog` and commit the regenerated catalog; update `packages/web/web/README.md` and the [web.md](../../../core-data-structures/web.md) prose. The implementing PR amends the [web capability seam RFC](../../implemented/architecture/2026-06-24-web-capability-seam.md)'s facts (it specifies the event in its interface sketch and test list), per [implemented/AGENTS.md](../../implemented/AGENTS.md).
|
||||
|
||||
## Why not keep it?
|
||||
|
||||
The web seam RFC specified the event deliberately — days after the adapter-change removal — as a minimal HMR-visibility signal. But the same RFC also made every status read derived-on-call and tool registration availability-independent, which is precisely why no consumer can need the signal: the design's other choices starved this one. Per AGENTS.md "RFCs are proposals, not golden truth", the event is the part of that proposal the code has since shown to over-reach; validating it against the repo's own precedent yields the verdict the precedent already recorded.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- No `providers-change` spelling outside this RFC and the amended seam RFC; the catalog is regenerated and fresh (`verify-cordis-catalog` green).
|
||||
- Registration/disposal HMR-safety tests still prove cleanup via `searchStatus()`/`fetchStatus()` derivation rather than via the event.
|
||||
|
||||
## Risks
|
||||
|
||||
A future provider-picker UI or diagnostics panel that wants live change notifications re-adds the event with that consumer — the identical judgment, and its reversal condition, is already recorded on the llm precedent.
|
||||
@@ -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 zero importers anywhere; `Inbox`/`InboxMessage` are imported only by 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. The loop deliberately ignores it and documents it as a footgun — the correlation id must be the loop's own `call.id`, because a `tools/execute` waterfall listener returning a mismatched id would otherwise orphan the call↔result pairing — and a regression test exists solely to prove the field is ignored. So every waterfall short-circuiter must fabricate a field whose only power is to be a bug if trusted; 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, and `toolErrorResult`, along with the loop's ignore-comment and the proves-ignored regression test — the hazard they guard disappears with the field.
|
||||
|
||||
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 `callId` removal waits for the in-flight interception-seams work that splits `tools/execute` into pre/post phases and currently carries the field verbatim — the argument transfers unchanged (post-execute listeners receive the execution object alongside the result), so the removal targets whichever seam shape is on master when implemented.
|
||||
|
||||
## 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
|
||||
|
||||
- The three surfaces appear only in this RFC; the agent-loop README lists only the consumed public surface; the inbox spec imports the source module.
|
||||
- The tools/execute contract tests pass with the shrunk result type; no waterfall test fabricates a `callId` on a result.
|
||||
|
||||
## Risks
|
||||
|
||||
All three are compile-visible removals with no runtime behavior change on any shipped path. The `callId` change lands on whatever execute-seam shape is current, as noted under sequencing.
|
||||
@@ -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 writers are two hand-built test fixtures that need an arbitrary non-message trigger (`packages/support/llm-replay/tests/llm-replay.spec.ts`, `packages/support/ui-stdio/tests/ui-stdio.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 two test fixtures to `injection` triggers (any non-`message` trigger serves their 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.
|
||||
|
||||
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 two fixtures assert the same replay behavior with `injection` triggers; the suite is green.
|
||||
|
||||
## Risks
|
||||
|
||||
None operational — nothing can construct these values today. The in-flight event-taxonomy work reworks the transient `agent/*` mirror events, not the durable vocabulary declarations, so there is no collision. 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,33 @@
|
||||
# RFC: Prune the unimplemented subagent seam vocabulary
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
The [subagent seam](../../implemented/feature/2026-06-21-subagent-capability-seam.md) shipped a two-tier capability design: start-time capability flags checked by the service, and optional runtime methods on `SubagentRun`. Three start-time features and both optional runtime methods have zero implementations and zero callers:
|
||||
|
||||
- **`outputSchema`/`structured` and `toolFilter`** (`SubagentCapabilities`, `SubagentStartRequest`, `SubagentResult` in `packages/subagent/subagent/src/types.ts`): every real provider declares `outputSchema: false, toolFilter: false` (`packages/subagent/subagent-spawn/src/index.ts`, `packages/subagent/subagent-fork/src/index.ts`, `packages/subagent/subagent-acp/src/index.ts`); the sole production `ctx.subagents.start` caller (`packages/subagent/tool-subagent/src/index.ts`) builds `{ prompt, parent, signal?, agentOptions? }` and structurally cannot set either; `structured` is produced only by the test mock (`packages/support/subagent-mock`) for its own spec. The service's capability check carries two assert rows whose only exercisers are the rejection tests.
|
||||
- **`SubagentRun.sendMessage` / `SubagentRun.resume`** (same file): implemented by NO provider — not even the mock; the spawn spec asserts their *absence*.
|
||||
|
||||
The only reason `dsh-subagent` depends on `dsh-tools` at all is `outputSchema`'s `SchemaSpec` type. Three subsequent subagent workstreams (per-session snapshot replay, the fork seed boundary, the ACP backend) landed around this surface without growing a single consumer.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove `outputSchema`/`structured`, `toolFilter`, `sendMessage`, and `resume` from the seam; shrink `SubagentCapabilities` to `{ depthLimit }`; drop the two capability-assert rows, the all-false flags on the three providers, the mock's structured branch and its `capabilities`/`structured` config knobs, and the tests that exist to pin the removed surface (the two rejection rows, the spawn absence test, the mock structured specs). Drop the `dsh-tools` peer/dev dependency from `packages/subagent/subagent/package.json`. Update the [subagent.md](../../../core-data-structures/subagent.md) pastes and the type-equiv manifest, and the README rows in `packages/subagent/subagent`, `packages/subagent/subagent-spawn`, `packages/subagent/subagent-fork`, and `packages/support/subagent-mock`. The implementing PR amends the seam RFC's capability catalog per [implemented/AGENTS.md](../../implemented/AGENTS.md).
|
||||
|
||||
**Keep** `depthLimit`/`maxDepth` and the capability-check mechanism itself: the in-process backend genuinely enforces the cap (`SubagentDepthError` in `packages/subagent/subagent-inprocess/src/index.ts`), recursion is the seam RFC's named risk, and one live capability row keeps the two-tier design demonstrated rather than merely remembered.
|
||||
|
||||
This is the seam-vocabulary echo of [prune dead methods from the persistence seam](../../implemented/simplification/2026-06-20-prune-dead-seam-methods.md): members every implementation must declare for nobody — weaker even, since here zero implementations exist.
|
||||
|
||||
## Why not keep it?
|
||||
|
||||
The two-kinds-of-capability design is the seam RFC's headline, and re-adding `outputSchema` later touches several files. But the design survives with `depthLimit` as its live example and the RFCs as its record, and the seam RFC itself concedes the shipped `toolFilter` shape is wrong (real enforcement needs a `tools/execute` veto, not schema filtering) — re-adding against a real implementing provider will pin a better contract than the current speculative one.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- The removed spellings appear only in this RFC and the amended seam RFCs; `SubagentCapabilities` is `{ depthLimit: boolean }`; the `dsh-tools` dependency edge is gone (`hygiene` green).
|
||||
- Depth-enforcement tests are unchanged and green.
|
||||
|
||||
## Risks
|
||||
|
||||
The in-flight hooks stack enriches subagent lifecycle event payloads (agent type, last assistant message) — adjacent files, no field overlap; coordinate landing order mechanically. Worth recording while here: nothing production sets `maxDepth` today (`tool-subagent` exposes no knob for it), so in-process recursion is uncapped — wiring the depth machinery this RFC keeps is a small feature gap, and an argument for keeping it, not for cutting it.
|
||||
@@ -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 spellings appear only in this RFC; 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,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,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 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.
|
||||
Reference in New Issue
Block a user