Coder/deepseek-harness
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(rfc): address Codex review findings on the RFC batch

Browse Source 
Two overstated evidence claims, reworded to the precise fact:
- prune-dead-core-spine-surface: runLoop has no importer OUTSIDE the
  package (its own agent.ts drives the loop with it); the dead surface
  is the public re-export, not the function.
- generic-long-running-tool-runtime census: BashTask.done has no
  consumer through the public seam, but is production-load-bearing
  inside dsh-bash-local (disposal awaits it); only the public
  completion surface is single-consumer.

Also fold the reviewer-located doc sites into the removal sets so the
implementing PRs need no re-discovery: the llm/pi-ai/compact-basic
README rows and the adding-an-llm-adapter cookbook line (prefill/image),
the content-block-vocabulary RFC's has-a-home consequence lines
(cache/prefill/image), the tools.md paste + type-equiv manifest row +
tools README row (callId), and the session-surface RFC's
full-rebuild-after-replacement sentence (invalidate).
This commit is contained in:
Tianyi Cui
2026-07-04 04:21:38 +08:00
parent e13bbcb5d5
commit 9389c5df19
5 changed files with 6 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md
View File

@@ -24,7 +24,7 @@ The runtime should own:
## 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.
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()` 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 consumer uses only the latter: 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

2
docs/rfc/proposed/simplification/2026-07-04-drop-image-content-block.md
View File

@@ -8,7 +8,7 @@ Status: proposed
## 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.
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 block list in `packages/llm/llm/README.md`, the pi-ai README's images-not-representable row, the compact-basic README's image-estimation row, 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?

2
docs/rfc/proposed/simplification/2026-07-04-drop-inert-request-knobs.md
View File

@@ -13,7 +13,7 @@ Both knobs are adapter-symmetric, so removal sheds them from both twins together
## 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 `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`.

4
docs/rfc/proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md
View File

@@ -7,12 +7,12 @@ Status: proposed
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).
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. 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.
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. 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 `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.

2
docs/rfc/proposed/simplification/2026-07-04-prune-producerless-vocabulary-variants.md
View File

@@ -12,7 +12,7 @@ The merge-extensible vocabulary maps are designed to grow by declaration merging
## 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.
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, 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.