From 51700d4685898a3ef62512c34f2183b7b532a5b1 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Mon, 29 Jun 2026 10:49:07 +0800 Subject: [PATCH] Fix ask_user_question review findings --- docs/cordis-catalog/events-and-services.md | 2 +- docs/core-data-structures/core.md | 1 + docs/core-data-structures/user-interaction.md | 79 ++++++++ docs/module-graph.md | 10 +- docs/rfc/README.md | 1 + .../feature/2026-06-25-ask-user-question.md | 39 ++++ packages/README.md | 8 +- packages/core/README.md | 1 - packages/core/user-interaction/package.json | 2 + packages/core/user-interaction/src/index.ts | 8 +- packages/core/user-interaction/tsconfig.json | 3 + packages/support/ui-stdio/src/index.ts | 2 +- .../support/ui-stdio/tests/ui-stdio.spec.ts | 13 ++ packages/ui/README.md | 3 + packages/ui/acp-agent/README.md | 4 +- packages/ui/acp-agent/package.json | 4 + packages/ui/acp-agent/src/index.ts | 4 + packages/ui/acp-agent/tests/acp-agent.spec.ts | 2 + packages/ui/acp-agent/tests/built-bin.e2e.ts | 6 +- packages/ui/acp-agent/tsconfig.json | 6 + packages/ui/acp/README.md | 5 +- packages/ui/acp/acp-feature-support.md | 4 +- packages/ui/acp/package.json | 3 + packages/ui/acp/src/index.ts | 156 ++++++++++++++- packages/ui/acp/tests/bridge.spec.ts | 179 +++++++++++++++++- packages/ui/acp/tests/harness.ts | 21 ++ packages/ui/acp/tsconfig.json | 3 + .../ui/stdio-agent/tests/built-bin.e2e.ts | 2 +- packages/ui/stdio-agent/tsconfig.json | 2 +- packages/{core => ui}/tool-ask-user/README.md | 0 .../{core => ui}/tool-ask-user/package.json | 0 .../{core => ui}/tool-ask-user/src/index.ts | 0 .../tool-ask-user/tests/tool-ask-user.spec.ts | 15 ++ .../{core => ui}/tool-ask-user/tsconfig.json | 8 +- pnpm-lock.yaml | 59 +++--- scripts/type-equiv.manifest.json | 6 + tsconfig.build.json | 2 +- tsconfig.json | 2 +- 38 files changed, 611 insertions(+), 54 deletions(-) create mode 100644 docs/core-data-structures/user-interaction.md create mode 100644 docs/rfc/implemented/feature/2026-06-25-ask-user-question.md rename packages/{core => ui}/tool-ask-user/README.md (100%) rename packages/{core => ui}/tool-ask-user/package.json (100%) rename packages/{core => ui}/tool-ask-user/src/index.ts (100%) rename packages/{core => ui}/tool-ask-user/tests/tool-ask-user.spec.ts (93%) rename packages/{core => ui}/tool-ask-user/tsconfig.json (74%) diff --git a/docs/cordis-catalog/events-and-services.md b/docs/cordis-catalog/events-and-services.md index 7d926727a0..8b861e6de1 100644 --- a/docs/cordis-catalog/events-and-services.md +++ b/docs/cordis-catalog/events-and-services.md @@ -463,7 +463,7 @@ registerProvider(provider: UserInteractionProvider): () => void async ask(request: AskUserQuestionRequest): Promise ``` -Source: [`packages/core/user-interaction/src/index.ts:72`](../../packages/core/user-interaction/src/index.ts) +Source: [`packages/core/user-interaction/src/index.ts:70`](../../packages/core/user-interaction/src/index.ts) ## Inherited tier (cordis core + loader/hmr/timer) diff --git a/docs/core-data-structures/core.md b/docs/core-data-structures/core.md index ad884aeff6..7fd62f38e6 100644 --- a/docs/core-data-structures/core.md +++ b/docs/core-data-structures/core.md @@ -19,6 +19,7 @@ Everything else is documented on a **sub-page**, not here. The rule that draws t | [session.md](session.md) | the full `SessionEventMap` variant catalog, `TurnTrigger`/`TurnEndReason`, `deriveMessages()`, the turn-enclosure invariant | | [persistence.md](persistence.md) | the durability seam: `SessionPersistence`, JSONL + SQLite backends, `session/flush`, crash recovery, `SessionHeader` | | [tools.md](tools.md) | `ToolDefinition` full fields, the schema DSL, `ToolExecution`/`ToolResult`, tool-presentation UI types, the `tools/execute` waterfall | +| [user-interaction.md](user-interaction.md) | the human question/answer seam: `AskUserQuestionRequest`/`Answer`, options, provider, structured errors | | [bash.md](bash.md) | the bash executor seam: `BashExecRequest`/`Spec`, `BashRunResult`, background `BashTask`s | | [subagent.md](subagent.md) | the subagent seam: the named-provider registry, `SubagentStartRequest`/`Result`/`Run`, the start-time-vs-runtime capability split | diff --git a/docs/core-data-structures/user-interaction.md b/docs/core-data-structures/user-interaction.md new file mode 100644 index 0000000000..0cf0864121 --- /dev/null +++ b/docs/core-data-structures/user-interaction.md @@ -0,0 +1,79 @@ +# User Interaction + +The user-interaction seam of [dsh-user-interaction](../../packages/core/user-interaction). It is the provider-neutral vocabulary a tool or permission plugin uses when it needs the human to answer before the agent can continue. UI surfaces provide the active `UserInteractionProvider`: `dsh-ui-stdio` renders questions in readline, and `dsh-acp` maps them to ACP form elicitations. + +Source: [`packages/core/user-interaction/src/index.ts`](../../packages/core/user-interaction/src/index.ts) + +## Question options + +`AskUserQuestionOption` is the selectable-choice shape. `label` is user-facing, while `value` is the model-facing answer returned when the option is selected; when omitted, providers use the label. + +```ts type-equiv +interface AskUserQuestionOption { + /** User-facing label. */ + label: string + /** Value returned to the model when selected. Defaults to `label`. */ + value?: string + /** Optional extra context rendered by capable UIs. */ + description?: string + /** Marks the recommended/default option. */ + recommended?: boolean +} +``` + +## Ask request + +`AskUserQuestionRequest` is the cross-package request. `options` being absent means free-form input; an optionless request remains free-form even when a caller sets `allowCustom: false`, because there is no selectable option to constrain the answer to. + +```ts type-equiv +interface AskUserQuestionRequest { + /** The question to display. */ + question: string + /** Optional short heading/group label. */ + header?: string + /** Optional choices the UI can render as a menu. */ + options?: AskUserQuestionOption[] + /** Whether free-form answers are accepted. Defaults to `true`. */ + allowCustom?: boolean + /** Calling agent, when the request came from an agent tool call. */ + agent?: Agent + /** Abort signal for the owning tool/step. */ + signal?: AbortSignal +} +``` + +## Answer + +Providers return the model-facing `answer` text and optionally echo the chosen option as metadata. Consumers should use `answer`; the option is for UI/session metadata and diagnostics. + +```ts type-equiv +interface AskUserQuestionAnswer { + /** Model-facing answer text. */ + answer: string + /** The selected option, when the answer came from `options`. */ + option?: AskUserQuestionOption +} +``` + +## Provider + +Only one provider may be active in a context. Provider registration is effect-bound so HMR/disposal removes the active UI. + +```ts type-equiv +interface UserInteractionProvider { + ask(request: AskUserQuestionRequest): Promise +} +``` + +## Errors + +`UserInteractionError` extends `HarnessError`, so `ctx.tools.execute()` preserves `{ name, code }` for model-facing tool failures such as `NO_PROVIDER`, `ASK_ABORTED`, or ACP-side cancellation. + +```ts type-equiv +class UserInteractionError extends HarnessError { + constructor(message: string, code: string, options?: ErrorOptions) { + super(message, code, options) + this.name = 'UserInteractionError' + } +} +``` diff --git a/docs/module-graph.md b/docs/module-graph.md index fb21a1184a..5dbf0d2672 100644 --- a/docs/module-graph.md +++ b/docs/module-graph.md @@ -32,11 +32,13 @@ graph TD tools --> llm tools --> system-prompt user-interaction --> agent + user-interaction --> llm acp --> agent acp --> llm acp --> session acp --> session-persistence acp --> tools + acp --> user-interaction agent-loop --> agent agent-loop --> llm agent-loop --> session @@ -81,6 +83,8 @@ graph TD acp-agent --> acp acp-agent --> agent-core acp-agent --> session-persistence-jsonl + acp-agent --> tool-ask-user + acp-agent --> user-interaction stdio-agent --> agent stdio-agent --> agent-core stdio-agent --> session @@ -113,8 +117,8 @@ graph TD | `session-persistence-jsonl` | `session`, `session-persistence` | | `session-persistence-sqlite` | `session`, `session-persistence` | | `tools` | `agent`, `llm`, `system-prompt` | -| `user-interaction` | `agent` | -| `acp` | `agent`, `llm`, `session`, `session-persistence`, `tools` | +| `user-interaction` | `agent`, `llm` | +| `acp` | `agent`, `llm`, `session`, `session-persistence`, `tools`, `user-interaction` | | `agent-loop` | `agent`, `llm`, `session`, `session-persistence`, `system-prompt`, `tools` | | `subagent` | `agent`, `llm`, `tools` | | `tool-ask-user` | `agent`, `tools`, `user-interaction` | @@ -125,7 +129,7 @@ graph TD | `subagent-inprocess` | `agent`, `llm`, `session`, `subagent` | | `subagent-mock` | `agent`, `llm`, `subagent` | | `tool-subagent` | `agent`, `llm`, `subagent`, `tools` | -| `acp-agent` | `acp`, `agent-core`, `session-persistence-jsonl` | +| `acp-agent` | `acp`, `agent-core`, `session-persistence-jsonl`, `tool-ask-user`, `user-interaction` | | `stdio-agent` | `agent`, `agent-core`, `session`, `session-persistence-jsonl`, `tool-ask-user`, `ui-stdio`, `user-interaction` | | `subagent-fork` | `agent`, `session`, `subagent`, `subagent-inprocess` | | `subagent-spawn` | `subagent`, `subagent-inprocess` | diff --git a/docs/rfc/README.md b/docs/rfc/README.md index 8667cccba2..da55b13bad 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -84,6 +84,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Rich ACP bash rendering — the terminal card (`_meta`) and command classification](implemented/feature/2026-06-18-acp-terminal-and-tool-rendering.md) | 2026-06-18 | | [Subagent capability seam](implemented/feature/2026-06-21-subagent-capability-seam.md) | 2026-06-21 | | [ACP subagent backend (out-of-process delegation)](implemented/feature/2026-06-22-acp-subagent-backend.md) | 2026-06-22 | +| [Ask-user question capability](implemented/feature/2026-06-25-ask-user-question.md) | 2026-06-25 | ### Simplification diff --git a/docs/rfc/implemented/feature/2026-06-25-ask-user-question.md b/docs/rfc/implemented/feature/2026-06-25-ask-user-question.md new file mode 100644 index 0000000000..bc5c140fa5 --- /dev/null +++ b/docs/rfc/implemented/feature/2026-06-25-ask-user-question.md @@ -0,0 +1,39 @@ +# RFC: Ask-user question capability + +Status: implemented + +## Problem + +The agent sometimes cannot proceed safely from model inference alone: it needs the human to choose a path, confirm a risky/default action, or provide missing information. Before this change, the only way to get that answer was for the model to ask in assistant text and then stop, which broke the normal tool-call loop: the agent had no structured way to pause, no option metadata for UIs, no abort/error taxonomy, and no way for non-stdio front doors to present the question consistently. + +This is a user-facing capability, but it also crosses package boundaries. A model-facing tool needs a provider-neutral request vocabulary; each UI surface needs to decide how to show and collect the answer; the agent loop should remain unchanged because a tool call already has the right async shape. + +## Decision + +Introduce `dsh-user-interaction` as the core interface package for `ctx.userInteraction`, and keep the model-facing consumer `dsh-tool-ask-user` under `packages/ui/tool-ask-user` rather than the core spine. The split is intentional: core owns the abstract seam and stable request/answer/error vocabulary; UI product surfaces own the affordance that asks a human and the concrete provider that collects the answer. The tool registers `ask_user_question`, forwards `{ question, header, options, allowCustom, agent, signal }`, and returns the provider-computed `answer` as the tool result. + +The request vocabulary supports a short `header`, the required `question`, optional mutually exclusive `options`, `description` for each option, a `recommended` marker, and `allowCustom`. `label` is user-facing display text; `value` is the model-facing answer for a selected option and defaults to `label`. Providers return `AskUserQuestionAnswer.answer` as the single source of truth; the selected `option` is metadata. The tool schema exposes `description` only, not the synonym `desc`, to keep the model-facing surface small. + +Optionless questions are always free-form, even if a caller passes `allowCustom: false`. The opposite would create an unanswerable prompt: with no option to select and free-form input disallowed, every human answer would be rejected forever. Providers therefore treat "no options" as the free-form shape. + +`UserInteractionError` extends `HarnessError`, so failures such as `NO_PROVIDER`, `ASK_ABORTED`, ACP cancellation, or missing session routing survive `ctx.tools.execute()` as machine-routable `{ name, code }` tool errors. This matches the structured-error taxonomy and lets the model or a wrapping plugin distinguish "user cancelled" from a generic thrown exception. + +## UI mappings + +`dsh-ui-stdio` renders the question in readline, sorts recommended options first, shows each option's `description` on the next line, accepts the recommended option on an empty answer, and rejects pending questions on abort, provider disposal, or stdin EOF. The stdio provider serializes multiple simultaneous questions with an internal queue so only one prompt owns stdin at a time. + +`dsh-acp` provides the same seam for ACP sessions. It routes an ask request from the calling `Agent` through the bridge's `agent→sessionId` reverse map and calls ACP `unstable_createElicitation` with a session-scoped form. Option choices become a `choice` single-select field with the recommended option as the schema default; free-form answers use `answer` for optionless questions and `custom_answer` when options plus custom input are allowed. ACP `decline`/`cancel`, a missing answer, a missing session, and a client without elicitation support all become structured `UserInteractionError`s. + +The ACP mapping deliberately uses elicitation, not `session/request_permission`. `request_permission` is still reserved for the separate permission gate: it is a yes/no-or-policy authorization protocol around tool execution. `ask_user_question` is a general information-gathering tool with optional free-form answers, so ACP form elicitation is the closer protocol fit. The bridge's session routing is shared with the future permission gate, but the user intent is different. + +## Risks / trade-offs + +ACP elicitation is currently marked unstable in the SDK. The fallback is still structured: if a client does not implement it, the tool returns `ASK_FAILED` rather than hanging. A later ACP stabilization may rename or reshape the method; that migration should stay inside `dsh-acp` because the core `ctx.userInteraction` vocabulary is provider-neutral. + +The feature gives the model a powerful pause primitive, so prompt guidance matters. The tool description tells the model to ask concise questions and use options when possible. Product policy can later wrap `tools/execute` to restrict when the tool is allowed, but the loop should not special-case it. + +`dsh-tool-ask-user` lives in `packages/ui` even though it is a tool, because it is a product-facing human-interaction affordance rather than providerless loop infrastructure. The core package remains only the abstract seam; `agent-core` does not load the tool. Front-door app packages such as `stdio-agent` and `acp-agent` opt into it alongside their UI provider. + +## Test plan + +Unit coverage pins provider registration/disposal, duplicate-provider rejection, abort-before-provider, structured tool errors through `ctx.tools.execute()`, option labels/values, and the model schema including the removal of `desc`. `dsh-ui-stdio` tests cover recommended-first display, descriptions, queued questions, EOF/abort cleanup, and optionless free-form input even with `allowCustom: false`. ACP bridge tests drive a real in-memory ACP connection with the real `ask_user_question` tool and verify both selected-option and optionless free-form elicitation paths continue the agent loop. diff --git a/packages/README.md b/packages/README.md index 0fa915f3ef..3ce54b9996 100644 --- a/packages/README.md +++ b/packages/README.md @@ -28,7 +28,7 @@ dsh-bash ← dsh-brand (abstract executor seam; b dsh-session ← dsh-llm, dsh-brand dsh-system-prompt ← dsh-llm dsh-agent ← dsh-llm, dsh-session, dsh-brand -dsh-user-interaction ← dsh-agent +dsh-user-interaction ← dsh-agent, dsh-llm dsh-tools ← dsh-llm, dsh-system-prompt, dsh-agent dsh-tool-ask-user ← dsh-tools, dsh-user-interaction dsh-bash-local ← dsh-bash (BashExecutor impl) @@ -37,7 +37,7 @@ dsh-llm-deepseek ← dsh-llm (DeepSeek adapter) dsh-llm-pi-ai ← dsh-llm (pi-ai-backed adapter) dsh-agent-loop ← dsh-llm, dsh-session, dsh-system-prompt, dsh-tools, dsh-agent dsh-invariants ← dsh-llm, dsh-session, dsh-agent (dev-mode contract checks) -dsh-acp ← dsh-agent, dsh-llm, dsh-session, dsh-session-persistence (ACP JSON-RPC bridge) +dsh-acp ← dsh-agent, dsh-llm, dsh-session, dsh-session-persistence, dsh-tools, dsh-user-interaction (ACP JSON-RPC bridge + user-interaction provider) dsh-ui-stdio ← dsh-agent, dsh-session, dsh-user-interaction (stdio readline UI plugin + user-interaction provider) dsh-llm-replay ← dsh-llm, dsh-session (record/replay adapter for keyless snapshot tests) dsh-subagent ← dsh-agent, dsh-llm, dsh-tools (abstract subagent provider-registry seam) @@ -48,7 +48,7 @@ dsh-subagent-acp ← dsh-subagent, dsh-agent, dsh-llm, @agentclientprotocol/sdk dsh-tool-subagent ← dsh-subagent, dsh-tools, dsh-agent (model-facing delegation tool) dsh-agent-core ← timer, dsh-llm, dsh-session, dsh-system-prompt, dsh-tools, dsh-agent, dsh-invariants, dsh-tool-bash, dsh-agent-loop (the providerless spine, as one bundle plugin) dsh-stdio-agent ← dsh-agent-core, dsh-user-interaction, dsh-tool-ask-user, dsh-ui-stdio, dsh-session-persistence-jsonl, dsh-agent, dsh-session (stdio chat APP + bin) -dsh-acp-agent ← dsh-agent-core, dsh-acp, dsh-session-persistence-jsonl (ACP server APP + bin) +dsh-acp-agent ← dsh-agent-core, dsh-acp, dsh-user-interaction, dsh-tool-ask-user, dsh-session-persistence-jsonl (ACP server APP + bin) ``` The rule: **extension** plugins depend on interfaces, never on the concrete loop. `dsh-agent-loop` is swappable — UI/hook/tool plugins keep working against the `dsh-agent` vocabulary if the loop is replaced. The sanctioned exception is a **composition/bundle** package like `dsh-agent-core`, whose whole job is to assemble the concrete spine: it depends on `dsh-agent-loop` (and the other concrete spine plugins) on purpose. The rule constrains plugins that EXTEND the system, not the bundle that COMPOSES it — swapping the loop means shipping a different bundle, not rewiring every extension. A swappable capability splits into interface / implementation / consumer packages (the bash trio is the template — see [capability seams](../docs/rfc/implemented/architecture/2026-06-13-capability-seams.md)). @@ -62,7 +62,6 @@ The rule: **extension** plugins depend on interfaces, never on the concrete loop | `system-prompt/` | `core` | Prompt-section + tool-schema assembly registry | `ctx.systemPrompt` | | `tools/` | `core` | Tool registry + `tools/execute` waterfall | `ctx.tools` | | `user-interaction/` | `core` | Abstract human question/answer seam | `ctx.userInteraction` | -| `tool-ask-user/` | `core` | Model-facing `ask_user_question` tool | (registers on `ctx.tools`) | | `agent/` | `core` | Agent interface, registry, `agent/*` event vocabulary | `ctx.agents` | | `agent-loop/` | `core` | THE concrete loop plugin: `ReactLoopAgent` + the loop driver | `ctx.agentLoop` | | `agent-core/` | `core` | Bundle plugin: the providerless/executor-less/UI-less spine as code (forwards `agent-loop`'s `agents`) | (loads the spine) | @@ -76,6 +75,7 @@ The rule: **extension** plugins depend on interfaces, never on the concrete loop | `session-persistence-sqlite/` | `session-persistence` | SQLite persistence backend | (registers `ctx.sessionPersistence`) | | `invariants/` | `support` | Dev-mode event-contract invariants + session-log freeze | (listens on `session/*`, `agent/*`) | | `acp/` | `ui` | Agent Client Protocol bridge: serves the agent to an ACP editor over JSON-RPC stdio | (drives `ctx.agents`/`ctx.sessions`) | +| `tool-ask-user/` | `ui` | Model-facing `ask_user_question` tool | (registers on `ctx.tools`) | | `stdio-agent/` | `ui` | Terminal stdio chat APP: agent-core spine + console logger + readline UI + a pre-created `main` agent, with a `bin` | (composition + `bin`) | | `acp-agent/` | `ui` | ACP server APP: agent-core spine + JSONL persistence + the `acp` bridge (no stdout logger), with a `bin` | (composition + `bin`) | | `ui-stdio/` | `support` | Minimal stdio (readline) UI plugin: renders `agent/*` events, feeds stdin lines to the agent | (drives `ctx.agents`) | diff --git a/packages/core/README.md b/packages/core/README.md index 8f65eb9ab9..21e4b3bab0 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -8,7 +8,6 @@ The packages every harness build is assembled from: the session log, the system- | `system-prompt/` | Prompt-section + tool-schema assembly registry | `ctx.systemPrompt` | | `tools/` | Tool registry + `tools/execute` waterfall | `ctx.tools` | | `user-interaction/` | Human question/answer seam for tools and permission flows | `ctx.userInteraction` | -| `tool-ask-user/` | Model-facing `ask_user_question` tool over `ctx.userInteraction` | (registers on `ctx.tools`) | | `agent/` | Agent interface, registry, `agent/*` event vocabulary | `ctx.agents` | | `agent-loop/` | The concrete loop plugin: `ReactLoopAgent` + the loop driver | `ctx.agentLoop` | | `agent-core/` | Bundle plugin: the providerless/executor-less/UI-less spine as code | (loads the spine) | diff --git a/packages/core/user-interaction/package.json b/packages/core/user-interaction/package.json index 2b96284aca..f333195ac7 100644 --- a/packages/core/user-interaction/package.json +++ b/packages/core/user-interaction/package.json @@ -23,10 +23,12 @@ "license": "BSD-3-Clause", "peerDependencies": { "@deepseek-ai/dsh-agent": "^0.0.1", + "@deepseek-ai/dsh-llm": "^0.0.1", "cordis": "^4.0.0-rc.6" }, "devDependencies": { "@deepseek-ai/dsh-agent": "workspace:^", + "@deepseek-ai/dsh-llm": "workspace:^", "cordis": "^4.0.0-rc.6" } } diff --git a/packages/core/user-interaction/src/index.ts b/packages/core/user-interaction/src/index.ts index 95c0847e27..ad6c1dde8e 100644 --- a/packages/core/user-interaction/src/index.ts +++ b/packages/core/user-interaction/src/index.ts @@ -9,6 +9,7 @@ import { Context, Service } from 'cordis' import type { Agent } from '@deepseek-ai/dsh-agent' +import { HarnessError } from '@deepseek-ai/dsh-llm' declare module 'cordis' { interface Context { @@ -58,12 +59,9 @@ export interface UserInteractionProvider { } /** Stable error taxonomy for user-interaction failures. */ -export class UserInteractionError extends Error { - readonly code: string - +export class UserInteractionError extends HarnessError { constructor(message: string, code: string, options?: ErrorOptions) { - super(message, options) - this.code = code + super(message, code, options) this.name = 'UserInteractionError' } } diff --git a/packages/core/user-interaction/tsconfig.json b/packages/core/user-interaction/tsconfig.json index 41720b3057..cf9888627c 100644 --- a/packages/core/user-interaction/tsconfig.json +++ b/packages/core/user-interaction/tsconfig.json @@ -16,6 +16,9 @@ }, { "path": "../agent" + }, + { + "path": "../../llm/llm" } ] } diff --git a/packages/support/ui-stdio/src/index.ts b/packages/support/ui-stdio/src/index.ts index 26a1e37754..ddb662719a 100644 --- a/packages/support/ui-stdio/src/index.ts +++ b/packages/support/ui-stdio/src/index.ts @@ -261,7 +261,7 @@ export function createStdioChat(ctx: Context, config: Config, runtime: StdioRunt return } - const allowCustom = pending.request.allowCustom ?? true + const allowCustom = options.length === 0 || (pending.request.allowCustom ?? true) if (allowCustom && text !== '') { finishQuestion(pending, { answer: text }) return diff --git a/packages/support/ui-stdio/tests/ui-stdio.spec.ts b/packages/support/ui-stdio/tests/ui-stdio.spec.ts index e40847ae51..cc78b81a98 100644 --- a/packages/support/ui-stdio/tests/ui-stdio.spec.ts +++ b/packages/support/ui-stdio/tests/ui-stdio.spec.ts @@ -328,6 +328,19 @@ describe('createStdioChat input', () => { await expect(answer).resolves.toEqual({ answer: 'Use defaults' }) }) + it('accepts free-form input for an optionless question even when allowCustom is false', async () => { + const { ctx, input } = await setup() + const answer = ctx.userInteraction.ask({ + question: 'Choose?', + allowCustom: false, + }) + await new Promise(r => setImmediate(r)) + + input.feed('Use the default path') + + await expect(answer).resolves.toEqual({ answer: 'Use the default path' }) + }) + it('rejects an active question when its signal aborts', async () => { const { ctx } = await setup() const controller = new AbortController() diff --git a/packages/ui/README.md b/packages/ui/README.md index 075dfd524d..322285c5e4 100644 --- a/packages/ui/README.md +++ b/packages/ui/README.md @@ -5,9 +5,12 @@ Integrations that expose the agent to an external editor or client. These are ** | Package | Role | ctx key | |---|---|---| | `acp/` | Agent Client Protocol bridge: serves the agent to an ACP editor (Zed) over JSON-RPC stdio | (drives `ctx.agents`/`ctx.sessions`) | +| `tool-ask-user/` | Model-facing `ask_user_question` tool over `ctx.userInteraction` | (registers on `ctx.tools`) | | `stdio-agent/` | Terminal stdio chat APP: the agent-core spine + console logger + readline UI + a pre-created `main` agent, with a `bin` | (composition + `bin`) | | `acp-agent/` | ACP server APP: the agent-core spine + JSONL persistence + the `acp` bridge (no stdout logger), with a `bin` | (composition + `bin`) | A UI integration is a client-driver plugin, not a loop change and not a capability seam: it consumes the existing `agent/*` event taxonomy and the `dsh-agent` factory. The readline `ui-stdio` plugin is the unstructured analogue but lives in `support/` because it exists chiefly for the examples and the coverage gate — `ui/` is reserved for surfaces shipped as product. +`tool-ask-user` lives here because it is a model-facing product affordance that depends on a UI/provider seam; it is not part of the providerless core spine. + `stdio-agent` and `acp-agent` are the two **app packages**: each composes the [`core/agent-core`](../core/agent-core/README.md) spine with its coupled front-door cluster (and owns the boot `bin`), so a leaf `cordis.yml` is just the swappable backends plus one app entry. They live in `ui/` because each IS a user-facing front door; the stdout-purity coupling (logger vs. no logger) becomes a property of the artifact rather than a leaf convention. diff --git a/packages/ui/acp-agent/README.md b/packages/ui/acp-agent/README.md index 3403ef2126..fdacbb7f17 100644 --- a/packages/ui/acp-agent/README.md +++ b/packages/ui/acp-agent/README.md @@ -11,8 +11,10 @@ stdout is the ACP JSON-RPC channel, so the cluster is defined as much by what it | Plugin | Why | |---|---| | `@deepseek-ai/dsh-agent-core` | the spine, pre-creating **no** agents (ACP `session/new` creates them on demand) | +| `@deepseek-ai/dsh-user-interaction` | the human question/answer seam used by confirmation tools | +| `@deepseek-ai/dsh-tool-ask-user` | the model-facing `ask_user_question` tool | | `@deepseek-ai/dsh-session-persistence-jsonl` | durable JSONL session log (the bridge advertises `loadSession`) | -| `@deepseek-ai/dsh-acp` | the bridge that owns stdout for JSON-RPC | +| `@deepseek-ai/dsh-acp` | the bridge that owns stdout for JSON-RPC and provides ACP-backed user answers | | ~~console logger~~ | **omitted** — it writes to stdout and would corrupt the protocol frames ([the stdout-purity footgun](../acp/README.md)) | | ~~`hmr`~~ | **omitted** — the editor owns the subprocess | diff --git a/packages/ui/acp-agent/package.json b/packages/ui/acp-agent/package.json index 72eb95b2f7..3e732a5441 100644 --- a/packages/ui/acp-agent/package.json +++ b/packages/ui/acp-agent/package.json @@ -35,6 +35,8 @@ "@deepseek-ai/dsh-acp": "^0.0.1", "@deepseek-ai/dsh-agent-core": "^0.0.1", "@deepseek-ai/dsh-session-persistence-jsonl": "^0.0.1", + "@deepseek-ai/dsh-tool-ask-user": "^0.0.1", + "@deepseek-ai/dsh-user-interaction": "^0.0.1", "cordis": "^4.0.0-rc.6", "schemastery": "^3.17.0" }, @@ -44,6 +46,8 @@ "@deepseek-ai/dsh-acp": "workspace:^", "@deepseek-ai/dsh-agent-core": "workspace:^", "@deepseek-ai/dsh-session-persistence-jsonl": "workspace:^", + "@deepseek-ai/dsh-tool-ask-user": "workspace:^", + "@deepseek-ai/dsh-user-interaction": "workspace:^", "cordis": "^4.0.0-rc.6", "schemastery": "^3.17.0" } diff --git a/packages/ui/acp-agent/src/index.ts b/packages/ui/acp-agent/src/index.ts index 625467cac2..c04f2fe72a 100644 --- a/packages/ui/acp-agent/src/index.ts +++ b/packages/ui/acp-agent/src/index.ts @@ -34,6 +34,8 @@ import z from 'schemastery' import * as acp from '@deepseek-ai/dsh-acp' import * as agentCore from '@deepseek-ai/dsh-agent-core' import SessionPersistenceJsonl from '@deepseek-ai/dsh-session-persistence-jsonl' +import UserInteractionService from '@deepseek-ai/dsh-user-interaction' +import * as toolAskUser from '@deepseek-ai/dsh-tool-ask-user' export const name = 'acp-agent' @@ -67,6 +69,8 @@ export const Config: z = z.object({ */ export function apply(ctx: Context, config: Config): void { ctx.plugin(agentCore) + ctx.plugin(UserInteractionService) + ctx.plugin(toolAskUser) ctx.plugin(SessionPersistenceJsonl, { root: config.persistenceRoot ?? './.sessions' }) ctx.plugin(acp, { model: config.model, systemPrompt: config.systemPrompt }) } diff --git a/packages/ui/acp-agent/tests/acp-agent.spec.ts b/packages/ui/acp-agent/tests/acp-agent.spec.ts index 7a02837fca..aa40079cb6 100644 --- a/packages/ui/acp-agent/tests/acp-agent.spec.ts +++ b/packages/ui/acp-agent/tests/acp-agent.spec.ts @@ -29,6 +29,8 @@ describe('dsh-acp-agent composition', () => { expect(ctx.get('sessions')).toBeDefined() expect(ctx.get('sessionPersistence')).toBeDefined() expect(ctx.get('agentLoop')).toBeDefined() + expect(ctx.get('userInteraction')).toBeDefined() + expect(ctx.get('tools')?.get('ask_user_question')).toBeDefined() // No pre-created agents — ACP session/new creates them on demand. expect(ctx.get('agents')!.list()).toHaveLength(0) await ctx.fiber.dispose() diff --git a/packages/ui/acp-agent/tests/built-bin.e2e.ts b/packages/ui/acp-agent/tests/built-bin.e2e.ts index 51c5d53c0b..e2bab79ee4 100644 --- a/packages/ui/acp-agent/tests/built-bin.e2e.ts +++ b/packages/ui/acp-agent/tests/built-bin.e2e.ts @@ -39,10 +39,12 @@ const acpBin = join(repoRoot, 'packages/ui/acp-agent/lib/bin.js') const dshPackages = [ 'core/agent-core', 'core/agent', 'core/session', 'core/system-prompt', - 'core/tools', 'core/agent-loop', 'llm/llm', 'llm/llm-deepseek', 'bash/bash', - 'bash/bash-local', 'bash/tool-bash', 'support/invariants', + 'core/tools', 'core/user-interaction', 'core/agent-loop', 'llm/llm', + 'llm/llm-deepseek', 'bash/bash', 'bash/bash-local', 'bash/tool-bash', + 'support/invariants', 'session-persistence/session-persistence', 'session-persistence/session-persistence-jsonl', 'ui/acp', 'ui/acp-agent', + 'ui/tool-ask-user', ] const vendorPackages = [ 'cordis', 'loader', 'include', 'timer', 'hmr', 'logger-console', diff --git a/packages/ui/acp-agent/tsconfig.json b/packages/ui/acp-agent/tsconfig.json index ffea8ec6f6..79a68c6337 100644 --- a/packages/ui/acp-agent/tsconfig.json +++ b/packages/ui/acp-agent/tsconfig.json @@ -23,6 +23,12 @@ { "path": "../../core/agent-core" }, + { + "path": "../../core/user-interaction" + }, + { + "path": "../tool-ask-user" + }, { "path": "../../session-persistence/session-persistence-jsonl" } diff --git a/packages/ui/acp/README.md b/packages/ui/acp/README.md index ad50383542..771cf47af3 100644 --- a/packages/ui/acp/README.md +++ b/packages/ui/acp/README.md @@ -8,7 +8,7 @@ It is a **client-driver / UI plugin**, the structured analogue of the readline ` `apply(ctx, config)` — wires an `AgentSideConnection` (from `@agentclientprotocol/sdk`) to `process.stdin`/`process.stdout` and implements the ACP `Agent` method surface. -`inject: ['agents', 'sessions', 'sessionPersistence', 'tools']` — programs against the interface packages only (never `dsh-agent-loop`). `sessionPersistence` is required because `initialize` advertises `loadSession: true`; `tools` lets a tool own how its calls render (`presentCall`/`presentResult`) — the bridge looks the definition up by name and falls back to a generic presentation when a tool declares none (see Tool-call presentation). +`inject: ['agents', 'sessions', 'sessionPersistence', 'tools', 'userInteraction']` — programs against the interface packages only (never `dsh-agent-loop`). `sessionPersistence` is required because `initialize` advertises `loadSession: true`; `tools` lets a tool own how its calls render (`presentCall`/`presentResult`) — the bridge looks the definition up by name and falls back to a generic presentation when a tool declares none (see Tool-call presentation). `userInteraction` lets the bridge provide ACP-backed answers for tools such as `ask_user_question`. ### Config @@ -29,10 +29,11 @@ It is a **client-driver / UI plugin**, the structured analogue of the readline ` | `session/prompt` | `agent.send()` | supports ACP `text` and `resource_link` blocks; rejects image/audio/embedded resource and empty prompts; one in-flight prompt PER session (independent); settles on the OWNING turn's end (a turn that ends in `error` rejects the RPC) | | `session/cancel` | `agent.cancel()` | the queue-aware cancel: aborts a running step, clears queued + steering work, and drops a turn about to start, then settles the prompt `cancelled` — for ONLY that session (a cancel never touches another session's stream or prompt) | | `session/update` | `session/event` | `agent_message_chunk` (text-delta), `agent_thought_chunk` (reasoning-delta), `user_message_chunk` (load replay), `tool_call`/`tool_call_update` (title/kind/rawInput/content owned by the TOOL via `presentCall`/`presentResult` — see Tool-call presentation) | +| `elicitation/create` | `ctx.userInteraction` provider | `ask_user_question` pauses the tool call and asks the ACP client for a session-scoped form; choices use a `choice` single-select field, free-form answers use `answer`/`custom_answer`, and cancel/decline returns a structured `UserInteractionError` | ## Multi-session -The bridge multiplexes N sessions over one connection. Live sessions are held in a `Map` (forward) with a `WeakMap` reverse map so `agent/*` events — which carry only the `Agent` — demux in O(1). Every `session/event` and `agent/status` is routed strictly to its owning record, so concurrent sessions never cross-settle or interleave their `session/update` notifications. State is per session: one in-flight prompt each, `session/cancel` aborts and settles only its own agent/prompt, and disposal drains every live session in parallel to quiescence. (Per-session *permission* ownership is reserved for the deferred permission gate — `TODO(rfc010-permission-gate)`.) +The bridge multiplexes N sessions over one connection. Live sessions are held in a `Map` (forward) with a `WeakMap` reverse map so `agent/*` events — and `ctx.userInteraction` requests, which carry only the calling `Agent` — demux in O(1). Every `session/event`, `agent/status`, and ask-user elicitation is routed strictly to its owning record, so concurrent sessions never cross-settle or interleave their `session/update` notifications. State is per session: one in-flight prompt each, `session/cancel` aborts and settles only its own agent/prompt, and disposal drains every live session in parallel to quiescence. (Per-session *permission* ownership is reserved for the deferred permission gate — `TODO(rfc010-permission-gate)`.) Background-task isolation rides on `dsh-tool-bash`: bash task ids are global and predictable, so each task carries an opaque owner token — the owning agent's `session.header.id` — stored on the task inside the executor (`dsh-bash`'s `ownerOf(id)` seam). `bash_output`/`bash_kill` reject a task whose token differs from the caller's session token, so one session's agent can't read or kill another's task. Ownership is by session TOKEN, not `Agent` object identity — a different `Agent` object on the same session may access the task — and because the token lives on the executor's task it survives a `tool-bash` HMR reload. diff --git a/packages/ui/acp/acp-feature-support.md b/packages/ui/acp/acp-feature-support.md index 429c862b27..56855bfd00 100644 --- a/packages/ui/acp/acp-feature-support.md +++ b/packages/ui/acp/acp-feature-support.md @@ -47,7 +47,7 @@ These are capabilities the bridge would *drive* on the editor. The harness runs | `terminal/wait_for_exit` | S | ❌ | ❌ | ❌ | As above. | | `terminal/kill` | S | ❌ | ❌ | ❌ | As above. | | `terminal/release` | S | ❌ | ❌ | ❌ | As above. | -| `elicitation/create` · `elicitation/complete` | U | ❌ | ✅ | ⚠️ | Structured user-input forms. Claude calls the `unstable_*` elicitation methods (to surface MCP server elicitations); Codex does NOT — its `CodexElicitationHandler` maps elicitations onto `session/request_permission` instead. | +| `elicitation/create` · `elicitation/complete` | U | ⚠️ | ✅ | ⚠️ | The bridge drives `unstable_createElicitation` for `ask_user_question` form prompts (session-scoped, no URL-mode flow yet). Claude calls the `unstable_*` elicitation methods for MCP server elicitations; Codex maps elicitations onto `session/request_permission`. | ## 3. Capabilities @@ -140,7 +140,7 @@ The bridge rejects unsupported prompt blocks rather than silently dropping them Ranked by how commonly the reference adapters ship them and how much UX they unlock: -1. **Permission gate** — `session/request_permission` + permission options. Tracked `TODO(rfc010-permission-gate)`; the reverse map is already wired. Foundational, and a prerequisite for modes. +1. **Permission gate** — `session/request_permission` + permission options. Tracked `TODO(rfc010-permission-gate)`; the reverse map is already wired and shared with `ask_user_question` routing. Foundational, and a prerequisite for modes. 2. **Session lifecycle** — `session/list` + `session/delete` (the persistence layer already lists), then `session/resume` / `session/close`. 3. **Modes / config options / model selection** — coupled to the permission gate. 4. **Agent plan** (`sessionUpdate: 'plan'`) — surface the loop's plan as structured entries. diff --git a/packages/ui/acp/package.json b/packages/ui/acp/package.json index 6973dc5e20..4220f4da52 100644 --- a/packages/ui/acp/package.json +++ b/packages/ui/acp/package.json @@ -32,6 +32,7 @@ "@deepseek-ai/dsh-session": "^0.0.1", "@deepseek-ai/dsh-session-persistence": "^0.0.1", "@deepseek-ai/dsh-tools": "^0.0.1", + "@deepseek-ai/dsh-user-interaction": "^0.0.1", "cordis": "^4.0.0-rc.6" }, "devDependencies": { @@ -45,6 +46,8 @@ "@deepseek-ai/dsh-system-prompt": "workspace:^", "@deepseek-ai/dsh-tool-bash": "workspace:^", "@deepseek-ai/dsh-tools": "workspace:^", + "@deepseek-ai/dsh-tool-ask-user": "workspace:^", + "@deepseek-ai/dsh-user-interaction": "workspace:^", "cordis": "^4.0.0-rc.6" } } diff --git a/packages/ui/acp/src/index.ts b/packages/ui/acp/src/index.ts index ec79e97443..7ab9aeac09 100644 --- a/packages/ui/acp/src/index.ts +++ b/packages/ui/acp/src/index.ts @@ -47,6 +47,9 @@ import { type AuthenticateRequest, type CancelNotification, type ContentBlock as AcpContentBlock, + type CreateElicitationRequest, + type ElicitationContentValue, + type EnumOption, type InitializeRequest, type InitializeResponse, type LoadSessionRequest, @@ -69,6 +72,12 @@ import type { ToolCallKind, ToolCallPresentation, ToolRegistry, ToolResultPresen // Side-effect type import: declaration-merges `ctx.sessionPersistence` onto // Context (the bridge injects it and reads `list()` for load cwd validation). import type {} from '@deepseek-ai/dsh-session-persistence' +import { + UserInteractionError, + type AskUserQuestionAnswer, + type AskUserQuestionOption, + type AskUserQuestionRequest, +} from '@deepseek-ai/dsh-user-interaction' import { acpPromptToText, harnessBlockToAcpContent, @@ -82,7 +91,7 @@ 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. -export const inject = ['agents', 'sessions', 'sessionPersistence', 'tools'] +export const inject = ['agents', 'sessions', 'sessionPersistence', 'tools', 'userInteraction'] /** * Build an ACP "invalid params" error whose human detail rides in the message. @@ -109,6 +118,119 @@ function sameWorkspaceCwd(left: string, right: string): boolean { return resolvePath(left) === resolvePath(right) } +function optionAnswer(option: AskUserQuestionOption): string { + return option.value ?? option.label +} + +function orderedOptions(options: readonly AskUserQuestionOption[] | undefined): AskUserQuestionOption[] { + return [...(options ?? [])].sort((a, b) => Number(Boolean(b.recommended)) - Number(Boolean(a.recommended))) +} + +function optionDescription(option: AskUserQuestionOption): string { + return option.description === undefined + ? option.label + : `${option.label}: ${option.description}` +} + +function selectedOption( + options: readonly AskUserQuestionOption[], + answer: string, +): AskUserQuestionOption | undefined { + return options.find(option => optionAnswer(option) === answer) +} + +function requireStringContent( + content: Record | null | undefined, + key: string, +): string | undefined { + const value = content?.[key] + return typeof value === 'string' && value.trim().length > 0 ? value : undefined +} + +function askAbortError(): UserInteractionError { + return new UserInteractionError('ask_user_question was aborted before the user answered', 'ASK_ABORTED') +} + +function withAbort(promise: Promise, signal: AbortSignal | undefined): Promise { + if (signal === undefined) return promise + if (signal.aborted) return Promise.reject(askAbortError()) + return new Promise((resolve, reject) => { + const onAbort = (): void => { + signal.removeEventListener('abort', onAbort) + reject(askAbortError()) + } + signal.addEventListener('abort', onAbort, { once: true }) + promise.then( + (value) => { + signal.removeEventListener('abort', onAbort) + resolve(value) + }, + (error: unknown) => { + signal.removeEventListener('abort', onAbort) + reject(new Error(String(error), { cause: error })) + }, + ) + }) +} + +function elicitationForQuestion( + sessionId: SessionId, + request: AskUserQuestionRequest, + options: AskUserQuestionOption[], +): CreateElicitationRequest { + const allowCustom = options.length === 0 || (request.allowCustom ?? true) + const title = request.header ?? 'Question' + if (options.length === 0) { + return { + sessionId, + mode: 'form', + message: request.question, + requestedSchema: { + type: 'object', + title, + properties: { + answer: { type: 'string', title: request.question }, + }, + required: ['answer'], + }, + } + } + + const choiceOptions: EnumOption[] = options.map(option => ({ + const: optionAnswer(option), + title: optionDescription(option), + })) + const recommended = options.find(option => option.recommended) + return { + sessionId, + mode: 'form', + message: request.question, + requestedSchema: { + type: 'object', + title, + properties: { + choice: { + type: 'string', + title: request.question, + description: allowCustom ? 'Choose one option, or fill a custom answer below.' : 'Choose one option.', + oneOf: choiceOptions, + ...recommended !== undefined ? { default: optionAnswer(recommended) } : {}, + }, + ...allowCustom + ? { + custom_answer: { + type: 'string' as const, + title: 'Custom answer', + description: 'Optional free-form answer. Leave empty to use the selected option.', + }, + } + : {}, + }, + required: allowCustom ? [] : ['choice'], + }, + } +} + /** Plugin config: the agent template ACP sessions are created from. */ export interface AcpConfig { /** Model name for created agents (must have a registered adapter). */ @@ -224,6 +346,7 @@ export function apply(ctx: Context, config: AcpConfig): void { const sessionPersistence = ctx.sessionPersistence const logger = ctx.logger const tools = ctx.tools + const userInteraction = ctx.userInteraction // A new ToolPresenter per session (and a throwaway per load replay), each given // this warn sink so a throwing tool presenter is logged, not propagated. const makePresenter = (): ToolPresenter => new ToolPresenter(tools, (message) => { logger.warn(message) }) @@ -254,6 +377,37 @@ export function apply(ctx: Context, config: AcpConfig): void { // `notify` never observes it unset — no undefined guard needed. let conn: AgentSideConnection + userInteraction.registerProvider({ + async ask(request: AskUserQuestionRequest): Promise { + if (request.agent === undefined) { + throw new UserInteractionError('ACP user questions must come from an agent-owned request', 'NO_AGENT') + } + const sessionId = bySession.get(request.agent) + if (sessionId === undefined) { + throw new UserInteractionError('ACP user question has no matching session', 'NO_SESSION') + } + const options = orderedOptions(request.options) + const response = await withAbort(conn.unstable_createElicitation( + elicitationForQuestion(sessionId, request, options), + ), request.signal).catch((error: unknown) => { + if (error instanceof UserInteractionError) throw error + throw new UserInteractionError('ACP elicitation request failed', 'ASK_FAILED', { cause: error }) + }) + if (response.action !== 'accept') { + throw new UserInteractionError('ask_user_question was cancelled by the user', 'ASK_CANCELLED') + } + const customAnswer = requireStringContent(response.content, 'custom_answer') + if (customAnswer !== undefined) return { answer: customAnswer } + + const answer = requireStringContent(response.content, options.length === 0 ? 'answer' : 'choice') + if (answer === undefined) { + throw new UserInteractionError('ask_user_question returned no answer', 'NO_ANSWER') + } + const option = selectedOption(options, answer) + return option === undefined ? { answer } : { answer, option } + }, + }) + /** * Reject any RPC after the bridge has torn down. The `AgentSideConnection` * receive loop can outlive the plugin fiber — under an ACP-only HMR reload the diff --git a/packages/ui/acp/tests/bridge.spec.ts b/packages/ui/acp/tests/bridge.spec.ts index e9ebca8d62..602b355b6f 100644 --- a/packages/ui/acp/tests/bridge.spec.ts +++ b/packages/ui/acp/tests/bridge.spec.ts @@ -4,7 +4,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { PROTOCOL_VERSION } from '@agentclientprotocol/sdk' import { AgentId } from '@deepseek-ai/dsh-agent' -import { makeBridgeHarness, textResponse, type BridgeHarness } from './harness.ts' +import { makeBridgeHarness, textResponse, toolCallResponse, type BridgeHarness } from './harness.ts' /** * End-to-end bridge specs over an in-memory transport: a real @@ -53,6 +53,183 @@ describe('acp bridge', () => { expect(text).toBe('hello there') }) + it('routes ask_user_question through ACP form elicitation and continues with the selected option', async () => { + harness = await makeBridgeHarness({ + storageDir, + withAskUser: true, + script: [ + toolCallResponse('ask-1', 'ask_user_question', { + header: 'Project config', + question: 'Which language should I use?', + options: [ + { label: 'TypeScript', value: 'ts', description: 'Good for UI apps' }, + { label: 'Python', value: 'py', description: 'Good for scripts', recommended: true }, + ], + allow_custom: false, + }), + textResponse('Python it is.'), + ], + }) + harness.onElicitation = () => ({ action: 'accept', content: { choice: 'py' } }) + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) + + const result = await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'ask me' }] }) + + expect(result.stopReason).toBe('end_turn') + expect(harness.elicitationRequests).toHaveLength(1) + expect(harness.elicitationRequests[0]).toMatchObject({ + sessionId, + mode: 'form', + message: 'Which language should I use?', + requestedSchema: { + title: 'Project config', + properties: { + choice: { + default: 'py', + oneOf: [ + { const: 'py', title: 'Python: Good for scripts' }, + { const: 'ts', title: 'TypeScript: Good for UI apps' }, + ], + }, + }, + required: ['choice'], + }, + }) + const toolResult = harness.ctx.agents.get(AgentId(sessionId))!.session.events.find(event => event.type === 'tool/result') + expect(JSON.stringify(toolResult)).toContain('py') + }) + + it('routes optionless ask_user_question through an ACP free-form answer field', async () => { + harness = await makeBridgeHarness({ + storageDir, + withAskUser: true, + script: [ + toolCallResponse('ask-1', 'ask_user_question', { + question: 'What should I name it?', + allow_custom: false, + }), + textResponse('Name recorded.'), + ], + }) + harness.onElicitation = () => ({ action: 'accept', content: { answer: 'apollo' } }) + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) + + await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'ask me' }] }) + + expect(harness.elicitationRequests[0]).toMatchObject({ + requestedSchema: { + properties: { answer: { type: 'string', title: 'What should I name it?' } }, + required: ['answer'], + }, + }) + const toolResult = harness.ctx.agents.get(AgentId(sessionId))!.session.events.find(event => event.type === 'tool/result') + expect(JSON.stringify(toolResult)).toContain('apollo') + }) + + it('supports ACP custom answers alongside choices', async () => { + harness = await makeBridgeHarness({ storageDir, withAskUser: true }) + harness.onElicitation = () => ({ action: 'accept', content: { custom_answer: 'Use Zig' } }) + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) + const agent = harness.ctx.agents.get(AgentId(sessionId))! + + const result = await harness.ctx.userInteraction.ask({ + agent, + question: 'Which language?', + options: [{ label: 'TypeScript' }], + }) + + expect(result).toEqual({ answer: 'Use Zig' }) + expect(harness.elicitationRequests[0]).toMatchObject({ + requestedSchema: { + properties: { + choice: { + description: 'Choose one option, or fill a custom answer below.', + oneOf: [{ const: 'TypeScript', title: 'TypeScript' }], + }, + custom_answer: { type: 'string' }, + }, + required: [], + }, + }) + }) + + it('returns raw ACP answers when they do not match a provided option', async () => { + harness = await makeBridgeHarness({ storageDir, withAskUser: true }) + harness.onElicitation = () => ({ action: 'accept', content: { choice: 'something else' } }) + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) + const agent = harness.ctx.agents.get(AgentId(sessionId))! + + await expect(harness.ctx.userInteraction.ask({ + agent, + question: 'Pick', + options: [{ label: 'A', value: 'a' }], + allowCustom: false, + })).resolves.toEqual({ answer: 'something else' }) + }) + + it('reports ACP ask-user routing and answer failures as structured errors', async () => { + harness = await makeBridgeHarness({ storageDir, withAskUser: true }) + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) + const agent = harness.ctx.agents.get(AgentId(sessionId))! + + await expect(harness.ctx.userInteraction.ask({ question: 'No agent?' })) + .rejects.toMatchObject({ name: 'UserInteractionError', code: 'NO_AGENT' }) + await expect(harness.ctx.userInteraction.ask({ agent: { id: 'other' } as typeof agent, question: 'No session?' })) + .rejects.toMatchObject({ code: 'NO_SESSION' }) + + harness.onElicitation = () => ({ action: 'cancel' }) + await expect(harness.ctx.userInteraction.ask({ agent, question: 'Cancel?' })) + .rejects.toMatchObject({ code: 'ASK_CANCELLED' }) + + harness.onElicitation = () => ({ action: 'accept', content: {} }) + await expect(harness.ctx.userInteraction.ask({ agent, question: 'Empty?' })) + .rejects.toMatchObject({ code: 'NO_ANSWER' }) + + harness.onElicitation = () => { throw new Error('client boom') } + await expect(harness.ctx.userInteraction.ask({ agent, question: 'Client fails?', signal: new AbortController().signal })) + .rejects.toMatchObject({ code: 'ASK_FAILED' }) + }) + + it('aborts ACP ask-user requests before and while waiting for elicitation', async () => { + harness = await makeBridgeHarness({ storageDir, withAskUser: true }) + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) + const agent = harness.ctx.agents.get(AgentId(sessionId))! + + const alreadyAborted = new AbortController() + alreadyAborted.abort() + await expect(harness.ctx.userInteraction.ask({ agent, question: 'Already?', signal: alreadyAborted.signal })) + .rejects.toMatchObject({ code: 'ASK_ABORTED' }) + + let abortedReads = 0 + const racingAbort = { + get aborted() { return abortedReads++ > 0 }, + addEventListener() {}, + removeEventListener() {}, + dispatchEvent() { return false }, + onabort: null, + reason: undefined, + throwIfAborted() {}, + } as AbortSignal + await expect(harness.ctx.userInteraction.ask({ agent, question: 'Raced?', signal: racingAbort })) + .rejects.toMatchObject({ code: 'ASK_ABORTED' }) + + let release: ((value: { action: 'accept'; content: { answer: string } }) => void) | undefined + harness.onElicitation = () => new Promise((resolve) => { release = resolve }) + const pendingAbort = new AbortController() + const ask = harness.ctx.userInteraction.ask({ agent, question: 'Pending?', signal: pendingAbort.signal }) + await new Promise(resolve => setImmediate(resolve)) + pendingAbort.abort() + + await expect(ask).rejects.toMatchObject({ code: 'ASK_ABORTED' }) + release?.({ action: 'accept', content: { answer: 'too late' } }) + }) + it('allows multiple concurrent sessions, each with a distinct id', async () => { harness = await makeBridgeHarness({ storageDir, script: [] }) await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) diff --git a/packages/ui/acp/tests/harness.ts b/packages/ui/acp/tests/harness.ts index 4f6b5ac17a..a49b5c75f9 100644 --- a/packages/ui/acp/tests/harness.ts +++ b/packages/ui/acp/tests/harness.ts @@ -25,11 +25,15 @@ import { ndJsonStream, type Agent as AcpAgent, type Client, + type CreateElicitationRequest, + type CreateElicitationResponse, type RequestPermissionRequest, type RequestPermissionResponse, type SessionNotification, type Stream, } from '@agentclientprotocol/sdk' +import UserInteractionService from '@deepseek-ai/dsh-user-interaction' +import * as ToolAskUser from '@deepseek-ai/dsh-tool-ask-user' import * as AcpPlugin from '../src/index.ts' import { type AcpConfig } from '../src/index.ts' @@ -117,6 +121,10 @@ export interface BridgeHarness { permissionRequests: RequestPermissionRequest[] /** Decide each permission request's outcome (default: cancelled). */ onPermission: (req: RequestPermissionRequest) => RequestPermissionResponse + /** Elicitation requests the bridge issued for ask_user_question. */ + elicitationRequests: CreateElicitationRequest[] + /** Decide each elicitation response (default: cancel). */ + onElicitation: (req: CreateElicitationRequest) => CreateElicitationResponse | Promise /** If set, the client's sessionUpdate throws this (tests notify error path). */ onSessionUpdateError: (() => void) | undefined /** @@ -158,6 +166,8 @@ export async function makeBridgeHarness(options: { * implementation over a mock in tests"). */ withBash?: boolean + /** Plug the REAL `ask_user_question` tool and ACP user-interaction provider. */ + withAskUser?: boolean } = { storageDir: '' }): Promise { const adapter = new MockAdapter(options.script ?? []) @@ -169,6 +179,10 @@ export async function makeBridgeHarness(options: { await ctx.plugin(AgentRegistry) await ctx.plugin(AgentLoop, { agents: [] }) await ctx.plugin(SessionPersistenceJsonl, { root: options.storageDir }) + await ctx.plugin(UserInteractionService) + if (options.withAskUser) { + await ctx.plugin(ToolAskUser) + } if (options.withBash) { await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 }) await ctx.plugin(ToolBash) @@ -197,6 +211,7 @@ export async function makeBridgeHarness(options: { const updates: CapturedUpdate[] = [] const sessionUpdates: { sessionId: string; update: CapturedUpdate }[] = [] const permissionRequests: RequestPermissionRequest[] = [] + const elicitationRequests: CreateElicitationRequest[] = [] const harness: BridgeHarness = { ctx, adapter, @@ -204,6 +219,8 @@ export async function makeBridgeHarness(options: { sessionUpdates, permissionRequests, onPermission: () => ({ outcome: { outcome: 'cancelled' } }), + elicitationRequests, + onElicitation: () => ({ action: 'cancel' }), onSessionUpdateError: undefined, client: undefined as unknown as ClientSideConnection, acpFiber: undefined as unknown as BridgeHarness['acpFiber'], @@ -229,6 +246,10 @@ export async function makeBridgeHarness(options: { permissionRequests.push(params) return Promise.resolve(harness.onPermission(params)) }, + unstable_createElicitation(params: CreateElicitationRequest): Promise { + elicitationRequests.push(params) + return Promise.resolve(harness.onElicitation(params)) + }, }) // Wire the bridge (agent side) and the client (test side). The test config diff --git a/packages/ui/acp/tsconfig.json b/packages/ui/acp/tsconfig.json index 5989363d7f..9e24fa0f7a 100644 --- a/packages/ui/acp/tsconfig.json +++ b/packages/ui/acp/tsconfig.json @@ -29,6 +29,9 @@ { "path": "../../core/tools" }, + { + "path": "../../core/user-interaction" + }, { "path": "../../session-persistence/session-persistence" } diff --git a/packages/ui/stdio-agent/tests/built-bin.e2e.ts b/packages/ui/stdio-agent/tests/built-bin.e2e.ts index d53296929d..b28bf382ad 100644 --- a/packages/ui/stdio-agent/tests/built-bin.e2e.ts +++ b/packages/ui/stdio-agent/tests/built-bin.e2e.ts @@ -34,7 +34,7 @@ const stdioBin = join(repoRoot, 'packages/ui/stdio-agent/lib/bin.js') // to the built `lib/` (package.json `main`), exactly as an installed dep would. const dshPackages = [ 'core/agent-core', 'core/agent', 'core/session', 'core/system-prompt', - 'core/tools', 'core/user-interaction', 'core/tool-ask-user', + 'core/tools', 'core/user-interaction', 'ui/tool-ask-user', 'core/agent-loop', 'llm/llm', 'bash/bash', 'bash/bash-local', 'bash/tool-bash', 'support/invariants', 'support/ui-stdio', 'session-persistence/session-persistence', diff --git a/packages/ui/stdio-agent/tsconfig.json b/packages/ui/stdio-agent/tsconfig.json index 6cb6130fb7..9cd7f8d998 100644 --- a/packages/ui/stdio-agent/tsconfig.json +++ b/packages/ui/stdio-agent/tsconfig.json @@ -33,7 +33,7 @@ "path": "../../core/user-interaction" }, { - "path": "../../core/tool-ask-user" + "path": "../tool-ask-user" }, { "path": "../../session-persistence/session-persistence-jsonl" diff --git a/packages/core/tool-ask-user/README.md b/packages/ui/tool-ask-user/README.md similarity index 100% rename from packages/core/tool-ask-user/README.md rename to packages/ui/tool-ask-user/README.md diff --git a/packages/core/tool-ask-user/package.json b/packages/ui/tool-ask-user/package.json similarity index 100% rename from packages/core/tool-ask-user/package.json rename to packages/ui/tool-ask-user/package.json diff --git a/packages/core/tool-ask-user/src/index.ts b/packages/ui/tool-ask-user/src/index.ts similarity index 100% rename from packages/core/tool-ask-user/src/index.ts rename to packages/ui/tool-ask-user/src/index.ts diff --git a/packages/core/tool-ask-user/tests/tool-ask-user.spec.ts b/packages/ui/tool-ask-user/tests/tool-ask-user.spec.ts similarity index 93% rename from packages/core/tool-ask-user/tests/tool-ask-user.spec.ts rename to packages/ui/tool-ask-user/tests/tool-ask-user.spec.ts index ca929533ff..b6fd9d92e4 100644 --- a/packages/core/tool-ask-user/tests/tool-ask-user.spec.ts +++ b/packages/ui/tool-ask-user/tests/tool-ask-user.spec.ts @@ -126,6 +126,21 @@ describe('ask_user_question tool', () => { expect(seen[0]).toMatchObject({ header: 'Confirm', agent }) }) + it('returns structured user-interaction errors through tool execution', async () => { + const ctx = await setup() + + const result = await ctx.tools.execute({ + callId: CallId('ask-no-provider'), + name: 'ask_user_question', + arguments: { question: 'Continue?' }, + }) + + expect(result).toMatchObject({ + isError: true, + error: { name: 'UserInteractionError', code: 'NO_PROVIDER' }, + }) + }) + it('uses an option label when the selected option has no explicit value', async () => { const ctx = await setup() ctx.userInteraction.registerProvider({ diff --git a/packages/core/tool-ask-user/tsconfig.json b/packages/ui/tool-ask-user/tsconfig.json similarity index 74% rename from packages/core/tool-ask-user/tsconfig.json rename to packages/ui/tool-ask-user/tsconfig.json index f51b8c4495..06805c0b8f 100644 --- a/packages/core/tool-ask-user/tsconfig.json +++ b/packages/ui/tool-ask-user/tsconfig.json @@ -21,16 +21,16 @@ "path": "../../llm/llm" }, { - "path": "../agent" + "path": "../../core/agent" }, { - "path": "../system-prompt" + "path": "../../core/system-prompt" }, { - "path": "../tools" + "path": "../../core/tools" }, { - "path": "../user-interaction" + "path": "../../core/user-interaction" } ] } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c6554ad952..78e26a0864 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -221,27 +221,6 @@ importers: specifier: ^4.0.0-rc.6 version: 4.0.0-rc.6(@cordisjs/plugin-include@1.0.4)(@cordisjs/plugin-loader@1.0.0-rc.4) - packages/core/tool-ask-user: - devDependencies: - '@deepseek-ai/dsh-agent': - specifier: workspace:^ - version: link:../agent - '@deepseek-ai/dsh-llm': - specifier: workspace:^ - version: link:../../llm/llm - '@deepseek-ai/dsh-system-prompt': - specifier: workspace:^ - version: link:../system-prompt - '@deepseek-ai/dsh-tools': - specifier: workspace:^ - version: link:../tools - '@deepseek-ai/dsh-user-interaction': - specifier: workspace:^ - version: link:../user-interaction - cordis: - specifier: ^4.0.0-rc.6 - version: 4.0.0-rc.6(@cordisjs/plugin-include@1.0.4)(@cordisjs/plugin-loader@1.0.0-rc.4) - packages/core/tools: devDependencies: '@deepseek-ai/dsh-agent': @@ -262,6 +241,9 @@ importers: '@deepseek-ai/dsh-agent': specifier: workspace:^ version: link:../agent + '@deepseek-ai/dsh-llm': + specifier: workspace:^ + version: link:../../llm/llm cordis: specifier: ^4.0.0-rc.6 version: 4.0.0-rc.6(@cordisjs/plugin-include@1.0.4)(@cordisjs/plugin-loader@1.0.0-rc.4) @@ -651,12 +633,18 @@ importers: '@deepseek-ai/dsh-system-prompt': specifier: workspace:^ version: link:../../core/system-prompt + '@deepseek-ai/dsh-tool-ask-user': + specifier: workspace:^ + version: link:../tool-ask-user '@deepseek-ai/dsh-tool-bash': specifier: workspace:^ version: link:../../bash/tool-bash '@deepseek-ai/dsh-tools': specifier: workspace:^ version: link:../../core/tools + '@deepseek-ai/dsh-user-interaction': + specifier: workspace:^ + version: link:../../core/user-interaction cordis: specifier: ^4.0.0-rc.6 version: 4.0.0-rc.6(@cordisjs/plugin-include@1.0.4)(@cordisjs/plugin-loader@1.0.0-rc.4) @@ -678,6 +666,12 @@ importers: '@deepseek-ai/dsh-session-persistence-jsonl': specifier: workspace:^ version: link:../../session-persistence/session-persistence-jsonl + '@deepseek-ai/dsh-tool-ask-user': + specifier: workspace:^ + version: link:../tool-ask-user + '@deepseek-ai/dsh-user-interaction': + specifier: workspace:^ + version: link:../../core/user-interaction cordis: specifier: ^4.0.0-rc.6 version: 4.0.0-rc.6(@cordisjs/plugin-include@vendor+include)(@cordisjs/plugin-loader@vendor+loader) @@ -710,7 +704,7 @@ importers: version: link:../../session-persistence/session-persistence-jsonl '@deepseek-ai/dsh-tool-ask-user': specifier: workspace:^ - version: link:../../core/tool-ask-user + version: link:../tool-ask-user '@deepseek-ai/dsh-ui-stdio': specifier: workspace:^ version: link:../../support/ui-stdio @@ -724,6 +718,27 @@ importers: specifier: ^3.17.0 version: 3.18.0 + packages/ui/tool-ask-user: + devDependencies: + '@deepseek-ai/dsh-agent': + specifier: workspace:^ + version: link:../../core/agent + '@deepseek-ai/dsh-llm': + specifier: workspace:^ + version: link:../../llm/llm + '@deepseek-ai/dsh-system-prompt': + specifier: workspace:^ + version: link:../../core/system-prompt + '@deepseek-ai/dsh-tools': + specifier: workspace:^ + version: link:../../core/tools + '@deepseek-ai/dsh-user-interaction': + specifier: workspace:^ + version: link:../../core/user-interaction + cordis: + specifier: ^4.0.0-rc.6 + version: 4.0.0-rc.6(@cordisjs/plugin-include@1.0.4)(@cordisjs/plugin-loader@1.0.0-rc.4) + packages/util/brand: devDependencies: cordis: diff --git a/scripts/type-equiv.manifest.json b/scripts/type-equiv.manifest.json index 7872ec8620..c4007ed3eb 100644 --- a/scripts/type-equiv.manifest.json +++ b/scripts/type-equiv.manifest.json @@ -30,6 +30,12 @@ { "doc": "docs/core-data-structures/tools.md", "symbol": "ToolExecution", "source": "packages/core/tools/src/index.ts" }, { "doc": "docs/core-data-structures/tools.md", "symbol": "ToolExecutionResult", "source": "packages/core/tools/src/index.ts" }, + { "doc": "docs/core-data-structures/user-interaction.md", "symbol": "AskUserQuestionOption", "source": "packages/core/user-interaction/src/index.ts" }, + { "doc": "docs/core-data-structures/user-interaction.md", "symbol": "AskUserQuestionRequest", "source": "packages/core/user-interaction/src/index.ts" }, + { "doc": "docs/core-data-structures/user-interaction.md", "symbol": "AskUserQuestionAnswer", "source": "packages/core/user-interaction/src/index.ts" }, + { "doc": "docs/core-data-structures/user-interaction.md", "symbol": "UserInteractionProvider", "source": "packages/core/user-interaction/src/index.ts" }, + { "doc": "docs/core-data-structures/user-interaction.md", "symbol": "UserInteractionError", "source": "packages/core/user-interaction/src/index.ts" }, + { "doc": "docs/core-data-structures/bash.md", "symbol": "BashExecRequest", "source": "packages/bash/bash/src/types.ts" }, { "doc": "docs/core-data-structures/bash.md", "symbol": "BashExecSpec", "source": "packages/bash/bash/src/types.ts" }, { "doc": "docs/core-data-structures/bash.md", "symbol": "BashRunResult", "source": "packages/bash/bash/src/types.ts" }, diff --git a/tsconfig.build.json b/tsconfig.build.json index bd6073825d..ff9af414a4 100644 --- a/tsconfig.build.json +++ b/tsconfig.build.json @@ -20,7 +20,7 @@ { "path": "./packages/core/agent" }, { "path": "./packages/core/user-interaction" }, { "path": "./packages/core/tools" }, - { "path": "./packages/core/tool-ask-user" }, + { "path": "./packages/ui/tool-ask-user" }, { "path": "./packages/core/agent-loop" }, { "path": "./packages/core/agent-core" }, { "path": "./packages/bash/bash" }, diff --git a/tsconfig.json b/tsconfig.json index 59c69f9684..9f250e9623 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -31,7 +31,7 @@ { "path": "./packages/core/agent" }, { "path": "./packages/core/user-interaction" }, { "path": "./packages/core/tools" }, - { "path": "./packages/core/tool-ask-user" }, + { "path": "./packages/ui/tool-ask-user" }, { "path": "./packages/core/agent-loop" }, { "path": "./packages/core/agent-core" }, { "path": "./packages/bash/bash" },