diff --git a/docs/rfc/README.md b/docs/rfc/README.md index 4d1f6aecdb..f427e03a4a 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -53,7 +53,7 @@ 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 | | [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 unconsumed web observation surface — the `providers-change` event and the status methods](proposed/simplification/2026-07-04-drop-unconsumed-web-observation-surface.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 | @@ -61,12 +61,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [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 | | [Remove the `agent/steering` mirror emit](proposed/simplification/2026-07-04-remove-agent-steering-mirror.md) | 2026-07-04 | -| [Tighten the hook-protocol contract — the `native` dialect, `suppressOutput`, and lib-owned `hook/result` semantics](proposed/simplification/2026-07-04-tighten-hook-protocol-contract.md) | 2026-07-04 | -| [Narrow the subagent seam to synchronous collect](proposed/simplification/2026-07-04-narrow-subagent-synchronous-collect.md) | 2026-07-04 | -| [Drop idle registry and status observation surfaces](proposed/simplification/2026-07-04-drop-idle-registry-observation-surfaces.md) | 2026-07-04 | -| [Remove defaults from the tool-schema DSL](proposed/simplification/2026-07-04-remove-tool-schema-defaults.md) | 2026-07-04 | -| [Trim unused hook protocol and bridge surface](proposed/simplification/2026-07-04-trim-hook-protocol-surface.md) | 2026-07-04 | -| [Narrow the pre-tool gate to shipped behavior](proposed/simplification/2026-07-04-narrow-pre-tool-gate.md) | 2026-07-04 | +| [Tighten the hook-protocol contract — dialect, discarded fields, double defaults, and lib-owned `hook/result` semantics](proposed/simplification/2026-07-04-tighten-hook-protocol-contract.md) | 2026-07-04 | | [Fold the stdio UI helper into the stdio app](proposed/simplification/2026-07-04-fold-stdio-ui-helper.md) | 2026-07-04 | ### Architecture diff --git a/docs/rfc/proposed/feature/2026-06-30-pre-tool-input-rewrite.md b/docs/rfc/proposed/feature/2026-06-30-pre-tool-input-rewrite.md index 4c91b0c4a4..3e731af4b9 100644 --- a/docs/rfc/proposed/feature/2026-06-30-pre-tool-input-rewrite.md +++ b/docs/rfc/proposed/feature/2026-06-30-pre-tool-input-rewrite.md @@ -16,7 +16,7 @@ In the loop, a tool call's arguments are committed to the log and read by live c 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.) +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) diff --git a/docs/rfc/proposed/process/2026-07-04-generate-rfc-index-tables.md b/docs/rfc/proposed/process/2026-07-04-generate-rfc-index-tables.md index f68d34f2eb..b3c88c47ad 100644 --- a/docs/rfc/proposed/process/2026-07-04-generate-rfc-index-tables.md +++ b/docs/rfc/proposed/process/2026-07-04-generate-rfc-index-tables.md @@ -4,40 +4,24 @@ Status: proposed ## Problem -`docs/rfc/README.md` is hand-maintained even though the repo already has a machine-readable RFC layout: every RFC lives at `docs/rfc/{lifecycle}/{class}/yyyy-mm-dd-topic.md`, and `scripts/verify-rfc-classification.ts` walks that tree to verify structure and index completeness. The current gate prevents drift, but every new RFC still edits the same README tables by hand. +`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 stacked hook work made the cost visible. PR #138 added implemented feature/testing/process rows while this simplification sweep added proposed simplification rows, and the only merge conflict when retargeting the sweep onto #138 was the RFC index table. That is predictable: high-churn proposal waves all touch the same few lines even though the truth is already in filenames and H1 titles. - -[The classification RFC](../../implemented/process/2026-06-20-rfc-classification.md) explicitly rejected auto-generating the README index so the file could stay curated. That was a reasonable first cut, but the repo now has enough RFC volume and stacked-PR churn that the hand-written table is the unstable part, not the curated prose. The verifier already does the expensive parsing; it just 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 in `docs/rfc/README.md`, but generate the per-lifecycle/per-class tables from the filesystem. +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. -- Add a `gen-rfc-index` script (or extend `verify-rfc-classification.ts` with `--write`) that scans RFC files, reads each H1, derives the first-proposed date from the filename, and writes the table rows under stable generated markers for each `## {Lifecycle}` / `### {Class}` section. -- Keep the class set and lifecycle set closed in one script-owned source of truth. -- Make `verify-rfc-classification` check that the generated sections are fresh, analogous to `verify-cordis-catalog`. -- Preserve manually curated prose, classification descriptions, and "when to write one" guidance outside the generated table blocks. -- Update [the classification RFC](../../implemented/process/2026-06-20-rfc-classification.md) to say the earlier "verify, do not generate" choice was superseded after stacked-PR conflicts made the tradeoff worse. +## Why not keep the verifier-only model? -The generated output should stay boring Markdown: the same tables reviewers read today, just mechanically produced from the path + title source of truth. - -## Why not keep the current verifier-only model? - -The current model catches mistakes but still forces every proposal to edit a shared hotspot. A failed verifier is also more annoying than a generator for a purely mechanical row: the author has already named and placed the file correctly, then has to copy the same facts into the index. That is exactly the kind of hand-maintained inventory the repo already proposes removing elsewhere. - -This does not turn the whole README into a build artifact. The prose remains curated. Only the parts whose content is derivable from RFC files become generated. +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 command) rewrites only the generated RFC table regions. -- `pnpm run verify-rfc-classification` fails when those generated regions are stale and passes after regeneration. -- Adding, moving, or deleting an RFC requires editing the RFC file itself; the README rows are produced mechanically. -- The generated rows use each RFC's H1 title and filename date, and preserve the existing lifecycle/class grouping. -- `pnpm run doc-sync` passes after implementation. +- `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 README can be jarring. Use explicit markers and keep the table output minimal so reviewers know what is owned by the script. -- Reading H1 titles makes malformed RFC headers a generator concern. That is useful pressure: a missing or nonstandard H1 should fail clearly. -- This supersedes an implemented process decision. The implementing PR must amend the old classification RFC so the historical record explains why the tradeoff changed. +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. diff --git a/docs/rfc/proposed/simplification/2026-07-04-drop-idle-registry-observation-surfaces.md b/docs/rfc/proposed/simplification/2026-07-04-drop-idle-registry-observation-surfaces.md deleted file mode 100644 index 142972f3c7..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-04-drop-idle-registry-observation-surfaces.md +++ /dev/null @@ -1,55 +0,0 @@ -# RFC: Drop idle registry and status observation surfaces - -Status: proposed - -## Problem - -Several registry services expose "something changed" or "what is registered" observation surfaces with no production observer. The older [LLM adapter-change simplification](../../implemented/simplification/2026-06-20-drop-unconsumed-llm-adapter-change-event.md) removed `llm/adapter-change` because it had declarations, emits, docs, and tests but no listener. The same pattern now exists in the remaining registry-change events: `tools/change`, `system-prompt/change`, and `web/providers-change`. - -`tools/change` is declared by `dsh-tools` and emitted from `ToolRegistry.register()` on register and dispose ([packages/core/tools/src/index.ts](../../../../packages/core/tools/src/index.ts)). `system-prompt/change` is declared by `dsh-system-prompt` and emitted when sections or tool-schema providers register and dispose ([packages/core/system-prompt/src/index.ts](../../../../packages/core/system-prompt/src/index.ts)). `web/providers-change` is declared by `dsh-web` and emitted when search or fetch providers register and dispose ([packages/web/web/src/index.ts](../../../../packages/web/web/src/index.ts)). Grepping those event names outside `docs/rfc/**` finds declarations, emit sites, READMEs, generated catalogs, and tests, but no production listener in `packages/*/src` or examples. - -Those events carry real complexity. Each registry yields a rollback disposer before emitting so a throwing change listener unwinds the just-added entry instead of leaking it into the registry. The packages then carry tests for listener-throw rollback paths that only the unused events can trigger. `web/providers-change` repeated the same pattern after the LLM adapter-change event was already proven unnecessary. - -There is a related one-shot observation surface in `dsh-llm`: `ctx.llm.models()` returns registered model names, but no production caller uses it. Search finds only service docs and tests, including adapter tests that use it as a registration assertion. The shipped model-call path resolves by `options.model` at `ctx.llm.stream()` time; no UI, router, or product config enumerates model names from the service. - -The same "status without observer" pattern now shows up in the web seam. `ctx.web.searchStatus()` and `ctx.web.fetchStatus()` are documented as diagnostics for `dsh-tool-web`, but the current tools execute directly through `ctx.web.search()` and `ctx.web.fetch()` ([packages/web/tool-web/src/search.ts](../../../../packages/web/tool-web/src/search.ts), [packages/web/tool-web/src/fetch.ts](../../../../packages/web/tool-web/src/fetch.ts)). The execution path already resolves the selected provider at call time and throws a structured `WebError` (`WEB_PROVIDER_UNAVAILABLE`, `WEB_PROVIDER_AMBIGUOUS`, `WEB_PROVIDER_CONFIGURED_MISSING`, `WEB_PROVIDER_CONFIGURED_UNAVAILABLE`) when the capability cannot run. The status methods duplicate that selection logic for tests and stale docs, not for a live product surface. - -## Proposal - -Remove the idle registry-observation surfaces that have no production consumer: - -- Delete `tools/change`, its emits, its JSDoc/README/generated-catalog entries, and listener-throw rollback tests. -- Delete `system-prompt/change`, its emits, its JSDoc/README/generated-catalog entries, and listener-throw rollback tests. -- Delete `web/providers-change`, its emits, its JSDoc/README/generated-catalog entries, and listener-throw rollback tests. -- Delete `LlmService.models()` and update LLM adapter/service tests to assert registration behavior through `stream()` resolution, duplicate-registration errors, disposal, or other behavior that a real caller observes. -- Delete `WebService.searchStatus()` / `fetchStatus()` and the `WebCapabilityStatus` contract if no other live type needs it. Web provider `status()` stays internal to provider resolution; callers observe availability by attempting `search()` / `fetch()` and handling `WebError`. - -Registration should remain effect-scoped and HMR-safe: duplicate checks still happen before mutation, the disposer still removes the registered entry, and existing consumers still read the live registry at use time. What disappears is only the speculative observer surface. - -## What stays - -This RFC does not remove live query or execution surfaces. `ctx.tools.schemas()` stays because the system-prompt registry and generated tool catalog use it. `ctx.web.search()` and `ctx.web.fetch()` stay because they are the model-facing web tools' execution path and they already carry the provider-selection error taxonomy. `ctx.agents.list()`, `ctx.sessions.list()`, and `ctx.sessionPersistence.list()` stay because production code uses them for background-task ownership, invariant seeding, write coordination, and ACP load-cwd validation. - -This RFC also does not touch live event seams such as `llm/stream`, `tools/execute`, `system-prompt/assemble`, `session/event`, `session/flush`, `agent/status`, or `fs/*`. Those have production listeners or are the documented extension points the architecture depends on. - -## Why not keep them for a future UI? - -A live tool palette, prompt-section inspector, web-provider status panel, or model picker might eventually want registry-change signals or status queries. But none exists today, and the current event payloads/status shapes are so minimal that a real UI would likely need to revisit them anyway. A future observer can reintroduce the smallest signal it actually consumes, with tests that prove the observer sees it. - -The pre-release stance cuts in favor of narrowing now. A public event with no listener is still API surface; if it survives until release, every later cleanup has to decide whether external consumers might be relying on it. - -## Acceptance criteria - -- `rg "tools/change|system-prompt/change|web/providers-change" packages examples docs --glob '!docs/rfc/**'` finds no remaining declared event, emit, README row, generated-catalog entry, or test outside historical RFC text. -- `rg "ctx\\.llm\\.models\\(|\\.models\\(\\)" packages/llm packages/core/agent-loop examples docs --glob '!docs/rfc/**'` finds no remaining `LlmService.models()` API use or docs entry. -- `rg "searchStatus|fetchStatus|WebCapabilityStatus" packages/web docs --glob '!docs/rfc/**'` finds no remaining public web status surface, docs entry, generated-catalog entry, or tests except provider-private status concepts that still feed execution. -- Registration/disposal tests still prove HMR cleanup for tools, prompt sections/tool providers, web providers, and LLM adapters without depending on observer events. -- The [web capability seam RFC](../../implemented/architecture/2026-06-24-web-capability-seam.md), package READMEs, the Cordis catalog, and core data-structure docs are updated to remove the event and status promises. -- `pnpm run test:coverage`, `pnpm run doc-sync`, and `pnpm run hygiene` pass after implementation. - -## Risks - -- Removing emitted events is a public-surface change. The repo is unreleased, and the consumer audit says the current consumers are tests and docs only. -- Tests lose an easy way to assert that registration happened. They should assert behavior instead: a registered tool appears in `schemas()`, a registered prompt section appears in `assemble()`, a web provider can execute or throw the expected `WebError`, and an adapter can stream for its model. -- Web tests lose a cheap status assertion. They should assert the behavior a real caller observes: successful `search()` / `fetch()` for a usable provider and structured `WebError` codes for unavailable, ambiguous, or misconfigured provider sets. -- A future UI may need observer hooks or status queries. That is fine; the hook/query should return with that UI, not ahead of it. diff --git a/docs/rfc/proposed/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md b/docs/rfc/proposed/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md new file mode 100644 index 0000000000..30d7f565d1 --- /dev/null +++ b/docs/rfc/proposed/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md @@ -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 provider unit 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. diff --git a/docs/rfc/proposed/simplification/2026-07-04-drop-web-providers-change-event.md b/docs/rfc/proposed/simplification/2026-07-04-drop-web-providers-change-event.md deleted file mode 100644 index ed40e54afe..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-04-drop-web-providers-change-event.md +++ /dev/null @@ -1,28 +0,0 @@ -# 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. diff --git a/docs/rfc/proposed/simplification/2026-07-04-fold-stdio-ui-helper.md b/docs/rfc/proposed/simplification/2026-07-04-fold-stdio-ui-helper.md index 4ef1a264b0..4af1ae9397 100644 --- a/docs/rfc/proposed/simplification/2026-07-04-fold-stdio-ui-helper.md +++ b/docs/rfc/proposed/simplification/2026-07-04-fold-stdio-ui-helper.md @@ -4,39 +4,24 @@ Status: proposed ## Problem -`@deepseek-ai/dsh-ui-stdio` lives under `packages/support/`, but its only runtime importer is the product app package `@deepseek-ai/dsh-stdio-agent` ([packages/ui/stdio-agent/src/index.ts](../../../../packages/ui/stdio-agent/src/index.ts)). Direct `createStdioChat()` uses are package-local tests and the production wrapper inside the same support package. The examples reach it by loading `dsh-stdio-agent`, not by composing the UI helper themselves. +`@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; the only other repo references are doc comments in two example e2e module docs and the dependency-graph rows in `packages/README.md`. [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. -That leaves an awkward package boundary. `support/` is documented as lower-compat dev/test/example infrastructure, and the `ui-stdio` README says it is a convenience REPL, not a product surface. But `dsh-stdio-agent` is a shipped app package whose front-door cluster always includes the readline UI, console logger, JSONL persistence, and a pre-created `main` agent. In practice the helper is not an independent swappable capability; it is an implementation detail of the stdio app. - -The boundary adds package metadata, workspace references, generated module-graph rows, README entries, publish lint surface, and a cross-group dependency from `packages/ui/stdio-agent` to `packages/support/ui-stdio`. It also creates a policy mismatch: a product UI app depends on a support package whose docs say it should not be treated as load-bearing 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 stdio UI helper into `@deepseek-ai/dsh-stdio-agent`. +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 the doc comments that name the package (the two example e2e module docs, `packages/README.md`, the ui group README). 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. -- Move the `createStdioChat` implementation, its `StdioRuntime` test seam, and its unit tests into `packages/ui/stdio-agent`. -- Delete the `packages/support/ui-stdio` package, package references, path aliases, dependency entries, module-graph rows, and support README row. -- Keep the testable runtime seam inside `dsh-stdio-agent` so EOF handling, rendering, disposal, and piped-vs-TTY behavior remain covered without hijacking process globals. -- Update docs that currently point at `../support/ui-stdio` to describe stdio rendering as part of the stdio app. +## Why not promote it to `ui/` instead? -After the fold, the stdio app owns its front door the same way `dsh-acp-agent` owns its ACP bridge cluster. The examples still load one app package; no leaf config has to learn a new plugin. - -## Why not promote it to `packages/ui/` instead? - -Promotion would fix the support/product mismatch but keep the extra package boundary. That would make sense if more than one product app composed `createStdioChat()` directly, or if the readline UI were a swappable UI integration in its own right. The current consumer audit says neither is true. The stdio app is the consumer and the owner. - -Re-extraction stays cheap while the repo is unreleased. If a second product app needs the same readline UI independently, split it back out then, with that consumer shaping the package contract. +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 -- `rg "@deepseek-ai/dsh-ui-stdio|support/ui-stdio|createStdioChat" packages examples docs scripts --glob '!docs/rfc/**' --glob '!**/lib/**'` finds no deleted package dependency or docs reference; `createStdioChat` remains only as an internal/tested helper under `packages/ui/stdio-agent` if the name survives. -- The stdio app still prints transcript events, handles stdin lines/EOF, renders todo updates, and disposes readline listeners under HMR. -- Echo/coding-agent keyless smoke tests still boot through the real Loader path and guard the named-export shape. -- Package manifests, tsconfig project references, 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 after implementation. +- `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 -- `dsh-ui-stdio` currently has focused tests with a small package-local setup. Moving them risks blurring app composition tests with UI rendering tests; keep the helper test seam and colocated unit tests to avoid that. -- A future standalone terminal UI may want the helper as a package. Reintroduce it when a second product consumer exists rather than keeping a boundary for hypothetical reuse. -- Docs that mention the stdio UI as a support example need careful wording so they still distinguish the non-product terminal demo from the ACP product surface. +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. diff --git a/docs/rfc/proposed/simplification/2026-07-04-narrow-pre-tool-gate.md b/docs/rfc/proposed/simplification/2026-07-04-narrow-pre-tool-gate.md deleted file mode 100644 index ee17588235..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-04-narrow-pre-tool-gate.md +++ /dev/null @@ -1,45 +0,0 @@ -# RFC: Narrow the pre-tool gate to shipped behavior - -Status: proposed - -## Problem - -The `tools/pre-execute` seam advertises two pieces of deferred capability that are not actually supported end to end: interactive `ask` permission and pre-tool argument rewrite. - -`PreToolDecision` includes `{ kind: 'ask' }`, but `ToolRegistry.execute()` treats every non-`allow` decision as a denied tool result because no permission UI exists yet ([packages/core/tools/src/index.ts](../../../../packages/core/tools/src/index.ts)). The only production producer is `dsh-hooks-claude`, which maps Claude Code `permissionDecision: "ask"` into that variant; Codex has no allow/ask path. The durable hook log can still record that an external hook asked, but the canonical typed seam cannot do anything distinct with it. The public union therefore has a third branch whose runtime semantics are "deny with a different default string." - -The same seam also has an unadvertised argument-rewrite escape hatch. The docs correctly say input rewrite is not offered because `assistant/message`, `tool/call`, and live presentation all see the model's original arguments before execution; changing only `exec.arguments` would make the UI/audit/history disagree with what ran. Yet `ToolExecution.arguments` is mutable, and dispatch reads `exec.arguments` after `tools/pre-execute`, so a listener can rewrite it anyway. A test shim does exactly that to thread a generated bash task id ([packages/bash/tool-bash/tests/integration.spec.ts](../../../../packages/bash/tool-bash/tests/integration.spec.ts)). The proposed [pre-tool input rewrite RFC](../feature/2026-06-30-pre-tool-input-rewrite.md) exists because doing this consistently is a design unit, not a hidden mutation. - -Both shapes are honest feature deferrals, but the public seam currently encodes them as if they were ready. That makes bridge code, docs, generated catalogs, and tests explain behavior whose only shipped result is "deny" or "mutate at your own risk." - -## Proposal - -Make `tools/pre-execute` express the behavior it can actually provide today: allow or deny a pending tool call, without argument mutation. - -- Remove `{ kind: 'ask' }` from `PreToolDecision`. The Claude bridge should still parse and log hook `ask` decisions, but map them to `deny` at the typed seam with an approval-not-supported reason until a real permission prompt exists. -- Update docs, generated catalogs, hook bridge README tables, and tests so `tools/pre-execute` is an allow/deny gate, not an allow/deny/ask gate. -- Make `ToolExecution.arguments` immutable by contract. At minimum mark it `readonly` and stop relying on a listener-mutated `exec.arguments` for dispatch; if a defensive runtime copy/freeze is needed to make the contract true, add it at the `ToolRegistry.execute()` boundary. -- Rewrite the one test shim that mutates `exec.arguments` to use a behavior-level helper instead of the hidden rewrite path. - -When permission prompts or consistent input rewrite lands, reintroduce the smallest explicit decision shape those features need. `ask` belongs with a real user approval loop; argument rewrite belongs with the audit/history/presentation update described by the proposed rewrite RFC. - -## What we give up - -Claude `permissionDecision: "ask"` no longer has a distinct typed-decision branch inside `dsh-tools`. The bridge can still preserve the external fact in `hook/result.decision` and still deny the call conservatively. That matches current product behavior without requiring every native plugin to handle an unusable branch. - -Internal tests lose a convenient mutable-object trick. That is a good loss: public tests should not depend on an unadvertised inconsistency that production docs warn against. - -## Acceptance criteria - -- `PreToolDecision` contains only `allow` and `deny`. -- `dsh-hooks-claude` still records hook `ask` in hook provenance, but returns a `deny` decision to `tools/pre-execute`. -- `rg "kind: 'ask'|PreToolDecision.*ask|ask.*degrades" packages docs --glob '!docs/rfc/**'` finds no remaining public pre-tool ask contract outside historical RFC text. -- `ToolExecution.arguments` is no longer a writable rewrite path, and `rg "exec\\.arguments\\s*=" packages examples --glob '!docs/rfc/**' --glob '!**/lib/**'` finds no mutation. -- The proposed pre-tool input rewrite RFC remains the future home for a consistent rewrite design. -- `pnpm run test:coverage`, `pnpm run test:snapshot`, `pnpm run doc-sync`, and `pnpm run hygiene` pass after implementation. - -## Risks - -- A native plugin author may already have experimented with `ask`. The repo is unreleased, and the branch currently cannot prompt a user; collapsing it now avoids shipping a promise that cannot be honored. -- Making arguments immutable may reveal more test helpers that were relying on mutation. Those helpers should move closer to the behavior they actually need instead of preserving a public inconsistency. -- Future permission and rewrite work will add back surface area. That is fine; the new surface should land with the product workflow and consistency guarantees that make it real. diff --git a/docs/rfc/proposed/simplification/2026-07-04-narrow-subagent-synchronous-collect.md b/docs/rfc/proposed/simplification/2026-07-04-narrow-subagent-synchronous-collect.md deleted file mode 100644 index 6f68dceb59..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-04-narrow-subagent-synchronous-collect.md +++ /dev/null @@ -1,61 +0,0 @@ -# RFC: Narrow the subagent seam to synchronous collect - -Status: proposed - -## Problem - -The implemented [subagent seam](../../implemented/feature/2026-06-21-subagent-capability-seam.md) shipped as a named-provider registry plus a synchronous model-facing consumer, but its public contract still carries several deferred capabilities that no production caller can exercise. `dsh-tool-subagent` builds a `SubagentStartRequest` with only `prompt`, `parent`, optional `signal`, and optional `agentOptions` ([packages/subagent/tool-subagent/src/index.ts](../../../../packages/subagent/tool-subagent/src/index.ts)); it never sends `outputSchema`, `maxDepth`, or `toolFilter`, never reads `SubagentResult.structured`, and never calls `SubagentRun.sendMessage` or `SubagentRun.resume`. - -That means the current start-time capability descriptor is mostly a contract between tests and docs. `SubagentCapabilities.outputSchema` and `toolFilter` are advertised false by every production provider, and the support mock is the only backend that exercises structured output. `depthLimit` is more subtle: the in-process providers advertise it and the shared driver can reject `request.maxDepth`, but no production tool request sets `maxDepth`, so the advertised recursion guard is dormant in the product path. - -The #138 hook stack made one earlier simplification idea too broad: `subagent/start` and `subagent/end` are now live. `dsh-hooks-claude` listens to `subagent/start` to run a `SubagentStart` hook and inject any returned `additionalContext` into the live child, and listens to `subagent/end` to run `SubagentStop` ([packages/hooks/hooks-claude/src/index.ts](../../../../packages/hooks/hooks-claude/src/index.ts)). Those lifecycle emits should stay. What remains idle is the registry-observation surface around the provider map: `ctx.subagents.getProvider()` and `ctx.subagents.list()` still have declarations, docs, generated-catalog entries, and tests, but no production caller. - -The new hook stack also exposes an overreach inside the lifecycle payload. [The subagent observe-enrichment RFC](../../implemented/feature/2026-06-30-subagent-observe-enrich.md) added `lastAssistantMessage` so a hooks bridge could forward the child output to a `SubagentStop` handler, but the current `SubagentStop` payload builder does not read it; it emits only `agent_id`, `agent_type`, and `stop_hook_active`. The field therefore buys a `structuredClone` branch, clone-failure containment, docs, and tests without changing any shipped hook behavior. If `SubagentStop` should carry the final child message, that should be implemented end to end; until then the payload should be honest. - -The result is an over-wide first-cut seam: every provider and every doc page has to explain structured output, tool filtering, depth flags, steering, resume, provider enumeration, and final-output lifecycle cloning even though the real model-facing behavior is "start a named child, await its final result, cancel or dispose it," plus observe-only lifecycle emits the Claude hook bridge actually consumes. - -## Proposal - -Make the subagent seam describe the behavior the harness actually uses today: synchronous collect plus the two live observe-only lifecycle emits. - -- Remove `SubagentCapabilities` and the `SubagentProvider.capabilities` field. -- Remove `SubagentStartRequest.outputSchema`, `maxDepth`, and `toolFilter`, along with `SubagentService.assertCapabilities`. -- Remove `SubagentResult.structured`. -- Remove optional runtime methods `SubagentRun.sendMessage` and `SubagentRun.resume`. -- Remove the public `SubagentService.getProvider()` and `SubagentService.list()` helpers; provider lookup stays private to `start(name, request)`. -- Keep `subagent/start` and `subagent/end`, but narrow their payloads to the fields the live bridge can use: `provider`, `id`, and on end `stopReason`. Remove `SubagentRunEndInfo.lastAssistantMessage`, the `structuredClone(result.output)` branch, and the clone-failure tests/docs. -- Remove in-process depth vocabulary that exists only to honor `maxDepth`: `AgentOptions.subagentDepth`, `depthOf`, `SubagentDepthError`, and the child-depth check in `startInProcessRun`. -- Update `dsh-subagent-spawn`, `dsh-subagent-fork`, `dsh-subagent-acp`, `dsh-subagent-mock`, `dsh-tool-subagent`, READMEs, [docs/core-data-structures/subagent.md](../../../core-data-structures/subagent.md), and the generated Cordis catalog to the narrower contract. - -After the cut, the provider contract is roughly: `name`, `start(request)`, and a `SubagentRun` with `{ id, result, cancel(), dispose() }`. The start request still carries the load-bearing fields: prompt, parent, optional signal, and optional child agent options. The service still emits `subagent/start` / `subagent/end` around that run because the hook bridge now consumes them. - -## Why not keep the dormant guard? - -The strongest counterargument is recursion: an in-process child can inherit the subagent tool and spawn again. That is a real product concern, but the current `maxDepth` field does not protect the production tool path because `dsh-tool-subagent` never sends it. A dormant guard reads like a safety property while providing none. - -If a hard recursion limit is needed, it should come back as an actually wired product policy, probably owned by `dsh-tool-subagent` config or a tool/filtering policy that every production subagent request passes through. That future implementation should be judged against the then-current product shape, not preserved as an optional per-request field that no caller supplies. - -## What we give up - -Programmatic callers lose prebuilt hooks for structured subagent output, child tool scoping, live steering, follow-up resume, provider enumeration, and final-output lifecycle telemetry. In an unreleased repo, that is an acceptable contraction: none of those hooks has a production caller, and preserving them makes every provider pay an explanation and test cost for speculative behavior. - -The in-process backends also lose the dormant depth bookkeeping. That does not weaken the shipped model-facing behavior because no shipped request uses it today. It makes the missing recursion policy honest. - -The Claude bridge would no longer be able to forward a child final message to `SubagentStop` without a later payload change. That is also honest: the current bridge does not forward it now. If that behavior becomes product-owned, reintroduce the field with the bridge payload and snapshot/unit coverage that prove the hook sees it. - -## Acceptance criteria - -- The public subagent contract contains only the synchronous collect surface: provider registration, `start(name, request)`, `SubagentRun.result`, `cancel`, and `dispose`. -- `rg "outputSchema|structured|maxDepth|toolFilter|sendMessage|resume\\(" packages/subagent packages/support/subagent-mock packages/subagent/tool-subagent docs --glob '!docs/rfc/**'` finds no remaining contract surface except unrelated prose or new historical references. -- `rg "getProvider\\(|ctx\\.subagents\\.list\\(" packages examples docs --glob '!docs/rfc/**'` finds no production API surface. -- `rg "lastAssistantMessage" packages docs --glob '!docs/rfc/**'` finds no live contract, clone branch, test, or generated-catalog entry. -- `subagent/start` and `subagent/end` still exist, and `dsh-hooks-claude` still handles `SubagentStart` / `SubagentStop`. -- The Cordis catalog, core data-structure docs, package READMEs, and type-equivalence manifest are updated. -- Focused subagent tests still prove registration HMR safety, duplicate provider rejection, missing provider rejection, in-process spawn/fork result collection, ACP result collection, abort bridging, and always-dispose behavior. -- `pnpm run test:coverage`, `pnpm run test:snapshot`, `pnpm run doc-sync`, and `pnpm run hygiene` pass after implementation. - -## Risks - -- A future subagent UI may want richer lifecycle payloads. Keep the live emits now, but reintroduce extra fields only with that UI and a payload it actually consumes. -- A future structured-output subagent may want `outputSchema`. Reintroduce it when a provider and consumer both honor it end to end, including validation semantics and model-facing schema design. -- A future recursion limit may be necessary. The replacement should be wired through the production subagent tool path instead of relying on an optional field the tool never sets. diff --git a/docs/rfc/proposed/simplification/2026-07-04-prune-unimplemented-subagent-vocabulary.md b/docs/rfc/proposed/simplification/2026-07-04-prune-unimplemented-subagent-vocabulary.md index 10b8a45a38..a68cb015c2 100644 --- a/docs/rfc/proposed/simplification/2026-07-04-prune-unimplemented-subagent-vocabulary.md +++ b/docs/rfc/proposed/simplification/2026-07-04-prune-unimplemented-subagent-vocabulary.md @@ -15,7 +15,9 @@ The only reason `dsh-subagent` depends on `dsh-tools` at all is `outputSchema`'s 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. +**Keep** `depthLimit`/`maxDepth` and the capability-check mechanism itself — with eyes open about its current reach. The in-process backend genuinely enforces the cap (`SubagentDepthError` in `packages/subagent/subagent-inprocess/src/index.ts`), but no production request sets `maxDepth` (`tool-subagent` exposes no knob for it), so on the shipped tool path the guard is dormant and recursion is uncapped. The alternative — remove the depth machinery too, on the argument that a dormant guard reads like a safety property while providing none — was considered and rejected: recursion is the seam RFC's named risk, the enforcement is real working code rather than vocabulary awaiting an implementation, and the honest completion is wiring a default cap through `tool-subagent` (a few-line feature) rather than deleting the only existing guard. One live capability row also keeps the two-tier design demonstrated rather than merely remembered. + +Adjacent surface examined and deliberately left alone: `SubagentService.getProvider()`/`list()` have test-harness consumers only, but the [prune-dead-seam-methods implementation note](../../implemented/simplification/2026-06-20-prune-dead-seam-methods.md) records precisely this shape being removed from the bash executor and reverted — a test harness IS a consumer for a one-line accessor over an already-tracked map. `SubagentRunEndInfo.lastAssistantMessage` is a recorded keep (the [subagent-observe-enrich RFC](../../implemented/feature/2026-06-30-subagent-observe-enrich.md)'s review dropped `agentType` and kept it deliberately, as the only final-message channel for out-of-process children); its currently-unwired bridge forwarding is a gap to close or a consumer to document, not surface for this RFC to cut. 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. @@ -30,4 +32,4 @@ The two-kinds-of-capability design is the seam RFC's headline, and re-adding `ou ## Risks -The subagent lifecycle events carry `lastAssistantMessage` on the end payload (the [subagent-observe-enrich RFC](../../implemented/feature/2026-06-30-subagent-observe-enrich.md)) — that enrichment lives in the service module, not the seam vocabulary this RFC shrinks, and the same RFC records dropping an `agentType` sibling for lacking a consumer: the judgment this RFC extends. The CC hooks bridge, the first outside consumer of those lifecycle events, reads only the event payloads and touches none of the surface removed here; the observe-enrich RFC's deferred control-flow redesign names implementing `resume` as its own future work — exactly the re-add trigger this RFC's pattern anticipates. 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. +The subagent lifecycle events carry `lastAssistantMessage` on the end payload — that enrichment lives in the service module, not the seam vocabulary this RFC shrinks, and the observe-enrich RFC records dropping an `agentType` sibling for lacking a consumer: the judgment this RFC extends. The CC hooks bridge, the first outside consumer of those lifecycle events, reads only the event payloads and touches none of the surface removed here; the observe-enrich RFC's deferred control-flow redesign names implementing `resume` as its own future work — exactly the re-add trigger this RFC's pattern anticipates. diff --git a/docs/rfc/proposed/simplification/2026-07-04-remove-tool-schema-defaults.md b/docs/rfc/proposed/simplification/2026-07-04-remove-tool-schema-defaults.md deleted file mode 100644 index 4d2428a2aa..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-04-remove-tool-schema-defaults.md +++ /dev/null @@ -1,42 +0,0 @@ -# RFC: Remove defaults from the tool-schema DSL - -Status: proposed - -## Problem - -`SchemaProp.default?: unknown` exists in the first-party tool-schema DSL ([packages/core/tools/src/schema.ts](../../../../packages/core/tools/src/schema.ts)). The converter copies it into the JSON Schema sent to the model, but the runtime validator does not apply defaults: an omitted optional argument remains omitted, and a missing required argument still fails. The code already marks this with `XXX(unused-default)`. - -No first-party tool definition in the repo sets `default`. Grepping `SchemaProp` defaults finds only the DSL itself, [docs/core-data-structures/tools.md](../../../core-data-structures/tools.md), and tests that assert the converter preserves a synthetic default. The behavior those tests pin is therefore model-visible metadata that no shipped tool emits and no runtime behavior honors. - -This is exactly the kind of small speculative knob that makes a custom DSL harder to explain. The [custom schema DSL RFC](../../implemented/architecture/2026-06-11-custom-schema-dsl.md) accepted a deliberately small subset until real tools demanded more; `default` was included in that early subset, but the real tools have not demanded it. - -## Proposal - -Remove `default` from the first-party `SchemaProp` DSL. - -- Delete `default?: unknown` from `SchemaProp`. -- Delete the `prop.default` to JSON Schema conversion line. -- Delete tests that assert synthetic defaults round-trip through `schemaSpecToJsonSchema`. -- Update `validateArgs` docs so they no longer describe default non-application as part of the DSL semantics. -- Update [docs/core-data-structures/tools.md](../../../core-data-structures/tools.md), the type-equivalence manifest output if needed, and any generated docs affected by the public type change. - -This does not ban defaults from every possible tool schema. `ToolRegistry.register()` still accepts raw model-facing `ToolSchema` objects, so a future MCP or raw-JSON-Schema producer can pass through provider-specific JSON Schema fields if needed. The simplification is only for the first-party typed DSL that `defineTool()` owns. - -## Why not apply defaults instead? - -Applying defaults would be a behavior change at the model boundary: `defineTool()` would need to synthesize missing arguments before the typed `execute` body runs, decide whether defaults apply recursively, and document how defaulted values interact with required fields and `InferArgs`. That is a real feature, not a cleanup, and no current tool needs it. - -Keeping metadata-only defaults is worse than doing nothing because it suggests the tool runtime has a defaulting story when it does not. Removing the field leaves one clear rule: optional arguments may be absent, required arguments must be present, and tools that want defaults put them in their own execution code. - -## Acceptance criteria - -- `SchemaProp` no longer has a `default` field, and `schemaSpecToJsonSchema()` no longer emits defaults from first-party DSL specs. -- `rg "unused-default|default\\?: unknown|prop\\.default|default:" packages/core/tools docs/core-data-structures/tools.md --glob '!docs/rfc/**'` finds no remaining DSL-default surface except unrelated JavaScript `default` syntax. -- Tool schema conversion, validation, type inference, and `defineTool()` tests still cover requiredness, enums, nested objects, arrays, invalid args, and presentation metadata. -- `pnpm run doc-sync`, including `doc-typecheck` and type-equivalence verification, passes after implementation. -- `pnpm run test:coverage` and `pnpm run hygiene` pass after implementation. - -## Risks - -- A future tool may want to tell the model a default value. That tool can either default inside `execute` and describe the behavior in prose, or a later RFC can reintroduce DSL defaults with real runtime semantics and at least one first-party consumer. -- Removing a type field breaks any external first-party DSL consumer. The repo is unreleased, so tightening the public type now is preferable to shipping a field whose semantics are "emitted but ignored." diff --git a/docs/rfc/proposed/simplification/2026-07-04-tighten-hook-protocol-contract.md b/docs/rfc/proposed/simplification/2026-07-04-tighten-hook-protocol-contract.md index 4fe0813c9c..42d222c6be 100644 --- a/docs/rfc/proposed/simplification/2026-07-04-tighten-hook-protocol-contract.md +++ b/docs/rfc/proposed/simplification/2026-07-04-tighten-hook-protocol-contract.md @@ -1,29 +1,32 @@ -# RFC: Tighten the hook-protocol contract — the `native` dialect, `suppressOutput`, and lib-owned `hook/result` semantics +# RFC: Tighten the hook-protocol contract — dialect, discarded fields, double defaults, and lib-owned `hook/result` semantics Status: proposed ## Problem -Three pieces of the `dsh-hook-protocol` 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: +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. **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. +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)). 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`). +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; and a dialect variant that the design's own thesis says will never be stamped is vocabulary without an interpreter — the bar the [subagent-observe-enrich RFC](../../implemented/feature/2026-06-30-subagent-observe-enrich.md)'s `agentType` drop records. Both return trivially with their first real producer (a transcript surface that has hook stdout to suppress; a native-provenance feature that logs hook events). On item 3, 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. +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` appears nowhere in source, tests, or parsed-field doc lists. -- One definition each of the truncation rule and the decision-string rule, in `dsh-hook-protocol`, exercised by both bridges' suites; the hook-matrix snapshot goldens are byte-identical. +- `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 -All three changes are invisible on the wire and in the goldens (`dialect` values emitted in practice are `claude`/`codex`; `suppressOutput` influences nothing; the folded semantics are the same rules). 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. +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. diff --git a/docs/rfc/proposed/simplification/2026-07-04-trim-hook-protocol-surface.md b/docs/rfc/proposed/simplification/2026-07-04-trim-hook-protocol-surface.md deleted file mode 100644 index d7de6d5d66..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-04-trim-hook-protocol-surface.md +++ /dev/null @@ -1,46 +0,0 @@ -# RFC: Trim unused hook protocol and bridge surface - -Status: proposed - -## Problem - -The #138 hook stack added a useful bridge layer, but the current public protocol still exposes a few fields and knobs that no shipped writer or reader uses. They are small individually; together they widen the durable hook log, the shared hook-protocol API, and both bridge configs. - -`HookDialect` includes `'native'`, but real `hook/invoked` writers are the Claude and Codex bridges only. The worked native-plugin test explicitly proves the opposite: a native plugin uses typed Cordis decisions and emits no `hook/*` session events. Grepping `dialect: 'native'` finds a hook-protocol unit test, docs, and type text, not production code. - -`hook/result.durationMs` is durable timing telemetry with no production reader. Both bridges write it; the ACP snapshot normalizer immediately scrubs it to `0` because wall-clock hook runtime is replay noise ([examples/acp-agent/tests/snapshot-normalize.ts](../../../../examples/acp-agent/tests/snapshot-normalize.ts)). The only remaining consumers are tests and generated goldens that exist because the field exists. Persisting a value that replay must erase is a smell: it is neither product behavior nor useful audit state. - -`MergedHookOutcome.systemMessages` is also unused. The codec should still parse `HookOutput.systemMessage` because the external protocols can emit it and both bridges warn when it appears, but the merged aggregate is never read; `rg "systemMessages|\\.systemMessages"` finds the merge helper, README prose, and merge tests only. The bridge already handles warnings per raw output before merge. - -Finally, both bridge configs carry optional process-level defaults that shipped configs do not set. `defaultTimeoutMs` duplicates the reference default (`600_000`) even though each command hook already has its own `timeout`; tests mostly cover schema-bypass fallback. `dsh-hooks-codex` also exposes `Config.model`, but the ACP configs load the Codex bridge with only `configPath`, and every hook payload already has an `Agent` whose `options.model` is the actual model for that run. - -## Proposal - -Remove the unused protocol and config surface while keeping the live external-hook behavior: - -- Change `HookDialect` to `'claude' | 'codex'` until a real native `hook/*` producer exists. Native plugins keep using the typed interception seams directly. -- Remove `durationMs` from the `hook/result` session event, `HookResultRecord`, `RunHookResult`, bridge append calls, docs, generated catalog, snapshots, and the snapshot normalizer's special-case scrub. Remove the injected `now` clock from `runHook()` if it becomes unnecessary after the field disappears. -- Remove `MergedHookOutcome.systemMessages` and its tests/docs. Keep `HookOutput.systemMessage` parsing and the bridge warnings. -- Remove `defaultTimeoutMs` from both bridge configs. Keep per-command `timeoutSec`; when absent, `runHook()` uses a single shared protocol constant for the reference default. -- Remove `dsh-hooks-codex` `Config.model`; stamp Codex payloads from `agent.options.model ?? ''` at the point that has an agent, with `''` only for no-agent fallback paths. - -## What stays - -This RFC does not remove `hook/invoked` / `hook/result` themselves. They are live provenance: bridges append them around actual hook execution and ACP snapshots persist them. It also does not remove parsing/warning for `updatedInput`, `systemMessage`, `continue:false`, or `suppressOutput`; those are deliberate faithful-but-degraded external-protocol fields documented by [the hook bridge RFC](../../implemented/feature/2026-06-30-hook-bridges.md). - -This RFC does not collapse the shared `dsh-hook-protocol` package into the bridges or build a single parameterized bridge engine. [The protocol-library RFC](../../implemented/feature/2026-06-30-hook-protocol-lib.md) explicitly keeps only the identical wire primitives shared and leaves per-dialect payload/config mapping in each bridge. - -## Acceptance criteria - -- `rg "HookDialect.*native|dialect: 'native'|claude.*/.*codex.*/.*native|claude.*codex.*native" packages/hooks docs/core-data-structures/session.md docs/cordis-catalog/events-and-services.md --glob '!docs/rfc/**'` finds no `HookDialect` branch, test writer, or `hook/*` docs claiming a native durable writer. -- `rg "durationMs" packages/hooks examples/acp-agent/tests docs/core-data-structures/session.md docs/cordis-catalog/events-and-services.md --glob '!docs/rfc/**'` finds no hook-result field, snapshot scrub, or generated-golden requirement outside unrelated timing concepts. -- `rg "systemMessages|\\.systemMessages" packages/hooks docs --glob '!docs/rfc/**'` finds no merged aggregate surface, while `systemMessage` parsing and bridge warnings remain covered. -- `rg "defaultTimeoutMs|Config\\.model|model\\?: string" packages/hooks docs --glob '!docs/rfc/**'` finds no bridge config knob for the removed defaults, while per-hook timeout support and Codex payload model stamping still work. -- Hook bridge unit tests and ACP hook snapshots still prove prompt-submit, pre-tool, post-tool, and stop behavior for both dialects. -- `pnpm run test:coverage`, `pnpm run test:snapshot`, `pnpm run doc-sync`, and `pnpm run hygiene` pass after implementation. - -## Risks - -- Durable hook timing can be useful diagnostics. If a product UI or trace viewer wants it, add live diagnostics or an intentionally durable telemetry event then; do not keep replay-noisy timing in the base hook-result record without a reader. -- A future native hook provenance logger might want `dialect: 'native'`. Add it with that logger. Until then, documenting native hooks as `hook/*` writers blurs the important design point that native plugins do not need the shell-hook log. -- A deployment could want a process-level Codex model override for hook payloads. The agent already knows its actual model, which is less surprising than a bridge-level default that can drift from the run being observed.