mirror of
https://github.com/nexu-io/open-design.git
synced 2026-10-27 09:51:12 +01:00
* refactor(chat): unify ask-user flow on <question-form>, delete AskUserQuestion 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.
* fix(chat): unify mid-conversation question-form across API/BYOK + analytics 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.
* fix(chat): make <ask-question> alias consistent across all paths + legacy 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).
* fix(chat): correct asked_user_question detector + legacy AUQ answer rendering
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.
---------
Co-authored-by: qiongyu1999 <2694684348@qq.com>
Co-authored-by: lefarcen <935902669@qq.com>
383 lines
12 KiB
TypeScript
383 lines
12 KiB
TypeScript
// Unit coverage for `countNewHtmlArtifacts`. Pins the v2
|
|
// `run_finished.artifact_count` invariant: incremental count of
|
|
// distinct `.html` paths the run produced or modified, deduped by
|
|
// path, with Read ops never counted and FAILED ops never counted
|
|
// (mrcfps review on PR #2590 — earlier version counted every
|
|
// matching `tool_use` regardless of whether the matching
|
|
// `tool_result` landed `isError: true`, so a permission-denied
|
|
// `Write index.html` still bumped `artifact_count` to 1 and
|
|
// corrupted the same funnel this helper is trying to repair).
|
|
//
|
|
// `server.ts` previously emitted `artifact_count: 0` literally, which
|
|
// suppressed every dashboard tile that breaks "generation success" by
|
|
// whether an artifact landed. These tests keep the new helper honest
|
|
// for the shapes the daemon actually sees on the wire (claude-stream,
|
|
// codex, ACP/MCP proxies).
|
|
|
|
import { describe, expect, it } from 'vitest';
|
|
|
|
import {
|
|
countDesignSystemPreviewModules,
|
|
countNewHtmlArtifacts,
|
|
didRunCreateDesignSystemFile,
|
|
runAskedUserQuestion,
|
|
} from '../src/run-artifacts.js';
|
|
|
|
let nextId = 0;
|
|
function freshId(prefix = 'tool'): string {
|
|
nextId += 1;
|
|
return `${prefix}-${nextId}`;
|
|
}
|
|
|
|
// Helper: emit a tool_use+tool_result pair. `isError` defaults to
|
|
// false so the common "successful Write on .html" case stays one
|
|
// line at the call site.
|
|
function pair(
|
|
name: string,
|
|
filePath: string,
|
|
isError = false,
|
|
id = freshId(),
|
|
) {
|
|
return [
|
|
{
|
|
event: 'agent',
|
|
data: {
|
|
type: 'tool_use',
|
|
id,
|
|
name,
|
|
input: { file_path: filePath },
|
|
},
|
|
},
|
|
{
|
|
event: 'agent',
|
|
data: {
|
|
type: 'tool_result',
|
|
toolUseId: id,
|
|
isError,
|
|
},
|
|
},
|
|
];
|
|
}
|
|
|
|
// Helper: emit a tool_use with no matching tool_result. Used to pin
|
|
// the "tool still in flight" / "adapter swallowed result" behavior.
|
|
function unfinished(name: string, filePath: string, id = freshId()) {
|
|
return [
|
|
{
|
|
event: 'agent',
|
|
data: {
|
|
type: 'tool_use',
|
|
id,
|
|
name,
|
|
input: { file_path: filePath },
|
|
},
|
|
},
|
|
];
|
|
}
|
|
|
|
describe('countNewHtmlArtifacts', () => {
|
|
it('returns 0 when the run produced no events', () => {
|
|
expect(countNewHtmlArtifacts([])).toBe(0);
|
|
});
|
|
|
|
it('returns 0 when no tool_use targets a .html file', () => {
|
|
expect(
|
|
countNewHtmlArtifacts([
|
|
...pair('Write', '/proj/notes.md'),
|
|
...pair('Edit', '/proj/styles.css'),
|
|
...pair('Read', '/proj/index.html'),
|
|
]),
|
|
).toBe(0);
|
|
});
|
|
|
|
it('counts a single successful Write on a .html path', () => {
|
|
expect(
|
|
countNewHtmlArtifacts(pair('Write', '/proj/index.html')),
|
|
).toBe(1);
|
|
});
|
|
|
|
it('does NOT count a tool_use whose tool_result reports isError=true', () => {
|
|
// Permission denied, path outside cwd, parent missing — all bounce
|
|
// the same way through the tool_result.isError channel and must
|
|
// not increment artifact_count.
|
|
expect(
|
|
countNewHtmlArtifacts(pair('Write', '/proj/index.html', true)),
|
|
).toBe(0);
|
|
});
|
|
|
|
it('does NOT count a tool_use that has no matching tool_result yet', () => {
|
|
// Run was sampled while a Write was still in flight; the
|
|
// safe-default is to undercount rather than promise an
|
|
// artifact we can't confirm landed.
|
|
expect(
|
|
countNewHtmlArtifacts(unfinished('Write', '/proj/index.html')),
|
|
).toBe(0);
|
|
});
|
|
|
|
it('dedupes multiple successful Write/Edit ops on the same path', () => {
|
|
expect(
|
|
countNewHtmlArtifacts([
|
|
...pair('Write', '/proj/index.html'),
|
|
...pair('Edit', '/proj/index.html'),
|
|
...pair('MultiEdit', '/proj/index.html'),
|
|
]),
|
|
).toBe(1);
|
|
});
|
|
|
|
it('keeps the path counted when a later edit on the same path fails', () => {
|
|
// First Write succeeded — the artifact exists. A subsequent Edit
|
|
// that errors doesn't take that fact away; the file is still on
|
|
// disk. So this still reports 1.
|
|
expect(
|
|
countNewHtmlArtifacts([
|
|
...pair('Write', '/proj/index.html'),
|
|
...pair('Edit', '/proj/index.html', true),
|
|
]),
|
|
).toBe(1);
|
|
});
|
|
|
|
it('counts distinct .html paths separately', () => {
|
|
expect(
|
|
countNewHtmlArtifacts([
|
|
...pair('Write', '/proj/index.html'),
|
|
...pair('Write', '/proj/about.html'),
|
|
...pair('Write', '/proj/contact.html'),
|
|
]),
|
|
).toBe(3);
|
|
});
|
|
|
|
it('handles the Codex `create_file` / `str_replace_edit` aliases', () => {
|
|
expect(
|
|
countNewHtmlArtifacts([
|
|
...pair('create_file', '/proj/a.html'),
|
|
...pair('str_replace_edit', '/proj/b.html'),
|
|
]),
|
|
).toBe(2);
|
|
});
|
|
|
|
it('accepts both `file_path` and `path` input shapes', () => {
|
|
const id = freshId();
|
|
expect(
|
|
countNewHtmlArtifacts([
|
|
{
|
|
event: 'agent',
|
|
data: {
|
|
type: 'tool_use',
|
|
id,
|
|
name: 'Write',
|
|
input: { path: '/proj/page.html' },
|
|
},
|
|
},
|
|
{
|
|
event: 'agent',
|
|
data: { type: 'tool_result', toolUseId: id, isError: false },
|
|
},
|
|
]),
|
|
).toBe(1);
|
|
});
|
|
|
|
it('treats .HTML / .Html case-insensitively', () => {
|
|
expect(
|
|
countNewHtmlArtifacts([
|
|
...pair('Write', '/proj/Page.HTML'),
|
|
...pair('Write', '/proj/Other.Html'),
|
|
]),
|
|
).toBe(2);
|
|
});
|
|
|
|
it('ignores non-agent events and malformed payloads', () => {
|
|
expect(
|
|
countNewHtmlArtifacts([
|
|
{ event: 'start', data: { runId: 'r1' } },
|
|
{ event: 'stderr', data: { chunk: 'log' } },
|
|
{ event: 'agent', data: null },
|
|
{ event: 'agent', data: { type: 'text_delta', text: 'hi' } },
|
|
...pair('Write', '/proj/index.html'),
|
|
]),
|
|
).toBe(1);
|
|
});
|
|
|
|
it('ignores Read / Grep / Bash even when their input names a .html file', () => {
|
|
expect(
|
|
countNewHtmlArtifacts([
|
|
...pair('Read', '/proj/index.html'),
|
|
...pair('Grep', '/proj/index.html'),
|
|
...pair('Bash', '/proj/index.html'),
|
|
]),
|
|
).toBe(0);
|
|
});
|
|
});
|
|
|
|
describe('didRunCreateDesignSystemFile', () => {
|
|
it('is true when the run wrote a DESIGN.md', () => {
|
|
expect(
|
|
didRunCreateDesignSystemFile([
|
|
...pair('Write', '/proj/DESIGN.md'),
|
|
]),
|
|
).toBe(true);
|
|
});
|
|
|
|
it('matches DESIGN.md case-insensitively', () => {
|
|
expect(
|
|
didRunCreateDesignSystemFile([
|
|
...pair('Edit', '/proj/design.md'),
|
|
]),
|
|
).toBe(true);
|
|
});
|
|
|
|
it('is false when the matching tool_result reported isError', () => {
|
|
expect(
|
|
didRunCreateDesignSystemFile([
|
|
...pair('Write', '/proj/DESIGN.md', true),
|
|
]),
|
|
).toBe(false);
|
|
});
|
|
|
|
it('is false when no DESIGN.md was touched', () => {
|
|
expect(
|
|
didRunCreateDesignSystemFile([
|
|
...pair('Write', '/proj/index.html'),
|
|
...pair('Read', '/proj/DESIGN.md'),
|
|
]),
|
|
).toBe(false);
|
|
});
|
|
});
|
|
|
|
describe('countDesignSystemPreviewModules', () => {
|
|
it('counts distinct preview/*.html paths the run wrote', () => {
|
|
expect(
|
|
countDesignSystemPreviewModules([
|
|
...pair('Write', '/proj/preview/colors.html'),
|
|
...pair('Write', '/proj/preview/typography.html'),
|
|
...pair('Write', '/proj/preview/components.html'),
|
|
]),
|
|
).toBe(3);
|
|
});
|
|
|
|
it('dedupes Write-then-Edit on the same preview path', () => {
|
|
expect(
|
|
countDesignSystemPreviewModules([
|
|
...pair('Write', '/proj/preview/colors.html'),
|
|
...pair('Edit', '/proj/preview/colors.html'),
|
|
]),
|
|
).toBe(1);
|
|
});
|
|
|
|
it('counts preview/index.html as a module', () => {
|
|
expect(
|
|
countDesignSystemPreviewModules([
|
|
...pair('Write', '/proj/preview/index.html'),
|
|
]),
|
|
).toBe(1);
|
|
});
|
|
|
|
it('ignores non-preview html paths', () => {
|
|
expect(
|
|
countDesignSystemPreviewModules([
|
|
...pair('Write', '/proj/index.html'),
|
|
...pair('Write', '/proj/docs/intro.html'),
|
|
]),
|
|
).toBe(0);
|
|
});
|
|
|
|
it('skips preview writes whose tool_result reported isError', () => {
|
|
expect(
|
|
countDesignSystemPreviewModules([
|
|
...pair('Write', '/proj/preview/colors.html', true),
|
|
...pair('Write', '/proj/preview/typography.html'),
|
|
]),
|
|
).toBe(1);
|
|
});
|
|
});
|
|
|
|
// Helper: emit a bare tool_use (no result) for a named tool.
|
|
function toolUse(name: string, id = freshId()) {
|
|
return [
|
|
{
|
|
event: 'agent',
|
|
data: { type: 'tool_use', id, name, input: {} },
|
|
},
|
|
];
|
|
}
|
|
|
|
// A renderable question-form body — JSON with a non-empty `questions` array.
|
|
// `runAskedUserQuestion` requires a *closed, renderable* block, not a bare tag.
|
|
const RENDERABLE_BODY = '{"questions":[{"question":"Which framework?"}]}';
|
|
|
|
// Helper: emit assistant streamed text as a `text_delta` agent event. Note the
|
|
// real persisted shape carries the chunk on `delta` (not `text`) — see
|
|
// packages/contracts/src/sse/chat.ts. Building the wrong field here is exactly
|
|
// what made the old tests pass while production runs detected nothing.
|
|
function questionFormText(text = `Quick brief <question-form id="q">${RENDERABLE_BODY}</question-form>`) {
|
|
return [{ event: 'agent', data: { type: 'text_delta', delta: text } }];
|
|
}
|
|
|
|
describe('runAskedUserQuestion', () => {
|
|
it('returns false for an empty event list', () => {
|
|
expect(runAskedUserQuestion([])).toBe(false);
|
|
});
|
|
|
|
it('returns true when the run emitted a renderable <question-form> clarification', () => {
|
|
expect(runAskedUserQuestion(questionFormText())).toBe(true);
|
|
});
|
|
|
|
it('reads the `delta` field of text_delta events (the real persisted shape)', () => {
|
|
// Guards the production bug where the helper read `text` and so appended
|
|
// nothing for real runs, leaving run_finished.asked_user_question false.
|
|
expect(
|
|
runAskedUserQuestion([
|
|
{ event: 'agent', data: { type: 'text_delta', delta: `<question-form id="q">${RENDERABLE_BODY}</question-form>` } },
|
|
]),
|
|
).toBe(true);
|
|
});
|
|
|
|
it('reassembles a marker split across text_delta chunks', () => {
|
|
expect(
|
|
runAskedUserQuestion([
|
|
{ event: 'agent', data: { type: 'text_delta', delta: 'ask a <question-form id="q">{"questions":[' } },
|
|
{ event: 'agent', data: { type: 'text_delta', delta: '{"question":"X"}]}</question-form>' } },
|
|
]),
|
|
).toBe(true);
|
|
});
|
|
|
|
// `<ask-question>` is an accepted alias for `<question-form>` (whitelisted by
|
|
// the UI parser and the daemon open-tag matcher). A model that drifts to the
|
|
// alias still renders the clarification banner, so the analytics signal must
|
|
// recognize it too — otherwise the run gets misclassified in the funnel.
|
|
it('returns true for the renderable <ask-question> alias', () => {
|
|
expect(
|
|
runAskedUserQuestion([
|
|
{ event: 'agent', data: { type: 'text_delta', delta: `one quick check <ask-question id="q">${RENDERABLE_BODY}</ask-question>` } },
|
|
]),
|
|
).toBe(true);
|
|
});
|
|
|
|
it('returns false when the tag is only shown as literal markup (no renderable body)', () => {
|
|
// A doc/code sample that mentions the markup must NOT be misclassified as a
|
|
// clarification turn and excluded from the artifact funnel.
|
|
expect(
|
|
runAskedUserQuestion([
|
|
{ event: 'agent', data: { type: 'text_delta', delta: 'Use a `<question-form id="x">` block like this in your skill.' } },
|
|
]),
|
|
).toBe(false);
|
|
});
|
|
|
|
it('returns false for a run that only wrote artifacts', () => {
|
|
expect(
|
|
runAskedUserQuestion([
|
|
...pair('Write', '/proj/index.html'),
|
|
...toolUse('Read'),
|
|
]),
|
|
).toBe(false);
|
|
});
|
|
|
|
it('detects the form even when mixed with other tool calls', () => {
|
|
expect(
|
|
runAskedUserQuestion([
|
|
...pair('Write', '/proj/index.html'),
|
|
...questionFormText(),
|
|
]),
|
|
).toBe(true);
|
|
});
|
|
});
|