diff --git a/AGENTS.md b/AGENTS.md index 9151f02b77d..fa900a800a1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -546,6 +546,7 @@ should always use the MCP tool equivalents. | — | `local_knowledge_search` | kirocrew-core | | — | `file_send` | kirocrew-core | | — | `autonudge_stop` | kirocrew-core | +| — | `ask_question` | kirocrew-core | | — | `artifact_folder_list` | kirocrew-core | | — | `artifact_folder_create` | kirocrew-core | | — | `artifact_folder_rename` | kirocrew-core | diff --git a/docs/system-specs/modules/learn-cron-dashboard.md b/docs/system-specs/modules/learn-cron-dashboard.md index e8eb95b9fe6..7e43540cea0 100644 --- a/docs/system-specs/modules/learn-cron-dashboard.md +++ b/docs/system-specs/modules/learn-cron-dashboard.md @@ -444,6 +444,26 @@ A pending tool approval has **two** pieces of state that must stay in lockstep: **Runner backstop totality**: `outcome` is pre-seeded to `"rejected"` before the approval `await`, because the `finally` runs on *every* exit including `CancelledError` (slot deletion and cleanup endpoints cancel `slot.task`). Assigning it only inside `try`/`except` would raise `UnboundLocalError` from the `finally`, replacing the cancellation with a spurious exception and skipping both the message marking and the Slack prompt cleanup. +### Agent Questions (`ask_question`) + +A second blocking round-trip, structurally parallel to tool approval but resolving to the user's **answer map** rather than an allow/deny boolean, and addressed to **one slot** rather than the whole gateway. User-facing documentation lives in `src/kiro_crew/docs/agent-questions.md`; this section is the contract. + +`DashboardState` owns `_pending_questions` (ask_id → payload) and `_question_futures` (ask_id → `asyncio.Future`), deliberately separate from the approval registries: + +- `request_question(ask_id, slot_key, questions, timeout)` — redacts every question/header/option string, registers the future, broadcasts WS `question_card` `{ask_id, slot, questions, ts}`, and awaits. Returns the answer map, or `None` on timeout/cancel/dismissal. Its `finally` always pops both registries and broadcasts `question_card_resolved` `{ask_id}`, so a card can never outlive its future and become an unanswerable control (the same failure class as a pending approval message whose future is gone). +- `resolve_question(ask_id, answers)` — resolves the future; `answers=None` means dismissed and is indistinguishable from a timeout to the caller. +- `cancel_questions_for_slot(slot_key)` — releases every question pending on a slot; returns the count. + +**Invariant: every stop/interrupt/delete path MUST release both blocking waits.** A pending question holds an MCP worker on a blocked HTTP request, so leaving it unresolved keeps the cooperative cancel from completing exactly as an unresolved approval future does. `chat_handlers._unblock_pending_waits(state, slot)` is the single chokepoint that calls `_reject_pending_approvals` **and** `cancel_questions_for_slot`; the force-stop, soft-stop, interrupt, and slot-delete paths all call it rather than the approval half alone, so a future third blocking wait is added in one place instead of four. + +**Session resets are the second such family.** The agent, model, bulk-model, reasoning-effort, and workspace switch handlers all reset the slot's session so the next message starts under the new setting, which tears down the agent process. A pending question lives in dashboard state rather than in the session, so it survives that teardown: the card stays on screen inviting an answer, and the blocked request holds an MCP worker until its own timeout with no agent left to receive the answer it eventually returns. `chat_handlers._reset_slot_session(state, slot, session_key)` is the corresponding chokepoint — it calls `_unblock_pending_waits` and then `state.sessions.reset(...)` — and is the ONLY place `sessions.reset` is called from a switch handler, so a sixth switch added later cannot quietly skip the release. A source-level test enforces that exactly one raw `sessions.reset` call remains (the one inside the helper). + +Bounds (`validation.py`, single source of truth): ≤ `_ASK_MAX_QUESTIONS` (4) questions, ≤ `_ASK_MAX_OPTIONS` (6) options each, question 500 / header 50 / label 200 / description 500 chars. The **answer** side is bounded too — ≤ `_ASK_MAX_QUESTIONS` entries, each value truncated to `_ASK_MAX_ANSWER_LEN` (2000) — because answers are echoed verbatim into the agent transcript and an oversized custom answer would consume model context. + +Timeout is caller-chosen within `_QUESTION_TIMEOUT_DEFAULT` (300s) … `_QUESTION_TIMEOUT_MAX` (540s). The ceiling is set by the ACP tool-stall watchdog (`acp/client.py::_TOOL_STALL_TIMEOUT`, 600s), which is armed once a tool call is dispatched: a blocked ask emits no progress frames, so a window at or beyond 600s lets the watchdog kill the turn and an answer then has no turn to return to. 540s keeps a 60s margin. It deliberately does NOT match the `wait` tool's 1800s — that is a different mechanism, and copying its number was a defect caught in review. The `ask_question` MCP tool sets its HTTP socket timeout to `timeout_secs + 30` so the socket cannot trip before the server-side wait and strand a question the user is still answering. The tool resolves its session **strictly** (env var or HMAC-verified pid sidecar, never the `/proc` ancestor walk) so a subagent cannot post a card into its parent's conversation, and refuses non-dashboard sessions by pointing at the `[OPTIONS:]` tag. + +Frontend: `chatSlice.pendingQuestions` is keyed **by slot** so concurrent `ask_question` calls from two slots cannot evict each other; `resolveQuestionCard` deletes by `ask_id` match so a stale resolution cannot clear a different slot's live card. Cards carrying no `ask_id` come from the legacy `AskUserQuestion` tool-call sniff in `chat_runner.py` and keep their send-as-a-normal-message behavior. + ### Key Endpoints **Status/System**: `/api/status`, `/api/system` (live metrics, 1s refresh, static fields cached), `/api/stream` (SSE), `/api/ws` (WebSocket — single multiplexed connection replacing SSE + polling for React SPA) @@ -471,6 +491,7 @@ A pending tool approval has **two** pieces of state that must stay in lockstep: **Logs**: GET `/api/logs` (SSE), GET/POST `/api/logs/level` (runtime log level control) **Task Runner**: GET `/api/taskrunner` (status with runs[], includes `agent`), POST `/api/taskrunner` (start, optional `agent` field), POST `/api/taskrunner/cancel` (per-task or all), DELETE `/api/taskrunner/{task_id}` (remove finished run), POST `/api/taskrunner/refine` (dynamic multi-turn with tool access), GET `/api/taskrunner/refine` (status with `waiting` field), POST `/api/taskrunner/refine/cancel`, POST `/api/taskrunner/refine/answer` (answer clarifying question) **Approvals**: GET `/api/approvals` (pending list), POST `/api/approvals/{id}/approve`, POST `/api/approvals/{id}/reject` +**Agent questions**: POST `/api/ask-question` (**blocks** until answered; body `{session_key, questions, timeout_secs?}`; returns `{status: "answered", ask_id, answers}` or `{status: "timeout", ask_id}`; 404 on unknown slot so a caller never blocks on a card nobody renders, 400 on invalid payload), POST `/api/ask-question/{ask_id}/answer` (body `{answers}` or `{dismissed: true}`; 404 once the question has been answered or has expired), GET `/api/ask-question/pending` (`[{ask_id, slot, questions, ts}]` — rehydration source, since `question_card` is a one-shot broadcast: without it a reload or WS reconnect leaves the agent blocked with no card on screen until its window elapses; the frontend re-syncs it on WS open exactly as it re-syncs `GET /api/approvals`). All three are **owner-only**, and denying app tokens alone is not sufficient: a dashboard session token is also minted for every allowed Slack user (`!dashboard`) and carries an empty app claim, so it clears the app gate while belonging to a non-owner who could otherwise address a card at any slot (phishing the owner with crafted options, then reading the typed answer out of its own blocked response) or resolve a card the owner is still looking at. `is_owner_dashboard_request` is reused rather than re-derived, so "owner" has one definition — an exact `owner_id` match, or a signed `local-app`/`local-startup` bootstrap subject when no owner is configured, which is the identity the `ask_question` MCP tool itself carries since its token is minted as `generate_token(owner_id or "local-app")`. **The WS side is owner-scoped too**: `DashboardState.broadcast_ws_owners` sends `question_card` and `question_card_resolved` to `_owner_ws_clients` only. Gating the endpoints alone would be cosmetic — a non-owner dashboard session registers as an ordinary WS client, so a plain `broadcast_ws` would deliver the owner's question text and options to it regardless. **Reveal**: POST `/api/reveal` (open file in Finder/file manager) **Terminal**: POST `/api/terminal/sessions` (create PTY session, returns `{session_id, shell}`; capped at `dashboard.terminal.max_sessions`), GET `/api/terminal/sessions` (list), DELETE `/api/terminal/sessions/{session_id}` (kill), `/api/ws/terminal/{sessionId}` (per-session WebSocket: binary frames carry raw PTY I/O redacted per read chunk via `_redact_terminal` (credentials + exfiltration URLs); JSON control frames are client→server `resize {cols, rows}` and server→client `title {text}` (foreground command name while one runs, else cwd basename), `cwd {path}` (the shell's full live working directory), `error {message}`, `pong`). A singleton poller (`poll_terminal_titles`, ~1s) probes each connected session off-loop for title and cwd, pushing each frame only on change; cwd resolution reads `/proc//cwd` on Linux and falls back to `lsof -d cwd` on macOS/BSD executed ONLY from trusted absolute paths (`/usr/sbin/lsof`, `/usr/bin/lsof` — never PATH resolution), failing closed when absent. The poller captures and revalidates the session's WebSocket after every executor hop so a mid-probe disconnect can never crash it, and reattaching a WebSocket resets the per-session title/cwd dedup markers so reconnected clients are re-pushed current values. POST `/api/terminal/redact` (body `{text}`, 256 KiB cap → 413) re-runs the same redactors over a COMPLETE selection before the frontend inserts it into the chat composer — per-chunk streaming redaction can miss a secret straddling a 4096-byte read boundary, and this contiguous re-scan closes that gap; the frontend fails closed (no insertion, visible retry state) unless it returns 200. The selection toolbar (frontend `CliPanel.tsx`) appears on text highlight with Send to chat (appends the redacted selection to the composer draft annotated `Terminal output (path):` — live `cwd` frame value preferred, spawn dir fallback — wrapped in a backtick-run-escaping code fence; never overwrites the draft, never auto-sends) and Copy (confirms only after `navigator.clipboard.writeText` resolves, with an explicit failure state). POST `/api/terminal/complete` (body `{session_id, token, folders_only?}`, where `token` is the DEQUOTED literal path the cursor sits in — `"../Kiro"`, `"src/"`, `""`, and `my dir/` for an on-screen `my\ dir/`, since the client decodes backslash escapes before asking) returns `{dir, prefix, entries: [{name, dir, at}], truncated}` and backs the panel's inline path completion: a word longer than `_COMPLETE_TOKEN_MAX` (4096) is **413**, an unparsable body or a non-string `session_id`/`token` 400, an unauthenticated caller 401, the feature flag off 403, and an unknown session id 404; SEL audit event: `terminal.complete`, emitted for EVERY outcome including success, with a fixed reason word as its only resource (`feature_disabled`, `invalid_body`, `token_too_long`, `unknown_session`, `no_cwd`, `sensitive_path`, `listed`) — deliberately coarse, because the route fires per keystroke and the token, prefix, resolved directory and entry names are all user filesystem contents that would turn the audit trail into a transcript of the user's typing and disk layout. The session's working directory is probed per request (`_session_cwd_cached`, a ~0.4 s TTL memo run off-loop) rather than reused from the ~1 s title poller's `last_cwd` — a completion issued immediately after a `cd` would otherwise resolve against the previous directory — while the memo keeps a held-down key from spawning an `lsof` per keystroke on macOS. The token's directory part resolves against that cwd with a leading `~` expanded, so `dir` is the absolute directory actually listed; when the cwd is unknowable (non-POSIX host, or the probe failed) the answer is a well-formed empty one with **`dir: null`** as the "nothing resolved" signal rather than an error the client must special-case. Matching inside that directory is a case-insensitive **substring** search, so a long name is reachable by its distinctive middle (`termi` → `KiroCrew-terminal-completion`), and each entry's `at` is the offset the fragment matched at, which both ranks the results (earliest match first, so a true prefix still wins; then directories, then name) and lets the client highlight the span. A fragment that *starts* with a dot is matched as a **prefix** instead, because there the dot is what unhides hidden entries rather than a distinctive part of a name — substring-matching it would pull in every `foo.bar` and defeat the filter it just switched on. Hidden entries appear only once that leading dot is typed, `folders_only` drops plain files (`is_dir` follows symlinks, as the shell does), entries whose name contains a C0/DEL/C1 control character or a lone surrogate are filtered at the source (a surrogate survives JSON but `TextEncoder` turns it into U+FFFD, so the client would type a path that does not exist) (the client TYPES an accepted completion into the PTY, so a name holding CR/LF would submit an executed command line and an ESC would inject an escape sequence — no escaping makes those safe to type), and a missing or unreadable directory yields an empty list rather than an error — at keystroke rate an unreadable path simply has no completions. Two independent caps bound the work: candidates are pulled lazily into a `heapq.nsmallest` of `_COMPLETE_MAX_ENTRIES` (200) so a directory with 100k entries is never materialized, and `_COMPLETE_MAX_SCAN` (20000) bounds how many entries are EXAMINED, since retention alone would still walk a million-entry directory while holding a pool thread; `truncated` is true for either cap. The target directory is vetted through `hooks.validate_file_path` — the named chokepoint the backend security rules require every file read to pass through, which canonicalizes with `realpath` and then refuses via `is_sensitive_path()` — before any scan (`_vetted_completion_dir`) — canonicalization comes FIRST because a benign-named symlink, or a symlinked parent component, whose target lands inside the governance trust-root would otherwise pass a name-based check and be enumerated through the link; a refusal (like a `realpath` failure) returns the same well-formed empty answer, disclosing nothing about whether the path exists. The enumeration is then PINNED TO A DESCRIPTOR (`_open_vetted_dir` opens the vetted directory `O_RDONLY|O_DIRECTORY|O_NOFOLLOW` and `os.scandir` is handed that fd, never the path string) because vetting a name and scanning that name are two resolutions of the same string: swapping the directory for a symlink to `~/.ssh` in between would otherwise enumerate the target. The open is itself verified — the fd's `(st_dev, st_ino)` must equal what the vetted name resolves to, so a swap in that remaining window fails closed — and the descriptor is closed on every path out, since `os.scandir(fd)` does not take ownership of it. The scan runs on `discovery_executor()`, not the `subprocess_executor()` shared with PTY teardown, so a slow directory cannot starve the `os.close` of session teardown, which can itself wedge in the kernel. The authority model is the live session id: the route lists a directory on behalf of an authenticated caller who already owns a PTY in this gateway, i.e. an interactive shell with the same filesystem access, so it grants nothing that session's own `ls` does not — which is what keeps it from being a general filesystem-enumeration endpoint, and why paths resolve without a root restriction, exactly as the shell would. Client side (`TerminalCompletion.tsx` + `utils/terminalCompletion.ts`, POSIX-only following the PTY backend): the word being completed is read back out of xterm's screen buffer on cursor movement, never mirrored from keystrokes, because a mirror drifts the moment the shell rewrites the line itself (zsh completion, autosuggestions, Ctrl-R, paste, vi mode) whereas the rendered row is by definition what the shell believes the line to be. The command word is located from an OSC `133;B` or OSC 697 `NewCmd`/`EndPrompt` prompt marker when the user's shell integration publishes one, and from a last-prompt-terminator heuristic otherwise. There are two triggers — a path-shaped word (contains `/`, or starts with `~` or `.`) for any command, and a bare word only when the command is a known path command (`ls ⎸` lists the cwd) — with flags and `$`/backtick starts never triggering, and `cd`/`pushd`/`mkdir`/`rmdir` requesting `folders_only`. The menu deliberately produces NOTHING rather than something plausible-but-wrong whenever the row cannot be reasoned about: on the alternate screen buffer (vim/less/htop sweep the cursor over arbitrary text, a redrawn `> cd ./src` satisfies the prompt heuristic, and an open menu would then steal Escape/Enter/Tab/arrows from the TUI), on a wrapped row, or one with a non-single-width cell or a multi-code-unit (combining) cell at or before the cursor (`translateToString` returns one physical row while `cursorX` counts cells, so the two coordinate systems only agree otherwise), mid-word (the chosen name would be inserted in front of the surviving suffix), and for a word V1 cannot parse — one containing a quote character, or ending in an unfinished escape. Backslash-escaped words ARE handled: the tokenizer does not break at an escaped space and the word is decoded before the request, so a name accepted with escapes can still be walked into. Accepted text is backslash-escaped through a single choke point (`buildInsertion`) against an allowlist of shell-safe characters, so an unforeseen metacharacter is escaped by default, and a name that would be read as something other than a local path additionally gets a `./` prefix when the word has no directory part — a leading `-`/`+`, or a `:` anywhere — those leads make the argument an OPTION rather than a path and no escaping changes that (`\-c` is still `-c`, and `vim +:!id` executes `id`); backslashes rather than quotes keep the result ONE shell word with nothing left open, so the echo still tokenizes as one word and accepting a directory can re-trigger on its contents. Enter and Tab re-read the live word before acting and abort (returning the key to the shell untouched) if it no longer matches the word the suggestions were computed for, and a failed request closes the menu rather than leaving the previous word's entries acceptable. Keys the menu claims (↑/↓, Tab, Enter, Escape) are cancelled at the DOM level, not merely by returning `false` from xterm's custom key handler: that only makes xterm return early *without* calling `preventDefault`, so Tab would still move focus out of the terminal and Enter would still fire `keypress` and reach the PTY as a CR, executing the line instead of completing it. All terminal endpoints require an authenticated caller and the feature flag (`dashboard.terminal.enabled`), and emit SEL API-access events. **Windows fail-fast**: PTY/fork are POSIX-only, so on Windows `POST /api/terminal/sessions` (`terminal.py:api_terminal_create`) returns **HTTP 501** with body `{"error": "The web terminal is not supported on Windows.", "reason": "unsupported_platform"}` before allocating a `session_id`. The gate is guarded on `platform_compat.IS_WINDOWS` and runs after auth + feature-enabled but before the `max_sessions` check, so the platform verdict is unambiguous (501 wins over 429). The wording is shared with the WebSocket open path (`api_terminal_ws`) via the module constants `_UNSUPPORTED_PLATFORM_MSG` / `_UNSUPPORTED_PLATFORM_REASON` so both surfaces render identically. SEL audit event: `terminal.session.create` with `outcome=denied`, `resources=unsupported_platform`. **File Picker**: POST `/api/upload` (macOS only — opens native osascript file picker, returns absolute paths) diff --git a/src/kiro_crew/config/prompt.md b/src/kiro_crew/config/prompt.md index 9d5fa0d4103..399263a95c8 100644 --- a/src/kiro_crew/config/prompt.md +++ b/src/kiro_crew/config/prompt.md @@ -20,6 +20,7 @@ These MCP tools are provided by KiroCrew (use directly, never via bash): - `cron_add` — schedule recurring or one-shot jobs. Use when user says "every", "daily", "remind me", "check regularly". When `script` is set, the cron executes a Python function directly (no LLM, zero tokens). Use for deterministic polling where reasoning adds no value. Scripts must live under `~/.kirocrew/crons/` (write the file first, then register with `script='~/.kirocrew/crons/file.py:function'`). Pass arguments via the `message` field — scripts read them as `ctx.message`. Use `ctx.notify()` to deliver messages, `raise Skip()` to retry, `raise Done(msg)` to deliver and remove the job, `raise Report(msg)` to deliver and keep the job running. Use `ctx.call_tool(server, tool, args)` to invoke MCP tools. When `command` is set, the cron executes a shell command directly (no LLM, zero tokens). Mutually exclusive with `script`. To dry-run a script cron during development, use `kirocrew cron preview -m ` (real MCP tools, Done/Report/Skip printed not delivered; runs in-process for debuggability, not sandboxed). - `cron_list` — show all scheduled jobs - `cron_remove` / `cron_remove_all` / `cron_pause` / `cron_resume` — manage jobs +- `ask_question` — ask the dashboard user 1–4 multiple-choice questions and pause the current turn until they answer. Use it only for a blocking decision needed before you can continue; when ending the turn, prefer a final `[OPTIONS: choice1 | choice2]` line instead. Dashboard sessions only. - `spawn_run` — spawn subagent(s) and wait for results. Pass `tasks` array for parallel work. This is the ONLY way to spawn subagents — do NOT use any other mechanism. - `spawn_list` — list running subagents diff --git a/src/kiro_crew/context.py b/src/kiro_crew/context.py index e5edd6a1676..2f4cfa4eec0 100644 --- a/src/kiro_crew/context.py +++ b/src/kiro_crew/context.py @@ -1825,19 +1825,24 @@ def build_message( "in the user's voice as an instruction to you — \"Merge it now\", not " "\"I'll merge it\".)" ) - # Dashboard-only, situational nudge for the suggest_followup tool. - # Gated to dashboard sessions because the tool rejects Slack/cron/ - # subagent contexts (they have no card surface). Deliberately framed - # as OPTIONAL and turn-END, not per-turn: the tool's own description - # carries the full contract, and with MCP Tool Search on the model - # otherwise may never surface it. This is awareness, not a mandate — - # it must not become noise on every reply. Distinct from [OPTIONS:] - # above: those are inline choices for THIS conversation; a follow-up - # card is a concrete NEXT task handed off (optionally to a worktree). + # Dashboard-only, situational nudges for tools that may otherwise + # never surface with MCP Tool Search. Gated here because both tools + # reject Slack/cron/subagent contexts (they have no card surface). + # ask_question is a MID-turn blocking decision; [OPTIONS:] remains + # the cheaper END-turn choice mechanism on every interactive surface. if session_key and ( session_key.startswith("dashboard:") or session_key.startswith("dashboard_") ): + parts.append( + "\n\n(If you need the user's answer to a blocking question BEFORE " + "you can continue the current turn, use the ask_question tool — it " + "pauses and returns the answer as the tool result. This is situational, " + "not per-turn: when you are ENDING your turn, use the final [OPTIONS:] " + "line instead, and do not interrupt the user for a non-blocking choice.)" + ) + # A follow-up card is distinct from both: it offers concrete NEXT + # tasks after work is done, optionally handing one to a worktree. parts.append( "\n\n(When you have FINISHED a substantive piece of work and see " "concrete, worth-doing next steps, you MAY offer them with the " diff --git a/src/kiro_crew/dashboard/chat_handlers.py b/src/kiro_crew/dashboard/chat_handlers.py index 8a4a2220dc7..71612380599 100644 --- a/src/kiro_crew/dashboard/chat_handlers.py +++ b/src/kiro_crew/dashboard/chat_handlers.py @@ -771,6 +771,52 @@ def _reject_pending_approvals(slot: _ChatSlot) -> None: ) +def _unblock_pending_waits(state: DashboardState, slot: _ChatSlot) -> None: + """Unblock EVERY thing a stop/interrupt could leave the runner waiting on. + + Two independent blocking waits exist per slot and both must be released or + the cooperative cancel times out into a hard kill: + + * pending tool approvals (:func:`_reject_pending_approvals`) + * pending agent questions from the ``ask_question`` tool + (:meth:`DashboardState.cancel_questions_for_slot`) — the blocked HTTP + request holds an MCP worker, so resolving the future is what lets that + socket close and the tool call return. + + They are combined here deliberately: a new blocking wait added later must + be released from every stop path, and three separate call sites each + needing their own second line is how one of them gets missed. + """ + _reject_pending_approvals(slot) + cancelled = state.cancel_questions_for_slot(slot.key) + if cancelled: + logger.info( + "Stop: cancelled %d pending question(s) on slot %s", cancelled, slot.key + ) + + +async def _reset_slot_session( + state: DashboardState, slot: _ChatSlot, session_key: str +) -> None: + """Reset a slot's agent session, releasing anything blocked on the old one. + + The switch handlers (agent, model, bulk model, reasoning effort, workspace) + reset the session so the next message starts under the new setting. That + tears down the agent process — but a pending ``ask_question`` lives in + dashboard state, not in the session, so without this it survives the reset: + the card stays on screen inviting an answer, and the blocked HTTP request + holds an MCP worker until its own timeout with no agent left to receive the + answer it eventually returns. + + Routing every reset through one helper rather than adding a second call at + each site is deliberate, and is the same reasoning as + :func:`_unblock_pending_waits`: five call sites each having to remember an + extra line is how one of them gets missed. + """ + _unblock_pending_waits(state, slot) + await state.sessions.reset(session_key) + + def _resolve_stop_event(slot: _ChatSlot, outcome: str) -> None: """Update the in-flight stop_event message in place with final state.""" stop_id = slot._stop_event_id @@ -851,8 +897,9 @@ async def _on_hard_force() -> None: slot._stop_state = "idle" state.push_slots_update() - # Unblock chat runner if it's suspended waiting for tool approval. - _reject_pending_approvals(slot) + # Unblock chat runner if it's suspended waiting for tool approval or on + # a pending ask_question card. + _unblock_pending_waits(state, slot) await state.sessions.stop_turn(_history_key_for(name), force=True, on_hard=_on_hard_force) sel().log_tool_invocation( session_key=_history_key_for(name), @@ -950,8 +997,9 @@ async def _on_hard() -> None: slot._stop_state = "idle" state.push_slots_update() - # Unblock chat runner if it's suspended waiting for tool approval. - _reject_pending_approvals(slot) + # Unblock chat runner if it's suspended waiting for tool approval or on a + # pending ask_question card. + _unblock_pending_waits(state, slot) outcome = await state.sessions.stop_turn( _history_key_for(name), force=False, preserve_queue=True, on_soft=_on_soft, on_hard=_on_hard @@ -1065,8 +1113,9 @@ async def _on_hard() -> None: slot.append("system", stop_msg, stop_msg) state.push_slots_update() - # Unblock chat runner if it's suspended waiting for tool approval. - _reject_pending_approvals(slot) + # Unblock chat runner if it's suspended waiting for tool approval or on a + # pending ask_question card. + _unblock_pending_waits(state, slot) outcome = await state.sessions.stop_turn( _history_key_for(name), @@ -1264,6 +1313,10 @@ async def api_chat_slot_delete(request: web.Request) -> web.Response: # Remove from dict before async operations state._slots.pop(name, None) + # Release any blocking wait before cancelling the task: a pending + # ask_question holds an MCP worker on a blocked HTTP request, and the slot + # is going away, so nobody will ever answer its card. + _unblock_pending_waits(state, slot) if slot.running and slot.task is not None: slot.task.cancel() try: @@ -1453,7 +1506,7 @@ async def api_chat_slot_agent(request: web.Request) -> web.Response: # Reset session so next message uses the new agent logger.info("Slot %s agent switched to %r, resetting session", name, agent_name or "kirocrew") - await state.sessions.reset(_history_key_for(name)) + await _reset_slot_session(state, slot, _history_key_for(name)) # Persist the new agent so the session resumes under the correct agent # after a gateway restart. Written after reset succeeds so we never # advertise an agent we couldn't actually switch to. @@ -1524,7 +1577,7 @@ async def api_chat_slot_model(request: web.Request) -> web.Response: return web.json_response({"ok": True, "model": model_name}) slot.model = model_name logger.info("Slot %s model switched to %r, resetting session", name, model_name or "auto") - await state.sessions.reset(_history_key_for(name)) + await _reset_slot_session(state, slot, _history_key_for(name)) state.push_slots_update() return web.json_response({"ok": True, "model": model_name}) @@ -1589,7 +1642,7 @@ async def api_chat_slots_model(request: web.Request) -> web.Response: # the new model with stale history (the model/history inconsistency), and a # single failure doesn't abort the whole bulk switch. try: - await state.sessions.reset(_history_key_for(name)) + await _reset_slot_session(state, slot, _history_key_for(name)) except Exception: logger.error("Bulk model switch: session reset failed for %s", name, exc_info=True) failed.append(name) @@ -1696,7 +1749,7 @@ async def api_chat_slot_reasoning_effort(request: web.Request) -> web.Response: if not _updated_live: # No live session (or live update failed): reset so the next cold # start picks up the new effort via the provider factory/overlay. - await state.sessions.reset(session_key) + await _reset_slot_session(state, slot, session_key) state.push_slots_update() return web.json_response({"ok": True, "reasoning_effort": effort}) @@ -1724,7 +1777,7 @@ async def api_chat_slot_workspace(request: web.Request) -> web.Response: slot.workspace = ws_name slot.project = default_project_dir(ws_name) logger.info("Slot %s workspace switched to %r, resetting session", name, ws_name) - await state.sessions.reset(_history_key_for(name)) + await _reset_slot_session(state, slot, _history_key_for(name)) state.push_slots_update() return web.json_response({"ok": True, "workspace": ws_name}) diff --git a/src/kiro_crew/dashboard/handlers/ask_question.py b/src/kiro_crew/dashboard/handlers/ask_question.py new file mode 100644 index 00000000000..b2227b57940 --- /dev/null +++ b/src/kiro_crew/dashboard/handlers/ask_question.py @@ -0,0 +1,297 @@ +"""Agent-question HTTP API — render a question card and block for the answer. + +Two endpoints form one blocking round-trip: + +``POST /api/ask-question`` + Called by the ``ask_question`` MCP tool. Validates the question payload, + broadcasts a ``question_card`` to the owning slot's dashboard clients, and + holds the request open until the user answers (or the window elapses). + +``POST /api/ask-question/{ask_id}/answer`` + Called by the dashboard when the user submits (or dismisses) the card. + Resolves the blocked request above. + +This mirrors the tool-approval round-trip in +:meth:`kiro_crew.dashboard.state.DashboardState.request_approval` — the +difference is that the resolution value is the user's answer map rather than an +allow/deny boolean, and the card is addressed to a single slot. +""" + +from __future__ import annotations + +import logging +import uuid + +from aiohttp import web + +from kiro_crew.dashboard.handlers.source_providers import is_owner_dashboard_request +from kiro_crew.dashboard.state import DashboardState +from kiro_crew.sel import sel +from kiro_crew.validation import ( + _ASK_MAX_ANSWER_LEN, + _ASK_MAX_QUESTION_LEN, + _ASK_MAX_QUESTIONS, + ValidationError, + validate_ask_user_question, +) + +logger = logging.getLogger(__name__) + + +def _slot_key_from_session(session_key: str) -> str: + """Map a ``dashboard:chat-N-TS`` session key to its bare slot key. + + The question card is addressed by slot key (what the frontend compares + against ``activeSlot``), while MCP callers hold a session key. + """ + if session_key.startswith("dashboard:"): + return session_key.split(":", 1)[1] + return session_key + + +def _deny_app_token(request: web.Request, operation: str) -> web.Response | None: + """Refuse app tokens on these MCP-only endpoints. Returns 403 or None. + + The middleware's ``_enforce_app_scope`` only checks that the *route* is in + the calling app's manifest ``permissions.api`` allowlist — it does not check + slot ownership. Without this gate, an app that lists ``/api/ask-question`` + in its manifest would pass scope enforcement and could then target ANY + slot, including the owner's: broadcast a crafted question card and read the + user's typed answer straight out of its own blocked HTTP response. That is + cross-slot phishing plus answer exfiltration, so these endpoints are + owner-only rather than ownership-scoped. + + Denying app tokens outright also removes the need to bind each pending + ``ask_id`` to an originating app: with only dashboard-user tokens accepted, + the sole party that can answer is the single dashboard owner — the actor the + card is addressed to. + + Callers are the ``ask_question`` MCP tool (via ``_post_user``, a + dashboard-user token) and the dashboard UI itself, so no legitimate caller + is an app. + """ + app_name = request.get("app", "") + if not app_name: + return None + try: + sel().log_api_access( + caller=app_name, + operation=operation, + outcome="denied", + source="app_isolation", + resources="/api/ask-question", + error="app tokens are not permitted on agent-question endpoints", + ) + except Exception: + logger.warning("SEL audit failed for app-token denial", exc_info=True) + return web.json_response( + {"error": "app token not permitted for this endpoint"}, status=403 + ) + + +def _deny_non_owner(request: web.Request, operation: str) -> web.Response | None: + """Require the dashboard owner on these endpoints. Returns 403 or None. + + Denying app tokens is not sufficient. A dashboard session token is also + minted for every *allowed Slack user* (``!dashboard``), and that token has + an empty app identity, so it clears ``_deny_app_token`` while belonging to + someone who is not the owner. Such a caller could address a card at any + slot — phishing the owner with crafted options and then reading the typed + answer out of its own blocked response — or resolve a card the owner is + still looking at, feeding the agent an answer the owner never gave. + + ``is_owner_dashboard_request`` is reused rather than re-derived so there is + one definition of "owner" in the dashboard: an exact match against the + configured ``owner_id``, or a signed local bootstrap subject when no owner + is configured. That matches the identity the ``ask_question`` MCP tool + itself carries, since its token is minted as ``owner_id or "local-app"``. + """ + if is_owner_dashboard_request(request): + return None + try: + sel().log_api_access( + caller=str(request.get("user") or "anonymous"), + operation=operation, + outcome="denied", + source="dashboard", + resources="/api/ask-question", + error="agent-question endpoints are owner-only", + ) + except Exception: + logger.warning("SEL audit failed for non-owner denial", exc_info=True) + return web.json_response({"error": "forbidden"}, status=403) + + +async def api_ask_question(request: web.Request) -> web.Response: + """POST /api/ask-question — show a question card and block for the answer. + + Body: ``{session_key, questions: [...], timeout_secs?}`` + + Responds ``{"status": "answered", "answers": {...}}`` once the user submits, + or ``{"status": "timeout"}`` when the window elapses / the card is dismissed. + """ + state: DashboardState = request.app["state"] + deny = _deny_app_token(request, "ask_question") + if deny is not None: + return deny + deny = _deny_non_owner(request, "ask_question") + if deny is not None: + return deny + try: + body = await request.json() + except Exception: + return web.json_response({"error": "invalid JSON"}, status=400) + if not isinstance(body, dict): + # Valid JSON is not necessarily an object: `[]`, `null` and bare scalars + # all parse, then blow up on `.get()` as a 500 instead of a 400. + return web.json_response({"error": "body must be a JSON object"}, status=400) + + session_key = str(body.get("session_key") or "") + slot_key = _slot_key_from_session(session_key) + if not slot_key: + return web.json_response({"error": "session_key is required"}, status=400) + # Refuse to address a slot that does not exist: otherwise the caller blocks + # for the full window on a card no client will ever render. + if slot_key not in state._slots: + return web.json_response( + {"error": f"unknown slot {slot_key!r} — no dashboard session to ask"}, + status=404, + ) + + try: + questions = validate_ask_user_question(body) + except ValidationError as exc: + return web.json_response({"error": str(exc)}, status=400) + + try: + timeout_secs = int(body.get("timeout_secs") or state._QUESTION_TIMEOUT_DEFAULT) + except (TypeError, ValueError): + return web.json_response({"error": "timeout_secs must be an integer"}, status=400) + + ask_id = uuid.uuid4().hex + try: + sel().log_tool_invocation( + session_key=session_key, + source="dashboard", + tool_name="ask_question", + outcome="invoked", + request_id=ask_id, + ) + except Exception: + logger.warning("SEL audit failed for ask_question", exc_info=True) + + try: + answers = await state.request_question( + ask_id=ask_id, + slot_key=slot_key, + questions=questions, + timeout=timeout_secs, + ) + except ValueError as exc: + # Raised when redaction collapses two questions into the same key, which + # is only detectable after the redaction pass — so it surfaces here as a + # 400 rather than from validate_ask_user_question. + return web.json_response({"error": str(exc)}, status=400) + if answers is None: + return web.json_response({"status": "timeout", "ask_id": ask_id}) + return web.json_response({"status": "answered", "ask_id": ask_id, "answers": answers}) + + +async def api_ask_question_pending(request: web.Request) -> web.Response: + """GET /api/ask-question/pending — list question cards still awaiting an answer. + + The ``question_card`` websocket event is a one-shot broadcast, so a client + that reloads or reconnects after it fired has no card on screen while the + agent is still blocked — the question is invisible until the wait elapses. + This is the rehydration source, mirroring ``GET /api/approvals`` for tool + approvals (the frontend re-syncs both on websocket open). + + Owner-only on the same grounds as the other two endpoints: the payload is + the question text addressed to the owner. + """ + state: DashboardState = request.app["state"] + deny = _deny_app_token(request, "ask_question_pending") + if deny is not None: + return deny + deny = _deny_non_owner(request, "ask_question_pending") + if deny is not None: + return deny + return web.json_response( + [ + { + "ask_id": ask_id, + "slot": p.get("slot", ""), + "questions": p.get("questions", []), + "ts": p.get("ts", 0), + } + for ask_id, p in state._pending_questions.items() + ] + ) + + +async def api_ask_question_answer(request: web.Request) -> web.Response: + """POST /api/ask-question/{ask_id}/answer — resolve a pending question. + + Body: ``{answers: {question: answer}}``, or ``{"dismissed": true}`` to + unblock the caller with no answer. + """ + state: DashboardState = request.app["state"] + deny = _deny_app_token(request, "ask_question_answer") + if deny is not None: + return deny + deny = _deny_non_owner(request, "ask_question_answer") + if deny is not None: + return deny + ask_id = request.match_info["ask_id"] + try: + body = await request.json() + except Exception: + return web.json_response({"error": "invalid JSON"}, status=400) + if not isinstance(body, dict): + return web.json_response({"error": "body must be a JSON object"}, status=400) + + if body.get("dismissed"): + answers: dict[str, str] | None = None + else: + raw = body.get("answers") + if not isinstance(raw, dict) or not raw: + return web.json_response( + {"error": "answers must be a non-empty object"}, status=400 + ) + if len(raw) > _ASK_MAX_QUESTIONS: + return web.json_response( + {"error": f"at most {_ASK_MAX_QUESTIONS} answers"}, status=400 + ) + # Keys and values are echoed back to the agent as tool output, so they + # are coerced to str (a nested object cannot smuggle structure into the + # transcript) and bounded. + # + # REJECT rather than truncate. Silently slicing resolves the wait and + # clears the card, so the agent proceeds on input the user cannot see was + # cut and has no way to resend — the answer is simply wrong. A 400 leaves + # the card up (the frontend only clears on success or a 404), so the user + # can shorten and retry. + answers = {str(k): str(v) for k, v in raw.items()} + for k, v in answers.items(): + if len(k) > _ASK_MAX_QUESTION_LEN: + return web.json_response( + {"error": f"question key exceeds {_ASK_MAX_QUESTION_LEN} characters"}, + status=400, + ) + if len(v) > _ASK_MAX_ANSWER_LEN: + return web.json_response( + { + "error": ( + f"answer exceeds {_ASK_MAX_ANSWER_LEN} characters " + "— shorten it and submit again" + ) + }, + status=400, + ) + + if not state.resolve_question(ask_id, answers): + return web.json_response( + {"error": "no pending question with that id (already answered or expired)"}, + status=404, + ) + return web.json_response({"ok": True}) diff --git a/src/kiro_crew/dashboard/handlers/sessions.py b/src/kiro_crew/dashboard/handlers/sessions.py index 4b837cdd07f..8baf4eaac66 100644 --- a/src/kiro_crew/dashboard/handlers/sessions.py +++ b/src/kiro_crew/dashboard/handlers/sessions.py @@ -597,6 +597,19 @@ async def _remove_slot_for_history_key(state: DashboardState, key: str) -> None: if not slot: # Reverse: history key has no prefix, but slot was stored with one slot = state._slots.pop("dashboard_" + key, None) + if slot: + # A pending ask_question is owned by the slot's running turn, but its + # future lives in DashboardState rather than on slot.task. History + # deletion tears down that task and provider directly, bypassing the + # normal stop/delete handlers; resolve the wait first so the MCP HTTP + # request returns and its finally block retracts the now-stale card. + cancelled = state.cancel_questions_for_slot(slot.key) + if cancelled: + logger.info( + "History delete: cancelled %d pending question(s) on slot %s", + cancelled, + slot.key, + ) if slot and slot.running and slot.task is not None: slot.task.cancel() try: diff --git a/src/kiro_crew/dashboard/server.py b/src/kiro_crew/dashboard/server.py index a67d3467f25..bd42f339fcb 100644 --- a/src/kiro_crew/dashboard/server.py +++ b/src/kiro_crew/dashboard/server.py @@ -684,6 +684,21 @@ def _register_mcp_routes(app: web.Application) -> None: app.router.add_patch("/api/autonudge/{loop_id}", api_autonudge_update) app.router.add_delete("/api/autonudge/{loop_id}", api_autonudge_delete) + # Agent questions — blocking question-card round-trip for the ask_question + # MCP tool. The POST holds open until the user answers, so it must not be + # wrapped in any short-timeout middleware. + from kiro_crew.dashboard.handlers.ask_question import ( + api_ask_question, + api_ask_question_answer, + api_ask_question_pending, + ) + + app.router.add_post("/api/ask-question", api_ask_question) + # Registered before the {ask_id} route so the literal path is not captured + # as an ask_id. + app.router.add_get("/api/ask-question/pending", api_ask_question_pending) + app.router.add_post("/api/ask-question/{ask_id}/answer", api_ask_question_answer) + # Artifacts — persistent, versioned LLM-generated UI app.router.add_get("/api/artifacts", api_artifacts_list) diff --git a/src/kiro_crew/dashboard/state.py b/src/kiro_crew/dashboard/state.py index 0852513402f..e33405728cf 100644 --- a/src/kiro_crew/dashboard/state.py +++ b/src/kiro_crew/dashboard/state.py @@ -1637,6 +1637,12 @@ def __init__( # Pending tool approvals: id → asyncio.Future[bool] self._pending_approvals: dict[str, dict] = {} self._approval_futures: dict[str, asyncio.Future] = {} # type: ignore[type-arg] + # Pending agent questions (ask_question MCP tool): ask_id → payload / + # Future[dict]. Distinct from _approval_futures because the resolution + # value is the user's answer map, not an allow/deny boolean, and the + # question card is addressed to one slot rather than the whole gateway. + self._pending_questions: dict[str, dict] = {} + self._question_futures: dict[str, asyncio.Future] = {} # type: ignore[type-arg] self._flush_task: asyncio.Task | None = None # type: ignore[type-arg] # Update progress tracking (shared across all connected clients) self._update_progress: dict[str, str] | None = None # {step, detail} @@ -1775,6 +1781,18 @@ def status_snapshot( # wait only this short window and then deny-fast, letting the turn proceed/fail # rather than hang. _BACKGROUND_APPROVAL_TIMEOUT_SECS = 180 # 3 minutes — deny-fast for unattended runs + # Agent questions block a live MCP tool call, so the ceiling is bounded by + # how long the agent transport will hold that call open — far shorter than + # the 2h approval window. Callers pick a value inside these bounds. + _QUESTION_TIMEOUT_DEFAULT = 300 # 5 minutes + # Hard ceiling set by the ACP tool-stall watchdog, NOT by the `wait` tool. + # `acp/client.py::_TOOL_STALL_TIMEOUT` is 600s and is armed once a tool call + # is dispatched; a blocked ask_question emits no progress frames, so a window + # at or beyond 600s lets the watchdog declare the turn dead and kill it — + # after which an answer has no turn left to return to. 540s keeps a 60s + # margin below the watchdog. `wait` can afford 1800s because it is a + # different mechanism; copying that number here was the bug. + _QUESTION_TIMEOUT_MAX = 540 # 9 minutes — 60s under the 600s tool-stall watchdog _FLUSH_INTERVAL = 5 # seconds between dirty-slot flushes _log = logging.getLogger(__name__) @@ -1941,6 +1959,140 @@ def resolve_approval(self, approval_id: str, approved: bool) -> bool: return True return False + async def request_question( + self, + ask_id: str, + slot_key: str, + questions: list[dict], + timeout: int | None = None, + ) -> dict[str, str] | None: + """Ask the dashboard user a multiple-choice question and block for the answer. + + Broadcasts a ``question_card`` carrying ``ask_id`` and awaits the + matching :meth:`resolve_question` call. Returns the user's answer map + (``{question: answer}``), or ``None`` when the wait timed out, the + caller was cancelled, or the user dismissed the card. + + ``questions`` MUST already have passed + :func:`kiro_crew.validation.validate_ask_user_question` — this method + redacts but does not re-shape the payload. + """ + loop = asyncio.get_running_loop() + fut: asyncio.Future[dict[str, str] | None] = loop.create_future() + + # The question text is model-authored and rendered in the dashboard, so + # it gets the same redaction pass as the approval payload. + safe_questions: list[dict] = [] + seen_redacted: set[str] = set() + for q in questions: + sq = dict(q) + for field in ("question", "header"): + val, _ = redact_exfiltration_urls(str(sq.get(field) or "")) + val, _ = redact_credentials(val) + sq[field] = val + # The answer map is keyed by the REDACTED question text (that is what + # the frontend renders and echoes back), and redaction is lossy: two + # questions that differ only inside a credential or URL collapse to + # the same key here even though validate_ask_user_question saw them + # as distinct. One answer would then silently overwrite the other and + # the agent would resume on incomplete input. Reject instead: a + # question the user cannot tell apart on screen is not answerable. + norm = " ".join(str(sq.get("question") or "").split()).casefold() + if norm in seen_redacted: + raise ValueError( + "questions collapse to identical text after redaction; " + "rephrase so each question is distinguishable" + ) + seen_redacted.add(norm) + safe_opts: list[dict] = [] + seen_redacted_labels: set[str] = set() + for o in sq.get("options") or []: + so = dict(o) + for field in ("label", "description"): + val, _ = redact_exfiltration_urls(str(so.get(field) or "")) + val, _ = redact_credentials(val) + so[field] = val + # Redaction is lossy. Distinct validated labels can collapse to + # the same rendered/returned value, just as question text can. + # Reject before registering the future or broadcasting a card: + # descriptions cannot disambiguate an answer that contains only + # the selected label. + norm_label = " ".join(str(so.get("label") or "").split()).casefold() + if norm_label in seen_redacted_labels: + raise ValueError( + "option labels collapse to identical text after redaction; " + "rephrase so every option is distinguishable" + ) + seen_redacted_labels.add(norm_label) + safe_opts.append(so) + sq["options"] = safe_opts + safe_questions.append(sq) + + payload = { + "ask_id": ask_id, + "slot": slot_key, + "questions": safe_questions, + "ts": time.time(), + } + self._pending_questions[ask_id] = payload + # Registered only now that the payload is known-good: an early raise + # above must not leave an orphan future nothing will ever resolve. + self._question_futures[ask_id] = fut + # Owner-only: the payload carries the model-authored question text and + # options addressed to the dashboard owner. A plain broadcast_ws would + # also deliver it to non-owner sessions, which would defeat the + # owner-gating on the HTTP endpoints. + self.broadcast_ws_owners("question_card", payload) + + window = timeout if timeout is not None else self._QUESTION_TIMEOUT_DEFAULT + window = max(1, min(int(window), self._QUESTION_TIMEOUT_MAX)) + try: + return await asyncio.wait_for(fut, timeout=window) + except asyncio.TimeoutError: + return None + except asyncio.CancelledError: + return None + finally: + self._pending_questions.pop(ask_id, None) + self._question_futures.pop(ask_id, None) + # Tell every owner client to drop the card — otherwise a timed-out + # or cancelled question stays clickable and submitting it 404s. + # Owner-scoped to match the card broadcast: a non-owner never + # received the card, so it has nothing to drop. + try: + self.broadcast_ws_owners("question_card_resolved", {"ask_id": ask_id}) + except Exception: + self._log.warning("WS broadcast failed for question resolution", exc_info=True) + + def resolve_question(self, ask_id: str, answers: dict[str, str] | None) -> bool: + """Resolve a pending agent question. Returns False when no such question. + + ``answers`` of ``None`` means the user dismissed the card without + answering; the blocked caller then sees the same result as a timeout. + """ + fut = self._question_futures.get(ask_id) + if fut is None or fut.done(): + return False + fut.set_result(answers) + return True + + def cancel_questions_for_slot(self, slot_key: str) -> int: + """Unblock every question pending on ``slot_key``. Returns how many. + + Called when a slot's turn is stopped or reset so a blocked ask_question + cannot outlive the turn that issued it and strand its MCP call. + """ + stale = [ + aid + for aid, p in self._pending_questions.items() + if p.get("slot") == slot_key + ] + cancelled = 0 + for aid in stale: + if self.resolve_question(aid, None): + cancelled += 1 + return cancelled + def start_flush_loop(self) -> None: """Start background loop that flushes dirty slots to disk every 5s.""" if self._flush_task is None: diff --git a/src/kiro_crew/docs/agent-questions.md b/src/kiro_crew/docs/agent-questions.md new file mode 100644 index 00000000000..b05115497a5 --- /dev/null +++ b/src/kiro_crew/docs/agent-questions.md @@ -0,0 +1,239 @@ +# Agent questions (`ask_question`) + +Lets an agent pause mid-turn, ask the dashboard user a multiple-choice question, +and receive the answer as a tool result — no extra turn, no text parsing. + +## Why this exists + +KiroCrew already had two ways to offer the user a choice, and neither could +return a value to a running turn: + +| Mechanism | Where it renders | Can block a turn? | +|---|---|---| +| `[OPTIONS: a \| b \| c]` text tag | every surface (dashboard chips, Slack/Discord/Telegram buttons) | No — it is an end-of-turn gate | +| `AskUserQuestion` tool → `question_card` | dashboard only | No — and the trigger tool does not exist in kiro-cli 2.14.0 | +| `ask_question` (this feature) | dashboard only | **Yes** | + +The `QuestionCard` component and the `question_card` websocket event already +existed, keyed off an ACP `tool_call` titled `AskUserQuestion`. That tool is not +present in kiro-cli 2.14.0 (the string appears nowhere in the binary), so the +whole pipeline was unreachable. `ask_question` supplies the missing trigger from +KiroCrew's own MCP server rather than waiting on the agent CLI. + +### Why not ACP elicitation + +kiro-cli 2.14.0 compiles the ACP `elicitation/create` schema (`form` and `url` +modes, `requestedSchema` with `enum` / `oneOf` single-select and array +multi-select) and gates it on `clientCapabilities.elicitation`. That would be the +ideal wire — its schema maps almost exactly onto `QuestionCard`'s data model. + +It is not usable yet: an MCP server issuing `elicitation/create` gets back +`-32601 method not found`, i.e. the MCP→ACP forwarding path is unimplemented. +`ask_question` therefore provides the capability in-process. (Advertising +`clientCapabilities.elicitation` so the native prompt lights up when upstream +ships the bridge is a separate change — PR #512 — with no functional coupling to +this one.) + +## Flow + +``` +agent calls ask_question (MCP) + └─ mcp_core: strict session resolution → POST /api/ask-question [blocks] + └─ handlers/ask_question.api_ask_question + ├─ validate_ask_user_question (payload normalization) + ├─ reject unknown slot with 404 (never block on a card nobody renders) + └─ DashboardState.request_question + ├─ redact question/header/option text + ├─ broadcast_ws("question_card", {ask_id, slot, questions}) + └─ await future (bounded) + … user clicks / types, hits Submit … + frontend POST /api/ask-question/{ask_id}/answer + └─ DashboardState.resolve_question → future resolves + └─ blocked POST returns {status: "answered", answers} + └─ agent receives "The user answered: …" + finally: broadcast_ws("question_card_resolved", {ask_id}) +``` + +This mirrors `DashboardState.request_approval` (the tool-approval round-trip). +The two differences: the resolution value is the user's answer map rather than an +allow/deny boolean, and the card is addressed to a single slot rather than to +the whole gateway. + +## Design decisions + +**Dashboard-only, strict session resolution.** `_resolve_session_key_strict` +(env var or HMAC-verified pid sidecar, never the `/proc` ancestor walk) — a +subagent living under a parent slot's process tree must not be able to post a +question card into the parent's conversation. Non-dashboard sessions get a +refusal pointing at `[OPTIONS:]`, which works on every surface. + +**Bounded wait, default 300s, ceiling 540s.** The ceiling is set by the ACP +tool-stall watchdog, not by the `wait` tool. `acp/client.py::_TOOL_STALL_TIMEOUT` +is 600s and is armed once a tool call is dispatched; a blocked `ask_question` +emits no progress frames, so a window at or beyond 600s lets the watchdog declare +the turn dead and kill it — and an answer arriving after that has no turn left to +return to. 540s keeps a 60s margin. (`wait` can afford 1800s because it is a +different mechanism; copying that number here was a bug, caught in review.) The +HTTP socket timeout is deliberately `timeout_secs + 30` so the socket cannot trip +first and strand a question the user is still answering. + +**Timeout and dismissal are indistinguishable to the agent.** Both yield no +answer. The tool's own output instructs the agent not to re-ask automatically — +an auto-retry loop would spam the user's chat. This is a prompt-level guard, not +an enforced rate limit. + +**`question_card_resolved` carries the `ask_id`.** It fires in the `finally` +block, so a timed-out or cancelled question is always retracted from the UI +instead of staying clickable and 404-ing on submit. The frontend reducer matches +on `ask_id` so a late resolution from an earlier question cannot wipe a newer +card the user is mid-way through. + +**Answer values are coerced to `str`.** Keys and values are echoed into the +agent's transcript, so a nested object cannot smuggle structure into it. + +**`cancel_questions_for_slot`** unblocks every question pending on a slot. It is +called from `chat_handlers._unblock_pending_waits`, the single chokepoint that +the force-stop, soft-stop, interrupt, and slot-delete paths all use (alongside +`_reject_pending_approvals`) — so a blocked ask cannot outlive the turn that +issued it. The two unblocks are combined in one helper on purpose: a pending +question holds an MCP worker on a blocked HTTP request exactly as an unresolved +approval future holds the runner, and three separate call sites each needing +their own second line is how one of them gets missed. + +**Session resets release it too.** The agent, model, bulk-model, reasoning-effort +and workspace switches all reset the slot's session, tearing down the agent +process — but a pending question lives in dashboard state, not in the session, so +it would otherwise survive the reset and wait out its full window with no agent +left to receive the answer. `chat_handlers._reset_slot_session` is the chokepoint +for that family and is the only place a switch handler may call +`sessions.reset`. + +**Legacy path untouched.** The `event.title == "AskUserQuestion"` sniff in +`chat_runner.py` remains. It cannot double-render: MCP tool calls arrive titled +`Running: @/`, so the equality check never matches, and cards from +that path carry no `ask_id` (the frontend keeps its send-as-message behavior for +those). + +## Payload limits + +Enforced by `validate_ask_user_question`, which is the single source of truth; +`ASK_QUESTION_SCHEMA` only shape-checks the agent's arguments. + +| Limit | Value | +|---|---| +| questions per card | 4 | +| options per question | 6 | +| question text | 500 chars | +| header badge | 50 chars | +| option label | 200 chars | +| option description | 500 chars | + +Malformed individual questions/options are skipped defensively; a payload with +no valid questions left is rejected with 400. + +## API + +`POST /api/ask-question` — blocks until answered. +Body: `{session_key, questions, timeout_secs?}`. +Returns `{status: "answered", ask_id, answers}` or `{status: "timeout", ask_id}`. +404 when the slot does not exist, 400 on an invalid payload. + +`POST /api/ask-question/{ask_id}/answer` — resolves a pending question. +Body: `{answers: {question: answer}}`, or `{dismissed: true}` to unblock with no +answer. 404 when no pending question owns that id. + +`GET /api/ask-question/pending` — question cards still awaiting an answer, as +`[{ask_id, slot, questions, ts}]` (question text already redacted). +`question_card` is a one-shot broadcast, so a reload or websocket reconnect after +it fired would otherwise leave the agent blocked with nothing on screen until the +window elapses. The frontend re-syncs this on websocket open, the same way it +re-syncs `GET /api/approvals`. + +**All three endpoints are owner-only.** Refusing app tokens is not enough: a +dashboard session token is also minted for every allowed Slack user +(`!dashboard`), and it carries an empty app claim, so it clears the app gate +while belonging to someone who is not the owner. That caller could address a card +at any slot — phishing the owner with crafted options and reading the typed +answer out of its own blocked response — or resolve a card the owner is still +looking at, feeding the agent an answer the owner never gave. +`is_owner_dashboard_request` is reused rather than re-derived so "owner" has one +definition: an exact `owner_id` match, or a signed `local-app` / `local-startup` +bootstrap subject when no owner is configured. That is also the identity the +`ask_question` tool itself carries, since its token is minted as +`generate_token(owner_id or "local-app")`. + +**The card is broadcast to owner sockets only.** `broadcast_ws_owners` sends both +`question_card` and `question_card_resolved` to `_owner_ws_clients`, never the +all-clients channel. Owner-gating the HTTP endpoints would buy nothing otherwise: +an allowed Slack user's `!dashboard` session registers as an ordinary WS client, +so a plain `broadcast_ws` would hand them the owner's question text, options, and +`ask_id` over the socket even though they cannot call the endpoints. + +## Frontend behaviour + +The card renders through one component, `PendingQuestionCard`, used by both the +single-chat view and every session-grid pane. Panes need it because in split +mode the agent that asked may not be the pane in focus, and a pane that rendered +the card without the `ask_id` branch would start a second turn and strand the +blocked tool call. + +**Submit requires an answer to every question.** The answer map is keyed by +question text, so a partial submit resumes the agent with a map missing entries +it asked for — it cannot distinguish "unanswered" from "never asked". A +multi-question card is one atomic ask. + +**One submission at a time.** Submit and Dismiss both lock while a request is in +flight. Without the guard a double-click fires two calls: the first resolves the +wait, the second 404s, and the 404 handler then sends the answer *again* as a +chat message — a duplicate turn from a single user intent. + +**Dismiss unblocks with no answer**, posting `{dismissed: true}` so the caller +gets a timeout-equivalent result instead of waiting out its window. The control +is offered only on `ask_id` cards; a legacy card blocks nothing, so it has +nothing to dismiss. + +**Cards clear by `ask_id`, never by slot.** A slow response for ask A must not +erase a newer ask B that already replaced it in the same slot, which would leave +B blocked with no card until its own timeout. `resolveQuestionCard` exists for +exactly this; `clearQuestionCard({slot})` is only for legacy cards, which have no +id to match on. + +**Only a 404 falls back to sending a message.** That is the sole proof the wait +is gone (already answered, dismissed, timed out, or its slot was reset). Any +other failure — offline, 5xx, tunnel throttle — is retryable and the agent is +almost certainly still blocked, so the card stays up for a retry; clearing it +would strand the tool call *and* start a second turn it could never join. + +**Reconnect reconciles in both directions.** Both WS events are one-shot, so a +reload can miss either one — a card that should be showing is absent, or one +resolved while disconnected is still on screen. `reconcileQuestions` decides what +to drop and re-add from three inputs: the pending map captured **before** the HTTP +snapshot was requested, the map once the response arrives, and the response +itself. That ordering is the correctness argument, because the response describes +the server as it was when the request was served and races live events both ways: +a card that arrives during the fetch is missing from the response and must not be +dropped (it would leave the agent blocked until timeout), and a card resolved +during the fetch is still in the response and must not be re-added (its submit +could only 404). Legacy cards are never dropped this way: the server has no record +of them, so their absence says nothing. + +**The welcome hero stands down while a card is pending.** It is centred in the +empty transcript, which is the space the card occupies above the composer, so +both mounted overlap — and an agent asking before it has produced any output is a +real first-turn case. + +## When the agent should use it + +Use `ask_question` to pause **mid-turn** on a decision that blocks progress. +Prefer the `[OPTIONS:]` tag when the turn is ending anyway — it is cheaper and +renders on every surface. The tool description states this so the tool does not +displace the tag everywhere. + +## Not covered + +- Non-dashboard surfaces. Slack/Discord have button support via `[OPTIONS:]` but + no blocking question card. +- Rate limiting. Nothing prevents an agent from asking repeatedly within a turn + beyond the prompt-level instruction. +- In an empty session the card visually overlaps the centered welcome + suggestions; with any chat history it sits normally above the composer. diff --git a/src/kiro_crew/docs/index.md b/src/kiro_crew/docs/index.md index 3537dc93594..5e5af23cf5f 100644 --- a/src/kiro_crew/docs/index.md +++ b/src/kiro_crew/docs/index.md @@ -38,6 +38,7 @@ agent backend and Slack credentials. | [Task Runner](task-runner.md) | Autonomous multi-step execution from spec files — hand it a task, walk away | | [Research Lab](research-lab.md) | Autonomous multi-cycle research campaigns — grill-tree scoping, adaptive agent execution, exportable reports | | [Dashboard](dashboard.md) | React web UI with multi-session chat, memory management, and live system metrics | +| [Agent Questions](agent-questions.md) | Let an agent pause mid-turn and ask you a clickable multiple-choice question | | [Slack](slack-integration.md) | DM-based interaction with tool approval, streaming, and channel monitoring | | [Agents](agents.md) | Switch between specialized agents per conversation, thread, or cron job | | [Skills](skills.md) | Drop-in markdown knowledge packs for domain-specific workflows | diff --git a/src/kiro_crew/mcp_core.py b/src/kiro_crew/mcp_core.py index b321ef488ea..9c2b9c85b8a 100644 --- a/src/kiro_crew/mcp_core.py +++ b/src/kiro_crew/mcp_core.py @@ -89,6 +89,7 @@ ARTIFACT_SAVE_SCHEMA, ARTIFACT_UPDATE_SCHEMA, ARTIFACT_VERSIONS_SCHEMA, + ASK_QUESTION_SCHEMA, AUTONUDGE_STOP_SCHEMA, CHANNEL_ID_RE, GET_CHAT_SESSION_SCHEMA, @@ -1278,6 +1279,84 @@ def _list_tools() -> list[dict[str, Any]]: }, }, }, + { + "name": "ask_question", + "description": ( + "Ask the dashboard user one or more multiple-choice questions and " + "BLOCK until they answer. Renders a question card in the chat: the " + "user clicks an option (or types a custom answer in the card's " + "free-text field) and the answer is returned to you as this tool's " + "result — no extra turn, no [OPTIONS:] tag. Use when you need a " + "decision mid-task and cannot usefully continue without it " + "(which of these approaches, which account, confirm before I " + "refactor). Prefer the [OPTIONS: a | b | c] text tag when you are " + "ENDING your turn anyway — this tool is for pausing mid-turn. " + "Dashboard sessions only; returns a timeout notice if the user " + "does not answer within timeout_secs." + ), + "inputSchema": { + "type": "object", + "properties": { + "questions": { + "type": "array", + "description": ( + "1-4 questions to show in one card, each with 1-6 options" + ), + "items": { + "type": "object", + "properties": { + "question": { + "type": "string", + "description": "The question text (max 500 chars)", + }, + "header": { + "type": "string", + "description": ( + "Short category badge shown before the " + "question, e.g. 'SCOPE' (max 50 chars)" + ), + }, + "options": { + "type": "array", + "description": "The clickable choices", + "items": { + "type": "object", + "properties": { + "label": { + "type": "string", + "description": "Option text (max 200)", + }, + "description": { + "type": "string", + "description": ( + "Optional gloss shown next to " + "the label (max 500)" + ), + }, + }, + "required": ["label"], + }, + }, + "multiSelect": { + "type": "boolean", + "description": ( + "Allow selecting several options (default false)" + ), + }, + }, + "required": ["question", "options"], + }, + }, + "timeout_secs": { + "type": "integer", + "description": ( + "How long to wait for the answer (15-540, default 300)" + ), + }, + }, + "required": ["questions"], + }, + }, { "name": "monitor_start", "description": ( @@ -2621,9 +2700,15 @@ def _delete_user(path: str) -> dict: return {"error": str(e)} -def _post_user(path: str, body: dict) -> dict: - """POST JSON to a user-token-gated route (e.g. ``POST /api/autonudge``).""" - return _write_user(path, body, method="POST") +def _post_user(path: str, body: dict, timeout: int = 10) -> dict: + """POST JSON to a user-token-gated route (e.g. ``POST /api/autonudge``). + + ``timeout`` is the socket timeout in seconds. It is a parameter because + ``ask_question`` deliberately blocks server-side until the dashboard user + answers, so it needs a window longer than the 10s default used by the + fire-and-forget callers. + """ + return _write_user(path, body, method="POST", timeout=timeout) def _patch_user(path: str, body: dict) -> dict: @@ -2631,7 +2716,7 @@ def _patch_user(path: str, body: dict) -> dict: return _write_user(path, body, method="PATCH") -def _write_user(path: str, body: dict, *, method: str) -> dict: +def _write_user(path: str, body: dict, *, method: str, timeout: int = 10) -> dict: """Send a JSON body to a user-token-gated route via *method*.""" token, why = _local_user_token_with_reason() if not token: @@ -2644,7 +2729,7 @@ def _write_user(path: str, body: dict, *, method: str) -> dict: ) try: # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected -- _API is the loopback dashboard base resolved from local config and path is code-constructed; no user-controlled URL reaches urlopen (same trust profile as _get_user/_delete_user) - with urllib.request.urlopen(req, timeout=10) as resp: + with urllib.request.urlopen(req, timeout=timeout) as resp: return json.loads(resp.read()) except urllib.error.HTTPError as e: return _http_error_body(e) @@ -4798,6 +4883,65 @@ def _redact(text: str) -> str: + ". No further nudges will fire." ) + if name == "ask_question": + args = validate_tool_args(args, ASK_QUESTION_SCHEMA) + # STRICT resolution (env-var only, no PID walk): the card renders in the + # resolved slot's chat and blocks that caller. A subagent must not be + # able to PID-walk into its parent's identity and post a question card + # into the parent's conversation. + sk = _resolve_session_key_strict() + if not sk.startswith("dashboard:"): + sel().log_tool_invocation( + session_key=sk, source="mcp", tool_name="ask_question", outcome="noop" + ) + return ( + "ask_question only works from a dashboard chat session " + f"(current session_key={sk!r}). From other surfaces, end your " + "turn with an [OPTIONS: a | b | c] tag instead — it renders " + "clickable buttons on every channel that supports them." + ) + timeout_secs = int(args.get("timeout_secs") or 300) + # Give the HTTP read a margin over the server-side wait so the socket + # does not trip first and strand a question the user is still answering. + resp = _post_user( + "/api/ask-question", + { + "session_key": sk, + "questions": args["questions"], + "timeout_secs": timeout_secs, + }, + timeout=timeout_secs + 30, + ) + if resp.get("error"): + sel().log_tool_invocation( + session_key=sk, source="mcp", tool_name="ask_question", outcome="error" + ) + return f"Failed to ask the question: {resp['error']}" + if resp.get("status") != "answered": + sel().log_tool_invocation( + session_key=sk, + source="mcp", + tool_name="ask_question", + outcome="timeout", + ) + return ( + f"No answer within {timeout_secs}s (the user did not respond or " + "dismissed the card). Do NOT re-ask automatically — proceed with " + "your best judgment and say which assumption you made, or ask in " + "plain text and end your turn." + ) + answers = resp.get("answers") or {} + sel().log_tool_invocation( + session_key=sk, + source="mcp", + tool_name="ask_question", + outcome="success", + metadata={"question_count": len(answers)}, + ) + return "The user answered:\n" + "\n".join( + f"- {q}: {a}" for q, a in answers.items() + ) + if name == "monitor_start": args = validate_tool_args(args, MONITOR_START_SCHEMA) # STRICT resolution (env-var only, no PID walk): monitor_start creates diff --git a/src/kiro_crew/validation.py b/src/kiro_crew/validation.py index d160728917d..2f5fea6c8ba 100644 --- a/src/kiro_crew/validation.py +++ b/src/kiro_crew/validation.py @@ -898,6 +898,33 @@ def validate_jsonrpc_request(req: dict[str, Any]) -> tuple[str, Any, dict[str, A ], ) +# Bounds for the question-card payload, shared by ASK_QUESTION_SCHEMA (agent-facing +# arg check) and validate_ask_user_question (authoritative payload normalization). +_ASK_MAX_QUESTIONS = 4 +_ASK_MAX_OPTIONS = 6 +_ASK_MAX_QUESTION_LEN = 500 +_ASK_MAX_HEADER_LEN = 50 +_ASK_MAX_LABEL_LEN = 200 +_ASK_MAX_DESC_LEN = 500 +# The answer side is bounded too: answers are echoed verbatim into the agent's +# transcript, so an oversized custom answer would consume model context. +_ASK_MAX_ANSWER_LEN = 2000 + +# ask_question renders the dashboard question card and blocks the tool call +# until the user answers. `questions` is only shape-checked here (a bounded +# list); the per-question/per-option limits are enforced server-side by +# validate_ask_user_question, which is the single source of truth for the card +# payload. timeout bounds mirror DashboardState._QUESTION_TIMEOUT_MAX. +ASK_QUESTION_SCHEMA = ToolSchema( + tool_name="ask_question", + fields=[ + FieldSpec("questions", list, required=True, max_items=_ASK_MAX_QUESTIONS), + # 540 not 1800: the ACP tool-stall watchdog (600s) kills the turn + # first, and an answer arriving after that has no turn to return to. + FieldSpec("timeout_secs", int, min_val=15, max_val=540), + ], +) + # delete_message reads args["channel"] and args["ts"] by subscript. Without a # schema, a call omitting either key raised KeyError, which is NOT caught by # call_tool_with_logging (only ValidationError is) and propagated out of the @@ -1785,6 +1812,7 @@ def _validate_cron_add_requires_message_or_script(args: dict[str, Any]) -> None: "autonudge_stop": AUTONUDGE_STOP_SCHEMA, "monitor_start": MONITOR_START_SCHEMA, "monitor_update": MONITOR_UPDATE_SCHEMA, + "ask_question": ASK_QUESTION_SCHEMA, "delete_message": DELETE_MESSAGE_SCHEMA, "local_knowledge_search": LOCAL_KNOWLEDGE_SEARCH_SCHEMA, "knowledge_dedup": KNOWLEDGE_DEDUP_SCHEMA, @@ -1954,13 +1982,7 @@ def validate_string_field( # ── AskUserQuestion Schema Validation ── - -_ASK_MAX_QUESTIONS = 4 -_ASK_MAX_OPTIONS = 6 -_ASK_MAX_QUESTION_LEN = 500 -_ASK_MAX_HEADER_LEN = 50 -_ASK_MAX_LABEL_LEN = 200 -_ASK_MAX_DESC_LEN = 500 +# Bounds live near ASK_QUESTION_SCHEMA above (single definition, two consumers). def validate_ask_user_question(raw: object) -> list[dict]: @@ -1977,6 +1999,7 @@ def validate_ask_user_question(raw: object) -> list[dict]: raise ValidationError("questions", "must be a non-empty list") result: list[dict] = [] + seen_questions: set[str] = set() for q in questions[:_ASK_MAX_QUESTIONS]: if not isinstance(q, dict): continue @@ -1988,16 +2011,37 @@ def validate_ask_user_question(raw: object) -> list[dict]: if not isinstance(raw_opts, list): continue opts: list[dict] = [] + seen_labels: set[str] = set() for o in raw_opts[:_ASK_MAX_OPTIONS]: if not isinstance(o, dict): continue label = str(o.get("label") or "")[:_ASK_MAX_LABEL_LEN] if not label: continue + # Option labels are their end-to-end identity: the frontend keys + # selection state by label and sends labels back as the answer. + # Descriptions are display-only, so duplicate normalized labels + # would make distinct-looking rows submit the same value. + norm_label = " ".join(label.split()).casefold() + if norm_label in seen_labels: + raise ValidationError( + "questions", "duplicate option labels are not allowed" + ) + seen_labels.add(norm_label) desc = str(o.get("description") or "")[:_ASK_MAX_DESC_LEN] opts.append({"label": label, "description": desc}) if not opts: continue + # Answers are keyed by question text end-to-end (the frontend builds an + # answer map keyed on the question string, and the tool result echoes + # that map). Two questions with the same text collapse to one entry — + # the user answers both but only the last reaches the blocked agent. + # Reject duplicates (normalized on whitespace/case) so a multi-question + # card can never silently drop an answer. + norm = " ".join(qt.split()).casefold() + if norm in seen_questions: + raise ValidationError("questions", "duplicate question text is not allowed") + seen_questions.add(norm) result.append( { "question": qt, diff --git a/test/test_ask_question_mcp_tool.py b/test/test_ask_question_mcp_tool.py new file mode 100644 index 00000000000..f8a6ec80d6b --- /dev/null +++ b/test/test_ask_question_mcp_tool.py @@ -0,0 +1,194 @@ +"""HTTP-level tests for the ``ask_question`` MCP tool dispatch. + +Exercises the real ``_call_tool_inner`` branch against a mock dashboard that +speaks the same user-token contract as production, so the test covers session +resolution, request body shape, the socket-timeout margin, and how each of the +three outcomes (answered / timeout / error) is rendered for the model. +""" + +from __future__ import annotations + +import json +from http.server import BaseHTTPRequestHandler, HTTPServer +from threading import Thread + +import pytest + +import kiro_crew.mcp_core as mcp_core +from kiro_crew.mcp_core import _call_tool_inner + +QUESTIONS = [ + { + "question": "Which approach?", + "header": "SCOPE", + "options": [{"label": "Option A"}, {"label": "Option B"}], + } +] + + +class _MockAskHandler(BaseHTTPRequestHandler): + """Mock dashboard for POST /api/ask-question.""" + + secret = "local-secret-xyz" + issued_token = "user-token-abc" + # Set per-test: the JSON body the ask endpoint responds with. + response: dict = {"status": "answered", "answers": {"Which approach?": "Option B"}} + status_code: int = 200 + received: list[dict] = [] + + def _json(self, status: int, body: dict) -> None: + self.send_response(status) + self.send_header("Content-Type", "application/json") + self.end_headers() + self.wfile.write(json.dumps(body).encode()) + + def _has_valid_token(self) -> bool: + query = self.path.split("?", 1)[1] if "?" in self.path else "" + return f"token={self.issued_token}" in query + + def do_GET(self): # noqa: N802 + if self.path.split("?", 1)[0] == "/api/token/local": + if self.headers.get("X-Local-Secret") != self.secret: + self._json(403, {"error": "invalid secret"}) + return + self._json(200, {"token": self.issued_token, "expires_in": 900}) + return + self._json(404, {"error": "not found"}) + + def do_POST(self): # noqa: N802 + if self.path.split("?", 1)[0] == "/api/ask-question": + if not self._has_valid_token(): + self._json(403, {"error": "Token required"}) + return + length = int(self.headers.get("Content-Length", 0) or 0) + type(self).received.append(json.loads(self.rfile.read(length) or b"{}")) + self._json(type(self).status_code, type(self).response) + return + self._json(404, {"error": "not found"}) + + def log_message(self, *args): # noqa: A002 + pass + + +@pytest.fixture() +def mock_dashboard(tmp_path, monkeypatch): + server = HTTPServer(("127.0.0.1", 0), _MockAskHandler) + port = server.server_address[1] + Thread(target=server.serve_forever, daemon=True).start() + + (tmp_path / ".local_secret").write_text(_MockAskHandler.secret) + monkeypatch.setattr(mcp_core, "config_dir", lambda: tmp_path) + monkeypatch.setattr(mcp_core, "_API", f"http://127.0.0.1:{port}") + monkeypatch.setenv("KIROCREW_SESSION_KEY", "dashboard:chat-3-1700000000") + monkeypatch.setattr(mcp_core, "_USER_TOKEN_CACHE", ("", 0.0)) + _MockAskHandler.received = [] + _MockAskHandler.status_code = 200 + _MockAskHandler.response = { + "status": "answered", + "answers": {"Which approach?": "Option B"}, + } + yield port + server.shutdown() + + +def test_answered_question_is_returned_to_the_model(mock_dashboard): + result = _call_tool_inner("ask_question", {"questions": QUESTIONS}) + assert "The user answered:" in result + assert "Which approach?: Option B" in result + + +def test_request_body_carries_full_session_key_and_questions(mock_dashboard): + _call_tool_inner("ask_question", {"questions": QUESTIONS, "timeout_secs": 120}) + assert len(_MockAskHandler.received) == 1 + body = _MockAskHandler.received[0] + # Unlike monitor_start (which needs the BARE slot key for autonudge), the + # ask endpoint takes the namespaced session key and derives the slot itself. + assert body["session_key"] == "dashboard:chat-3-1700000000" + assert body["questions"] == QUESTIONS + assert body["timeout_secs"] == 120 + + +def test_timeout_response_tells_the_model_not_to_re_ask(mock_dashboard): + _MockAskHandler.response = {"status": "timeout"} + result = _call_tool_inner("ask_question", {"questions": QUESTIONS, "timeout_secs": 30}) + assert "No answer within 30s" in result + # The instruction matters: an auto-retry loop would spam the user's chat. + assert "Do NOT re-ask automatically" in result + + +def test_multi_question_answers_are_all_rendered(mock_dashboard): + _MockAskHandler.response = { + "status": "answered", + "answers": {"Which approach?": "Option A", "Which account?": "prod"}, + } + result = _call_tool_inner("ask_question", {"questions": QUESTIONS}) + assert "Which approach?: Option A" in result + assert "Which account?: prod" in result + + +def test_error_response_is_surfaced_not_swallowed(mock_dashboard): + _MockAskHandler.status_code = 404 + _MockAskHandler.response = {"error": "unknown slot 'chat-9'"} + result = _call_tool_inner("ask_question", {"questions": QUESTIONS}) + assert "Failed to ask the question" in result + assert "chat-9" in result + + +def test_non_dashboard_session_is_refused_with_options_hint(mock_dashboard, monkeypatch): + """Slack/Discord/cron have no question card — steer to the [OPTIONS:] tag.""" + monkeypatch.setenv("KIROCREW_SESSION_KEY", "slack:1700000000.123456") + result = _call_tool_inner("ask_question", {"questions": QUESTIONS}) + assert "only works from a dashboard chat session" in result + assert "[OPTIONS:" in result + # Must not have hit the endpoint at all. + assert _MockAskHandler.received == [] + + +def test_subagent_without_session_key_is_refused(mock_dashboard, monkeypatch): + """Strict resolution: no env key means no card in someone else's chat.""" + monkeypatch.delenv("KIROCREW_SESSION_KEY", raising=False) + monkeypatch.delenv("KIROCREW_HOST_PID", raising=False) + result = _call_tool_inner("ask_question", {"questions": QUESTIONS}) + assert "only works from a dashboard chat session" in result + assert _MockAskHandler.received == [] + + +def test_socket_timeout_exceeds_server_window(mock_dashboard, monkeypatch): + """The HTTP read must outlive the server-side wait, or it trips first.""" + seen: dict[str, int] = {} + real_post = mcp_core._post_user + + def spy(path: str, body: dict, timeout: int = 10): + seen["timeout"] = timeout + return real_post(path, body, timeout=timeout) + + monkeypatch.setattr(mcp_core, "_post_user", spy) + _call_tool_inner("ask_question", {"questions": QUESTIONS, "timeout_secs": 300}) + assert seen["timeout"] > 300 + + +def test_questions_is_required(mock_dashboard): + # _call_tool is the agent-facing entrypoint: it runs schema validation and + # converts a ValidationError into a message, rather than letting it escape + # the stdio loop (which would kill the whole MCP server for the session). + result = mcp_core._call_tool("ask_question", {}) + assert "questions" in result.lower() + assert _MockAskHandler.received == [] + + +def test_inner_dispatch_raises_for_missing_questions(mock_dashboard): + """The inner branch itself does not swallow the schema error.""" + from kiro_crew.validation import ValidationError + + with pytest.raises(ValidationError): + _call_tool_inner("ask_question", {}) + + +def test_ask_question_is_advertised_in_the_tool_list(): + names = {t["name"] for t in mcp_core._list_tools()} + assert "ask_question" in names + spec = next(t for t in mcp_core._list_tools() if t["name"] == "ask_question") + assert spec["inputSchema"]["required"] == ["questions"] + # The description must steer away from using this when ending a turn, + # otherwise it displaces the cheaper [OPTIONS:] tag everywhere. + assert "[OPTIONS:" in spec["description"] diff --git a/test/test_ask_question_roundtrip.py b/test/test_ask_question_roundtrip.py new file mode 100644 index 00000000000..8f28d00fa06 --- /dev/null +++ b/test/test_ask_question_roundtrip.py @@ -0,0 +1,1029 @@ +"""Tests for the blocking ask_question round-trip. + +Covers the three states the agent can observe (answered / timed out / +dismissed), the redaction pass on the broadcast payload, the slot-scoped +cancel, and the two HTTP handlers. +""" + +from __future__ import annotations + +import asyncio +import json +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from kiro_crew.dashboard.state import DashboardState + +# ── Helpers ── + + +def _state() -> DashboardState: + """A DashboardState with WS broadcast captured instead of sent. + + The two channels are captured separately on purpose: question payloads must + go to the OWNER channel only, so a test can assert the all-clients channel + stays empty. ``broadcasts`` is the owner channel because that is where every + question event belongs. + """ + st = DashboardState.__new__(DashboardState) + st._pending_questions = {} + st._question_futures = {} + st.broadcasts: list[tuple[str, dict]] = [] # type: ignore[attr-defined] + st.broadcasts_all: list[tuple[str, dict]] = [] # type: ignore[attr-defined] + st.broadcast_ws_owners = lambda kind, payload: st.broadcasts.append( # type: ignore[assignment,attr-defined] + (kind, payload) + ) + st.broadcast_ws = lambda kind, payload: st.broadcasts_all.append( # type: ignore[assignment,attr-defined] + (kind, payload) + ) + st._log = MagicMock() + return st + + +def _questions(text: str = "Which approach?") -> list[dict]: + return [ + { + "question": text, + "header": "SCOPE", + "options": [ + {"label": "Option A", "description": "the safe one"}, + {"label": "Option B", "description": ""}, + ], + "multiSelect": False, + } + ] + + +# ── request_question / resolve_question ── + + +@pytest.mark.asyncio +async def test_answered_question_returns_answer_map() -> None: + st = _state() + + async def answer_soon() -> None: + # Yield until request_question has registered its future. + for _ in range(50): + if "a1" in st._question_futures: + break + await asyncio.sleep(0) + assert st.resolve_question("a1", {"Which approach?": "Option B"}) + + _, result = await asyncio.gather( + answer_soon(), + st.request_question("a1", "chat-1", _questions(), timeout=5), + ) + assert result == {"Which approach?": "Option B"} + # Registries are cleaned up so a late duplicate answer cannot land. + assert st._pending_questions == {} + assert st._question_futures == {} + + +@pytest.mark.asyncio +async def test_broadcasts_question_card_then_resolved() -> None: + st = _state() + + async def answer_soon() -> None: + for _ in range(50): + if "a2" in st._question_futures: + break + await asyncio.sleep(0) + st.resolve_question("a2", {"Which approach?": "Option A"}) + + await asyncio.gather( + answer_soon(), + st.request_question("a2", "chat-7", _questions(), timeout=5), + ) + kinds = [k for k, _ in st.broadcasts] # type: ignore[attr-defined] + assert kinds == ["question_card", "question_card_resolved"] + card = st.broadcasts[0][1] # type: ignore[attr-defined] + assert card["ask_id"] == "a2" + assert card["slot"] == "chat-7" + # The resolved event carries the id so a stale one cannot clear a newer card. + assert st.broadcasts[1][1] == {"ask_id": "a2"} # type: ignore[attr-defined] + + +@pytest.mark.asyncio +async def test_timeout_returns_none_and_clears_card() -> None: + st = _state() + result = await st.request_question("a3", "chat-1", _questions(), timeout=1) + assert result is None + assert st._pending_questions == {} + # The card must be retracted, otherwise it stays clickable and 404s. + assert ("question_card_resolved", {"ask_id": "a3"}) in st.broadcasts # type: ignore[attr-defined] + + +@pytest.mark.asyncio +async def test_dismissal_is_indistinguishable_from_timeout() -> None: + st = _state() + + async def dismiss_soon() -> None: + for _ in range(50): + if "a4" in st._question_futures: + break + await asyncio.sleep(0) + st.resolve_question("a4", None) + + _, result = await asyncio.gather( + dismiss_soon(), + st.request_question("a4", "chat-1", _questions(), timeout=5), + ) + assert result is None + + +@pytest.mark.asyncio +async def test_timeout_is_clamped_to_max() -> None: + st = _state() + seen: dict[str, float] = {} + + async def fake_wait_for(fut, timeout): # type: ignore[no-untyped-def] + seen["timeout"] = timeout + raise asyncio.TimeoutError + + with patch("asyncio.wait_for", fake_wait_for): + await st.request_question("a5", "chat-1", _questions(), timeout=999_999) + assert seen["timeout"] == DashboardState._QUESTION_TIMEOUT_MAX + + +@pytest.mark.asyncio +async def test_question_text_is_redacted_before_broadcast() -> None: + st = _state() + # The stub redactors below key off opaque sentinels rather than a hostname + # substring. Matching on a hostname fragment (`"evil.example.com" in s`) is + # the incomplete-URL-substring-sanitization anti-pattern — CodeQL flags it + # even in a test double, and rightly so: it is the exact shape of a real + # sanitizer bug. What this test actually asserts is that request_question + # routes every text field through the redactors, so the stubs need no URL + # parsing at all. + leaky = _questions("Post to LEAKY_URL_SENTINEL please") + leaky[0]["options"][0]["label"] = "LEAKY_CRED_SENTINEL" + + with patch( + "kiro_crew.dashboard.state.redact_exfiltration_urls", + side_effect=lambda s: ( + ("", 1) if "LEAKY_URL_SENTINEL" in s else (s, 0) + ), + ), patch( + "kiro_crew.dashboard.state.redact_credentials", + side_effect=lambda s: ( + ("", 1) if "LEAKY_CRED_SENTINEL" in s else (s, 0) + ), + ): + await st.request_question("a6", "chat-1", leaky, timeout=1) + + card = st.broadcasts[0][1] # type: ignore[attr-defined] + assert card["questions"][0]["question"] == "" + assert card["questions"][0]["options"][0]["label"] == "" + # The caller's list must not be mutated in place. + assert "LEAKY_URL_SENTINEL" in leaky[0]["question"] + + +def test_resolve_unknown_question_returns_false() -> None: + st = _state() + assert st.resolve_question("nope", {"q": "a"}) is False + + +@pytest.mark.asyncio +async def test_cancel_questions_for_slot_only_targets_that_slot() -> None: + st = _state() + task_a = asyncio.ensure_future( + st.request_question("mine", "chat-1", _questions(), timeout=30) + ) + task_b = asyncio.ensure_future( + st.request_question("other", "chat-2", _questions(), timeout=30) + ) + for _ in range(50): + if len(st._question_futures) == 2: + break + await asyncio.sleep(0) + + assert st.cancel_questions_for_slot("chat-1") == 1 + assert await task_a is None + assert not task_b.done() + + st.resolve_question("other", {"Which approach?": "Option A"}) + assert await task_b == {"Which approach?": "Option A"} + + +# ── HTTP handlers ── + + +def _as_owner(request: MagicMock, user: str = "local-app") -> MagicMock: + """Give a fake request the dashboard-owner identity. + + ``is_owner_dashboard_request`` needs BOTH an explicit empty app claim and a + dashboard-user subject. With no ``owner_id`` configured on the state + fixture, the signed local bootstrap subject ``local-app`` IS the owner -- + the same identity the ``ask_question`` MCP tool carries, since its token is + minted as ``generate_token(owner_id or "local-app")``. + + It reads the claim three ways (``in``, ``[]`` and ``.get``), so all three are + wired to one dict rather than left as default MagicMock attributes -- a bare + ``request["app"]`` returns a MagicMock, which is not ``""`` and silently + fails the gate. + """ + claims = {"app": "", "user": user} + request.__contains__.side_effect = lambda k: k in claims + request.__getitem__.side_effect = lambda k: claims[k] + request.get = lambda k, d="": claims.get(k, d) + return request + + +@pytest.mark.asyncio +async def test_handler_rejects_unknown_slot() -> None: + from kiro_crew.dashboard.handlers.ask_question import api_ask_question + + st = _state() + st._slots = {} + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + + async def _json() -> dict: + return {"session_key": "dashboard:chat-9", "questions": _questions()} + + request.json = _json + resp = await api_ask_question(request) + # 404 rather than blocking for the full window on a card nobody renders. + assert resp.status == 404 + + +@pytest.mark.asyncio +async def test_handler_rejects_invalid_question_payload() -> None: + from kiro_crew.dashboard.handlers.ask_question import api_ask_question + + st = _state() + st._slots = {"chat-1": MagicMock()} + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + + async def _json() -> dict: + return {"session_key": "dashboard:chat-1", "questions": []} + + request.json = _json + resp = await api_ask_question(request) + assert resp.status == 400 + + +@pytest.mark.asyncio +async def test_answer_handler_resolves_pending_question() -> None: + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + + st = _state() + task = asyncio.ensure_future( + st.request_question("h1", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if "h1" in st._question_futures: + break + await asyncio.sleep(0) + + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + request.match_info = {"ask_id": "h1"} + + async def _json() -> dict: + return {"answers": {"Which approach?": "Option B"}} + + request.json = _json + resp = await api_ask_question_answer(request) + assert resp.status == 200 + assert await task == {"Which approach?": "Option B"} + + +@pytest.mark.asyncio +async def test_answer_handler_404s_on_expired_question() -> None: + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + + st = _state() + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + request.match_info = {"ask_id": "gone"} + + async def _json() -> dict: + return {"answers": {"q": "a"}} + + request.json = _json + resp = await api_ask_question_answer(request) + assert resp.status == 404 + + +@pytest.mark.asyncio +async def test_answer_handler_coerces_nested_values_to_str() -> None: + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + + st = _state() + task = asyncio.ensure_future( + st.request_question("h2", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if "h2" in st._question_futures: + break + await asyncio.sleep(0) + + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + request.match_info = {"ask_id": "h2"} + + async def _json() -> dict: + return {"answers": {"q": {"nested": ["structure"]}}} + + request.json = _json + await api_ask_question_answer(request) + answers = await task + assert answers is not None + # Structure is flattened so it cannot smuggle shape into the transcript. + assert isinstance(answers["q"], str) + + +@pytest.mark.asyncio +async def test_oversized_answer_is_rejected_not_truncated() -> None: + """Truncating would resolve the wait on input the user cannot see was cut. + + Answers are echoed into the model context, so they are bounded — but slicing + silently clears the card and lets the agent proceed on a mangled answer with + no way to resend. A 400 leaves the card up (the frontend clears only on + success or 404) so the user can shorten and retry. + """ + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + from kiro_crew.validation import _ASK_MAX_ANSWER_LEN + + st = _state() + task = asyncio.ensure_future( + st.request_question("cap1", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if "cap1" in st._question_futures: + break + await asyncio.sleep(0) + + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + request.match_info = {"ask_id": "cap1"} + + async def _json() -> dict: + return {"answers": {"q": "x" * (_ASK_MAX_ANSWER_LEN + 1)}} + + request.json = _json + resp = await api_ask_question_answer(request) + assert resp.status == 400 + assert str(_ASK_MAX_ANSWER_LEN) in json.loads(resp.text)["error"] + # Critically, the wait is still open: the answer was not accepted, so the + # user can retry rather than the agent resuming on truncated input. + assert not task.done() + + assert st.resolve_question("cap1", None) + assert await task is None + + +@pytest.mark.asyncio +async def test_answer_at_the_limit_is_accepted() -> None: + """The boundary itself must still work, or the cap is off by one.""" + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + from kiro_crew.validation import _ASK_MAX_ANSWER_LEN + + st = _state() + task = asyncio.ensure_future( + st.request_question("cap2", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if "cap2" in st._question_futures: + break + await asyncio.sleep(0) + + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + request.match_info = {"ask_id": "cap2"} + + async def _json() -> dict: + return {"answers": {"q": "x" * _ASK_MAX_ANSWER_LEN}} + + request.json = _json + resp = await api_ask_question_answer(request) + assert resp.status == 200 + answers = await task + assert answers is not None and len(answers["q"]) == _ASK_MAX_ANSWER_LEN + + +@pytest.mark.asyncio +async def test_too_many_answer_entries_rejected() -> None: + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + from kiro_crew.validation import _ASK_MAX_QUESTIONS + + st = _state() + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + request.match_info = {"ask_id": "cap2"} + + async def _json() -> dict: + return {"answers": {f"q{i}": "a" for i in range(_ASK_MAX_QUESTIONS + 1)}} + + request.json = _json + resp = await api_ask_question_answer(request) + assert resp.status == 400 + + +@pytest.mark.asyncio +async def test_dismissed_body_unblocks_with_no_answer() -> None: + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + + st = _state() + task = asyncio.ensure_future( + st.request_question("h3", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if "h3" in st._question_futures: + break + await asyncio.sleep(0) + + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + request.match_info = {"ask_id": "h3"} + + async def _json() -> dict: + return {"dismissed": True} + + request.json = _json + resp = await api_ask_question_answer(request) + assert resp.status == 200 + assert await task is None + + +# ── Route registration ── + + +def test_ask_question_routes_are_registered() -> None: + """Guards the wiring itself: a handler nobody can reach is a silent no-op.""" + from aiohttp import web + + from kiro_crew.dashboard.server import _register_mcp_routes + + app = web.Application() + _register_mcp_routes(app) + routes = {(r.method, r.resource.canonical) for r in app.router.routes() if r.resource} + assert ("POST", "/api/ask-question") in routes + assert ("POST", "/api/ask-question/{ask_id}/answer") in routes + + +# ── Authorization: app tokens are refused (GPT HIGH, round 3) ── + + +@pytest.mark.asyncio +async def test_app_token_cannot_ask_a_question() -> None: + """An app token must not be able to post a card into any slot. + + `_enforce_app_scope` only checks the route is in the app's manifest + allowlist, not slot ownership — so without this gate an app listing + /api/ask-question could target the owner's slot, broadcast a crafted card, + and read the typed answer out of its own blocked response. + """ + from kiro_crew.dashboard.handlers.ask_question import api_ask_question + + st = _state() + st._slots = {"chat-1": MagicMock()} + request = MagicMock() + request.app = {"state": st} + request.__contains__.return_value = True + request.get = lambda k, d="": "evil-app" if k == "app" else d + + async def _json() -> dict: + return {"session_key": "dashboard:chat-1", "questions": _questions()} + + request.json = _json + resp = await api_ask_question(request) + assert resp.status == 403 + # Specifically the app gate, not the owner gate that follows it: otherwise + # this test would keep passing if the app denial were deleted. + assert "app token" in json.loads(resp.text)["error"] + # And it must not have broadcast anything. + assert st.broadcasts == [] # type: ignore[attr-defined] + + +@pytest.mark.asyncio +async def test_app_token_cannot_answer_a_question() -> None: + """The answer endpoint resolves by ask_id alone, so it needs the same gate.""" + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + + st = _state() + task = asyncio.ensure_future( + st.request_question("authz1", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if "authz1" in st._question_futures: + break + await asyncio.sleep(0) + + request = MagicMock() + request.app = {"state": st} + request.match_info = {"ask_id": "authz1"} + request.__contains__.return_value = True + request.get = lambda k, d="": "evil-app" if k == "app" else d + + async def _json() -> dict: + return {"answers": {"Which approach?": "Option A"}} + + request.json = _json + resp = await api_ask_question_answer(request) + assert resp.status == 403 + # The question must still be pending — the app cannot resolve it. + assert not task.done() + st.resolve_question("authz1", None) + await task + + +@pytest.mark.asyncio +async def test_dashboard_user_token_is_still_allowed() -> None: + """The gate must not lock out the legitimate caller (empty app).""" + from kiro_crew.dashboard.handlers.ask_question import api_ask_question + + st = _state() + st._slots = {"chat-1": MagicMock()} + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + + async def _json() -> dict: + return { + "session_key": "dashboard:chat-1", + "questions": _questions(), + "timeout_secs": 15, + } + + request.json = _json + with patch("kiro_crew.dashboard.handlers.ask_question.sel"): + resp = await api_ask_question(request) + # Times out (nobody answers) rather than being refused. + assert resp.status == 200 + + +# ── Body shape: valid JSON that is not an object (GPT MEDIUM, round 3) ── + + +@pytest.mark.asyncio +async def test_non_object_body_is_400_not_500() -> None: + """`[]` / `null` / scalars parse fine then blow up on .get() as a 500.""" + from kiro_crew.dashboard.handlers.ask_question import ( + api_ask_question, + api_ask_question_answer, + ) + + for payload in ([], None, "str", 7): + st = _state() + st._slots = {"chat-1": MagicMock()} + + ask = MagicMock() + ask.app = {"state": st} + _as_owner(ask) + + async def _json(p=payload): + return p + + ask.json = _json + resp = await api_ask_question(ask) + assert resp.status == 400, f"ask: {payload!r}" + + ans = MagicMock() + ans.app = {"state": st} + ans.match_info = {"ask_id": "x"} + _as_owner(ans) + ans.json = _json + resp = await api_ask_question_answer(ans) + assert resp.status == 400, f"answer: {payload!r}" + + +# ── cancel_questions_for_slot is actually wired (Arbiter BLOCK item 1) ── + + +def test_unblock_pending_waits_releases_both_waits() -> None: + """The shared chokepoint must release approvals AND questions. + + `cancel_questions_for_slot` previously had no production caller while + agent-questions.md documented it as a guarantee — a documented safety + property with no call site is worse than no property. + """ + from unittest.mock import MagicMock, patch + + from kiro_crew.dashboard.chat_handlers import _unblock_pending_waits + + state = MagicMock() + state.cancel_questions_for_slot.return_value = 2 + slot = MagicMock() + slot.key = "chat-1" + + with patch( + "kiro_crew.dashboard.chat_handlers._reject_pending_approvals" + ) as rejected: + _unblock_pending_waits(state, slot) + + rejected.assert_called_once_with(slot) + state.cancel_questions_for_slot.assert_called_once_with("chat-1") + + +def test_every_stop_path_uses_the_combined_chokepoint() -> None: + """No stop path may call the approval half alone. + + Asserted on source because the alternative — three separate integration + tests through the stop handlers — would still not catch a FOURTH path added + later, which is precisely how this defect arose. + """ + from pathlib import Path + + src = ( + Path(__file__).resolve().parents[1] + / "src/kiro_crew/dashboard/chat_handlers.py" + ).read_text(encoding="utf-8") + + # The only permitted _reject_pending_approvals reference outside its own + # definition is the one inside _unblock_pending_waits. + body = src.split("def _unblock_pending_waits", 1) + assert len(body) == 2, "the combined chokepoint helper is gone" + before, after = body + # Its definition and docstring reference are fine; count real call sites in + # the rest of the module (after the helper). + stray = [ + ln + for ln in after.splitlines() + if "_reject_pending_approvals(slot)" in ln + ] + assert len(stray) == 1, ( + "every stop/interrupt/delete path must call _unblock_pending_waits, not " + f"_reject_pending_approvals directly; stray call sites: {stray}" + ) + assert after.count("_unblock_pending_waits(state, slot)") >= 4, ( + "expected the force-stop, soft-stop, interrupt and slot-delete paths to " + "use the combined chokepoint" + ) + + +# ── Authorization: owner-only, not merely "not an app" (GPT HIGH, round 4) ── + + +@pytest.mark.asyncio +async def test_non_owner_dashboard_token_cannot_ask() -> None: + """A non-owner dashboard session must not be able to address a card. + + Every allowed Slack user can mint a dashboard token (`!dashboard`), and that + token carries an EMPTY app claim -- so it clears the app-token gate while + belonging to someone who is not the owner. Such a caller could target any + slot, phish the owner with crafted options, and read the typed answer out of + its own blocked response. + """ + from kiro_crew.dashboard.handlers.ask_question import api_ask_question + + st = _state() + st.owner_id = "U_OWNER" + st._slots = {"chat-1": MagicMock()} + request = MagicMock() + request.app = {"state": st} + _as_owner(request, user="U_SOMEONE_ELSE") + + async def _json() -> dict: + return {"session_key": "dashboard:chat-1", "questions": _questions()} + + request.json = _json + resp = await api_ask_question(request) + assert resp.status == 403 + + +@pytest.mark.asyncio +async def test_non_owner_dashboard_token_cannot_answer() -> None: + """Nor resolve a card the owner is still looking at. + + Otherwise a non-owner feeds the blocked agent an answer the owner never gave. + """ + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + + st = _state() + st.owner_id = "U_OWNER" + task = asyncio.ensure_future( + st.request_question("ask-1", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if st._question_futures: + break + await asyncio.sleep(0) + + request = MagicMock() + request.app = {"state": st} + request.match_info = {"ask_id": "ask-1"} + _as_owner(request, user="U_SOMEONE_ELSE") + + async def _json() -> dict: + return {"answers": {"Which approach?": "Option A"}} + + request.json = _json + resp = await api_ask_question_answer(request) + assert resp.status == 403 + # The owner's card is untouched: still pending, still blocking. + assert not task.done() + + assert st.resolve_question("ask-1", None) + assert await task is None + + +@pytest.mark.asyncio +async def test_configured_owner_is_allowed() -> None: + """The gate must not lock out the legitimate owner.""" + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_answer + + st = _state() + st.owner_id = "U_OWNER" + task = asyncio.ensure_future( + st.request_question("ask-2", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if st._question_futures: + break + await asyncio.sleep(0) + + request = MagicMock() + request.app = {"state": st} + request.match_info = {"ask_id": "ask-2"} + _as_owner(request, user="U_OWNER") + + async def _json() -> dict: + return {"answers": {"Which approach?": "Option A"}} + + request.json = _json + resp = await api_ask_question_answer(request) + assert resp.status == 200 + assert await task == {"Which approach?": "Option A"} + + +# ── Reconnect rehydration (GPT MEDIUM, round 4) ── + + +@pytest.mark.asyncio +async def test_pending_endpoint_lists_unanswered_cards() -> None: + """`question_card` is one-shot, so a reload needs a rehydration source. + + Without this the agent stays blocked with nothing on screen until its window + elapses. + """ + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_pending + + st = _state() + task = asyncio.ensure_future( + st.request_question("ask-3", "chat-7", _questions(), timeout=30) + ) + for _ in range(50): + if st._question_futures: + break + await asyncio.sleep(0) + + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + resp = await api_ask_question_pending(request) + assert resp.status == 200 + rows = json.loads(resp.text) + assert [(r["ask_id"], r["slot"]) for r in rows] == [("ask-3", "chat-7")] + assert rows[0]["questions"][0]["question"] == "Which approach?" + + st.resolve_question("ask-3", None) + assert await task is None + + # Once resolved it must disappear, or a reload resurrects a dead card. + resp = await api_ask_question_pending(request) + assert json.loads(resp.text) == [] + + +@pytest.mark.asyncio +async def test_pending_endpoint_is_owner_only() -> None: + from kiro_crew.dashboard.handlers.ask_question import api_ask_question_pending + + st = _state() + st.owner_id = "U_OWNER" + request = MagicMock() + request.app = {"state": st} + _as_owner(request, user="U_SOMEONE_ELSE") + resp = await api_ask_question_pending(request) + assert resp.status == 403 + + +# ── Session resets release the blocking wait (GPT MEDIUM, round 4) ── + + +def test_every_session_reset_goes_through_the_chokepoint() -> None: + """Switch handlers reset the session, which tears down the agent. + + A pending question lives in dashboard state rather than in the session, so + it survives the reset: the card stays on screen and the blocked request holds + an MCP worker with no agent left to receive the answer. Asserted on source + because the alternative -- an integration test per switch handler -- still + would not catch a SIXTH handler added later, which is exactly how the stop + paths drifted before. + """ + src = ( + Path(__file__).resolve().parents[1] + / "src/kiro_crew/dashboard/chat_handlers.py" + ).read_text(encoding="utf-8") + + body = src.split("async def _reset_slot_session", 1) + assert len(body) == 2, "the reset chokepoint is gone" + # Exactly one raw `sessions.reset` may remain: the one inside the helper. + assert body[1].count("await state.sessions.reset(") == 1, ( + "a switch handler resets the session directly, so a pending " + "ask_question would outlive the agent it was waiting on" + ) + assert body[1].count("await _reset_slot_session(") >= 5, ( + "expected the agent, model, bulk-model, reasoning-effort and workspace " + "switches to reset through the chokepoint" + ) + + +@pytest.mark.asyncio +async def test_reset_chokepoint_cancels_pending_questions() -> None: + """The helper itself must actually release the wait, not just exist.""" + from kiro_crew.dashboard.chat_handlers import _reset_slot_session + + st = _state() + st.sessions = MagicMock() + + async def _reset(_key: str) -> None: + return None + + st.sessions.reset = _reset + task = asyncio.ensure_future( + st.request_question("ask-4", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if st._question_futures: + break + await asyncio.sleep(0) + + slot = MagicMock() + slot.key = "chat-1" + with patch("kiro_crew.dashboard.chat_handlers._reject_pending_approvals"): + await _reset_slot_session(st, slot, "dashboard:chat-1") + + # Unblocked with no answer rather than left hanging until timeout. + assert await task is None + + +# ── Owner-scoped broadcast (GPT HIGH, round 5) ── + + +@pytest.mark.asyncio +async def test_question_events_go_only_to_owner_sockets() -> None: + """The card must not fan out to non-owner dashboard sockets. + + Owner-gating the HTTP endpoints buys nothing if the payload still reaches + every socket: an allowed Slack user's `!dashboard` session registers as an + ordinary WS client, so a plain broadcast would hand them the owner's question + text, options, and ask_id. + """ + st = _state() + task = asyncio.ensure_future( + st.request_question("ask-own", "chat-1", _questions(), timeout=30) + ) + for _ in range(50): + if st._question_futures: + break + await asyncio.sleep(0) + + assert [k for k, _ in st.broadcasts] == ["question_card"] + # The all-clients channel must stay untouched for both events. + assert st.broadcasts_all == [] # type: ignore[attr-defined] + + st.resolve_question("ask-own", None) + assert await task is None + assert [k for k, _ in st.broadcasts] == ["question_card", "question_card_resolved"] + assert st.broadcasts_all == [] # type: ignore[attr-defined] + + +def test_broadcast_ws_owners_targets_the_owner_client_set() -> None: + """The helper itself must send to _owner_ws_clients, not _ws_clients.""" + st = DashboardState.__new__(DashboardState) + owner_ws = MagicMock() + st._owner_ws_clients = {owner_ws} + st._ws_clients = {owner_ws, MagicMock()} + sent: list[str] = [] + st._send_ws_owners = lambda msg: sent.append(msg) # type: ignore[assignment] + st._send_ws_all = lambda msg: pytest.fail( # type: ignore[assignment] + "question payloads must never use the all-clients channel" + ) + + st.broadcast_ws_owners("question_card", {"ask_id": "x"}) + assert len(sent) == 1 + assert json.loads(sent[0]) == {"type": "question_card", "data": {"ask_id": "x"}} + + +# ── Round 7: watchdog-bounded window + post-redaction collision ── + + +def test_question_window_stays_under_the_tool_stall_watchdog() -> None: + """The wait must end before ACP declares the turn dead. + + `acp/client.py::_TOOL_STALL_TIMEOUT` is armed once a tool call is dispatched, + and a blocked ask_question emits no progress frames — so a window at or beyond + that value lets the watchdog kill the turn, after which an answer has no turn + left to return to. The ceiling was copied from the `wait` tool (1800s), which + is a different mechanism; that was the bug. + """ + from kiro_crew.acp.client import _TOOL_STALL_TIMEOUT + from kiro_crew.validation import ASK_QUESTION_SCHEMA + + assert DashboardState._QUESTION_TIMEOUT_MAX < _TOOL_STALL_TIMEOUT + assert DashboardState._QUESTION_TIMEOUT_DEFAULT <= DashboardState._QUESTION_TIMEOUT_MAX + # The agent-facing schema must not advertise more than the server will honour, + # or a caller asks for 1800s, gets silently clamped, and the turn dies anyway. + spec = {f.name: f for f in ASK_QUESTION_SCHEMA.fields}["timeout_secs"] + assert spec.max_val is not None + assert spec.max_val <= DashboardState._QUESTION_TIMEOUT_MAX + assert spec.max_val < _TOOL_STALL_TIMEOUT + + +@pytest.mark.asyncio +async def test_questions_identical_after_redaction_are_rejected() -> None: + """Redaction is lossy, and the answer map is keyed by the REDACTED text. + + Two questions differing only inside a credential pass the pre-redaction + duplicate check, then collapse to one key here — so one answer would + overwrite the other and the agent would resume on incomplete input. + """ + st = _state() + colliding = _questions("Use LEAKY_CRED_SENTINEL now?") + [ + { + "question": "Use LEAKY_CRED_SENTINEL2 now?", + "header": "SCOPE", + "options": [{"label": "Yes", "description": ""}], + } + ] + + with patch( + "kiro_crew.dashboard.state.redact_credentials", + side_effect=lambda s: ( + ("Use now?", 1) if "LEAKY_CRED_SENTINEL" in s else (s, 0) + ), + ), patch( + "kiro_crew.dashboard.state.redact_exfiltration_urls", + side_effect=lambda s: (s, 0), + ): + with pytest.raises(ValueError, match="after redaction"): + await st.request_question("ask-collide", "chat-1", colliding, timeout=30) + + # And it must not leave an orphan future behind: nothing would ever resolve it. + assert st._question_futures == {} + assert st._pending_questions == {} + assert st.broadcasts == [] # type: ignore[attr-defined] + + +@pytest.mark.asyncio +async def test_option_labels_identical_after_redaction_are_rejected() -> None: + """Distinct validated labels must remain distinct after lossy redaction.""" + st = _state() + colliding = _questions() + colliding[0]["options"] = [ + {"label": "Deploy LEAKY_CRED_SENTINEL", "description": "staging"}, + {"label": "Deploy LEAKY_CRED_SENTINEL2", "description": "production"}, + ] + + with patch( + "kiro_crew.dashboard.state.redact_credentials", + side_effect=lambda s: ( + ("Deploy ", 1) if "LEAKY_CRED_SENTINEL" in s else (s, 0) + ), + ), patch( + "kiro_crew.dashboard.state.redact_exfiltration_urls", + side_effect=lambda s: (s, 0), + ): + with pytest.raises(ValueError, match="option labels.*after redaction"): + await st.request_question("ask-option-collide", "chat-1", colliding, timeout=30) + + # Reject before registering or broadcasting; no unanswerable card may exist. + assert st._question_futures == {} + assert st._pending_questions == {} + assert st.broadcasts == [] # type: ignore[attr-defined] + + +@pytest.mark.asyncio +async def test_collision_surfaces_as_400_not_500() -> None: + from kiro_crew.dashboard.handlers.ask_question import api_ask_question + + st = _state() + st._slots = {"chat-1": MagicMock()} + request = MagicMock() + request.app = {"state": st} + _as_owner(request) + + async def _json() -> dict: + return {"session_key": "dashboard:chat-1", "questions": _questions()} + + request.json = _json + with patch.object( + DashboardState, "request_question", side_effect=ValueError("collapse after redaction") + ): + resp = await api_ask_question(request) + assert resp.status == 400 + assert "redaction" in json.loads(resp.text)["error"] diff --git a/test/test_ask_user_question_validation.py b/test/test_ask_user_question_validation.py index 15ac42666b3..2e95b6f1f30 100644 --- a/test/test_ask_user_question_validation.py +++ b/test/test_ask_user_question_validation.py @@ -98,8 +98,10 @@ def test_truncates_long_description(self): assert len(result[0]["options"][0]["description"]) == 500 def test_max_4_questions(self): - q = self._valid_input()["questions"][0] - inp = {"questions": [q] * 6} + # Distinct texts: identical texts are rejected (see + # test_rejects_duplicate_question_text); this asserts the count cap. + base = self._valid_input()["questions"][0] + inp = {"questions": [dict(base, question=f"Question {i}?") for i in range(6)]} result = validate_ask_user_question(inp) assert len(result) == 4 @@ -109,6 +111,17 @@ def test_max_6_options(self): result = validate_ask_user_question(inp) assert len(result[0]["options"]) == 6 + def test_rejects_duplicate_option_labels_normalized(self): + # Labels are the selection identity and returned answer; descriptions + # cannot distinguish duplicate labels for the blocked agent. + inp = self._valid_input() + inp["questions"][0]["options"] = [ + {"label": "Deploy", "description": "staging"}, + {"label": " deploy ", "description": "production"}, + ] + with pytest.raises(ValidationError, match="duplicate option labels"): + validate_ask_user_question(inp) + def test_skips_option_without_label(self): inp = self._valid_input() inp["questions"][0]["options"].append({"label": "", "description": "empty"}) @@ -140,3 +153,20 @@ def test_multiple_questions(self): assert len(result) == 2 assert result[1]["header"] == "H2" assert result[1]["multiSelect"] is True + + def test_rejects_duplicate_question_text(self): + # Answers are keyed by question text end-to-end, so two questions with + # the same text collapse to one answer map entry — the user answers + # both but only the last reaches the blocked agent. Reject duplicates. + q1 = {"question": "Pick one?", "options": [{"label": "A"}, {"label": "B"}]} + q2 = {"question": "Pick one?", "options": [{"label": "X"}, {"label": "Y"}]} + with pytest.raises(ValidationError, match="duplicate question text"): + validate_ask_user_question({"questions": [q1, q2]}) + + def test_rejects_duplicate_question_text_normalized(self): + # Normalization is case- and whitespace-insensitive so trivially + # different renderings of the same prompt are still caught. + q1 = {"question": "Pick one?", "options": [{"label": "A"}, {"label": "B"}]} + q2 = {"question": "pick one?", "options": [{"label": "X"}, {"label": "Y"}]} + with pytest.raises(ValidationError, match="duplicate question text"): + validate_ask_user_question({"questions": [q1, q2]}) diff --git a/test/test_context.py b/test/test_context.py index 27ad9a87fb1..3eb67369ec2 100644 --- a/test/test_context.py +++ b/test/test_context.py @@ -136,10 +136,9 @@ def test_cc_interactive_reminder_uses_options_tag(self, tmp_path): assert "[OPTIONS:" in msg, "CC interactive reminder must use the [OPTIONS:] tag" assert "AskUserQuestion" not in msg, "CC must not be steered to AskUserQuestion for options" - def test_followup_nudge_only_in_dashboard_sessions(self, tmp_path): - """The suggest_followup nudge must appear for dashboard sessions (where the - tool works) and be absent everywhere else — Slack/cron/subagent contexts - reject the tool, so prompting them for it is pure noise.""" + def test_dashboard_tool_nudges_only_in_dashboard_sessions(self, tmp_path): + """Card-tool nudges appear only where their dashboard surfaces exist; + Slack/cron/subagent contexts must not be prompted to call either tool.""" builder = ContextBuilder( memory=MemoryStore(workspace=tmp_path / "ws"), skills=SkillsLoader(skills_path=tmp_path / "skills", install_builtins=False), @@ -148,17 +147,20 @@ def test_followup_nudge_only_in_dashboard_sessions(self, tmp_path): dash, _ = builder.build_message( "done", is_new_session=False, interactive=True, session_key="dashboard:chat-1" ) + assert "ask_question" in dash, "dashboard session must get the question nudge" + assert "BEFORE" in dash and "ENDING" in dash assert "suggest_followup" in dash, "dashboard session must get the follow-up nudge" for sk in (None, "cron:job-1", "subagent:abc", "slack:C123"): other, _ = builder.build_message( "done", is_new_session=False, interactive=True, session_key=sk ) + assert "ask_question" not in other, f"{sk!r} must NOT get the question nudge" assert "suggest_followup" not in other, f"{sk!r} must NOT get the follow-up nudge" - def test_followup_nudge_requires_interactive(self, tmp_path): - """A non-interactive turn (e.g. a cron/automation run) gets neither the - OPTIONS reminder nor the follow-up nudge.""" + def test_dashboard_tool_nudges_require_interactive(self, tmp_path): + """A non-interactive turn (e.g. automation) gets neither the OPTIONS + reminder nor either dashboard-card tool nudge.""" builder = ContextBuilder( memory=MemoryStore(workspace=tmp_path / "ws"), skills=SkillsLoader(skills_path=tmp_path / "skills", install_builtins=False), @@ -167,6 +169,7 @@ def test_followup_nudge_requires_interactive(self, tmp_path): msg, _ = builder.build_message( "done", is_new_session=False, interactive=False, session_key="dashboard:chat-1" ) + assert "ask_question" not in msg assert "suggest_followup" not in msg def test_memory_injected(self, tmp_path): diff --git a/test/test_remove_slot_for_history_key.py b/test/test_remove_slot_for_history_key.py index 666c78ab606..6893951694e 100644 --- a/test/test_remove_slot_for_history_key.py +++ b/test/test_remove_slot_for_history_key.py @@ -69,6 +69,27 @@ async def test_running_task_cancelled(self): assert slot.task.cancelled() state.sessions.destroy.assert_awaited_once_with("dashboard:chat-1-100") + @pytest.mark.asyncio + async def test_pending_question_cancelled_before_running_task(self): + """History deletion must not leave a DashboardState-owned question + future alive after its slot task and provider have been destroyed.""" + slot = _make_slot("dashboard_chat-1-100", running=True) + state = _make_state({"dashboard_chat-1-100": slot}) + task_was_done: list[bool] = [] + + def cancel_questions(slot_key: str) -> int: + assert slot_key == slot.key + task_was_done.append(slot.task.done()) + return 1 + + state.cancel_questions_for_slot = MagicMock(side_effect=cancel_questions) + + await _remove_slot_for_history_key(state, "dashboard_chat-1-100") + + state.cancel_questions_for_slot.assert_called_once_with(slot.key) + assert task_was_done == [False] + assert slot.task.cancelled() + @pytest.mark.asyncio async def test_non_running_task_not_cancelled(self): slot = _make_slot("dashboard_chat-1-100", running=False) diff --git a/test/test_stop_handler_idempotent.py b/test/test_stop_handler_idempotent.py index 31798eaa5de..bf3ab0ee217 100644 --- a/test/test_stop_handler_idempotent.py +++ b/test/test_stop_handler_idempotent.py @@ -45,6 +45,14 @@ def __init__(self, slot): def push_slots_update(self): self._push_count += 1 + def cancel_questions_for_slot(self, slot_key): + """No pending ask_question cards in this fixture. + + Present because the stop path releases BOTH blocking waits (approvals + and agent questions) through `_unblock_pending_waits`. + """ + return 0 + class TestStopHandlerIdempotent: """Repeat /stop press returns info without creating another card.""" diff --git a/website/src/api/client.ts b/website/src/api/client.ts index 97c4df20b29..62f5cdafe45 100644 --- a/website/src/api/client.ts +++ b/website/src/api/client.ts @@ -1044,6 +1044,15 @@ export const api = { spawnClear: () => del('/api/spawn').then(j), approvals: (): Promise<{ id: string; source?: string; tool?: string; tool_input?: string; tool_call_id?: string; slot?: string; ts?: number }[]> => fetch('/api/approvals').then(j), resolveApproval: (id: string, action: 'approve' | 'reject') => post('/api/approvals/' + encodeURIComponent(id) + '/' + action, {}).then(j), + /** Question cards still awaiting an answer, for rehydration after a reload or + * websocket reconnect (`question_card` is a one-shot broadcast). */ + pendingQuestions: (): Promise<{ ask_id: string; slot: string; questions: { question: string; header?: string; multiSelect?: boolean; options: { label: string; description?: string }[] }[]; ts?: number }[]> => + fetch('/api/ask-question/pending').then(j), + /** Resolve a pending agent question (ask_question MCP tool). Pass no answers + * to dismiss, which unblocks the agent with a timeout-equivalent result. */ + answerQuestion: (askId: string, answers?: Record) => + post('/api/ask-question/' + encodeURIComponent(askId) + '/answer', + answers ? { answers } : { dismissed: true }).then(j), // Logs logLevel: () => fetch('/api/logs/level').then(j), setLogLevel: (level: string) => post('/api/logs/level', { level }).then(j), diff --git a/website/src/components/ChatPane.tsx b/website/src/components/ChatPane.tsx index 58e0037f60a..b611421e80e 100644 --- a/website/src/components/ChatPane.tsx +++ b/website/src/components/ChatPane.tsx @@ -8,6 +8,7 @@ import ChatMessageList from '../app-sdk/ChatMessageList' import ToolCallLine from '../pages/chat/ToolCallLine' import type { ChatMessage } from '../types' import ChatInput from './ChatInput' +import PendingQuestionCard from './PendingQuestionCard' import QueueStack, { SubagentDeliveryProgress, isSystemDelivery } from './QueueStack' import SubagentProgressBar from '../pages/chat/SubagentProgressBar' import AgentDropdownList from './AgentDropdownList' @@ -267,6 +268,30 @@ export default function ChatPane({ )} + {/* The pending ask_question card renders per pane: in split mode the + agent that asked may not be the pane the user is looking at, and + without this its card never appears anywhere, so it waits out its + full window. */} + { + api + .sendChat(text, slotKey) + .then((res) => { + if (!res || !res.ok) throw new Error(`send failed (${res?.status ?? 'no response'})`) + }) + .catch(() => { + setInput((prev) => (prev.trim() ? `${prev}\n${text}` : text)) + }) + }} + /> + void +} + +/** + * The pending `ask_question` card for one slot, with the answer round-trip. + * + * Shared by the single-chat view and the session-grid panes. It exists as a + * component rather than inline JSX because both surfaces need identical submit + * semantics: a pane that rendered the card but not the `ask_id` branch would + * silently start a second turn and strand the blocked tool call. One + * implementation means a pane cannot drift from the main view. + */ +export default function PendingQuestionCard({ slotKey, onFallbackSend }: PendingQuestionCardProps) { + const dispatch = useAppDispatch() + // Optional-chained: existing tests build partial preloaded chat state without + // the pendingQuestions key. + const pending = useAppSelector((s) => pendingQuestionFor(s.chat.pendingQuestions, slotKey)) + /* Which ask the in-flight request belongs to, NOT a bare boolean. + One submission at a time: without a guard a double-click fires two + answerQuestion calls -- the first resolves the wait, the second 404s, and the + 404 handler then sends the answer AGAIN as a chat message. + Keyed by ask_id rather than a boolean because this component is mounted + UNCONDITIONALLY inside each grid pane (it returns null when no card is + pending, so its state survives): a plain `busy` left true after a successful + submit would render the pane's every later card with Submit and Dismiss + permanently disabled -- an unanswerable card, and a blocked agent. Comparing + against the current ask makes a new card self-clearing, and also stops a + stale in-flight response from locking it. */ + const [busyFor, setBusyFor] = useState(null) + if (!pending) return null + + const cardSlot = pending.slot + const askId = pending.ask_id + const busy = !!askId && busyFor === askId + const asText = (answers: Record) => Object.values(answers).join('\n') + + /* Clearing by ask_id, never by slot: a slow response for ask A must not erase + a newer ask B that already replaced it in the same slot, which would leave + B on screen-less and blocked until its own timeout. */ + const clearThisCard = () => { + if (askId) dispatch(resolveQuestionCard({ ask_id: askId })) + else dispatch(clearQuestionCard({ slot: cardSlot })) + } + + const resolve = (answers: Record | undefined) => { + if (!askId || busy) return + setBusyFor(askId) + api + .answerQuestion(askId, answers) + .then(() => clearThisCard()) + .catch((err) => { + // 404 is the only proof the wait is gone (already answered, dismissed, + // timed out, or its slot was reset) — then the answer is still worth + // keeping as a message. + if (err instanceof ApiError && err.status === 404) { + clearThisCard() + if (answers) { + const text = asText(answers) + if (text.trim()) onFallbackSend(text) + } + return + } + // Anything else (offline, 5xx, tunnel throttle) is retryable and the + // agent is almost certainly STILL blocked. Keep the card so the user can + // retry: clearing it would strand the tool call and start a second turn + // it could never join. + }) + // Released on EVERY path, success included. The success path clears the + // card, but this component stays mounted in a grid pane, so a lock left + // set here would disable the pane's next card too. + .finally(() => { + // A prior ask may settle after this pane has already submitted a newer + // card. Release only this request's lock; clearing unconditionally would + // unlock the newer request and permit a duplicate submission. + setBusyFor((current) => (current === askId ? null : current)) + }) + } + + return ( + resolve(undefined) : undefined} + onSubmit={(answers) => { + if (!askId) { + // Legacy card: nothing is blocked, so the answer is just a message. + const text = asText(answers) + if (text.trim()) onFallbackSend(text) + clearThisCard() + return + } + resolve(answers) + }} + /> + ) +} diff --git a/website/src/components/QuestionCard.tsx b/website/src/components/QuestionCard.tsx index 5b466ee652b..0329c64bdb4 100644 --- a/website/src/components/QuestionCard.tsx +++ b/website/src/components/QuestionCard.tsx @@ -16,9 +16,15 @@ interface Question { interface QuestionCardProps { questions: Question[] onSubmit: (answers: Record) => void + /** Unblock the agent with no answer. Omitted for legacy cards, which have + * nothing blocked on them and so have nothing to dismiss. */ + onDismiss?: () => void + /** True while a submission is in flight: both controls lock so a second + * click cannot produce a duplicate resolution or a duplicate chat turn. */ + busy?: boolean } -function QuestionCard({ questions, onSubmit }: QuestionCardProps) { +function QuestionCard({ questions, onSubmit, onDismiss, busy = false }: QuestionCardProps) { const [selections, setSelections] = useState>>({}) const [customInputs, setCustomInputs] = useState>({}) @@ -51,7 +57,13 @@ function QuestionCard({ questions, onSubmit }: QuestionCardProps) { onSubmit(answers) } - const hasAnyAnswer = questions.some((_, i) => (selections[i]?.size ?? 0) > 0 || customInputs[i]?.trim()) + /* Every question must be answered before Submit unlocks. The answer map is + keyed by question text, so a partial submit resumes the blocked agent with + a map missing entries it asked for -- it cannot tell "unanswered" from + "never asked" and proceeds on incomplete input. A multi-question card is + one atomic ask, so the gate is `every`, not `some`. */ + const isAnswered = (i: number) => (selections[i]?.size ?? 0) > 0 || !!customInputs[i]?.trim() + const allAnswered = questions.every((_, i) => isAnswered(i)) return (
@@ -84,20 +96,31 @@ function QuestionCard({ questions, onSubmit }: QuestionCardProps) { type="text" aria-label="Custom answer" placeholder="Or type a custom answer..." + maxLength={2000} value={customInputs[qIdx] || ''} onChange={e => { setCustomInputs(prev => ({ ...prev, [qIdx]: e.target.value })) setSelections(prev => ({ ...prev, [qIdx]: new Set() })) }} - onKeyDown={e => { if (e.key === 'Enter' && hasAnyAnswer) handleSubmit() }} + onKeyDown={e => { if (e.key === 'Enter' && allAnswered && !busy) handleSubmit() }} className="mt-2 w-full px-3 py-2 rounded-lg border border-border bg-bg text-text text-[13px] placeholder:text-muted focus:border-accent focus:outline-none" />
))} -
+
+ {onDismiss && ( + + )}