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] 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