From 84a9b7374c2a85248fdcd268b6e4dc98141edebc Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 12 Jul 2026 03:11:21 +0800 Subject: [PATCH 1/4] docs: propose further simplifications --- docs/config-catalog.md | 2 +- docs/cordis-catalog/services.md | 2 +- docs/rfc/INDEX.md | 7 ++- .../2026-06-20-unify-agent-and-session-id.md | 54 ++++++------------- ...026-07-04-prune-dead-core-spine-surface.md | 45 +++++++++++----- ...12-collapse-workflow-to-foreground-core.md | 32 +++++++++++ ...6-07-12-drop-unconsumed-registry-events.md | 32 +++++++++++ ...6-07-12-prune-unconsumed-registry-modes.md | 32 +++++++++++ ...2026-07-12-prune-unused-web-seam-fields.md | 30 +++++++++++ ...-12-simplify-session-log-representation.md | 36 +++++++++++++ packages/core/system-prompt/src/index.ts | 2 + packages/ui/acp-agent/src/index.ts | 2 + packages/ui/acp/src/index.ts | 2 + packages/ui/stdio-agent/src/index.ts | 2 + packages/ui/stdio-agent/src/stdio-chat.ts | 2 + 15 files changed, 227 insertions(+), 55 deletions(-) create mode 100644 docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md create mode 100644 docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md create mode 100644 docs/rfc/proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md create mode 100644 docs/rfc/proposed/simplification/2026-07-12-prune-unused-web-seam-fields.md create mode 100644 docs/rfc/proposed/simplification/2026-07-12-simplify-session-log-representation.md diff --git a/docs/config-catalog.md b/docs/config-catalog.md index 18ef659ee0..4940b0c966 100644 --- a/docs/config-catalog.md +++ b/docs/config-catalog.md @@ -808,7 +808,7 @@ export interface Config { } ``` -Source: [`packages/core/system-prompt/src/index.ts:264`](../packages/core/system-prompt/src/index.ts) +Source: [`packages/core/system-prompt/src/index.ts:266`](../packages/core/system-prompt/src/index.ts) ## `@deepseek-ai/dsh-tool-cordis` diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index ddf2a860e4..cbf33d094b 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -260,7 +260,7 @@ protect(protection: PromptProtection): () => Promise | void async assemble(context: AssembleContext = {}): Promise ``` -Source: [`packages/core/system-prompt/src/index.ts:379`](../../packages/core/system-prompt/src/index.ts) +Source: [`packages/core/system-prompt/src/index.ts:381`](../../packages/core/system-prompt/src/index.ts) ## `ctx.tools` — `ToolRegistry` diff --git a/docs/rfc/INDEX.md b/docs/rfc/INDEX.md index 654b68ffb5..1b1f956399 100644 --- a/docs/rfc/INDEX.md +++ b/docs/rfc/INDEX.md @@ -16,7 +16,12 @@ Generated by `pnpm run gen-rfc-index` from the RFC tree — never edit by hand; | Title | First proposed | |---|---| | [Unify the agent id and the session id](proposed/simplification/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 | -| [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 | +| [Prune dead public and result surface](proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md) | 2026-07-04 | +| [Collapse workflows to the exercised foreground core](proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md) | 2026-07-12 | +| [Drop unconsumed registry events](proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md) | 2026-07-12 | +| [Prune unconsumed registry modes](proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md) | 2026-07-12 | +| [Prune unused web seam fields](proposed/simplification/2026-07-12-prune-unused-web-seam-fields.md) | 2026-07-12 | +| [Simplify session-log representation](proposed/simplification/2026-07-12-simplify-session-log-representation.md) | 2026-07-12 | ### Architecture diff --git a/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md b/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md index 19dee0ed56..1308da453e 100644 --- a/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md +++ b/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md @@ -4,57 +4,35 @@ Status: proposed ## Problem -The agent factory carries TWO ids for what is, in every live consumer, one thing: +The agent factory carries two ids for what every supported ownership path treats as one live agent/session pair: `agentId`, the `AgentRegistry` handle, and `sessionId`, the event-sourced/persisted-log identity. `CreateAgentOptions` takes both; `ResumeAgentOptions` takes `agentId` plus `resumeSessionId`; in-process subagents mint two independent UUIDs despite recording lineage separately. -- `agentId` — the `AgentRegistry` handle (the actor identity; the registry rejects a duplicate). -- `sessionId` — the event-sourced session / persisted-log identity (`session.header.id`). +ACP already uses the same value for both identities. Where they diverge, consumers maintain translations rather than use the distinction: stdio keeps `labelBySession` solely to recover an agent label from session events, ACP keeps reverse ownership state, and hooks expose both values for authors to reconcile. No production path reattaches one stable actor id to several sessions or drives one session through several agent ids. -`CreateAgentOptions` takes both separately; `ResumeAgentOptions` takes an `agentId` plus a `resumeSessionId`. They diverge in exactly three places: +The [agent-scope design](../../implemented/architecture/2026-07-08-agent-scope-contexts.md) makes the cost concrete. The `AgentLoop` factory reserves agent ids and session ids independently during asynchronous setup, with paired rollback paths, even though successful creation always publishes one pair; the registry and store then recheck live publication. PR #224 correctly closes the former duplicate-session ownership hole by reserving and rechecking both ids, so identity unification is no longer a correctness fix; it is a way to delete the second reservation/index/translation system. -- **Config-driven create** (`AgentLoop.create`): a stable `agentId` (e.g. `"echo"`) with a fresh per-run `sessionId` (`${id}-session-`). -- **Resume**: a caller-supplied `agentId` (e.g. `"main"`) on a persisted `resumeSessionId`. -- **In-process subagent children**: the backend mints the child's `agentId` and `sessionId` as two independent UUIDs (`packages/subagent/subagent-inprocess/src/index.ts`) that nothing distinguishes — `parentSession` records lineage independently. - -Where a live consumer looks an agent up, no lookup needs an id translation: the ACP bridge — the primary production path — already unifies the two (`agentId === sessionId === `; both factory call sites brand `AgentId(sessionId)` directly, and its reverse lookup keys on the `Agent` object itself), and the CC hooks bridge resolves subagent children directly by the `agentId` its lifecycle event carries. The one production population whose two ids actually DIVERGE is the in-process subagent children — the same cosmetic separation as the config path, and the same one-field simplification under unification. One consumer already pays the two-id tax: ui-stdio keeps a `labelBySession` map (seeded from the registry, maintained by `agent/created`/`agent/disposed` listeners) solely to translate `session.header.id` back to an agent id for its turn labels — machinery that deletes outright when the ids unify. And the CC hooks bridge stamps `session_id: agent.session.header.id` into every hook payload, so under unification a subagent hook's `session_id` and `agent_id` become the same string — one less identity for a hook author to reconcile. - -The separation is **latent generality no consumer exercises**: nothing reads a *stable* `agentId` back across runs (each process starts fresh, and persistence keys off the session id, never the agent id). The config path's "stable agentId, fresh sessionId" buys nothing concrete — it is cosmetic. And the `agentId !== sessionId` case is precisely what opens the bash owner-token alias hole: the bash completion-notice routes by `session.header.id`, but the registry enforces uniqueness only on `agentId`, so a programmatic caller registering two agents with different agent ids but the SAME session id can mis-route a notice (see [agent lifecycle and ownership seams](../../implemented/architecture/2026-06-18-agent-lifecycle-and-ownership-seams.md) § Seam precondition). The current code documents this as a precondition rather than guaranteeing it. +Session itself repeats the same fact as `Session.id` and `Session.header.id`. Valid store paths construct them equal, but the constructor does not enforce equality and production consumers choose between the two. The duplicate creates an impossible-but-representable mismatch inside the object that owns session identity. ## Proposal -Make an agent BE its session: one id. An agent's registry handle IS its `session.header.id`. +Make an agent's registry id equal its session id. `CreateAgentOptions` accepts one id used for both registration and session creation; resume registers the agent under the resumed session id; subagent creation mints one combined id; Session keeps one identity home by deriving `id` from `header.id` or removing the alias. Replace the two reservation sets and rollback branches with one combined identity reservation, and remove maps/fields whose sole job is translating between the ids. -- `CreateAgentOptions` drops the separate `sessionId` — the single `id` is both the registry handle and the live/persisted session id. (ACP already passes the same UUID for both, so its call site simplifies to one field.) -- `ResumeAgentOptions` drops the separate `agentId` — resuming `sessionId` X registers the agent under id X. (ACP already does this.) -- The config path (`AgentLoop.create`) uses its configured `id` directly as the session id, applying whatever resume-or-create policy it adopts (today it appends a per-run uuid to avoid colliding with an on-disk log; that policy moves onto the single id, e.g. the config id IS the session and a durable backend resumes it — to be settled in the implementing PR). -- The registry's existing unique-`agentId` check becomes, by construction, a unique-session-id guarantee — the bash alias hole is closed with NO new defensive invariant: two agents cannot share a session id because the session id is the agent id. +The config-driven path must first settle its currently hidden resume-or-create policy. Today it uses a stable agent label and fresh UUID-suffixed session id to avoid colliding with a durable log on the next run. Under unification it must deliberately resume the fixed id, mint a fresh combined id, or expose an explicit policy; implementation must not pick silently. + +After stdio's translation map disappears, rerun the consumer search for `agent/created` and `agent/disposed`. If it is empty, remove those notifications together with `AgentRegistry.announced`/`announce()` and their publication rollback machinery. PR #224 deliberately hardened those lifecycle semantics, so this follow-on removal is conditional on proving that identity unification eliminated their last owner. ## Alternatives considered -### Why not just enforce session-id uniqueness in `AgentRegistry.register()`? - -That was the review's first suggestion. It would couple the generic registry to a session-uniqueness assumption (the registry tracks *agents*, not sessions) and entrench the very separation this RFC removes. Unifying the ids closes the hole more cleanly — there is nothing left to enforce. +**Keep separate actor and session identities.** This leaves room for handoff, one actor traversing many logs, or many actors adopting one log. None is supported today. If that product direction arrives, it deserves an explicit actor/handoff seam with ownership semantics rather than two ids that happen to differ in a few constructors. ## Acceptance criteria -- `ctx.agents.create`/`resume` take a single id; the ACP bridge passes one id. -- The config-driven agent path has a deliberate, documented session-id policy (no silent per-run id divergence that no consumer reads). -- The bash owner-token alias hole is gone by construction (no two live agents can share a session id). -- All existing behavior the tests pin (ACP create/resume/load, config startup, durability) still holds — or the tests change WITH the behavior where the divergence was an artifact (per AGENTS.md "tests document behavior, not golden truth"). +- Agent create/resume and subagent creation carry one identity; `Session` stores it in one place. +- The factory keeps one in-flight reservation/rollback path while preserving PR #224's duplicate and quiescence guarantees. +- ACP, stdio, hooks, bash ownership, persistence, and lineage need no agent/session id translation. +- The config-driven resume-or-create policy is explicit and covered across a durable restart. +- `agent/created`/`agent/disposed` are removed only if a post-change production search finds no listener; otherwise they and their publication semantics stay. +- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. ## Risks -This touches public factory interfaces (`CreateAgentOptions`, `ResumeAgentOptions`, `AgentFactory`) and the config-agent id scheme, so it is a deliberate cross-package change, not a local patch — it ships as its own PR (converged with Codex); the bash owner-token precondition it closes is documented in [agent lifecycle and ownership seams](../../implemented/architecture/2026-06-18-agent-lifecycle-and-ownership-seams.md). - -The genuine risks of collapsing the two ids into one (the case AGAINST this proposal — to be weighed honestly before implementing): - -- **It forecloses a one-agent-resumes-many-sessions / one-session-driven-by-many-agents future.** Today the separate ids leave room for an agent (a stable actor) to detach from one session and attach to another, or for a handoff where a new agent process adopts an existing session under a new actor handle. Unifying makes "agent" and "session" the same lifetime, so any such future needs a NEW seam (e.g. an explicit `actorId` distinct from the session) — re-introducing the very separation we removed. We judge this generality currently unused, but it is a door this change closes. - -- **Subagents / fork / spawn may WANT a stable actor id across forked sessions.** The [subagent seam](../../implemented/feature/2026-06-21-subagent-capability-seam.md) runs a child agent seeded from a parent's event log (fork). If a future design wants "the same agent identity across a fork" (parent and child share an actor but have distinct session logs), a unified id blocks it. The implementing PR must check the intended fork/spawn model BEFORE unifying, or accept that fork always mints a fresh combined id. (As shipped, each subagent child mints its own distinct agent id — `parentSession` records lineage — so the seam does not currently rely on a shared actor id, but unifying would foreclose adding one.) - -- **The config-driven resume-or-create policy becomes load-bearing, not cosmetic.** Today the per-run-uuid session id quietly sidesteps the "a fixed id collides with its own on-disk log on the second run" problem. Once the id is unified and stable, a config agent restarting MUST decide resume-vs-fresh deliberately — there is no longer a throwaway session id to hide behind. Getting this wrong reintroduces the create-collision the uuid was avoiding (a durable backend refuses to re-create an id whose log exists). This is the one real design decision the implementing PR owns, and it is easy to get subtly wrong. - -- **Persisted/on-disk identity becomes the agent identity.** Unifying means the registry handle is now a persisted, externally-meaningful string (a session id a client chose), not an internal label. A caller that previously used a short human label (`"main"`) as the agent id now must use the session id. This is fine for ACP (already a UUID) but is a semantic narrowing for any programmatic embedder that relied on naming its agents independently of session storage. - -- **Migration churn touches every create/resume call site and its tests.** `CreateAgentOptions`/`ResumeAgentOptions` shape changes ripple to ACP, the config path, the agent-loop factory, and ~dozens of test fixtures that currently pass distinct `agentId`/`sessionId` (some deliberately distinct to exercise the divergence — those tests change WITH the behavior, per AGENTS.md "tests document behavior, not golden truth"). The risk is mechanical but broad; a missed call site is a type error, but a missed *test* could silently lose coverage of a path. - -The one real design question the implementing PR must settle first is the config-driven resume-or-create policy once the id is unified (today's per-run-uuid behavior is a demo simplification already flagged `TODO(demo)`). If, on closer look, the fork/spawn or multi-session-actor futures turn out to be wanted, this RFC should be REJECTED in favor of the lighter "enforce session-id uniqueness in the registry" guard — the alias hole is not reachable via ACP, so keeping the ids separate and merely documenting (or mechanically enforcing) the precondition remains a valid alternative. +This forecloses latent multi-session-actor and session-handoff designs, makes persisted client-chosen session identity the registry identity, and touches every factory fixture. The config restart decision is blocking, not mechanical. If separate actor identity becomes a real requirement, reject this RFC and retain PR #224's already-correct dual reservation system. diff --git a/docs/rfc/proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md b/docs/rfc/proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md index c6bfbe967c..5a130057cb 100644 --- a/docs/rfc/proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md +++ b/docs/rfc/proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md @@ -1,32 +1,49 @@ -# RFC: Prune dead core-spine surface — `SurfaceManager.invalidate()`, the loop-internal exports, `ToolExecutionResult.callId` +# RFC: Prune dead public and result surface Status: proposed ## Problem -Three pieces of public spine surface share one defect class: their only possible role is to be ignored, or their trigger is unreachable. +Several package-root exports, result fields, and convenience methods have no production consumer. They survive because tests import internals through public entry points or because a type anticipated a caller that never arrived. Each item is small in isolation, but together they enlarge the SDK contract, generated catalogs, documentation, and regression matrix without enabling a shipped path. -1. **`SurfaceManager.invalidate()`** (`packages/core/session/src/surface.ts`). Its documented trigger — "the log has been replaced wholesale (e.g. after Session seed)" — is structurally unreachable: seeding happens inside the `Session` constructor, `_surface` is created lazily on first access, and the log reference is never reassigned afterward, so no constructed `SurfaceManager` ever observes a wholesale replacement. Sole caller: its own unit test. A rollback primitive protecting a scenario the implementation cannot produce. -2. **The `runLoop`, `Inbox`, and `InboxMessage` exports** (`packages/core/agent-loop/src/index.ts`). `runLoop` has no importer outside the package — the only callers are the package's own internals (the agent constructs its loop with it), so the public re-export has zero consumers; `Inbox`/`InboxMessage` likewise reach outside code only through the package's own inbox spec (switchable to the source module). The exports contradict the package's own docs — the inbox module doc says the public surface is `Agent.send()`/`Agent.steer()` — and the [architecture dependency rule](../../../architecture.md): nothing programs against `dsh-agent-loop`; a replacement loop is a different bundle built on `dsh-agent`, not a consumer of this package's internals. `ReactLoopAgent` stays exported (cross-package tests construct it by package name). -3. **`ToolExecutionResult.callId`** (`packages/core/tools/src/index.ts`; the input `ToolExecution.callId` stays). Zero consumers read it. A `tools/execute` wrapper may construct or replace a result, but the registry rejects any `callId` that differs from the immutable execution identity and rebuilds later outcomes from protected snapshots; `tools/post-execute` receives that same execution beside the result, and the observe-only `tools/result` notification receives both as immutable values. The loop independently correlates with its model call's `call.id`, while ACP correlates through the session event's `data.callId`. The result field is therefore a compulsory copy of information already present at every extension point, plus validation and regression tests whose only job is to prove the copy cannot disagree. +The production corpus is `packages/*/*/src`, example sources/config, and runtime scripts; tests, package READMEs, generated catalogs, and RFC prose are evidence of publication but not consumers. Exact-symbol searches produce the following inventory: + +| Surface | Production evidence | Simplification | +| --- | --- | --- | +| `SurfaceManager.invalidate()` | Only its unit test calls it; seeding completes before the lazily-created manager exists and the session never replaces its log reference. | Delete it and its impossible wholesale-replacement contract. | +| `ToolExecutionResult.callId` | Every hook already receives the immutable `ToolExecution`; the loop and ACP correlate through the call/session event. No consumer reads the duplicate result field. | Remove the field, copy/mismatch guards, and tests that prove the duplicate cannot disagree. | +| `ReactLoopAgent` root export | Outside-package named imports are tests; production programs against `Agent` and creates/resumes through `ctx.agents`. | Return/interface-type `Agent` and make the concrete loop class package-internal; keep the deliberate synchronous config-only `AgentLoop.create()` path. | +| `workflow-workerthread` protocol/runtime/session re-exports and named `WorkerWorkflowEngine` | Every package-name consumer uses the default engine; the workflow RFC already defines the worker wire protocol as private. | Keep the default plugin class/config contract; drop the duplicate named class export and keep protocol modules source-private. | +| `code-runtime-worker` protocol/bootstrap re-exports | Outside-package production/e2e consumers use `WorkerCodeRuntime` and config, not `BootstrapPort`, `PatchableStream`, or worker message/boot types. | Keep the runtime class/config contract and make its wire/bootstrap vocabulary source-private. | +| `providerWording` and `completedTurnPrefix` root exports | Each has one same-package production caller; only the balanced-prefix helper has a same-package white-box test. | Make them source-private and test provider behavior. | +| `depthOf`, `SubagentDepthError`, `SENSITIVE_ENV_PATTERN`, `waitForExit`, and `exitsWithin` root exports | Production subagent backends consume the in-process runner and subprocess construction/disposal helpers, not these enforcement/test internals. | Keep depth/environment/exit behavior but make the helpers and error/regex source-private; test through spawn and disposal. | +| `PersistenceCoordinator.inits`, backend `inits` accessors, `seedCoversPrefix`, and `assertSerializable` | The accessors exist for white-box tests; the helpers have no outside production importer. | Observe initialization through `session/flush` and internalize the helpers. Keep both backends, `SessionHeader`, and SQLite's version contract. | +| `LlmService.models()` | Tests/docs only; production resolves a configured adapter directly. | Delete the convenience while keeping both LLM adapters. | +| `LlmError.status` and replay status | Adapters/replay populate it, but production branches on stable error code/message and never reads raw status. | Remove the unread field and replay plumbing while preserving error classification. | +| `BlockAssembler.push()` return value | Both production callers ignore the returned completed block. | Return `void`; keep the deliberately public `blocks()`/`message()` contract. | +| `compactRegion`'s separate `session` argument | The sole production caller passes the same object already present as `agent.session`; the API permits an incoherent pair. | Use `agent.session` as the one source of truth. | +| `CompactionResult.startSeq`, `summarySeq`, `endSeq`, and `summary` | The production consumer reads only shadowed range/seq/token accounting; the durable log owns summary and event identity. | Remove the four result echoes while keeping both shared transcript renderers. | +| `BasicCompactService` estimation/summarization visibility | No outside production caller invokes the five methods; the implemented RFC names only `estimateContentTokens()` and `summarize()` as subclass hooks. | Make those two `protected` and the three orchestration-only estimators private. | +| `CodeLogEntry.source`/`level` and `RunCodeMeta.dispatches` | Every production consumer maps logs to text; no presenter/model path reads the other fields or the persisted dispatch count. | Make code-runtime logs strings (or text-only entries) and remove result-meta dispatch plumbing; keep the local counter that mints deterministic dispatch ids. | +| `ToolNotFoundError.toolName`, `SystemPrompt.config`, and `BashTask.command` | Each stored public value has no production reader. | Drop the unread field while retaining error messages, resolved configuration behavior, and task lifecycle. | + +The earlier version of this RFC also named `runLoop`, `Inbox`, and `InboxMessage`; the agent-scope branch has already made those package-internal, so they are no longer proposed work. ## 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 (deny, dispatch, `toolErrorResult`, post-execute snapshots), its around-wrapper mismatch validation, the loop's ignore-comment, and the tests that prove the duplicate id cannot matter. The result's consumed `additionalContext` ferry and the execution object's authoritative `callId` stay untouched. Update the `ToolExecutionResult` paste in [tools.md](../../../core-data-structures/tools.md) (and its `scripts/type-equiv.manifest.json` row) and the result-shape row in `packages/core/tools/README.md`; for the `invalidate()` removal, amend the [session-surface RFC](../../implemented/architecture/2026-06-18-session-surface.md)'s full-rebuild-after-wholesale-replacement sentence per [implemented/AGENTS.md](../../implemented/AGENTS.md). - -Sequencing: the surface-cache work (tool-pairing balance caching) neither uses nor touches `invalidate`, so that removal can land after or alongside it mechanically. The full execution pipeline carries the immutable execution object through pre-policy, guards, around-dispatch wrappers, post-policy, and final result observation; nothing needs the result to repeat its id. +Remove or demote every row as one bounded coordinated public-surface cleanup. Update package READMEs, JSDoc, generated API/event catalogs, type-equivalence records, exports maps where needed, and tests so they exercise the owning public seam instead of preserving test-only entry points. Do not collapse any capability seam, LLM adapter, persistence backend, or lifecycle quiescence contract. ## Alternatives considered -### Why not keep them? - -A future consumer that swaps a session's log in place would want a reset primitive — it re-adds `invalidate` with itself. A replacement-loop author might want to reuse the inbox or the driver — the architecture already answers that a replacement loop is a different bundle. An isolated result-logging listener might want self-contained correlation on the result — the execution object is in scope at every listener, and a field that exists only to be ignored is worse than absent: it invites exactly the orphaned-pairing bug the loop comment warns about. +**Keep test conveniences and self-contained results public.** Public helpers can make white-box tests convenient, self-contained result fields can look ergonomic, and future embedders might want the concrete loop or enumeration methods. Those benefits are hypothetical; today they make every implementation and document explain states that no shipped caller can observe. A real consumer can introduce the smallest contract it needs, with its ownership and failure semantics known. ## Acceptance criteria -- `invalidate()` and the result `callId` appear only in this RFC; `runLoop`/`Inbox`/`InboxMessage` remain package-internal only — no re-export from the package index and no outside-package importer; the agent-loop README lists only the consumed public surface; the inbox spec imports the source module. -- The complete tool-pipeline contract tests pass with the shrunk result type; the around-wrapper mismatch test, mutation-guard id assertions, and proves-ignored loop test disappear with the duplicate field. +- Exact-symbol searches show no removed surface outside this RFC and any implemented-RFC amendments. +- Every surface listed in this RFC is absent or demoted as specified; deliberately retained extension/test contracts outside the inventory are unchanged. +- Tool execution, compaction, both LLM adapters, both persistence backends, workflow isolation, and agent creation/resume retain their shipped behavior. +- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. ## Risks -All three are compile-visible removals with no runtime behavior change on any shipped path. +Most removals are compile-visible but runtime-neutral. The compaction argument cleanup deliberately forbids a session/context mismatch that no caller uses; the remaining changes can require external pre-release embedders to import less or adjust result shapes. The repository is unreleased, so carrying unsupported surface is the larger foundation cost. diff --git a/docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md b/docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md new file mode 100644 index 0000000000..e80cbd9414 --- /dev/null +++ b/docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md @@ -0,0 +1,32 @@ +# RFC: Collapse workflows to the exercised foreground core + +Status: proposed + +## Problem + +The workflow capability executes foreground JavaScript that composes subagents, but it also carries an unconsumed progress-observation system. No production listener subscribes to any of the six `workflow/*` events; listeners exist only in workflow tests. Nevertheless the seam defines run/phase/agent outcome snapshots, the worker sends phase/log/agent lifecycle protocol messages, the host clones payloads and keeps a `liveAgents` pairing ledger, and the engine maintains run ids solely to correlate those notifications. + +The progress vocabulary is not merely unused; it cannot serve its only named future owner without redesign. `WorkflowRunInfo` contains `{id, meta}` but no parent agent, session, or tool-call identity, while the model-facing tool never exposes the run id. A global ACP listener could not route an event to the correct client session. `meta.phases` is never consulted, `phase(title)` does not validate against it, phase `detail`/`model` and agent `label`/`phase` feed only events, and `whenToUse` is validated and copied but never rendered or selected. `phase()` and `log()` still cross the worker boundary despite having no receiver. + +`WorkflowError.fatal` is the same speculative branch in miniature: every production construction is fatal, `fatal: false` exists only in tests, and combinators already distinguish workflow failures with `instanceof`. + +## Proposal + +Keep the exercised core: `agent(prompt, { schema, model })`, `parallel`, `pipeline`, `args`, concurrency/agent caps, cancellation, bounded disposal, structured results, worker isolation, and foreground tool collection. Remove all `workflow/*` events and their event-only info/outcome types; remove `phase()`, `log()`, agent `label`/`phase`, phase declarations, `whenToUse`, and their worker messages/host observers; collapse workflow metadata to the name the tool actually uses; remove event-only run ids/meta snapshots and the synthesized agent-end ledger. Make `WorkflowError` one fatal error class without a boolean mode or `isFatalWorkflowError()` helper. + +Amend the implemented dynamic-workflow RFC and update the seam/tool/worker READMEs, tool schema, generated catalogs and package graph, worker type-equivalence records, unit tests, and workflow snapshot/header fixtures. Progress UI work, if commissioned, starts from a correlation contract that names the parent agent/session/tool call instead of reviving this protocol unchanged. + +## Alternatives considered + +**Keep the prebuilt observation vocabulary for a future UI.** The current shape resembles Claude Code dynamic-workflow metadata, and PR #233 deliberately added host pairing state and synthesized agent-end events so observers would see balanced lifecycles; this proposal reopens that recent choice rather than treating the machinery as accidental. Removing it gives up compatibility-by-shape and makes progress UI a new design task, but the existing shape still lacks routable ownership, so its hardened pairing cannot make the named ACP owner viable without redesign. + +## Acceptance criteria + +- The workflow public seam contains only execution, cancellation, result, and disposal contracts with a production consumer. +- No workflow event, phase/log protocol message, run-id generator, progress-only metadata, host pairing ledger, or fatal-mode branch remains. +- Parallel/pipeline behavior, caps, cancellation quiescence, worker containment, structured output, and the model-facing workflow scenarios retain coverage. +- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. + +## Risks + +This is a compile-visible contraction of the workflow DSL and event taxonomy. Existing unshipped scripts that use `phase`, `log`, labels, or descriptive metadata must shrink, and a future observer must add a better-correlated seam. The execution semantics that make workflows useful do not change. diff --git a/docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md b/docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md new file mode 100644 index 0000000000..ccb2d0af44 --- /dev/null +++ b/docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md @@ -0,0 +1,32 @@ +# RFC: Drop unconsumed registry events + +Status: proposed + +## Problem + +Four registry notifications are produced but have no production listener. The generated producer/consumer matrix and exact event-name searches find only declarations, emit sites, invariant metadata, tests, generated catalogs, and prose for `tools/change`, `system-prompt/change`, `skill/provider-added`, and `skill/provider-removed`. + +No shipped path uses these signals for invalidation: request assembly deliberately reruns for every step, tool/system-prompt membership is now agent-scoped, and skill discovery reads providers on demand. PR #224 also makes the payloadless tool/system-prompt notices less coherent because a change may be scope-local but the event cannot identify that scope. + +Earlier registry work retained tool/system-prompt notifications as low-cost hooks for a hypothetical live UI even while the equivalent LLM and web notifications were removed. The new evidence is that no owner has appeared, per-step assembly needs no signal, and scope-local membership has made the old payload insufficient for that hypothetical owner. This proposal does not include `subagent/provider-added`/`removed`, which `tool-subagent` consumes to tolerate concurrent sibling-plugin loading. + +## Proposal + +Delete the four declarations and every emit path, rollback-order branch, invariant-table entry, test, and generated catalog/matrix row that exists only for them. Remove the corresponding registry README/JSDoc contract. Where tests used an event to observe cleanup, assert public lookup or assembled output instead. + +Amend the [agent-scope RFC](../../implemented/architecture/2026-07-08-agent-scope-contexts.md) and reconstructable-request documentation so current behavior has one home: request inputs are recomputed at the request boundary, not maintained by invalidation signals. + +## Alternatives considered + +**Keep cheap notifications for future plugins.** An external plugin could subscribe later, and provider lifecycle signals can solve sibling-load races. The subagent event demonstrates the bar: it has a real concurrent loader consumer and a payload tailored to that job. These four have neither; a future consumer should introduce the scoped identity and timing it demonstrably needs. + +## Acceptance criteria + +- The generated event matrix contains no row or registry-subject inventory entry for the four notifications. +- Tool schema assembly, system-prompt assembly, skill discovery, ordinary effect rollback/disposal, and registry lookup cleanup behave unchanged; listener-triggered rollback disappears with the events. +- The real subagent provider lifecycle consumer remains covered. +- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. + +## Risks + +This deliberately removes pre-release plugin observation points. A future live registry UI would need a new scoped snapshot/change contract instead of subscribing to payloadless global notifications. diff --git a/docs/rfc/proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md b/docs/rfc/proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md new file mode 100644 index 0000000000..500a4254a4 --- /dev/null +++ b/docs/rfc/proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md @@ -0,0 +1,32 @@ +# RFC: Prune unconsumed registry modes + +Status: proposed + +## Problem + +Two registries implement modes that no production registration populates. + +The skill service's embedded-runtime subsystem has zero production caller of `ctx.skills.register()`. It adds a reserved `runtime` provider name, a runtime map/rank/source, duplicate policy, a second revision in cache keys, normalization, disposers, and tests alongside the provider seam every shipped skill already uses. `SkillSummary.whenToUse` and candidate/definition `path` are parsed and copied but never read by a production consumer: the model catalog renders name/description, resource loading uses `resourceBase`, and providers own their locator. The deliberately open `metadata` extension point stays. + +The agent-scope work generalized system-prompt tool and variable providers to scope-local registration, but every production `systemPrompt.tools()` and `systemPrompt.variable()` registration is global. Scoped assembly executes the merge path each step but finds no scoped tool/variable contribution. Scoped sections and protections are live and stay. Supporting empty scope-local tool/variable layers adds maps plus merge/shadow/cleanup branches for combinations the product never constructs. + +## Proposal + +Remove `SkillService.register()`, `SkillRegistration`, the runtime pseudo-provider and reserved-name rules, runtime revisions/cache branches, and runtime-only source/rank normalization. Tests that need an embedded skill register a small real provider. Retain `providerRevision` as the in-flight discovery epoch, but key completed catalogs by cwd alone: every provider mutation synchronously clears the cache, and the post-await revision comparison already prevents inserting stale work. Remove `whenToUse`, `SkillCandidate.path`, and `SkillDefinition.path` from the skill contract and local-provider copies while retaining provider locator/root paths; retain `metadata`, `disableModelInvocation`, `source`, `provider`, `locator`, and `resourceBase` as either deliberate extension vocabulary or production-consumed fields. + +Keep system-prompt sections/protections scoped, but make tool-schema and variable providers global-only and delete their scoped maps/merge logic. Fail loud if a caller attempts these unsupported scope/mode combinations instead of silently widening them. Keep both global and scoped tool guards: the agent-scope/interception design deliberately defines them as owner-final policy APIs. Amend the skill-system and agent-scope RFCs, READMEs, JSDoc, catalogs, and tests. + +## Alternatives considered + +**Keep all registry modes for embedders.** Runtime skill registration is convenient, and scoped variables/tool-schema fragments could support per-agent prompt customization. Neither has a shipped owner. A real embedded skill can be a provider; per-agent executable tools already belong in `agent.ctx.tools`; per-agent prompt facts can use scoped sections or context-aware global providers. + +## Acceptance criteria + +- Skill collection has one provider-backed path, a cwd-only completed-cache key, and a revision epoch only for in-flight invalidation; retained skill fields have a production reader or a recorded deliberate extension contract. +- System-prompt tools/variables have one global path; sections/protections retain their scoped behavior. +- Global and scoped tool guards, native finality, and Code Mode finality behavior remain covered. +- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. + +## Risks + +These are compile-visible contractions of two pre-release registries. The fail-loud rule must distinguish an unsupported scoped registration from an ordinary global registration without disturbing Cordis effect cleanup, and skill-local frontmatter parsing must continue to preserve and validate the supported metadata schema. diff --git a/docs/rfc/proposed/simplification/2026-07-12-prune-unused-web-seam-fields.md b/docs/rfc/proposed/simplification/2026-07-12-prune-unused-web-seam-fields.md new file mode 100644 index 0000000000..1a8495426e --- /dev/null +++ b/docs/rfc/proposed/simplification/2026-07-12-prune-unused-web-seam-fields.md @@ -0,0 +1,30 @@ +# RFC: Prune unused web seam fields + +Status: proposed + +## Problem + +The web capability carries request/result/status values that every shipped implementation populates but no production consumer reads. `WebSearchResult.providerId` and `query` and `WebFetchResult.providerId` are result echoes; `tool-web` formats only content/sources/truncation or final URL/status/body/truncation, and no other runtime reads them. Search providers return `WebProviderStatus.reason`, but resolution checks only `available` and intentionally emits a generic unavailable diagnostic. + +`WebFetchRequest.timeoutMs` is likewise never set by a production caller. `tool-web` supplies only the URL, uses the tool definition's timeout plus `exec.signal` for the caller deadline, and relies on the local provider's configured default as a backstop. The unused per-request override forces `web-fetch-local` to expose `maxTimeoutMs`, clamp two timeout sources, and document/test precedence no product path can select. `WebExecContext` is another one-field wrapper: every caller allocates `{ signal }` and every provider immediately unwraps `exec?.signal`; no second execution-control field exists. + +## Proposal + +Remove the search/fetch `providerId` result echoes and search `query` echo; callers already own the request and provider selection. Shrink provider status to availability alone, preferably a boolean-returning method if that produces the clearest seam. Remove per-request fetch timeout, `maxTimeoutMs`, and their clamp/validation branches while retaining the provider's configurable default timeout and tool-level deadline. Replace `WebExecContext` with a direct optional `AbortSignal` parameter. + +Update all web implementations, the model-facing tool, package READMEs/JSDoc, type-equivalence records, and tests. Keep the interface/implementation/consumer package split, provider selection, source citations, final-URL/status data, truncation reporting, and all safety limits. + +## Alternatives considered + +**Keep self-describing results, per-request deadlines, and an extensible execution-context object.** Result echoes can help generic telemetry, a request timeout can help trusted programmatic callers, and the wrapper leaves room for future controls. No such consumer/second field exists; carrying duplicate identity, a second deadline policy, and wrap/unwrap plumbing through every provider makes the current contract harder to implement and explain. If telemetry or per-call budget control arrives, it should define which deadline wins, where provider identity is observed, and whether multiple controls justify a context object. + +## Acceptance criteria + +- Every retained web request/result/status field has a production reader or is required to execute the provider request. +- Tool-visible search/fetch output, provider fallback, abort behavior, configured timeout backstop, truncation, and citations remain covered. +- No `maxTimeoutMs`, request-timeout precedence branch, or one-field execution-context wrapper remains. +- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. + +## Risks + +Pre-release programmatic callers lose result provenance echoes and per-request fetch deadlines. The provider still has a deployment-configurable timeout and respects cancellation, so the simplification removes configurability rather than a safety bound. diff --git a/docs/rfc/proposed/simplification/2026-07-12-simplify-session-log-representation.md b/docs/rfc/proposed/simplification/2026-07-12-simplify-session-log-representation.md new file mode 100644 index 0000000000..d322e246df --- /dev/null +++ b/docs/rfc/proposed/simplification/2026-07-12-simplify-session-log-representation.md @@ -0,0 +1,36 @@ +# RFC: Simplify session-log representation + +Status: proposed + +## Problem + +The session log maintains two representations that cost more machinery than their consumers require: a pseudo-linked surface and custom request-header deltas. + +`SurfaceManager` stores the same order in an array, a seq map, and mutable `prev`/`next` links. Production never reads `prev`; compact's sole `next` read is the successor of an array position. Replacement already uses `indexOf`, so the links do not make its dominant operation constant-time. A seq array with linear replacement lookup has the same asymptotic replacement cost and one representation to validate. + +The request-header subsystem implements a custom system/tool delta codec and transmission-decision layer even though its contract says deltas are an encoding optimization, not a reconstructability requirement. A canonical full `request/header` only when the assembled header changes preserves replay while deleting `SystemDelta`, `ToolsDelta`, round-trip fallback, and the durable `request/header-delta` variant. Codec-only vocabulary disappears with the codec, not because its individual arms were invalid. + +This proposal deliberately retains append and replacement `sourceEventSeqs`, crash-repair provenance, and all `SessionStartSource` variants: implemented RFCs give those fields an audit/interception role that zero current readers does not overturn. + +## Proposal + +Make `SurfaceManager.nodes` a `readonly number[]` of event sequences and remove the public `SurfaceNode` shape. Keep the existing replacement generation; update tool-pairing balance and compaction callers to use array values/indices for predecessor, successor, and replacement ranges, removing node links and the seq-to-node map. Replace header deltas with canonical full changed-header snapshots and remove the delta codec/event/tests. + +Amend the session-surface and reconstructable-request RFCs where they describe the removed encoding. Update event types/invariants, request logging/replay, persistence fixtures, generated catalogs, package docs, and snapshots. Give changed full snapshots an explicit `change` reason (or simplify the reason contract as part of the same implementation), so they are distinguishable from initial/resume/fallback writes. + +`SESSION_FORMAT_VERSION` is deliberately pinned at `0`, so an old v0 log containing `request/header-delta` would otherwise pass the version check and silently lose header changes after the delta fold is deleted. Seed/load validation must reject that legacy event fail-loud at the format boundary; no compatibility fold or migration is added. + +## Alternatives considered + +**Keep linked nodes and compact deltas for possible scale.** Links could help a future cursor API, and deltas can reduce logs when large tool schemas change by a small amount. No shipped cursor uses the links, while full snapshots trade disk size for substantially simpler correctness. If header volume proves material, compression or a measured canonical-delta scheme can be designed around real traces. + +## Acceptance criteria + +- `SurfaceManager.nodes` is one ordered seq array with no `SurfaceNode`, link fields, or seq-to-node map; incremental append processing and replacement-generation invalidation remain. +- Replaying full changed-header snapshots reconstructs exactly the same requests; no header-delta event/type/codec remains. +- A v0 seed or persisted log containing legacy `request/header-delta` is rejected before replay, with coverage for JSONL and SQLite load paths. +- Current-version JSONL/SQLite replay, provenance, crash repair, compaction, snapshots, invariants, typecheck, coverage, doc-sync, build, and hygiene pass. + +## Risks + +Full headers increase log volume, and linear replacement lookup could be slower on very large surfaces. Replacements are already linear because the implementation calls `indexOf`; benchmarks should be added only if real traces show the simpler array is a bottleneck. Because the format version remains `0`, forgetting the explicit legacy-event rejection would be silent data corruption rather than a type error; the fail-loud load test is therefore part of the proposal, not optional cleanup. diff --git a/packages/core/system-prompt/src/index.ts b/packages/core/system-prompt/src/index.ts index 6d3ed3af58..d91b0eb07d 100644 --- a/packages/core/system-prompt/src/index.ts +++ b/packages/core/system-prompt/src/index.ts @@ -99,6 +99,8 @@ export interface PromptSection { export interface AssembledSection { /** The contributing section's unique name. */ name: string + // TODO(assembled-section-order): drop this output field; registry order has + // already sorted the array, and no production renderer/listener reads it. /** The contributing section's order (sections arrive sorted ascending). */ order: number /** The resolved (but not yet interpolated) section text. */ diff --git a/packages/ui/acp-agent/src/index.ts b/packages/ui/acp-agent/src/index.ts index 27b919f756..2de52bda60 100644 --- a/packages/ui/acp-agent/src/index.ts +++ b/packages/ui/acp-agent/src/index.ts @@ -72,6 +72,8 @@ export const Config: z = z.object({ // schemastery's native [] default would read as an invalid configured list. toolOrder: z.array(z.string()).default(undefined as unknown as string[]), tools: ToolRegistry.Config, + // TODO(single-default-literal): share this schema default and the defensive + // apply() fallback through one named constant while retaining both boundaries. persistenceRoot: z.string().default('./.sessions'), skills: agentCore.SkillConfigSchema, }) diff --git a/packages/ui/acp/src/index.ts b/packages/ui/acp/src/index.ts index a3ce9b5117..1abd687af1 100644 --- a/packages/ui/acp/src/index.ts +++ b/packages/ui/acp/src/index.ts @@ -362,6 +362,8 @@ export function apply(ctx: Context, config: AcpConfig): void { // this warn sink so a throwing tool presenter is logged, not propagated. const makePresenter = (agent?: Agent): ToolPresenter => new ToolPresenter(tools, (message) => { logger.warn(message) }, agent) + // TODO(derive-acp-session-id): derive an event's id from agent.session and + // verify sessions.get(id)?.agent === agent; then this reverse map disappears. // Live sessions keyed by id (RFC 011 multi-session), plus an agent→sessionId // reverse map so `agent/*` events (which carry only the Agent) demux in O(1). // The two stay in lockstep: a record is added to `sessions` and the agent to diff --git a/packages/ui/stdio-agent/src/index.ts b/packages/ui/stdio-agent/src/index.ts index 1037c28112..127851eaf5 100644 --- a/packages/ui/stdio-agent/src/index.ts +++ b/packages/ui/stdio-agent/src/index.ts @@ -93,6 +93,8 @@ export const Config: z = z.object({ // schemastery's native [] default would read as an invalid configured list. toolOrder: z.array(z.string()).default(undefined as unknown as string[]), tools: ToolRegistry.Config, + // TODO(single-default-literal): share these schema defaults and defensive + // apply() fallbacks through named constants while retaining both boundaries. persistenceRoot: z.string().default('./.sessions'), welcome: z.string().default('ready.'), skills: agentCore.SkillConfigSchema, diff --git a/packages/ui/stdio-agent/src/stdio-chat.ts b/packages/ui/stdio-agent/src/stdio-chat.ts index 4c37f40d37..8e3c1f467b 100644 --- a/packages/ui/stdio-agent/src/stdio-chat.ts +++ b/packages/ui/stdio-agent/src/stdio-chat.ts @@ -36,6 +36,8 @@ export const inject = ['agents', 'userInteraction'] export interface Config { /** Banner printed once on start, before the first `> ` prompt. */ welcome?: string + // TODO(fixed-stdio-agent): this app-internal plugin is mounted only for the + // precreated `main` agent; remove configurability and its config-only test. /** Id of the agent stdin drives (`send`/`steer`) and whose status gates the EOF exit; rendering is global. Defaults to `'main'`. */ agent?: string } From 90adf1003f39f92381819c27db2a3f56ca615ed0 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 13 Jul 2026 15:06:54 +0800 Subject: [PATCH 2/4] docs: refine simplification audit after master merge --- docs/config-catalog.md | 2 +- docs/rfc/INDEX.md | 2 +- .../2026-06-20-unify-agent-and-session-id.md | 2 +- ...026-07-04-prune-dead-core-spine-surface.md | 23 ++++++++++--- ...12-collapse-workflow-to-foreground-core.md | 9 ++++-- ...6-07-12-prune-unconsumed-registry-modes.md | 32 ------------------- ...-12-prune-unused-skill-registry-surface.md | 27 ++++++++++++++++ ...-12-simplify-session-log-representation.md | 10 +++--- .../coding-agent/tests/keyless-smoke.e2e.ts | 2 ++ packages/core/agent/src/index.ts | 2 ++ packages/guard/repeat-tool-guard/src/index.ts | 2 ++ packages/ui/acp/src/index.ts | 9 ++++-- packages/workflow/tool-workflow/README.md | 4 +-- 13 files changed, 74 insertions(+), 52 deletions(-) delete mode 100644 docs/rfc/proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md create mode 100644 docs/rfc/proposed/simplification/2026-07-12-prune-unused-skill-registry-surface.md diff --git a/docs/config-catalog.md b/docs/config-catalog.md index e4e7ad8d3f..93f8e311f9 100644 --- a/docs/config-catalog.md +++ b/docs/config-catalog.md @@ -31,7 +31,7 @@ export interface AcpConfig { Depends on: `Stream` (`@agentclientprotocol/sdk`) -Source: [`packages/ui/acp/src/index.ts:248`](../packages/ui/acp/src/index.ts) +Source: [`packages/ui/acp/src/index.ts:250`](../packages/ui/acp/src/index.ts) ## `@deepseek-ai/dsh-acp-agent` diff --git a/docs/rfc/INDEX.md b/docs/rfc/INDEX.md index e4a2320f76..21ee29a58d 100644 --- a/docs/rfc/INDEX.md +++ b/docs/rfc/INDEX.md @@ -19,7 +19,7 @@ Generated by `pnpm run gen-rfc-index` from the RFC tree — never edit by hand; | [Prune dead public and result surface](proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md) | 2026-07-04 | | [Collapse workflows to the exercised foreground core](proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md) | 2026-07-12 | | [Drop unconsumed registry events](proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md) | 2026-07-12 | -| [Prune unconsumed registry modes](proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md) | 2026-07-12 | +| [Prune unused skill registry surface](proposed/simplification/2026-07-12-prune-unused-skill-registry-surface.md) | 2026-07-12 | | [Prune unused web seam fields](proposed/simplification/2026-07-12-prune-unused-web-seam-fields.md) | 2026-07-12 | | [Simplify session-log representation](proposed/simplification/2026-07-12-simplify-session-log-representation.md) | 2026-07-12 | diff --git a/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md b/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md index aaad0877c7..b3670dd9a1 100644 --- a/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md +++ b/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md @@ -10,7 +10,7 @@ ACP already uses the same value for both identities. Where they diverge, stdio k The [agent-scope runtime](../../implemented/architecture/2026-07-12-agent-scope-runtime-design.md) has no reservation side tables: create and resume use one `AgentCreationTransaction`, and agent/session entries use the same final-entry collision rule. Separate ids therefore do not duplicate asynchronous liveness, rollback, or quiescence machinery. Identity unification is only an API and representation simplification: it deletes one caller-supplied id, one UUID per in-process child, and the remaining translation paths without changing the transaction lifecycle. -Session itself repeats the same fact as `Session.id` and `Session.header.id`. Valid store paths construct them equal, but the constructor does not enforce equality and production consumers choose between the two. The duplicate creates an impossible-but-representable mismatch inside the object that owns session identity. +Session itself repeats the same fact as `Session.id` and `Session.header.id`. Construction rejects a header whose id differs, so the aliases are constrained equal; the durable boundary must nevertheless validate the duplicate, and production consumers choose between its two homes. ## Proposal diff --git a/docs/rfc/proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md b/docs/rfc/proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md index 58cc6d788c..cca7c57b34 100644 --- a/docs/rfc/proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md +++ b/docs/rfc/proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md @@ -6,7 +6,7 @@ Status: proposed Several package-root exports, result fields, and convenience methods have no production consumer. They survive because tests import internals through public entry points or because a type anticipated a caller that never arrived. Each item is small in isolation, but together they enlarge the SDK contract, generated catalogs, documentation, and regression matrix without enabling a shipped path. -The production corpus is `packages/*/*/src`, example sources/config, and runtime scripts; tests, package READMEs, generated catalogs, and RFC prose are evidence of publication but not consumers. Exact-symbol searches produce the following inventory: +The production corpus is `packages/*/*/src`, example sources/config, and runtime scripts. Tests, package READMEs, and RFC prose are evidence of publication but not fixed callers. `cordis_inspect` makes `packages/cordis/tool-cordis/src/api-catalog.ts` model-visible, and `cordis_mount` can invoke injected services through guarded real-service proxies, so catalogued service methods and returned shapes are a genuine dynamic product surface. The table therefore distinguishes absence of a fixed repository caller from unreachability: rows touching catalogued vocabulary intentionally contract what model-written mounts can discover and call, while package-root implementation helpers are not reached through that service façade. Exact-symbol searches produce the following inventory: | Surface | Production evidence | Simplification | | --- | --- | --- | @@ -15,17 +15,28 @@ The production corpus is `packages/*/*/src`, example sources/config, and runtime | `ReactLoopAgent` root export | Outside-package named imports are tests; production programs against `Agent` and creates/resumes through `ctx.agents`. | Return/interface-type `Agent` and make the concrete loop class package-internal; keep the deliberate synchronous config-only `AgentLoop.create()` path. | | `workflow-workerthread` protocol/runtime/session re-exports and named `WorkerWorkflowEngine` | Every package-name consumer uses the default engine; the workflow RFC already defines the worker wire protocol as private. | Keep the default plugin class/config contract; drop the duplicate named class export and keep protocol modules source-private. | | `code-runtime-worker` protocol/bootstrap re-exports | Outside-package production/e2e consumers use `WorkerCodeRuntime` and config, not `BootstrapPort`, `PatchableStream`, or worker message/boot types. | Keep the runtime class/config contract and make its wire/bootstrap vocabulary source-private. | +| ACP translation/presenter root exports | `agentOptions`, `streamSessionEventUpdate`, `todosToPlan`, `ToolPresenter`, `nullToolPresenter`, and `TerminalRendering` have only same-file or ACP-test consumers; the sole outside-package production consumer mounts the plugin namespace. | Keep `name`, `inject`, `Config`, `AcpConfig`, and `apply`; make translation/presentation helpers source-private and test them in-package. | | `providerWording` and `completedTurnPrefix` root exports | Each has one same-package production caller; only the balanced-prefix helper has a same-package white-box test. | Make them source-private and test provider behavior. | | `depthOf`, `SubagentDepthError`, `SENSITIVE_ENV_PATTERN`, `waitForExit`, and `exitsWithin` root exports | Production subagent backends consume the in-process runner and subprocess construction/disposal helpers, not these enforcement/test internals. | Keep depth/environment/exit behavior but make the helpers and error/regex source-private; test through spawn and disposal. | -| `PersistenceCoordinator.inits`, backend `inits` accessors, `seedCoversPrefix`, and `assertSerializable` | The accessors exist for white-box tests; the helpers have no outside production importer. | Observe initialization through `session/flush` and internalize the helpers. Keep both backends, `SessionHeader`, and SQLite's version contract. | -| `LlmService.models()` | Tests/docs only; production resolves a configured adapter directly. | Delete the convenience while keeping both LLM adapters. | +| `PersistenceCoordinator.inits`, backend `inits` accessors, `seedCoversPrefix`, and `assertSerializable` | The accessors exist for white-box tests; `seedCoversPrefix` has no outside production importer; `assertSerializable` has no production caller and duplicates the coordinator append boundary's lossless snapshot. | Observe initialization through `session/flush`, make `seedCoversPrefix` source-private, and delete `assertSerializable`. Keep both backends, `SessionHeader`, and SQLite's version contract. | | `LlmError.status` and replay status | Adapters/replay populate it, but production branches on stable error code/message and never reads raw status. | Remove the unread field and replay plumbing while preserving error classification. | | `BlockAssembler.push()` return value | Both production callers ignore the returned completed block. | Return `void`; keep the deliberately public `blocks()`/`message()` contract. | -| `compactRegion`'s separate `session` argument | The sole production caller passes the same object already present as `agent.session`; the API permits an incoherent pair. | Use `agent.session` as the one source of truth. | +| `compactRegion`'s separate `session` argument | The fixed caller passes the same object already present as `agent.session`; the model-visible mount API can also call the method, but accepting two identities permits a mounted plugin to provide an incoherent pair. | Keep the manual-region seam while deliberately narrowing it to `agent.session` as the one source of truth. | | `CompactionResult.startSeq`, `summarySeq`, `endSeq`, and `summary` | The production consumer reads only shadowed range/seq/token accounting; the durable log owns summary and event identity. | Remove the four result echoes while keeping both shared transcript renderers. | | `BasicCompactService` estimation/summarization visibility | No outside production caller invokes the five methods; the implemented RFC names only `estimateContentTokens()` and `summarize()` as subclass hooks. | Make those two `protected` and the three orchestration-only estimators private. | | `CodeLogEntry.source`/`level` and `RunCodeMeta.dispatches` | Every production consumer maps logs to text; no presenter/model path reads the other fields or the persisted dispatch count. | Make code-runtime logs strings (or text-only entries) and remove result-meta dispatch plumbing; keep the local counter that mints deterministic dispatch ids. | | `ToolNotFoundError.toolName`, `SystemPrompt.config`, and `BashTask.command` | Each stored public value has no production reader. | Drop the unread field while retaining error messages, resolved configuration behavior, and task lifecycle. | +| Backend package-root implementation helpers | The exact inventory below is called only through relative same-package imports. Production namespace imports mount the retained plugin contract without reading these properties; named root consumers are tests. | Retain each adapter/provider/service and its config/error contract; stop exporting the listed helper functions/constants at package roots. | +| Consumer package-root implementation helpers | The exact inventory below has only same-package production callers. Production namespace imports mount plugin contracts without reading helper properties; named root consumers are tests. | Retain plugin contracts and stable error codes; move tests to package-local modules or public behavior and stop exporting the listed helpers at package roots. | + +### Grouped helper-export inventory + +- `dsh-llm-deepseek`: `httpErrorCode`, `serializeMessages`, `serializeRequest`, `DONE`, `parseSse`, `mapFinishReason`, `mapUsage`, and `translate`; `dsh-llm-pi-ai`: `buildModel`, `mapStopReason`, `mapUsage`, `toPiContext`, and `toStreamChunks`. +- `dsh-bash-local`: `DEFAULT_GRACE_MS`, `ENV_OVERRIDES`, `killGroup`, `OutputCollector`, and `runBash`; `dsh-bash-sandbox`: `shellQuote`, `classifyDenial`, and `classifyRunnerFailure`; `dsh-sandbox-local`: `bwrapProfileArgs`, `landlockProfileArgs`, and `seatbeltProfileArgs`. The public mutable test-injection fields and their types are outside this proposal. +- `dsh-fs-local`: `applyLiteralEdit`, `listDirectory`, `probe`, `readForEdit`, `readTextForDiff`, `readWholeText`, `resolveLocalTarget`, `restoreLineEndings`, `streamWholeText`, and `writeFileAtomic`. +- `dsh-web-fetch-local`: `classifyContentType`, `decoderForCharset`, `isSameOrigin`, `parseCharset`, and `validateFetchUrl`; `dsh-web-search-exa`: `mapExaResponse` and `mapExaResult`; `dsh-web-search-deepseek`: `citationSnippets` and `mapAnthropicResponse`; `dsh-web-search-perplexity`: `mapPerplexityResponse` and `mapPerplexityResult`. +- `dsh-tool-fs`: `READ_LIMIT`, `STREAM_MIN_SIZE`, `READ_MAX_BYTES`, `READ_MAX_LINE_LENGTH`, `DIFF_CONTEXT`, `applyReadTool`, `parseReadArgs`, `applyWriteTool`, `formatWriteOutput`, `parseWriteArgs`, `applyEditTool`, `formatEditOutput`, `parseEditArgs`, `buildWindow`, `formatReadOutput`, `computeHunkDiffs`, and `diffsFromMeta`. +- `dsh-tool-web`: `WEB_SEARCH_MAX_RESULTS`, `applyWebSearchTool`, `formatSearchOutput`, `parseSearchArgs`, `presentSearchCall`, `applyWebFetchTool`, `formatFetchOutput`, `parseFetchArgs`, `presentFetchCall`, `renderBody`, and `htmlToMarkdown`; `dsh-timeout-policy`: `toolTimeoutResult`; `dsh-compact-basic`: `resolveConfig`; `dsh-tool-bash`: `renderResult`. ## Proposal @@ -35,6 +46,8 @@ Remove or demote every row as one bounded coordinated public-surface cleanup. Up **Keep test conveniences and self-contained results public.** Public helpers can make white-box tests convenient, self-contained result fields can look ergonomic, and future embedders might want the concrete loop or enumeration methods. Those benefits are hypothetical; today they make every implementation and document explain states that no shipped caller can observe. A real consumer can introduce the smallest contract it needs, with its ownership and failure semantics known. +**Keep every catalogued member for model-written mounts.** The self-referential toolset is a real generic consumer route, not generated-doc noise. Its value comes from an accurate, composable service surface, however, not from preserving duplicate fields or incoherent argument pairs indefinitely; each catalogued contraction above removes a fact available elsewhere on the same execution, agent, or result and updates the API reference in the same change. + ## Acceptance criteria - Exact-symbol searches show no removed surface outside this RFC and any implemented-RFC amendments. @@ -44,4 +57,4 @@ Remove or demote every row as one bounded coordinated public-surface cleanup. Up ## Risks -Most removals are compile-visible but runtime-neutral. The compaction argument cleanup deliberately forbids a session/context mismatch that no caller uses; the remaining changes can require external pre-release embedders to import less or adjust result shapes. The repository is unreleased, so carrying unsupported surface is the larger foundation cost. +Most removals are compile-visible but runtime-neutral. The compaction argument cleanup deliberately forbids a session/context mismatch while retaining the manual-region seam. External pre-release embedders and existing model-written mounts may import fewer helpers, pass fewer arguments, or receive narrower result shapes; this is an intentional product-surface contraction, not merely generated-catalog cleanup. The repository is unreleased, so carrying unsupported surface is the larger foundation cost. diff --git a/docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md b/docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md index 2540d36b91..d112aa2a3f 100644 --- a/docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md +++ b/docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md @@ -8,11 +8,15 @@ The workflow capability executes foreground JavaScript that composes subagents, The progress vocabulary is not merely unused; it cannot serve its only named future owner without redesign. `WorkflowRunInfo` contains `{id, meta}` but no parent agent, session, or tool-call identity, while the model-facing tool never exposes the run id. A global ACP listener could not route an event to the correct client session. `meta.phases` is never consulted, `phase(title)` does not validate against it, phase `detail`/`model` and agent `label`/`phase` feed only events, and `whenToUse` is validated and copied but never rendered or selected. `phase()` and `log()` still cross the worker boundary despite having no receiver. +The live handle repeats event-era data after those observers disappear. `WorkflowRun.id` has no non-event consumer, while the tool reads `run.meta.name` only to render a value it already owns as `args.meta.name`; neither belongs on the execution/cancellation handle. + +Cancellation also has two public channels for one synchronous start. `WorkflowStartRequest.signal` is passed to the worker host, while the sole production caller separately bridges the same signal to `WorkflowRun.cancel()`. Because `start()` returns the run before control can yield, there is no readiness window that requires request-time cancellation; the duplicate signal adds host listener/disarm state without closing a race. + `WorkflowError.fatal` is the same speculative branch in miniature: every production construction is fatal, `fatal: false` exists only in tests, and combinators already distinguish workflow failures with `instanceof`. ## Proposal -Keep the exercised core: `agent(prompt, { schema, model })`, `parallel`, `pipeline`, `args`, concurrency/agent caps, cancellation, bounded disposal, structured results, worker isolation, and foreground tool collection. Remove all `workflow/*` events and their event-only info/outcome types; remove `phase()`, `log()`, agent `label`/`phase`, phase declarations, `whenToUse`, and their worker messages/host observers; collapse workflow metadata to the name the tool actually uses; remove event-only run ids/meta snapshots and the synthesized agent-end ledger. Make `WorkflowError` one fatal error class without a boolean mode or `isFatalWorkflowError()` helper. +Keep the exercised core: `agent(prompt, { schema, model })`, `parallel`, `pipeline`, `args`, concurrency/agent caps, cancellation, bounded disposal, structured results, worker isolation, and foreground tool collection. Remove all `workflow/*` events and their event-only info/outcome types; remove `phase()`, `log()`, agent `label`/`phase`, phase declarations, `whenToUse`, and their worker messages/host observers; collapse workflow metadata to the name the tool actually uses; remove event-only run ids/meta snapshots and the synthesized agent-end ledger. Shrink `WorkflowRun` to `result`, `cancel()`, and `dispose()`; the tool renders the request-owned name. Remove `WorkflowStartRequest.signal` and the worker host's input-signal listener/disarm state, retaining the caller-owned bridge from its abort signal to `run.cancel()`. Make `WorkflowError` one fatal error class without a boolean mode or `isFatalWorkflowError()` helper. Amend the implemented dynamic-workflow RFC and update the seam/tool/worker READMEs, tool schema, generated catalogs and package graph, worker type-equivalence records, unit tests, and workflow snapshot/header fixtures. Progress UI work, if commissioned, starts from a correlation contract that names the parent agent/session/tool call instead of reviving this protocol unchanged. @@ -24,9 +28,10 @@ Amend the implemented dynamic-workflow RFC and update the seam/tool/worker READM - The workflow public seam contains only execution, cancellation, result, and disposal contracts with a production consumer. - No workflow event, phase/log protocol message, run-id generator, progress-only metadata, host pairing ledger, or fatal-mode branch remains. +- The run handle has no id/meta echoes, and cancellation has one holder-owned channel after synchronous `start()` returns. - Parallel/pipeline behavior, caps, cancellation quiescence, worker containment, structured output, and the model-facing workflow scenarios retain coverage. - Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. ## Risks -This is a compile-visible contraction of the workflow DSL and event taxonomy. Existing unshipped scripts that use `phase`, `log`, labels, or descriptive metadata must shrink, and a future observer must add a better-correlated seam. The execution semantics that make workflows useful do not change. +This is a compile-visible contraction of the workflow DSL, event taxonomy, handle, and start request. Existing workflow calls that supply descriptive metadata, and scripts that use `phase`, `log`, or labels, must shrink; programmatic callers bridge their own abort source to the returned handle; and a future observer must add a better-correlated seam. The execution semantics that make workflows useful do not change. diff --git a/docs/rfc/proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md b/docs/rfc/proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md deleted file mode 100644 index c85af20f43..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-12-prune-unconsumed-registry-modes.md +++ /dev/null @@ -1,32 +0,0 @@ -# RFC: Prune unconsumed registry modes - -Status: proposed - -## Problem - -Two registries implement modes that no production registration populates. - -The skill service's embedded-runtime subsystem has zero production caller of `ctx.skills.register()`. It adds a reserved `runtime` provider name, a runtime map/rank/source, duplicate policy, a second revision in cache keys, normalization, disposers, and tests alongside the provider seam every shipped skill already uses. `SkillSummary.whenToUse` and candidate/definition `path` are parsed and copied but never read by a production consumer: the model catalog renders name/description, resource loading uses `resourceBase`, and providers own their locator. The deliberately open `metadata` extension point stays. - -`SystemPrompt` supports scope-local tool and variable providers, but every production `systemPrompt.tools()` and `systemPrompt.variable()` registration is global. Scoped assembly executes the merge path each step but finds no scoped tool/variable contribution. Scoped sections are live and stay. Supporting empty scope-local tool/variable layers adds maps plus merge/shadow/cleanup branches for combinations the product never constructs. - -## Proposal - -Remove `SkillService.register()`, `SkillRegistration`, the runtime pseudo-provider and reserved-name rules, runtime revisions/cache branches, and runtime-only source/rank normalization. Tests that need an embedded skill register a small real provider. Retain `providerRevision` as the in-flight discovery epoch, but key completed catalogs by cwd alone: every provider mutation synchronously clears the cache, and the post-await revision comparison already prevents inserting stale work. Remove `whenToUse`, `SkillCandidate.path`, and `SkillDefinition.path` from the skill contract and local-provider copies while retaining provider locator/root paths; retain `metadata`, `disableModelInvocation`, `source`, `provider`, `locator`, and `resourceBase` as either deliberate extension vocabulary or production-consumed fields. - -Keep system-prompt sections scoped, but make tool-schema and variable providers global-only and delete their scoped maps/merge logic. Fail loud if a caller attempts these unsupported scope/mode combinations instead of silently widening them. Keep both global and scoped tool guards: the interception design deliberately defines them as monotonic policy APIs. Amend the skill-system and agent-scope RFCs, READMEs, JSDoc, catalogs, and tests. - -## Alternatives considered - -**Keep all registry modes for embedders.** Runtime skill registration is convenient, and scoped variables/tool-schema fragments could support per-agent prompt customization. Neither has a shipped owner. A real embedded skill can be a provider; per-agent executable tools already belong in `agent.ctx.tools`; per-agent prompt facts can use scoped sections or context-aware global providers. - -## Acceptance criteria - -- Skill collection has one provider-backed path, a cwd-only completed-cache key, and a revision epoch only for in-flight invalidation; retained skill fields have a production reader or a recorded deliberate extension contract. -- System-prompt tools/variables have one global path, while sections retain their scoped behavior. -- Global and scoped tool guards plus structured-output commit behavior in native and Code Mode remain covered. -- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. - -## Risks - -These are compile-visible contractions of two pre-release registries. The fail-loud rule must distinguish an unsupported scoped registration from an ordinary global registration without disturbing Cordis effect cleanup, and skill-local frontmatter parsing must continue to preserve and validate the supported metadata schema. diff --git a/docs/rfc/proposed/simplification/2026-07-12-prune-unused-skill-registry-surface.md b/docs/rfc/proposed/simplification/2026-07-12-prune-unused-skill-registry-surface.md new file mode 100644 index 0000000000..1df837e026 --- /dev/null +++ b/docs/rfc/proposed/simplification/2026-07-12-prune-unused-skill-registry-surface.md @@ -0,0 +1,27 @@ +# RFC: Prune unused skill registry surface + +Status: proposed + +## Problem + +The skill service's embedded-runtime subsystem has zero production caller of `ctx.skills.register()`. It adds a reserved `runtime` provider name, a runtime map/rank/source, duplicate policy, a second revision in cache keys, normalization, disposers, and tests alongside the provider seam every shipped skill already uses. `SkillSummary.whenToUse` and candidate/definition `path` are parsed and copied but never read by a production consumer: the model catalog renders name/description, resource loading uses `resourceBase`, and providers own their locator. The deliberately open `metadata` extension point stays. + +## Proposal + +Remove `SkillService.register()`, `SkillRegistration`, the runtime pseudo-provider and reserved-name rules, runtime revisions/cache branches, and runtime-only source/rank normalization. Tests that need an embedded skill register a small real provider. Retain `providerRevision` as the in-flight discovery epoch, but key completed catalogs by cwd alone: every provider mutation synchronously clears the cache, and the post-await revision comparison already prevents inserting stale work. Remove `whenToUse`, `SkillCandidate.path`, and `SkillDefinition.path` from the skill contract and local-provider copies while retaining provider locator/root paths; retain `metadata`, `disableModelInvocation`, `source`, `provider`, `locator`, and `resourceBase` as either deliberate extension vocabulary or production-consumed fields. + +Amend the skill-system RFC, README, JSDoc, catalogs, and tests. Agent-scoped system-prompt sections, tool providers, and variables are explicitly outside this proposal: final #224 made all three part of the `setup(agent.ctx)` contributor contract, so absence of a fixed in-repo scoped registration is not evidence of non-consumption. + +## Alternatives considered + +**Keep runtime skill registration for embedders.** It is a deliberate synchronous direct-definition convenience in the implemented skill RFC. A small provider wrapper can expose the same embedded data under effect-owned lifetime, but it must implement async `list()`/`get()`, carry provider identity, and accept provider duplicate semantics. The proposal chooses that one regular path over preserving a second ranking, validation, cache-invalidation, and lookup path. + +## Acceptance criteria + +- Skill collection has one provider-backed path, a cwd-only completed-cache key, and a revision epoch only for in-flight invalidation; retained skill fields have a production reader or a recorded deliberate extension contract. +- Agent-scoped prompt sections, variables, tool providers, tool guards, and structured-output commit behavior in native and Code Mode remain unchanged. +- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. + +## Risks + +This is a compile-visible contraction of the pre-release skill registry. External programmatic `list()`/`get()` consumers lose `whenToUse` routing hints and candidate/definition `path`; the shipped model catalog never renders them, and resource resolution keeps its explicit `resourceBase` plus provider-owned opaque locator, but those fields are not observationally identical. Skill-local frontmatter parsing must continue to preserve and validate the supported metadata schema, and external providers remain able to supply embedded, filesystem, remote, or other skill sources. diff --git a/docs/rfc/proposed/simplification/2026-07-12-simplify-session-log-representation.md b/docs/rfc/proposed/simplification/2026-07-12-simplify-session-log-representation.md index d322e246df..f335afafe1 100644 --- a/docs/rfc/proposed/simplification/2026-07-12-simplify-session-log-representation.md +++ b/docs/rfc/proposed/simplification/2026-07-12-simplify-session-log-representation.md @@ -8,15 +8,15 @@ The session log maintains two representations that cost more machinery than thei `SurfaceManager` stores the same order in an array, a seq map, and mutable `prev`/`next` links. Production never reads `prev`; compact's sole `next` read is the successor of an array position. Replacement already uses `indexOf`, so the links do not make its dominant operation constant-time. A seq array with linear replacement lookup has the same asymptotic replacement cost and one representation to validate. -The request-header subsystem implements a custom system/tool delta codec and transmission-decision layer even though its contract says deltas are an encoding optimization, not a reconstructability requirement. A canonical full `request/header` only when the assembled header changes preserves replay while deleting `SystemDelta`, `ToolsDelta`, round-trip fallback, and the durable `request/header-delta` variant. Codec-only vocabulary disappears with the codec, not because its individual arms were invalid. +The request-header subsystem implements a custom system/tool delta codec and transmission-decision layer even though its contract says deltas are an encoding optimization, not a reconstructability requirement. Retaining the initial/resume full snapshot at each loop-instance boundary, then writing a canonical full `request/header` whenever that instance's assembled header changes, preserves replay while deleting `SystemDelta`, `ToolsDelta`, round-trip fallback, and the durable `request/header-delta` variant. Codec-only vocabulary disappears with the codec, not because its individual arms were invalid. This proposal deliberately retains append and replacement `sourceEventSeqs`, crash-repair provenance, and all `SessionStartSource` variants: implemented RFCs give those fields an audit/interception role that zero current readers does not overturn. ## Proposal -Make `SurfaceManager.nodes` a `readonly number[]` of event sequences and remove the public `SurfaceNode` shape. Keep the existing replacement generation; update tool-pairing balance and compaction callers to use array values/indices for predecessor, successor, and replacement ranges, removing node links and the seq-to-node map. Replace header deltas with canonical full changed-header snapshots and remove the delta codec/event/tests. +Make `SurfaceManager.nodes` a `readonly number[]` of event sequences and remove the public `SurfaceNode` shape. Keep the internal replace-generation signal; update tool-pairing balance and compaction callers to use array values/indices for predecessor, successor, and replacement ranges, removing node links and the seq-to-node map. Replace post-anchor header deltas with canonical full changed-header snapshots and remove the delta codec/event/tests; initial and resume anchors remain full snapshots even when the folded header is unchanged. -Amend the session-surface and reconstructable-request RFCs where they describe the removed encoding. Update event types/invariants, request logging/replay, persistence fixtures, generated catalogs, package docs, and snapshots. Give changed full snapshots an explicit `change` reason (or simplify the reason contract as part of the same implementation), so they are distinguishable from initial/resume/fallback writes. +Amend the session-surface and reconstructable-request RFCs where they describe the removed encoding. Update event types/invariants, request logging/replay, persistence fixtures, generated catalogs, package docs, and snapshots. Replace the codec-only `fallback` reason with an explicit `change` reason for post-anchor full snapshots, distinguishing them from the retained `initial` and `resume` anchors. `SESSION_FORMAT_VERSION` is deliberately pinned at `0`, so an old v0 log containing `request/header-delta` would otherwise pass the version check and silently lose header changes after the delta fold is deleted. Seed/load validation must reject that legacy event fail-loud at the format boundary; no compatibility fold or migration is added. @@ -26,10 +26,10 @@ Amend the session-surface and reconstructable-request RFCs where they describe t ## Acceptance criteria -- `SurfaceManager.nodes` is one ordered seq array with no `SurfaceNode`, link fields, or seq-to-node map; incremental append processing and replacement-generation invalidation remain. +- `SurfaceManager.nodes` is one ordered seq array with no `SurfaceNode`, link fields, or seq-to-node map; incremental append processing and the internal replace-generation signal remain, while the separate public `invalidate()` deletion stays owned by the dead-surface RFC. - Replaying full changed-header snapshots reconstructs exactly the same requests; no header-delta event/type/codec remains. - A v0 seed or persisted log containing legacy `request/header-delta` is rejected before replay, with coverage for JSONL and SQLite load paths. -- Current-version JSONL/SQLite replay, provenance, crash repair, compaction, snapshots, invariants, typecheck, coverage, doc-sync, build, and hygiene pass. +- New-shape v0 JSONL/SQLite replay, provenance, crash repair, compaction, snapshots, invariants, typecheck, coverage, doc-sync, build, and hygiene pass. ## Risks diff --git a/examples/coding-agent/tests/keyless-smoke.e2e.ts b/examples/coding-agent/tests/keyless-smoke.e2e.ts index c6b9f930a8..ea223bbad4 100644 --- a/examples/coding-agent/tests/keyless-smoke.e2e.ts +++ b/examples/coding-agent/tests/keyless-smoke.e2e.ts @@ -25,6 +25,8 @@ import { afterEach, describe, expect, it } from 'vitest' * product. */ +// TODO(loader-smoke-harness): extract the shared spawn/tempdir/timeout/EOF +// harness used here, code-mode-keyless-smoke, and cordis-agent's keyless smoke. // The dsh-stdio-agent bin (the demo:repl entry) and this example's cordis.yml. // The bin resolves its config-path arg from CWD; the test spawns from a temp // cwd, so we pass the example config's ABSOLUTE path. diff --git a/packages/core/agent/src/index.ts b/packages/core/agent/src/index.ts index b77e3e8a60..7864f5938c 100644 --- a/packages/core/agent/src/index.ts +++ b/packages/core/agent/src/index.ts @@ -202,6 +202,8 @@ interface FactorySlot { */ export class AgentRegistry extends Service { private store = new Map() + // TODO(agent-entry-mirror): derive exact-object checks from store.get(agent.id) + // plus entry.agent identity; this WeakMap mirrors the authoritative id map. private entries = new WeakMap() private factory: FactorySlot | undefined diff --git a/packages/guard/repeat-tool-guard/src/index.ts b/packages/guard/repeat-tool-guard/src/index.ts index 919d0541ba..6a5662d693 100644 --- a/packages/guard/repeat-tool-guard/src/index.ts +++ b/packages/guard/repeat-tool-guard/src/index.ts @@ -202,6 +202,8 @@ export function apply(ctx: Context, config: Config): void { throw new Error(`repeat-tool-guard: invalid argumentsPreviewChars ${argumentsPreviewChars} — must be an integer >= 1`) } + // TODO(agent-keyed-repeat-chain): key a WeakMap by the Agent itself; that + // removes the disposal-only status listener and cannot collide on id reuse. const chains = new Map() /** Whether a tool participates in the chain (untracked calls are transparent: they neither count nor reset). */ diff --git a/packages/ui/acp/src/index.ts b/packages/ui/acp/src/index.ts index de2dec6278..a3fdc38f97 100644 --- a/packages/ui/acp/src/index.ts +++ b/packages/ui/acp/src/index.ts @@ -107,6 +107,8 @@ export const name = 'acp' // because `initialize` advertises `loadSession: true`. `tools` lets a tool own // how its calls render (`presentCall`/`presentResult`); the bridge looks up the // definition by name and falls back to a generic presentation when absent. +// TODO(acp-session-inject): drop `sessions`; this bridge never reads +// ctx.sessions, and agent/session ownership is already behind ctx.agents. export const inject = ['agents', 'sessions', 'sessionPersistence', 'tools', 'userInteraction'] /** @@ -353,11 +355,12 @@ export function apply(ctx: Context, config: AcpConfig): void { const makePresenter = (agent?: Agent): ToolPresenter => new ToolPresenter(tools, (message) => { logger.warn(message) }, agent) // TODO(derive-acp-session-id): derive an event's id from agent.session and - // verify sessions.get(id)?.agent === agent; then this reverse map disappears. + // verify sessions.get(id)?.agent === agent; then remove this reverse map and + // SessionRecord.sessionId, whose sole read duplicates the same identity. // Live sessions keyed by id (RFC 011 multi-session), plus an agent→sessionId // reverse map so `agent/*` events (which carry only the Agent) demux in O(1). - // The two stay in lockstep: a record is added to `sessions` and the agent to - // `bySession` together, and removed together. + // The forward record and weak reverse entry are installed together; removing + // the record releases its strong Agent reference, so the WeakMap entry expires. const sessions = new Map() const bySession = new WeakMap() // Session ids whose `session/load` is mid-`resume()` (the slot is reserved diff --git a/packages/workflow/tool-workflow/README.md b/packages/workflow/tool-workflow/README.md index 44160d8fc1..a9c8900c4e 100644 --- a/packages/workflow/tool-workflow/README.md +++ b/packages/workflow/tool-workflow/README.md @@ -4,7 +4,7 @@ The model-facing **`workflow` tool**: run a JavaScript orchestration script that ## What the model sees -Two parameters: `script` (required — the full `export const meta = {...}` + body text; the tool DESCRIPTION carries the complete authoring contract: hooks, semantics, the supported schema subset) and `args` (optional JSON object exposed to the script as the `args` global; a bare list is wrapped as a field, a deliberate deviation from Claude Code's any-JSON `args` so the wire schema stays honest). The plugin also contributes a `tool:` system-prompt section carrying the usage policy — use the tool only on an explicit user ask for a workflow / large orchestration; prefer plain subagent calls for one or two delegations — per the convention that tool guidance ships with the tool plugin, never in the deployment persona. +Three parameters: `meta` (required identity data: `name`, `description`, and optional progress annotations), `script` (required plain JavaScript body — no `export const meta` statement; the tool description carries the complete authoring contract), and `args` (optional JSON object exposed to the script as the `args` global; wrap a bare list in a field so the wire schema stays honest). The plugin also contributes a `tool:` system-prompt section carrying the usage policy — use the tool only on an explicit user ask for a workflow / large orchestration; prefer plain subagent calls for one or two delegations — per the convention that tool guidance ships with the tool plugin, never in the deployment persona. ## Lifecycle @@ -12,7 +12,7 @@ Collection is SYNCHRONOUS this cut (like [`dsh-tool-subagent`](../../subagent/to ## Render intent -Decided up front (per the [render-intent RFC](../../../docs/rfc/implemented/architecture/2026-07-02-tool-render-intent-union.md)): a `generic` card titled `workflow: `, the name sniffed TEXTUALLY from `args.script` (presentation must be a pure function of args, so it cannot ask the engine to parse); the script text rides as `rawInput`. The result keeps the generic card. +Decided up front (per the [render-intent RFC](../../../docs/rfc/implemented/architecture/2026-07-02-tool-render-intent-union.md)): a `generic` card titled `workflow: `, read directly from `args.meta.name` (presentation is a pure function of args and does not ask the engine to parse); the script text rides as `rawInput`. The result keeps the generic card. ## Config From 35804737fa9d5c7d8f27299408e2fa60e2ff6d02 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 13 Jul 2026 21:55:38 +0800 Subject: [PATCH 3/4] docs: address simplification RFC review --- docs/rfc/INDEX.md | 7 ++-- ...am-workflow-progress-through-tool-calls.md | 39 +++++++++++++++++++ .../2026-06-20-unify-agent-and-session-id.md | 2 +- ...6-07-12-drop-unconsumed-registry-events.md | 32 --------------- ...2-drop-unconsumed-skill-provider-events.md | 32 +++++++++++++++ ...12-collapse-workflow-to-foreground-core.md | 2 +- ...-12-prune-unused-skill-registry-surface.md | 2 +- 7 files changed, 78 insertions(+), 38 deletions(-) create mode 100644 docs/rfc/proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md delete mode 100644 docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md create mode 100644 docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-skill-provider-events.md rename docs/rfc/{proposed => rejected}/simplification/2026-07-12-collapse-workflow-to-foreground-core.md (97%) rename docs/rfc/{proposed => rejected}/simplification/2026-07-12-prune-unused-skill-registry-surface.md (96%) diff --git a/docs/rfc/INDEX.md b/docs/rfc/INDEX.md index 21ee29a58d..f6dcdf6a0c 100644 --- a/docs/rfc/INDEX.md +++ b/docs/rfc/INDEX.md @@ -10,6 +10,7 @@ Generated by `pnpm run gen-rfc-index` from the RFC tree — never edit by hand; |---|---| | [Pre-tool input rewrite — a consistent design](proposed/feature/2026-06-30-pre-tool-input-rewrite.md) | 2026-06-30 | | [Claude Code and Codex subagent backends (out-of-process delegation to external coding agents)](proposed/feature/2026-07-07-claude-code-and-codex-subagent-backends.md) | 2026-07-07 | +| [Stream workflow progress through tool calls](proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md) | 2026-07-13 | ### Simplification @@ -17,9 +18,7 @@ Generated by `pnpm run gen-rfc-index` from the RFC tree — never edit by hand; |---|---| | [Unify the agent id and the session id](proposed/simplification/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 | | [Prune dead public and result surface](proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md) | 2026-07-04 | -| [Collapse workflows to the exercised foreground core](proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md) | 2026-07-12 | -| [Drop unconsumed registry events](proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md) | 2026-07-12 | -| [Prune unused skill registry surface](proposed/simplification/2026-07-12-prune-unused-skill-registry-surface.md) | 2026-07-12 | +| [Drop unconsumed skill provider events](proposed/simplification/2026-07-12-drop-unconsumed-skill-provider-events.md) | 2026-07-12 | | [Prune unused web seam fields](proposed/simplification/2026-07-12-prune-unused-web-seam-fields.md) | 2026-07-12 | | [Simplify session-log representation](proposed/simplification/2026-07-12-simplify-session-log-representation.md) | 2026-07-12 | @@ -203,6 +202,8 @@ Generated by `pnpm run gen-rfc-index` from the RFC tree — never edit by hand; | [Return the ACP bridge to one live session per connection](rejected/simplification/2026-06-20-single-session-acp-bridge.md) | 2026-06-20 | | [Truncate interrupted final turns on load](rejected/simplification/2026-06-20-truncate-interrupted-turns.md) | 2026-06-20 | | [Prune the unimplemented subagent seam vocabulary](rejected/simplification/2026-07-04-prune-unimplemented-subagent-vocabulary.md) | 2026-07-04 | +| [Collapse workflows to the exercised foreground core](rejected/simplification/2026-07-12-collapse-workflow-to-foreground-core.md) | 2026-07-12 | +| [Prune unused skill registry surface](rejected/simplification/2026-07-12-prune-unused-skill-registry-surface.md) | 2026-07-12 | ### Architecture diff --git a/docs/rfc/proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md b/docs/rfc/proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md new file mode 100644 index 0000000000..8965880712 --- /dev/null +++ b/docs/rfc/proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md @@ -0,0 +1,39 @@ +# RFC: Stream workflow progress through tool calls + +Status: proposed + +## Problem + +The workflow engine intentionally emits balanced `workflow/*` observation events for run, phase, narration, and child-agent progress, but no production consumer presents them. Editors therefore show one pending workflow tool card until the final result even while the engine already reports which phase is active, what the script logged, and which children started or settled. The [dynamic-workflows decision](../../implemented/feature/2026-07-05-dynamic-workflows.md) explicitly reserves ACP progress UI for this event stream. + +Making `dsh-acp` listen to workflow events directly would invert the capability boundary: the generic UI bridge would depend on an optional workflow package and special-case one tool name. The tool pipeline already owns the routing facts a live update needs—agent and call id—but exposes only pure pending/final presenters, so a long-running tool has no provider-neutral way to report transient UI state between them. + +## Proposal + +Add a live progress channel to `dsh-tools`. The registry-owned `ToolExecution` gains `reportProgress(view): boolean`, where `view` is a detached provider-neutral generic progress snapshot containing an optional replacement title and UI-facing content blocks. Progress cannot change the call's args-derived card tag, kind, raw input, locations, terminal intent, or diff intent; it updates only the live title/content within the presentation chosen up front. While the execution is active, the method validates and snapshots the view, then dispatches a contained, agent-scoped `tools/progress` observation carrying the authoritative execution identity and snapshot. Once final-result processing begins it returns `false` and emits nothing, so a late asynchronous reporter cannot overwrite a terminal card. Observer exceptions are logged and cannot fail the tool. + +`dsh-acp` consumes `tools/progress` generically. It resolves the execution's agent through its existing agent-to-session map and emits an in-progress `tool_call_update` for the same call id. Because reporting is available only inside the tool execution pipeline, the durable `tool/call` and its ACP `tool_call` always precede the first update; closing the reporter before `tools/result` ensures no progress update follows the completed/failed card. Progress is live UI state rather than model input or durable history: session replay continues to reconstruct the pending and final cards from `tool/call` and `tool/result` without replaying transient updates. + +`dsh-tool-workflow` becomes the first producer. It keeps a plugin-owned reducer keyed by `WorkflowRun.id`, installed synchronously after `ctx.workflows.start()` returns and before worker messages can be delivered. The reducer consumes the existing phase, log, agent-start, agent-end, and end events, reporting a replacement snapshot with the current phase, latest log line, active child labels, and completed/failed/cancelled counts. It does not accumulate a narration transcript; settled children leave the active set and become counters. The initial snapshot comes from the returned run's id/meta, and `workflow/end`, tool settlement, or plugin disposal removes the reducer entry. The six workflow events, their metadata, paired child lifecycle, run handle, cancellation channels, and observer containment remain unchanged; third-party observers can continue consuming them directly. + +Update the tool execution/presentation docs, generated event and API catalogs, workflow package docs, and the workflow data-structure catalog. ACP integration coverage must exercise the real workflow tool and worker seam with a scripted model boundary; the primary ACP snapshot suite adds one workflow-progress scenario because this changes the editor-facing transcript. + +## Alternatives considered + +**Delete the workflow observation surface.** Rejected in [the collapse-workflow simplification](../../rejected/simplification/2026-07-12-collapse-workflow-to-foreground-core.md): the events and their balanced lifecycle are intentional, and the missing piece is a consumer. + +**Teach ACP about workflows directly.** This could map `WorkflowRunInfo` to a session and card, but it would make the generic bridge depend on an optional capability and bypass the rule that tools own presentation intent. A tool-progress channel solves the same routing problem for every long-running tool. + +**Persist every progress update as a session event.** That would make live narration replayable, but it would permanently enlarge logs with state whose authoritative durable outcome is already the tool call/result pair. If resumable workflow progress becomes a product requirement, it needs a workflow-journaling design rather than UI snapshots disguised as durable facts. + +## Acceptance criteria + +- `ToolExecution.reportProgress()` is registry-owned, agent-scoped, snapshotting, observer-contained, and returns `false` without dispatch after terminal processing starts. +- ACP routes progress to the correct call in the correct live session; concurrent workflows in different sessions cannot cross-talk, and no `tool_call_update` appears before its `tool_call` or after its terminal update. +- Workflow progress shows the current phase, latest log line, active children, and outcome counts while preserving all existing `workflow/*` events and run semantics. +- Cancellation, worker death, tool failure, session close, and plugin disposal release reducer state; replay emits only the durable pending/final card pair. +- Unit, workflow integration, ACP integration, snapshot, typecheck, coverage, doc-sync, module-graph, build, and hygiene gates pass. + +## Risks + +This adds a public live-progress method and event to the tool seam, so implementations must keep the active/terminal boundary exact and detach snapshots before observers see them. A workflow can emit many progress changes; the bounded reducer avoids transcript growth but still sends one UI update per meaningful event. If measured clients need coalescing, it must be a defaulted validated bridge configuration rather than a hardcoded throttle. Transient progress intentionally disappears on replay, so the final tool result remains the only durable workflow card content. diff --git a/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md b/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md index b3670dd9a1..1a69f9ebe6 100644 --- a/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md +++ b/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md @@ -18,7 +18,7 @@ Make an agent's registry id equal its session id. `CreateAgentOptions` accepts o The config-driven path must first settle its currently hidden resume-or-create policy. Today it uses a stable agent label and fresh UUID-suffixed session id to avoid colliding with a durable log on the next run. Under unification it must deliberately resume the fixed id, mint a fresh combined id, or expose an explicit policy; implementation must not pick silently. -`agent/created` and `agent/disposed` remain outside this proposal. They are paired publication lifecycle events, not identity aliases; any later consumer-free removal belongs in the dedicated [registry-event simplification](./2026-07-12-drop-unconsumed-registry-events.md) after a fresh search. +`agent/created` and `agent/disposed` remain outside this proposal. They are paired publication lifecycle events, not identity aliases; any later consumer-free removal needs its own proposal after a fresh search. ## Alternatives considered diff --git a/docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md b/docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md deleted file mode 100644 index 52e30879e2..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-registry-events.md +++ /dev/null @@ -1,32 +0,0 @@ -# RFC: Drop unconsumed registry events - -Status: proposed - -## Problem - -Four registry notifications are produced but have no production listener. The generated producer/consumer matrix and exact event-name searches find only declarations, emit sites, invariant metadata, tests, generated catalogs, and prose for `tools/change`, `system-prompt/change`, `skill/provider-added`, and `skill/provider-removed`. - -No shipped path uses these signals for invalidation: request assembly deliberately reruns for every step, tool/system-prompt membership may be agent-scoped, and skill discovery reads providers on demand. The payloadless tool/system-prompt notices are also insufficient for a scoped observer because a change may be local to one agent but the event cannot identify that scope. - -Earlier registry work retained tool/system-prompt notifications as low-cost hooks for a hypothetical live UI even while the equivalent LLM and web notifications were removed. The new evidence is that no owner has appeared, per-step assembly needs no signal, and scope-local membership has made the old payload insufficient for that hypothetical owner. This proposal does not include `subagent/provider-added`/`removed`, which `tool-subagent` consumes to tolerate concurrent sibling-plugin loading. - -## Proposal - -Delete the four declarations and every emit path, rollback-order branch, invariant-table entry, test, and generated catalog/matrix row that exists only for them. Remove the corresponding registry README/JSDoc contract. Where tests used an event to observe cleanup, assert public lookup or assembled output instead. - -Amend the [agent-scope RFC](../../implemented/architecture/2026-07-08-agent-scope-contexts.md) and reconstructable-request documentation so current behavior has one home: request inputs are recomputed at the request boundary, not maintained by invalidation signals. - -## Alternatives considered - -**Keep cheap notifications for future plugins.** An external plugin could subscribe later, and provider lifecycle signals can solve sibling-load races. The subagent event demonstrates the bar: it has a real concurrent loader consumer and a payload tailored to that job. These four have neither; a future consumer should introduce the scoped identity and timing it demonstrably needs. - -## Acceptance criteria - -- The generated event matrix contains no row or registry-subject inventory entry for the four notifications. -- Tool schema assembly, system-prompt assembly, skill discovery, ordinary effect rollback/disposal, and registry lookup cleanup behave unchanged; listener-triggered rollback disappears with the events. -- The real subagent provider lifecycle consumer remains covered. -- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. - -## Risks - -This deliberately removes pre-release plugin observation points. A future live registry UI would need a new scoped snapshot/change contract instead of subscribing to payloadless global notifications. diff --git a/docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-skill-provider-events.md b/docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-skill-provider-events.md new file mode 100644 index 0000000000..9a0d9d2cb7 --- /dev/null +++ b/docs/rfc/proposed/simplification/2026-07-12-drop-unconsumed-skill-provider-events.md @@ -0,0 +1,32 @@ +# RFC: Drop unconsumed skill provider events + +Status: proposed + +## Problem + +Two skill-registry notifications are produced but have no production listener. The generated producer/consumer matrix and exact event-name searches find only declarations, emit sites, tests, generated catalogs, and prose for `skill/provider-added` and `skill/provider-removed`. + +Skill discovery reads the current provider map on demand, provider registration synchronously clears completed catalogs, and the post-await revision check prevents stale discovery from entering the cache. No sibling plugin waits for a skill provider through these events, unlike the live `subagent/provider-added` consumer that tolerates concurrent sibling loading. + +`tools/change` and `system-prompt/change` are explicitly outside this proposal. Existing simplification decisions retain them as intentional observation points for live tool and prompt UIs, and self-referential mounted plugins already use `tools/change`. This proposal also leaves `subagent/provider-added`/`removed` unchanged because `tool-subagent` has a production lifecycle consumer. + +## Proposal + +Delete the two skill-provider declarations and every emit path, rollback-order branch, test, and generated catalog/matrix row that exists only for them. Remove the corresponding skill-registry README/JSDoc contract. Where tests used an event to observe cleanup, assert provider lookup or collected output instead. + +Amend the skill-system RFC and package documentation so provider registration is described as direct effect-owned state with cache invalidation, not as a lifecycle notification contract. + +## Alternatives considered + +**Keep skill-provider notifications for future plugins.** A third-party plugin could observe provider availability, but direct provider registration and on-demand lookup are the extension contract; no current consumer needs a push signal. If a future sibling-load race appears, it can introduce a notification with the identity and readiness semantics that consumer requires, as the subagent registry did. + +## Acceptance criteria + +- The generated event matrix contains no row for `skill/provider-added` or `skill/provider-removed`. +- Skill discovery, direct runtime registration, provider effect rollback/disposal, cache invalidation, and registry lookup cleanup behave unchanged; listener-triggered rollback disappears with the events. +- `tools/change`, `system-prompt/change`, and the real subagent provider lifecycle consumer remain documented and covered. +- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. + +## Risks + +This removes pre-release skill-provider observation points while retaining both ways third-party plugins contribute skills: direct runtime registration and provider registration. A future consumer that needs live provider availability must add a purpose-built notification rather than relying on these generic events. diff --git a/docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md b/docs/rfc/rejected/simplification/2026-07-12-collapse-workflow-to-foreground-core.md similarity index 97% rename from docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md rename to docs/rfc/rejected/simplification/2026-07-12-collapse-workflow-to-foreground-core.md index d112aa2a3f..7624629d41 100644 --- a/docs/rfc/proposed/simplification/2026-07-12-collapse-workflow-to-foreground-core.md +++ b/docs/rfc/rejected/simplification/2026-07-12-collapse-workflow-to-foreground-core.md @@ -1,6 +1,6 @@ # RFC: Collapse workflows to the exercised foreground core -Status: proposed +Status: rejected — Workflow progress is an intentional observation surface; make it useful through a consumer instead of deleting it. ## Problem diff --git a/docs/rfc/proposed/simplification/2026-07-12-prune-unused-skill-registry-surface.md b/docs/rfc/rejected/simplification/2026-07-12-prune-unused-skill-registry-surface.md similarity index 96% rename from docs/rfc/proposed/simplification/2026-07-12-prune-unused-skill-registry-surface.md rename to docs/rfc/rejected/simplification/2026-07-12-prune-unused-skill-registry-surface.md index 1df837e026..3f585eb64e 100644 --- a/docs/rfc/proposed/simplification/2026-07-12-prune-unused-skill-registry-surface.md +++ b/docs/rfc/rejected/simplification/2026-07-12-prune-unused-skill-registry-surface.md @@ -1,6 +1,6 @@ # RFC: Prune unused skill registry surface -Status: proposed +Status: rejected — Direct runtime skill registration is an intentional extension path for third-party plugins. ## Problem From be67ca41ca724c8a18ef98d440e740dee37a477c Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 13 Jul 2026 22:29:23 +0800 Subject: [PATCH 4/4] docs: address review bot feedback --- ...6-07-13-stream-workflow-progress-through-tool-calls.md | 8 +++++--- .../2026-07-12-prune-unused-skill-registry-surface.md | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/rfc/proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md b/docs/rfc/proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md index 8965880712..200ed50b1e 100644 --- a/docs/rfc/proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md +++ b/docs/rfc/proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md @@ -14,7 +14,9 @@ Add a live progress channel to `dsh-tools`. The registry-owned `ToolExecution` g `dsh-acp` consumes `tools/progress` generically. It resolves the execution's agent through its existing agent-to-session map and emits an in-progress `tool_call_update` for the same call id. Because reporting is available only inside the tool execution pipeline, the durable `tool/call` and its ACP `tool_call` always precede the first update; closing the reporter before `tools/result` ensures no progress update follows the completed/failed card. Progress is live UI state rather than model input or durable history: session replay continues to reconstruct the pending and final cards from `tool/call` and `tool/result` without replaying transient updates. -`dsh-tool-workflow` becomes the first producer. It keeps a plugin-owned reducer keyed by `WorkflowRun.id`, installed synchronously after `ctx.workflows.start()` returns and before worker messages can be delivered. The reducer consumes the existing phase, log, agent-start, agent-end, and end events, reporting a replacement snapshot with the current phase, latest log line, active child labels, and completed/failed/cancelled counts. It does not accumulate a narration transcript; settled children leave the active set and become counters. The initial snapshot comes from the returned run's id/meta, and `workflow/end`, tool settlement, or plugin disposal removes the reducer entry. The six workflow events, their metadata, paired child lifecycle, run handle, cancellation channels, and observer containment remain unchanged; third-party observers can continue consuming them directly. +`dsh-tool-workflow` becomes the first producer. Each tool execution installs a compact event capture before calling `ctx.workflows.start()`, because a valid engine may emit progress synchronously inside `start()`. Until the call returns, the capture reduces observed events into candidate states keyed by `WorkflowRunInfo.id`; it then selects the returned `WorkflowRun.id`, discards other candidates, reports the accumulated snapshot, and routes later matching events directly. If `start()` throws, the capture is disposed and its candidates are dropped. This preserves engine swappability without adding observer correlation to `WorkflowStartRequest` or requiring progress to wait until `start()` returns. + +The reducer consumes the existing start, phase, log, agent-start, agent-end, and end events, reporting a replacement snapshot with the current phase, latest log line, active child labels, and completed/failed/cancelled counts. It does not accumulate a narration transcript; settled children leave the active set and become counters. `workflow/end`, tool settlement, or plugin disposal removes the reducer entry and event capture. The six workflow events, their metadata, paired child lifecycle, run handle, cancellation channels, and observer containment remain unchanged; third-party observers can continue consuming them directly. Update the tool execution/presentation docs, generated event and API catalogs, workflow package docs, and the workflow data-structure catalog. ACP integration coverage must exercise the real workflow tool and worker seam with a scripted model boundary; the primary ACP snapshot suite adds one workflow-progress scenario because this changes the editor-facing transcript. @@ -30,10 +32,10 @@ Update the tool execution/presentation docs, generated event and API catalogs, w - `ToolExecution.reportProgress()` is registry-owned, agent-scoped, snapshotting, observer-contained, and returns `false` without dispatch after terminal processing starts. - ACP routes progress to the correct call in the correct live session; concurrent workflows in different sessions cannot cross-talk, and no `tool_call_update` appears before its `tool_call` or after its terminal update. -- Workflow progress shows the current phase, latest log line, active children, and outcome counts while preserving all existing `workflow/*` events and run semantics. +- Workflow progress shows the current phase, latest log line, active children, and outcome counts while preserving all existing `workflow/*` events and run semantics; a seam test engine that emits start, phase, log, child, and end events synchronously inside `start()` loses none of that reducer state. - Cancellation, worker death, tool failure, session close, and plugin disposal release reducer state; replay emits only the durable pending/final card pair. - Unit, workflow integration, ACP integration, snapshot, typecheck, coverage, doc-sync, module-graph, build, and hygiene gates pass. ## Risks -This adds a public live-progress method and event to the tool seam, so implementations must keep the active/terminal boundary exact and detach snapshots before observers see them. A workflow can emit many progress changes; the bounded reducer avoids transcript growth but still sends one UI update per meaningful event. If measured clients need coalescing, it must be a defaulted validated bridge configuration rather than a hardcoded throttle. Transient progress intentionally disappears on replay, so the final tool result remains the only durable workflow card content. +This adds a public live-progress method and event to the tool seam, so implementations must keep the active/terminal boundary exact and detach snapshots before observers see them. The pre-start capture can briefly observe unrelated workflow runs, so it holds only compact candidate state keyed by run id and drops every non-matching candidate as soon as `start()` returns. A workflow can emit many progress changes; the bounded reducer avoids transcript growth but still sends one UI update per meaningful event after correlation. If measured clients need coalescing, it must be a defaulted validated bridge configuration rather than a hardcoded throttle. Transient progress intentionally disappears on replay, so the final tool result remains the only durable workflow card content. diff --git a/docs/rfc/rejected/simplification/2026-07-12-prune-unused-skill-registry-surface.md b/docs/rfc/rejected/simplification/2026-07-12-prune-unused-skill-registry-surface.md index 3f585eb64e..cafd50e8a5 100644 --- a/docs/rfc/rejected/simplification/2026-07-12-prune-unused-skill-registry-surface.md +++ b/docs/rfc/rejected/simplification/2026-07-12-prune-unused-skill-registry-surface.md @@ -10,7 +10,7 @@ The skill service's embedded-runtime subsystem has zero production caller of `ct Remove `SkillService.register()`, `SkillRegistration`, the runtime pseudo-provider and reserved-name rules, runtime revisions/cache branches, and runtime-only source/rank normalization. Tests that need an embedded skill register a small real provider. Retain `providerRevision` as the in-flight discovery epoch, but key completed catalogs by cwd alone: every provider mutation synchronously clears the cache, and the post-await revision comparison already prevents inserting stale work. Remove `whenToUse`, `SkillCandidate.path`, and `SkillDefinition.path` from the skill contract and local-provider copies while retaining provider locator/root paths; retain `metadata`, `disableModelInvocation`, `source`, `provider`, `locator`, and `resourceBase` as either deliberate extension vocabulary or production-consumed fields. -Amend the skill-system RFC, README, JSDoc, catalogs, and tests. Agent-scoped system-prompt sections, tool providers, and variables are explicitly outside this proposal: final #224 made all three part of the `setup(agent.ctx)` contributor contract, so absence of a fixed in-repo scoped registration is not evidence of non-consumption. +Amend the skill-system RFC, README, JSDoc, catalogs, and tests. Agent-scoped system-prompt sections, tool providers, and variables are explicitly outside this proposal: the [agent-scope contributor contract](../../implemented/architecture/2026-07-08-agent-scope-contexts.md) intentionally allows all three to be registered during `setup(agentCtx)` through the agent-owned context, so absence of a fixed in-repo scoped registration is not evidence of non-consumption. ## Alternatives considered