diff --git a/docs/config-catalog.md b/docs/config-catalog.md index dce169bda8..6172295667 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` @@ -813,7 +813,7 @@ export interface Config { } ``` -Source: [`packages/core/system-prompt/src/index.ts:225`](../packages/core/system-prompt/src/index.ts) +Source: [`packages/core/system-prompt/src/index.ts:227`](../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 cf41cfdd5f..0aea3f1589 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -257,7 +257,7 @@ variable(name: string, provider: (context: AssembleContext) => string | undefine async assemble(context: AssembleContext = {}): Promise ``` -Source: [`packages/core/system-prompt/src/index.ts:340`](../../packages/core/system-prompt/src/index.ts) +Source: [`packages/core/system-prompt/src/index.ts:342`](../../packages/core/system-prompt/src/index.ts) ## `ctx.tools` — `ToolRegistry` diff --git a/docs/rfc/INDEX.md b/docs/rfc/INDEX.md index 01fd7c2b4e..b338edfc52 100644 --- a/docs/rfc/INDEX.md +++ b/docs/rfc/INDEX.md @@ -10,13 +10,18 @@ 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 | +| [Interactive side sessions and merge-back](proposed/feature/2026-07-08-interactive-side-sessions.md) | 2026-07-08 | +| [Stream workflow progress through tool calls](proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md) | 2026-07-13 | ### Simplification | 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 | +| [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 | ### Architecture @@ -199,6 +204,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-08-interactive-side-sessions.md b/docs/rfc/proposed/feature/2026-07-08-interactive-side-sessions.md new file mode 100644 index 0000000000..79a40b66af --- /dev/null +++ b/docs/rfc/proposed/feature/2026-07-08-interactive-side-sessions.md @@ -0,0 +1,48 @@ +# RFC: Interactive side sessions and merge-back + +Status: proposed + +## Problem + +A user deep in a live session often wants to fork the conversation — ask "why did we structure it this way", explore an alternative, get an explanation — without polluting the main context and without abandoning the surface they are in. The harness has every primitive this needs and no product face for it: [the session-store fork API](../../implemented/feature/2026-06-30-session-store-fork-api.md) produces a `Session` with no attached agent and no way for a client to reach it, and [the fork subagent](../../implemented/feature/2026-06-21-subagent-capability-seam.md) seeds a child with the parent's prefix but runs it as a model-driven task whose whole transcript collapses into one tool result — neither yields a forked conversation the USER can talk to. + +The return path is missing entirely: nothing carries a conclusion from one branch back into another. Whatever the user learns in an exploration branch is copy-pasted by hand or lost, with no provenance and no replayable record. + +Terminal-first competitors ship half of each: single-shot side questions with inherited context, and user-switched branch copies that force a client restart. None ship the return verb. A harness that owns its session store and context assembly can do both cheaply — and can do so without breaking the provider prefix cache, which third-party wrappers structurally cannot. + +## Proposal + +A **side session** is an ordinary live session forked from a source session at its last completed turn, attached to its own agent, framed as a read-only advisor, with one new verb — **merge-back** — that hands a condensed note to the parent. + +- **Fork + attach composes existing primitives.** The child is created via `ctx.agents.create({ seed, meta })` with the parent's balanced completed-turn prefix (the same slice the fork subagent takes) and `parentSession`/`seedLength` lineage stamped in `meta`. No new core service and no session-store change, for the same reasons [the fork API RFC](../../implemented/feature/2026-06-30-session-store-fork-api.md) rejected a standalone fork service. +- **Advisor framing rides the log, not the system prompt.** The rules ("you are a read-only side advisor; explain, do not mutate; refuse task continuation") are `inject()`ed as a `context/message` with source `{ kind: 'plugin', plugin: 'sidechat' }` immediately after creation. The child's system prompt stays byte-identical to the parent's, so the provider's prefix cache covers the inherited history. +- **Merge-back is one condense turn plus one injection.** The child is prompted for a bounded handback note (a hard length cap), which is then `inject()`ed into the PARENT as a `context/message` with the same plugin source. The parent's next request sees it at its chronological position; replay and [reconstructability](../../implemented/architecture/2026-07-05-reconstructable-requests.md) hold by construction; no new event type enters [the session vocabulary](../../../core-data-structures/session.md). +- **The surface binding is deliberately unspecified.** How a user invokes the fork and the merge, and how a handback note is presented, are client concerns; while the harness speaks through protocols whose UI it does not control, this RFC pins only the surface-agnostic mechanics above and leaves presentation to the first surface the project owns. + +Scope also excludes: rewind productization (mechanically the same fork-to-an-earlier-boundary, an entirely different product question), session tree views, a model-facing side-session tool, and `forkName`/`mergedInto` metadata. + +## Prototype + +A spike (branch `spike/side-sessions-b`) validates the mechanics against the live adapter: a fork that leaves the source log untouched, a child seeded with the full inherited prefix, a multi-turn advisor exchange that demonstrably uses parent history, and a merge-back the parent's next turn quotes correctly. + +## Alternatives considered + +- **Carry side conversations through the subagent seam.** Rejected: a subagent child is a model-driven run — the parent's model spawns it, drives it, and consumes its result as a tool result. A side session is user-driven, needs its own client-visible session and lifecycle, and must outlive any single parent turn. +- **Persona via `system-prompt/assemble` section filtering.** Rejected as the default path: any system-prompt byte change invalidates the provider prefix cache from token 0, forfeiting the cheap fork that makes side sessions attractive on long histories. The filter seam remains available for deployments that prefer hard prompt separation over cache reuse. +- **A dedicated `sidechat/*` event family for the handback.** Deferred: `context/message` with a mandatory plugin source already satisfies durability, provenance, and replay. A first-class event earns its catalog, persistence, and snapshot costs only if a UI needs to render handbacks as dedicated cards. +- **Binding the proposal to a protocol surface now.** Rejected in review: the harness currently speaks through client-owned UIs it does not control, so any presentation contract written today would be speculative. The RFC pins the surface-agnostic mechanics and defers presentation to the first surface the project owns. +- **Surface-level mirroring of the handback.** Rejected in review: a visible record emitted outside the log vanishes on replay while the model still sees it. Whatever surface eventually renders the handback must derive its presentation from the durable `context/message`, so live and replayed views come from the same event. + +## Acceptance criteria + +- Forking a live session yields a child agent seeded with the source's balanced completed-turn prefix, with `parentSession` and `seedLength` in its header and a system prompt byte-identical to the parent's; the source log is untouched by the fork. +- The advisor framing is exactly one plugin-sourced `context/message` at the head of the child's appended history — never a system-prompt change. +- Merge-back appends exactly one length-capped `context/message` to the parent with source `plugin: sidechat`; the parent's next request sees it, and replay reproduces it at the same position. +- Parent and child run concurrently without cross-talk between their logs or streams. +- Coverage: unit tests for the fork/attach and merge-back mechanics; surface-level snapshot coverage lands with whichever surface first binds the feature. + +## Risks + +- **Read-only is advisory in v1.** The rules are injected context, not enforcement; a determined prompt can still drive mutating tools. The hard gate is a `tools/pre-execute` deny via [the interception seams](../../implemented/feature/2026-06-30-interception-seams.md), and the RFC's advisor framing is written so that gate can be added without changing the mechanics. +- **A compacted source forks its compacted view.** The child inherits the summary, not the original turns; whichever surface binds the feature should disclose this once [compaction](../../implemented/feature/2026-06-18-compaction-capability-seam.md) ships in this path. +- **Handback notes spend parent tokens.** The length cap and one-note-per-merge bound the cost, but a user who merges repeatedly accumulates notes; a future consolidation pass belongs to the compaction work, not here. 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..200ed50b1e --- /dev/null +++ b/docs/rfc/proposed/feature/2026-07-13-stream-workflow-progress-through-tool-calls.md @@ -0,0 +1,41 @@ +# 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. 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. + +## 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; 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. 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/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..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 @@ -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 each live agent/session pair: `agentId`, the `AgentRegistry` routing 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, stdio keeps `labelBySession` solely to recover an agent label from session events, and hooks expose both values for authors to reconcile. No production path reattaches one live agent object 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 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. -- **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`. 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 -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 final registry entries; 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. Keep the existing creation transaction, final-entry collision checks, and exact-entry detach semantics; remove only maps and 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. + +`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 -### 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 routing and log identities.** The config-driven loop uses a stable configured agent id with a fresh UUID session on each fresh process start. That is a real use of the distinction: a stable routing/display label plus a new durable conversation. Unification can proceed only after choosing whether this path resumes a fixed identity, mints a combined per-run identity, or exposes the policy explicitly. If the stable label is a required product contract, reject this proposal rather than hiding it in another map. ## 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 existing creation transaction keeps final-entry collision, exact-entry detach, rollback, and quiescence guarantees without adding identity-specific lifecycle state. +- 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 routing identity is a real requirement, reject this RFC and retain the current caller-supplied pair plus final-entry arbitration. 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..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 @@ -1,32 +1,60 @@ -# 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, 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 | +| --- | --- | --- | +| `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. | +| 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; `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 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 -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? +**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. -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 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 -- `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 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-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-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..f335afafe1 --- /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. 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 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. 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. + +## 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 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. +- New-shape v0 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/docs/rfc/rejected/simplification/2026-07-12-collapse-workflow-to-foreground-core.md b/docs/rfc/rejected/simplification/2026-07-12-collapse-workflow-to-foreground-core.md new file mode 100644 index 0000000000..7624629d41 --- /dev/null +++ b/docs/rfc/rejected/simplification/2026-07-12-collapse-workflow-to-foreground-core.md @@ -0,0 +1,37 @@ +# RFC: Collapse workflows to the exercised foreground core + +Status: rejected — Workflow progress is an intentional observation surface; make it useful through a consumer instead of deleting it. + +## 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 payloads, the worker sends phase/log/agent lifecycle protocol messages, the host forwards them through 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. + +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. 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. + +## Alternatives considered + +**Keep the prebuilt observation vocabulary for a future UI.** The current shape resembles Claude Code dynamic-workflow metadata, and the host deliberately pairs each forwarded agent start with either the worker's end or a synthesized terminal end. Removing it gives up compatibility-by-shape and makes progress UI a new design task, but the existing payloads still lack routable ownership, so balanced lifecycles alone 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. +- 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, 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/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 new file mode 100644 index 0000000000..cafd50e8a5 --- /dev/null +++ b/docs/rfc/rejected/simplification/2026-07-12-prune-unused-skill-registry-surface.md @@ -0,0 +1,27 @@ +# RFC: Prune unused skill registry surface + +Status: rejected — Direct runtime skill registration is an intentional extension path for third-party plugins. + +## 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: 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 + +**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/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/core/system-prompt/src/index.ts b/packages/core/system-prompt/src/index.ts index 63ba6b350d..d9d19c7f15 100644 --- a/packages/core/system-prompt/src/index.ts +++ b/packages/core/system-prompt/src/index.ts @@ -103,6 +103,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/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-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 5a5ce9e51f..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'] /** @@ -352,10 +354,13 @@ 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 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/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 } 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