refactor(chat): unify ask-user flow on <question-form>, delete AskUserQuestion mechanism#4114
Conversation
…rQuestion mechanism
There were two parallel "ask the user a clarifying question" mechanisms:
A. The AskUserQuestion SDK tool — an inline interactive card in the chat
stream (AskUserQuestionCard / StreamingAskUserQuestionCard), whose answer
was fed back into the still-open stream-json child via
POST /api/runs/:id/tool-result + submitChatRunToolResult.
B. The <question-form> markdown artifact — a QuestionsBanner entry point in
chat, the form rendered in the right-hand Questions tab
(QuestionsPanel / QuestionFormView), answers returned as the next user
message.
Two visual languages for the same job, and mechanism A's card was visually
broken (the text status "Awaiting your answer" was stuffed into the 24px
.op-status icon badge and overflowed onto the title). Mechanism A was pinned to
mid-conversation clarification while B was pinned to turn-1 discovery — an
artificial split.
This deletes mechanism A entirely and keeps only B, and widens <question-form>
from "turn-1 discovery only" to ANY turn so mid-conversation clarification flows
through the same banner → Questions tab → next user message loop.
Web:
- Delete apps/web/src/runtime/ask-user-question.ts (parser trio).
- ToolCard.tsx: drop the AUQ card/streaming-card/dispatch/imports and the
onAnswerToolUse / onSubmitForm props.
- AssistantMessage.tsx: drop the live AUQ render path,
suppressAskUserQuestionFallbackText, the AUQ name from SNAPSHOT_TOOL_NAMES,
and the onSubmitForm bridge. Keep suppressDuplicateQuestionForms.
- Delete the whole onSubmitForm / onAssistantFormSubmitStart return chain
across AssistantMessage / ChatPane / ProjectView / SideChatTab, and
submitChatRunToolResult in providers/daemon.ts.
Daemon (conservative — keep the generic stream-json input skeleton):
- applyClaudeStreamJsonRunBookkeeping: drop the AUQ detection branch and
run.pendingHostAnswers; stdin now closes on any non-tool_use clean terminal
turn.
- Delete run-tool-results.ts, submitToolResultToRun, and the
POST /api/runs/:id/tool-result endpoint.
- run-artifacts.ts:runAskedUserQuestion now scans streamed text for a
<question-form> marker (reassembled across text_delta chunks) instead of an
AUQ tool_use, preserving the run_finished.asked_user_question analytics
signal.
- Keep --input-format stream-json / promptInputFormat: 'stream-json' as generic
mid-turn input infra; refresh the comments that referenced AUQ.
System prompt / contracts:
- Replace the Claude-only AskUserQuestion clarification section with generic
"emit <question-form> for mid-conversation clarification" guidance; relax the
API-mode override wording; drop "Do not call AskUserQuestion" from the
skip-discovery override.
i18n / CSS:
- Remove the tool.askQuestion* keys from types.ts + all locales.
- Remove the op-ask-question* block and .op-status-awaiting from tools.css;
keep .op-status (the icon badge) and the qf-* classes that mechanism B uses.
Docs / tests:
- AGENTS.md: rewrite the runtime-conventions + chat-UI sections; add an
"Asking the user questions" section documenting the single mechanism.
- Delete the AUQ-only tests; rewrite the quiet-period bookkeeping, run-artifacts,
prompt, and plugin-folder tests to the new behavior.
Validation: pnpm guard (54), full typecheck, web tests, daemon suite
(4351 pass), contracts tests; replayed two Claude stream-json traces through the
mock CLI (incl. the [form answers — discovery] trace) — 0 parse failures, clean
end_turn termination.
lefarcen
added
size/XXL
Visual regression reviewHead: 11 changed · 24 unchanged · 0 missing baseline · 0 failed Changed cases
Unchanged cases
Visual diff is advisory only and does not block merging. |
Siri-Ray
left a comment
Siri-Ray
left a comment
There was a problem hiding this comment.
@elihahah666 thanks for the thoughtful cleanup here. I reviewed the daemon stdin lifecycle changes, the question-form rendering path, the endpoint/caller deletion, and the prompt/analytics updates. I found one non-blocking consistency issue where the newly documented API/BYOK mirroring does not appear to be implemented yet; details inline.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found two blocking regressions in the unified ask-user flow: the contracts/API prompt path is still turn-1-only, and the AUQ removal drops the compatibility path for already-persisted AUQ runs. I also left one smaller inline note on the new analytics detector.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
…lytics alias Address review feedback on the ask-user unification: - contracts composer (used by API/BYOK 'plain' mode) now mirrors the daemon-side '## Clarifying questions mid-conversation' section and widens the API_MODE_OVERRIDE allowed-output list from 'discovery on turn 1' to 'discovery (turn 1) and mid-conversation clarification', so BYOK/API runs route follow-up choices through the unified Questions tab instead of drifting back to plain markdown option lists. Makes the AGENTS.md 'mirrored through packages/contracts/src/prompts/system.ts' claim true. - runAskedUserQuestion now matches the '<ask-question>' alias (already whitelisted by the UI parser and daemon open-tag matcher), so a model that drifts to the alias still records run_finished.asked_user_question instead of being misclassified in the artifact funnel. - Tests: contracts api-mode asserts mid-conversation forms are permitted in both daemon and plain mode; run-artifacts asserts the alias is detected.
Siri-Ray
left a comment
Siri-Ray
left a comment
There was a problem hiding this comment.
@elihahah666 thanks for continuing to tighten this ask-user unification. I re-reviewed the current head across the daemon stdin lifecycle, question-form prompt updates, web rendering path, and the follow-up alias analytics fix. I found one non-blocking consistency issue in the newly added alias handling; details inline.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@elihahah666 I re-checked the current f8dd697 head after the follow-up fixes. The prompt mirroring and alias analytics updates look better, but I still found two merge-safe implementation follow-ups in the legacy-history and alias replay paths; details inline.
…gacy AUQ renderer
Follow-ups from re-review of the ask-user unification:
- The <ask-question> alias is already accepted by the UI parser and the
daemon open-tag matcher, but two consumers still only recognized the
canonical <question-form>: db.listProjectsAwaitingInput's awaiting-input
filter and the next-turn transcript sanitizer. Both now match either tag,
so an alias-form turn (a) marks the project awaiting input and (b) gets
scrubbed from the prior assistant turn instead of replaying verbatim and
re-triggering the discovery-form loop.
- Re-add a read-only legacy AskUserQuestion renderer in ToolCard. The
interactive mechanism is gone, but persisted AUQ tool_use events survive
in upgraded chat history; without this they fell through to GenericCard and
surfaced the raw {"questions":[...]} JSON. The card now renders an inert
question summary (model-authored text, no new i18n keys, no submit path).
Tests: alias awaiting-input case (project-status), alias-form sanitizer
regression (sse), and legacy AUQ render + unparseable fallback (tool-renderers).
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@elihahah666 I re-reviewed the current da000d3 head across the unified question-form flow, the alias follow-ups, and the legacy AUQ compatibility path. The main refactor looks much tighter now, but I still found two merge-safe implementation issues in the new analytics matcher and the historical AUQ fallback renderer; details are inline.
Siri-Ray
left a comment
Siri-Ray
left a comment
There was a problem hiding this comment.
@elihahah666 thanks for continuing to tighten this cleanup. I re-reviewed the current head across the daemon run lifecycle, question-form prompt path, legacy AUQ rendering, alias handling, and the updated tests. I found one merge-safe analytics regression in the new question-form detector; details inline.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.…endering
Re-review follow-ups:
- runAskedUserQuestion read `data.text` for text_delta events, but the
persisted SSE shape carries the chunk on `delta` (packages/contracts/src/
sse/chat.ts), so production <question-form> runs appended nothing and
run_finished.asked_user_question was permanently false (the old tests passed
only because they built the wrong `text` field). Now reads `delta` (still
accepts `text` defensively).
- It also matched a bare open-tag, so a run that merely shows the literal
markup in a generated doc / code sample / HTML artifact could be
misclassified as a clarification turn and excluded from the artifact funnel.
Now requires a renderable closed block via a shared detector.
- Extracted the renderable-form detector + helpers out of server.ts into
apps/daemon/src/question-form-detect.ts so the missing-artifacts guard,
awaiting-input status, and run analytics share ONE check (the codebase
comment already anticipated promoting this once it had a third consumer).
The `<ask-question>` alias is covered by that shared matcher.
- LegacyAskUserQuestionCard now parses the persisted answer from
result.content and surfaces it ("question -> answer") so two historical AUQ
runs with different answers no longer render identically.
Tests: run-artifacts now uses the real `delta` shape + renderable bodies, adds
a literal-markup negative case; tool-renderers asserts the persisted answer is
surfaced.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@elihahah666 I re-reviewed the current 0f1585a head across the shared renderable question-form detector, the run_finished.asked_user_question follow-up, and the legacy AskUserQuestion history rendering. The latest fixes line up with the earlier review threads, and I did not find any further actionable issues in the changed ranges on this head.
Siri-Ray
left a comment
Siri-Ray
left a comment
There was a problem hiding this comment.
@elihahah666 I re-reviewed the current head across the removed AskUserQuestion/tool-result path, the unified <question-form> prompt flow, daemon analytics and awaiting-input detection, the legacy AUQ history renderer, and the web transcript sanitization updates. The prior concerns around API/BYOK prompt mirroring, alias handling, persisted text_delta.delta, and legacy answer visibility look addressed in this revision. I did attempt focused local tests, but this reviewer worktree has no node_modules, so vitest was unavailable; the static pass did not turn up any actionable changed-range issues. Thanks for continuing to tighten this cleanup and for keeping the legacy history path legible.
|
All gates green — @nettee and @Siri-Ray both approved the latest head and CI is passing. Great work iterating through those review rounds carefully, @elihahah666 🎉 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
4 participants
Why
While building on Open Design I hit a broken UI: a chat message showed a
"Question" card whose text status badge ("Awaiting your answer") was stuffed
into the 24px
.op-statusicon badge and overflowed onto the title. Digging in,the real problem was bigger than CSS — the repo carries two parallel
mechanisms for the same job of asking the user a clarifying question:
AskUserQuestionSDK tool: an inline interactive card in the chatstream (
AskUserQuestionCard/StreamingAskUserQuestionCard), whose answeris fed back into the still-open stream-json child via
POST /api/runs/:id/tool-result+submitChatRunToolResult.<question-form>artifact: aQuestionsBannerentry point inchat, the form rendered in the right-hand Questions tab
(
QuestionsPanel/QuestionFormView), answers returned as the next usermessage.
Two visual languages and two interaction models for one concept, with A's card
visually broken, and an artificial split where A was pinned to mid-conversation
clarification and B to turn-1 discovery. This is the technical-debt / UX-fix that
the PR addresses: delete A, keep only B, and widen
<question-form>to anyturn so mid-conversation clarification flows through the same
banner → Questions tab → next-user-message loop.
What users will see
the first-turn discovery brief or a mid-conversation "you marked a region but
didn't say what to change" — now appears as the "Answer a few questions…"
banner in chat, opening the form in the right-hand Questions tab. One
consistent surface instead of two.
Surface area
POST /api/runs/:id/tool-resultendpointand the
submitChatRunToolResultweb caller (return-path deletion).tool.askQuestion*keys fromtypes.tsandall locales.
tab banner instead of an inline chat card.
(No new UI page/dialog is added — the surviving surface,
QuestionsBanner+Questions tab, already existed. The system-prompt change that widens
<question-form>to any turn is prompt-layer only; the front-end alreadysupported a question-form in any assistant message.)
Screenshots
Before: the broken inline card (status text overflowing the icon badge) — see
the originating report. After: clarifying questions surface as the existing
QuestionsBanner→ Questions tab, validated live on an isolated worktreeruntime. No new UI chrome is introduced, so there is no new entry point to show
beyond the pre-existing banner.
Bug fix verification
This is a refactor that also removes a visual bug whose fix is structural
(deleting the broken card entirely), so the acceptance is the migrated test
suite plus human validation rather than a single red-spec:
apps/daemon/tests/chat-run-artifact-quiet-period.test.ts— the obsoletependingHostAnswers-pending assertion was removed and replaced with two specspinning the simplified stdin-close decision (closes on a non-
tool_usecleanterminal turn; stays open on
tool_use).apps/daemon/tests/run-artifacts.test.ts—runAskedUserQuestionnow assertsdetection of a
<question-form>marker (incl. one split acrosstext_deltachunks), replacing the AUQ-tool-use detection.
mid-conversation clarification and confirmed the banner → Questions tab → reply
loop, and that the broken card no longer appears.
Validation
pnpm guard(54 pass),pnpm typecheck(all packages green).pnpm --filter @open-design/web test,pnpm --filter @open-design/contracts test.pnpm --filter @open-design/daemon test— 4351 pass / 4 skip / 4 todo,346 files, exit 0.
04097377(17-tool single turn) and
21747360([form answers — discovery]) through themock CLI — 0 parse failures, well-formed stream-json terminating on
end_turn(the clean-terminal path the simplified bookkeeping closes stdin on).
Adjacent / out of scope
docs/plans/2026-06-01-sandbox-orchestration-hardening-architecture.mditem I6proposes hardening
POST /api/runs/:id/tool-result. That endpoint is deletedby this PR, so I6 is now moot — left untouched here as a dated archival
plan; flagging it so a future contributor doesn't harden a removed endpoint.