Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions agent/agent_runtime_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3200,6 +3200,7 @@ def _execute(next_args: dict) -> Any:
question=next_args.get("question", ""),
choices=next_args.get("choices"),
multi_select=next_args.get("multi_select", False),
questions=next_args.get("questions"),
callback=agent.clarify_callback,
),
next_args,
Expand Down
1 change: 1 addition & 0 deletions agent/tool_executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -2120,6 +2120,7 @@ def _execute(next_args: dict) -> Any:
question=next_args.get("question", ""),
choices=next_args.get("choices"),
multi_select=next_args.get("multi_select", False),
questions=next_args.get("questions"),
callback=agent.clarify_callback,
)
function_result, function_args, middleware_trace, _execution_blocked, _execution_dispatched = _managed_values(_run_agent_tool_execution_middleware(
Expand Down
86 changes: 86 additions & 0 deletions apps/desktop/e2e/batch-clarify.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/**
* E2E batch clarify test — the multi-question clarify card must mount ONCE.
*
* Regression coverage for the duplicated-card bug: `tool.start` carries the
* model's tool_call_id while `clarify.request` carries a gateway-generated
* request_id. A batch payload has no top-level `question`, so the two rows
* only merge when the correlation key comes from the question list
* (`batchClarifyMatchValue` in lib/chat-messages.ts). Before that fix this
* exact flow rendered two identical interactive cards.
*
* The flow runs the real chain: composer → gateway → agent → clarify tool →
* clarify.request event → renderer, against the mock inference server.
*/

import { expect, test } from './test'

import { type MockBackendFixture, setupMockBackend, waitForAppReady } from './fixtures'
import { BATCH_CLARIFY_QUESTIONS, BATCH_CLARIFY_TRIGGER } from './mock-server'

let fixture: MockBackendFixture | null = null

test.beforeAll(async () => {
fixture = await setupMockBackend()
await waitForAppReady(fixture!, 120_000)
})

test.afterAll(async () => {
await fixture?.cleanup()
fixture = null
})

test.describe('batch clarify card', () => {
test('renders exactly one card and completes via per-question locks', async () => {
const page = fixture!.page
const composer = page.locator('[contenteditable="true"]').first()
await composer.waitFor({ state: 'visible', timeout: 10_000 })

await composer.click()
await composer.type(BATCH_CLARIFY_TRIGGER, { delay: 20 })
await page.keyboard.press('Enter')

// The live batch form marks itself with data-clarify-batch=<count>.
const batchCard = page.locator('form[data-clarify-batch]')
await batchCard.first().waitFor({ state: 'visible', timeout: 60_000 })

// THE regression assertion: one card, not two.
await expect(batchCard).toHaveCount(1)
await expect(batchCard).toHaveAttribute('data-clarify-batch', String(BATCH_CLARIFY_QUESTIONS.length))

// Both questions render inside the single card.
for (const entry of BATCH_CLARIFY_QUESTIONS) {
await expect(batchCard.getByText(entry.question)).toHaveCount(1)
}

// Each question text also appears exactly once in the whole transcript —
// catches a duplicate that mounts outside a form[data-clarify-batch].
for (const entry of BATCH_CLARIFY_QUESTIONS) {
await expect(page.getByText(entry.question)).toHaveCount(1)
}

// Answer both questions: stage picks locally (no server traffic yet).
const confirmButton = batchCard.locator('button[type="submit"]')
await expect(confirmButton).toContainText('Confirm and continue')
await expect(confirmButton).toBeDisabled()

await batchCard.getByRole('button', { name: /Coffee/ }).click()
await expect(confirmButton).toBeDisabled()

await batchCard.getByRole('button', { name: /Morning/ }).click()
await expect(confirmButton).toBeEnabled()

// ONE confirm submits the whole batch.
await confirmButton.click()

// The settled card lists both questions with their locked answers.
const settled = page.locator('[data-clarify-settled]')
await settled.waitFor({ state: 'visible', timeout: 30_000 })
await expect(settled.getByText(BATCH_CLARIFY_QUESTIONS[0].question)).toBeVisible()
await expect(settled.getByText('Coffee', { exact: true })).toBeVisible()
await expect(settled.getByText(BATCH_CLARIFY_QUESTIONS[1].question)).toBeVisible()
await expect(settled.getByText('Morning', { exact: true })).toBeVisible()

// And still no duplicate live card lingering after settle.
await expect(page.locator('form[data-clarify-batch]')).toHaveCount(0)
})
})
52 changes: 52 additions & 0 deletions apps/desktop/e2e/mock-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,40 @@ const BLOCKING_CLARIFY_TURN: ScriptedTurn = {
toolCalls: [{ name: 'clarify', args: { question: BLOCKING_CLARIFY_QUESTION, choices: ['Yes', 'No'] } }],
}

/**
* A marker that makes the mock emit a blocking BATCH clarify tool call
* (multi-question form). Regression coverage for the duplicated-card bug:
* the tool.start row and the clarify.request row carry different ids and a
* batch payload has no top-level question, so the correlation key must come
* from the question list or the card mounts twice.
*/
export const BATCH_CLARIFY_TRIGGER = 'E2E_BATCH_CLARIFY_TRIGGER'
export const BATCH_CLARIFY_QUESTIONS = [
{ question: 'Pick a batch drink?', choices: ['Coffee', 'Tea'] },
{ question: 'Pick a batch time?', choices: ['Morning', 'Night'] },
]

const BATCH_CLARIFY_TURN: ScriptedTurn = {
text: '',
toolCalls: [{ name: 'clarify', args: { questions: BATCH_CLARIFY_QUESTIONS } }],
}

function includesBatchClarifyTrigger(value: unknown): boolean {
if (typeof value === 'string') {
return value.includes(BATCH_CLARIFY_TRIGGER)
}

if (Array.isArray(value)) {
return value.some(includesBatchClarifyTrigger)
}

if (value && typeof value === 'object') {
return Object.values(value).some(includesBatchClarifyTrigger)
}

return false
}

function includesBlockingClarifyTrigger(value: unknown): boolean {
if (typeof value === 'string') {
return value.includes(BLOCKING_CLARIFY_TRIGGER)
Expand Down Expand Up @@ -484,6 +518,24 @@ export function startMockServer(options: MockServerOptions = {}): Promise<MockSe
return
}

if (includesBatchClarifyTrigger(parsed.messages)) {
// Only the FIRST completion of the conversation scripts the batch
// clarify. The trigger text stays in message history, so once the
// answered tool result is present the turn falls through to the
// canned reply — otherwise the mock loops the quiz forever.
const hasToolResult = Array.isArray(parsed.messages)
&& parsed.messages.some((message: { role?: string }) => message?.role === 'tool')

if (!hasToolResult) {
if (stream) {
streamScriptedTurn(res, model, BATCH_CLARIFY_TURN)
} else {
nonStreamingScriptedTurn(res, model, BATCH_CLARIFY_TURN)
}
return
}
}

if (includesBlockingClarifyTrigger(parsed.messages)) {
if (stream) {
streamScriptedTurn(res, model, BLOCKING_CLARIFY_TURN)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,4 +169,45 @@ describe('clarify.request stream hydration', () => {

expect(clarifyParts()).toHaveLength(1)
})

it('merges a BATCH tool.start row with its clarify.request (no top-level question)', async () => {
await mountStream()

// The batch shape: tool args carry `questions`, no top-level `question`.
// The correlation key must come from the question list, or the two ids
// mount two cards (the duplicate seen in the field).
toolStart({
args: { questions: [{ question: 'Drink?' }, { question: 'Productive when?' }] },
name: 'clarify',
tool_id: 'call-batch'
})
clarifyRequest({
questions: [
{ qid: 'q0', question: 'Drink?' },
{ qid: 'q1', question: 'Productive when?' }
],
request_id: 'req-batch'
})

expect(clarifyParts()).toHaveLength(1)
})

it('does not duplicate when the batch clarify.request arrives before tool.start', async () => {
await mountStream()

clarifyRequest({
questions: [
{ qid: 'q0', question: 'Drink?' },
{ qid: 'q1', question: 'Productive when?' }
],
request_id: 'req-batch-2'
})
toolStart({
args: { questions: [{ question: 'Drink?' }, { question: 'Productive when?' }] },
name: 'clarify',
tool_id: 'call-batch-2'
})

expect(clarifyParts()).toHaveLength(1)
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { invalidateSlashCompletions } from '@/lib/slash-completion-cache'
import { type AgentNoticePayload, clearAgentNotice, nativeNoticeInput, showAgentNotice } from '@/store/agent-notices'
import { reconcileApprovalModeForProfile } from '@/store/approval-mode'
import { billingCtaLabel, clearBillingBlock, runBillingRecovery, setBillingBlock } from '@/store/billing-block'
import { clearClarifyRequest, normalizeChoices, setClarifyRequest, warnDroppedChoices } from '@/store/clarify'
import { clearClarifyRequest, normalizeChoices, normalizeQuestions, setClarifyRequest, warnDroppedChoices } from '@/store/clarify'
import { setSessionCompacting } from '@/store/compaction'
import { refreshBackgroundProcesses } from '@/store/composer-status'
import { $gateway, activeGatewayConnectionId } from '@/store/gateway'
Expand Down Expand Up @@ -1165,8 +1165,65 @@ export function useGatewayEventHandler(deps: GatewayEventDeps) {
const rawChoices = payload?.choices
const choices = normalizeChoices(rawChoices)
const multiSelect = payload?.multi_select === true
// Batch (multi-question) clarify: `questions` replaces question/choices
// on the wire. `answers` rides along only on reconnect replay, carrying
// the per-question locks the server already accepted.
const questions = normalizeQuestions(payload?.questions)
const lockedAnswers =
typeof payload?.answers === 'object' && payload?.answers !== null
? Object.fromEntries(
Object.entries(payload.answers as Record<string, unknown>).filter(
(entry): entry is [string, string] => typeof entry[1] === 'string'
)
)
: undefined

if (requestId && questions.length > 0) {
setClarifyRequest({
choices: null,
lockedAnswers,
multiSelect: false,
question: '',
questions,
requestId,
sessionId: sessionId ?? null
})

if (sessionId) {
// Same hydration-race guard as the single-question path below: the
// form mounts from the tool row, so upsert a stable one keyed by
// the request id in case tool.start was missed.
upsertToolCall(
sessionId,
{
args: {
questions: questions.map(q => ({
choices: q.choices ?? undefined,
multi_select: q.multiSelect || undefined,
question: q.question
}))
},
name: 'clarify',
tool_id: requestId
},
'running',
event.type,
occurredAt
)
updateSessionState(sessionId, state => ({ ...state, needsInput: true }))

if (requestId && question) {
if (sessionId === activeSessionIdRef.current) {
requestScrollToBottom()
}
}

dispatchNativeNotification({
body: questions.map(q => q.question).join(' · '),
kind: 'input',
sessionId,
title: translateNow('notifications.native.inputTitle')
})
} else if (requestId && question) {
if (rawChoices != null && choices.length === 0) {
warnDroppedChoices('gateway', question, rawChoices)
}
Expand Down
Loading
Loading