diff --git a/apps/mobile/src/features/threads/PendingUserInputCard.tsx b/apps/mobile/src/features/threads/PendingUserInputCard.tsx index bd3cb03b17b6..e5e940ad2183 100644 --- a/apps/mobile/src/features/threads/PendingUserInputCard.tsx +++ b/apps/mobile/src/features/threads/PendingUserInputCard.tsx @@ -270,6 +270,23 @@ export function PendingUserInputCard(props: PendingUserInputCardProps) { ) : null} {props.pendingUserInput.questions.map((question) => { const draft = props.drafts[question.id]; + const minSelections = question.minSelections ?? (question.maxSelections === 0 ? 0 : 1); + const maxSelections = question.maxSelections; + const selectionHint = + maxSelections === 0 + ? "Leave all options unselected." + : maxSelections === minSelections + ? `Select ${minSelections} option${minSelections === 1 ? "" : "s"}.` + : maxSelections !== undefined + ? minSelections === 0 + ? `Select up to ${maxSelections} option${maxSelections === 1 ? "" : "s"}.` + : `Select ${minSelections} to ${maxSelections} options.` + : minSelections === 0 + ? "Select any number of options." + : `Select at least ${minSelections} option${minSelections === 1 ? "" : "s"}.`; + const showSelectionHint = + question.multiSelect && + (question.minSelections !== undefined || maxSelections !== undefined); return ( @@ -278,6 +295,14 @@ export function PendingUserInputCard(props: PendingUserInputCardProps) { {question.question} + {showSelectionHint || question.required === false ? ( + + {showSelectionHint ? selectionHint : null} + {question.required === false + ? `${showSelectionHint ? " " : ""}You can skip this question.` + : null} + + ) : null} {question.options.map((option) => { const optionValue = option.value ?? option.label.trim(); diff --git a/apps/mobile/src/lib/threadActivity.test.ts b/apps/mobile/src/lib/threadActivity.test.ts index 5471f24704a9..ea68437d7dc6 100644 --- a/apps/mobile/src/lib/threadActivity.test.ts +++ b/apps/mobile/src/lib/threadActivity.test.ts @@ -1989,6 +1989,102 @@ describe("pending user input answers", () => { }); }); + it("omits unanswered optional questions while preserving required defaults", () => { + const question = { ...singleSelectQuestion, required: false }; + + expect( + buildPendingUserInputAnswers([question, multiSelectQuestion], { + scope: { selectedOptionValues: ["Orders"] }, + }), + ).toEqual({ scope: ["Orders"] }); + expect(buildPendingUserInputAnswers([question], { runtime: { customAnswer: " " } })).toEqual( + {}, + ); + expect(buildPendingUserInputAnswers([singleSelectQuestion], {})).toBeNull(); + }); + + it.each([ + { attachmentsBlocked: true }, + { attachmentCount: 1, attachmentsBlocked: true }, + { attachmentCount: 1 }, + { customAnswer: "Unlisted answer" }, + { selectedOptionValues: ["unknown"] }, + ])("keeps invalid optional drafts blocking submission: %j", (draft) => { + const question = { ...singleSelectQuestion, required: false, allowCustomAnswer: false }; + + expect(buildPendingUserInputAnswers([question], { runtime: draft })).toBeNull(); + }); + + it.each([0, undefined])( + "submits empty arrays with a zero maximum and minimum %j", + (minSelections) => { + const question = { + ...multiSelectQuestion, + allowCustomAnswer: false, + required: true, + ...(minSelections === undefined ? {} : { minSelections }), + maxSelections: 0, + }; + + expect(buildPendingUserInputAnswers([question], {})).toEqual({ scope: [] }); + expect( + buildPendingUserInputAnswers([question], { scope: { selectedOptionValues: ["Orders"] } }), + ).toBeNull(); + expect(buildPendingUserInputAnswers([{ ...question, required: false }], {})).toEqual({}); + }, + ); + + it("requires one selection when no zero maximum is declared", () => { + const question = { ...multiSelectQuestion, required: true, allowCustomAnswer: false }; + + expect(buildPendingUserInputAnswers([question], {})).toBeNull(); + expect(buildPendingUserInputAnswers([{ ...question, maxSelections: 2 }], {})).toBeNull(); + }); + + it("enforces multi-select limits and omits unanswered optional arrays", () => { + const question = { + ...multiSelectQuestion, + options: [...multiSelectQuestion.options, { label: "Sales", description: "Sales" }], + allowCustomAnswer: false, + minSelections: 2, + maxSelections: 2, + }; + + expect(buildPendingUserInputAnswers([question], {})).toBeNull(); + expect( + buildPendingUserInputAnswers([question], { scope: { selectedOptionValues: ["Orders"] } }), + ).toBeNull(); + expect( + buildPendingUserInputAnswers([question], { + scope: { selectedOptionValues: ["Orders", "Listings"] }, + }), + ).toEqual({ scope: ["Orders", "Listings"] }); + expect( + buildPendingUserInputAnswers([question], { + scope: { selectedOptionValues: ["Orders", "Listings", "Sales"] }, + }), + ).toBeNull(); + expect(buildPendingUserInputAnswers([{ ...question, required: false }], {})).toEqual({}); + expect( + buildPendingUserInputAnswers([{ ...question, required: false }], { + scope: { selectedOptionValues: ["Orders"] }, + }), + ).toBeNull(); + expect( + buildPendingUserInputAnswers([{ ...question, required: false }], { + scope: { selectedOptionValues: ["unknown"] }, + }), + ).toBeNull(); + expect( + buildPendingUserInputAnswers([{ ...question, required: false, minSelections: 0 }], { + scope: { selectedOptionValues: ["unknown"] }, + }), + ).toBeNull(); + expect( + buildPendingUserInputAnswers([multiSelectQuestion], { scope: { attachmentCount: 1 } }), + ).toEqual({ scope: "" }); + }); + it("clears selected options while a custom answer is active", () => { expect( setPendingUserInputCustomAnswer( diff --git a/apps/mobile/src/lib/threadActivity.ts b/apps/mobile/src/lib/threadActivity.ts index a77491795704..275472f3f3a3 100644 --- a/apps/mobile/src/lib/threadActivity.ts +++ b/apps/mobile/src/lib/threadActivity.ts @@ -367,11 +367,27 @@ function resolvePendingUserInputAnswer( const selectedOptionValues = normalizeSelectedOptionValues(question, draft?.selectedOptionValues); if (question.multiSelect) { - return selectedOptionValues.length > 0 - ? selectedOptionValues - : question.allowCustomAnswer !== false && (draft?.attachmentCount ?? 0) > 0 + if ( + selectedOptionValues.length === 0 && + ((draft?.selectedOptionValues?.length ?? 0) > 0 || + normalizeDraftAnswer(draft?.customAnswer) !== null || + (question.allowCustomAnswer === false && (draft?.attachmentCount ?? 0) > 0)) + ) + return null; + if ( + selectedOptionValues.length < + (question.minSelections ?? (question.maxSelections === 0 ? 0 : 1)) || + (question.maxSelections !== undefined && selectedOptionValues.length > question.maxSelections) + ) { + return selectedOptionValues.length === 0 && + question.minSelections === undefined && + question.maxSelections === undefined && + question.allowCustomAnswer !== false && + (draft?.attachmentCount ?? 0) > 0 ? "" : null; + } + return selectedOptionValues; } return ( selectedOptionValues[0] ?? @@ -1632,7 +1648,16 @@ export function buildPendingUserInputAnswers( const answers: Record> = {}; for (const question of questions) { - const answer = resolvePendingUserInputAnswer(question, draftAnswers[question.id]); + const draft = draftAnswers[question.id]; + if ( + question.required === false && + !draft?.attachmentsBlocked && + normalizeDraftAnswer(draft?.customAnswer) === null && + (draft?.selectedOptionValues?.length ?? 0) === 0 && + (draft?.attachmentCount ?? 0) === 0 + ) + continue; + const answer = resolvePendingUserInputAnswer(question, draft); if (answer === null) { return null; } diff --git a/apps/server/scripts/acp-mock-agent.ts b/apps/server/scripts/acp-mock-agent.ts index 5548bebac84c..b1d84a1a142d 100644 --- a/apps/server/scripts/acp-mock-agent.ts +++ b/apps/server/scripts/acp-mock-agent.ts @@ -12,7 +12,7 @@ import * as NodeRuntime from "@effect/platform-node/NodeRuntime"; import * as EffectAcpAgent from "effect-acp/agent"; import * as AcpError from "effect-acp/errors"; -import type * as AcpSchema from "effect-acp/schema"; +import * as AcpSchema from "effect-acp/schema"; import type * as AcpCompat from "effect-acp/compat"; import { beginAcpMockPrompt } from "./acpMockCancellationState.ts"; @@ -34,6 +34,10 @@ const emitBackgroundToolDuringAnswer = process.env.T3_ACP_EMIT_BACKGROUND_TOOL_DURING_ANSWER === "1"; const emitAskQuestion = process.env.T3_ACP_EMIT_ASK_QUESTION === "1"; const emitElicitation = process.env.T3_ACP_EMIT_ELICITATION === "1"; +const elicitationSchemaJson = process.env.T3_ACP_ELICITATION_SCHEMA; +const decodeElicitationSchema = Schema.decodeUnknownEffect( + Schema.fromJsonString(AcpSchema.ElicitationSchema), +); const emitMcpToolApprovalElicitation = process.env.T3_ACP_EMIT_MCP_TOOL_APPROVAL_ELICITATION === "1"; const emitUrlElicitation = process.env.T3_ACP_EMIT_URL_ELICITATION === "1"; @@ -1781,12 +1785,19 @@ const program = Effect.gen(function* () { sessionId: requestedSessionId, message: "Approve this request?", mode: "form", - requestedSchema: { - type: "object", - properties: { - approved: { type: "boolean", title: "Approved" }, - }, - }, + requestedSchema: + elicitationSchemaJson === undefined + ? { + type: "object", + properties: { + approved: { type: "boolean", title: "Approved" }, + }, + } + : yield* decodeElicitationSchema(elicitationSchemaJson).pipe( + Effect.mapError(() => + AcpError.AcpRequestError.invalidParams("Invalid elicitation schema"), + ), + ), ...(emitMcpToolApprovalElicitation ? { _meta: { codex_approval_kind: "mcp_tool_call" } } : {}), diff --git a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts index 031fa17c0506..20fba1ca17c8 100644 --- a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts +++ b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts @@ -107,6 +107,7 @@ const serverConfigLayer = ServerConfig.layerTest(process.cwd(), { const testLayer = Layer.mergeAll(NodeServices.layer, IdAllocator.layer, serverConfigLayer); const ACP_TEST_DRIVER = ProviderDriverKind.make("acp-test"); const decodeUnknownJson = Schema.decodeUnknownOption(Schema.fromJsonString(Schema.Unknown)); +const encodeElicitationSchemaJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); describe("acpProjectedCommandExitCode", () => { const successOutput = { type: "Bash", exit_code: 0 }; @@ -3882,6 +3883,371 @@ describe("AcpAdapterV2", () => { }).pipe(Effect.provide(testLayer), Effect.scoped), ); + it.live.each([ + { name: "wrapped scalar answers", overrides: {}, accept: true }, + { + name: "scalar and native primitive answers", + overrides: { choice: " opaque ", legacy: "", approved: true, ratio: 1.25, count: 3 }, + accept: true, + }, + { + name: "false boolean answers", + overrides: { approved: ["false"] }, + accept: true, + expectedApproved: false, + }, + { name: "exponent numbers", overrides: { ratio: "1.25e0" }, accept: true }, + { + name: "empty arrays without a minimum", + overrides: { scopes: [] }, + accept: true, + omitMinimum: true, + expectedScopes: [], + }, + { name: "missing required answers", overrides: {}, missing: "choice", accept: false }, + { + name: "choice labels instead of opaque values", + overrides: { choice: "Primary" }, + accept: false, + }, + { name: "trimmed opaque values", overrides: { choice: "opaque" }, accept: false }, + { + name: "multiple scalar choices", + overrides: { choice: [" opaque ", "other"] }, + accept: false, + }, + { name: "unknown boolean values", overrides: { approved: "yes" }, accept: false }, + { name: "non-decimal numbers", overrides: { ratio: "0x1" }, accept: false }, + { name: "non-finite numbers", overrides: { ratio: Number.POSITIVE_INFINITY }, accept: false }, + { name: "numbers outside the bounds", overrides: { ratio: "11" }, accept: false }, + { name: "fractional integers", overrides: { count: "1.5" }, accept: false }, + { + name: "fractions rounded to integers", + overrides: { count: "1.0000000000000001" }, + accept: false, + }, + { + name: "fractions rounded at the safe integer boundary", + overrides: { count: "9007199254740991.1" }, + unboundedInteger: true, + accept: false, + }, + { + name: "nonzero integers underflowed to zero", + overrides: { count: "1e-400" }, + unboundedInteger: true, + accept: false, + }, + { + name: "exact decimal integers", + overrides: { count: "3.000" }, + accept: true, + }, + { + name: "exact exponent integers", + overrides: { count: "300e-2" }, + accept: true, + }, + { + name: "zero with a negative exponent", + overrides: { count: "0e-400" }, + unboundedInteger: true, + expectedCount: 0, + accept: true, + }, + { + name: "unsafe positive integers", + overrides: { count: "9007199254740993" }, + unboundedInteger: true, + accept: false, + }, + { + name: "unsafe negative integers", + overrides: { count: "-9007199254740993" }, + unboundedInteger: true, + accept: false, + }, + { + name: "safe integer boundary", + overrides: { count: "9007199254740991" }, + unboundedInteger: true, + expectedCount: Number.MAX_SAFE_INTEGER, + accept: true, + }, + { name: "non-string answers", overrides: { code: 1 }, accept: false }, + { name: "strings outside the length bounds", overrides: { code: "TOOLONG" }, accept: false }, + { + name: "unsupported pattern constraints", + overrides: {}, + stringConstraint: { pattern: "^[A-Z]+$" }, + unsupportedConstraint: true, + accept: false, + }, + { + name: "catastrophic pattern constraints", + overrides: {}, + stringConstraint: { pattern: "^(a+)+$" }, + unsupportedConstraint: true, + accept: false, + }, + { + name: "unsupported email format constraints", + overrides: {}, + stringConstraint: { format: "email" }, + unsupportedConstraint: true, + accept: false, + }, + { + name: "unknown format constraints", + overrides: {}, + stringConstraint: { format: "unknown" }, + unsupportedConstraint: true, + accept: false, + }, + { name: "non-array multi-select answers", overrides: { scopes: "read" }, accept: false }, + { + name: "unknown multi-select values", + overrides: { scopes: ["read", "admin"] }, + accept: false, + }, + { name: "wrong multi-select item types", overrides: { scopes: ["read", true] }, accept: false }, + { name: "too few multi-select values", overrides: { scopes: [] }, accept: false }, + { + name: "too many multi-select values", + overrides: { scopes: ["read", " write ", "read"] }, + accept: false, + }, + ])("handles ACP form elicitation with $name", (testCase) => + Effect.gen(function* () { + const childProcessSpawner = yield* ChildProcessSpawner.ChildProcessSpawner; + const fileSystem = yield* FileSystem.FileSystem; + const idAllocator = yield* IdAllocator.IdAllocatorV2; + const path = yield* Path.Path; + const serverConfig = yield* ServerConfig.ServerConfig; + const selfInvocation = yield* resolveSelfInvocation(); + const mockAgentPath = yield* path.fromFileUrl( + new URL("../../../scripts/acp-mock-agent.ts", import.meta.url), + ); + const response = yield* Deferred.make(); + const instanceId = ProviderInstanceId.make("acp-test-typed-elicitation"); + const requestedSchema = { + type: "object", + properties: { + choice: { + type: "string", + oneOf: [ + { const: " opaque ", title: "Primary", description: "Keep this ID" }, + { const: "other", title: "Other" }, + ], + }, + legacy: { + type: "string", + enum: ["", "secondary"], + }, + approved: { type: "boolean" }, + ratio: { type: "number", minimum: 0, maximum: 10 }, + count: { + type: "integer", + ...(testCase.unboundedInteger ? {} : { minimum: 1, maximum: 4 }), + }, + scopes: { + type: "array", + ...(testCase.omitMinimum ? {} : { minItems: 1 }), + maxItems: 2, + items: { type: "string", enum: ["read", " write "] }, + }, + code: { + type: "string", + minLength: 2, + maxLength: 4, + ...testCase.stringConstraint, + }, + optional: { type: "string" }, + }, + required: ["choice", "legacy", "approved", "ratio", "count", "scopes", "code"], + } as const; + const adapter = makeAcpAdapterV2({ + crypto: yield* Crypto.Crypto, + instanceId, + flavor: { + driver: ACP_TEST_DRIVER, + capabilities: AcpProviderCapabilitiesV2, + makeRuntime: makeMockRuntime({ + childProcessSpawner, + mockAgentPath, + environment: { + T3_ACP_EMIT_ELICITATION: "1", + T3_ACP_ELICITATION_SCHEMA: encodeElicitationSchemaJson(requestedSchema), + }, + wrapRuntime: (runtime) => ({ + ...runtime, + handleElicitation: (handler) => + runtime.handleElicitation((params, requestContext) => + handler(params, requestContext).pipe( + Effect.tap((result) => Deferred.succeed(response, result)), + ), + ), + }), + }), + }, + fileSystem, + idAllocator, + serverConfig, + selfInvocation, + }); + const threadId = ThreadId.make("thread-acp-typed-elicitation"); + const runtimePolicy = ProviderAdapterV2RuntimePolicy.make({ + runtimeMode: "approval-required", + interactionMode: "default", + cwd: process.cwd(), + }); + const modelSelection = { instanceId, model: "default" } as const; + const runtime = yield* adapter.openSession({ + threadId, + providerSessionId: ProviderSessionId.make("provider-session-acp-typed-elicitation"), + modelSelection, + runtimePolicy, + }); + const providerThread = yield* runtime.ensureThread({ + threadId, + modelSelection, + runtimePolicy, + }); + yield* runtime.startTurn( + makeTurnInput({ + threadId, + providerThread, + instanceId, + runtimePolicy, + now: yield* DateTime.now, + }), + ); + if (testCase.unsupportedConstraint) { + assert.deepEqual(yield* Deferred.await(response), { action: "decline" }); + return; + } + const pending = Option.getOrThrow( + yield* runtime.events.pipe( + Stream.filter( + (event) => + event.type === "turn_item.updated" && event.turnItem.type === "user_input_request", + ), + Stream.runHead, + ), + ); + if (pending.type !== "turn_item.updated" || pending.turnItem.type !== "user_input_request") { + return yield* Effect.die("Expected a pending user-input form"); + } + const scopesQuestion = pending.turnItem.questions.find( + (question) => question.id === "scopes", + ); + assert.equal(scopesQuestion?.minSelections, testCase.omitMinimum ? 0 : 1); + assert.equal(scopesQuestion?.maxSelections, 2); + assert.deepEqual( + pending.turnItem.questions.map( + ({ id, options, multiSelect, allowCustomAnswer, required }) => ({ + id, + options, + multiSelect, + allowCustomAnswer, + required, + }), + ), + [ + { + id: "choice", + options: [ + { label: "Primary", description: "Keep this ID", value: " opaque " }, + { label: "Other", description: "Other", value: "other" }, + ], + multiSelect: false, + allowCustomAnswer: false, + required: true, + }, + { + id: "legacy", + options: [ + { label: "Option 1", description: "Option 1", value: "" }, + { label: "secondary", description: "secondary", value: "secondary" }, + ], + multiSelect: false, + allowCustomAnswer: false, + required: true, + }, + { + id: "approved", + options: [ + { label: "true", description: "Yes", value: "true" }, + { label: "false", description: "No", value: "false" }, + ], + multiSelect: false, + allowCustomAnswer: false, + required: true, + }, + { id: "ratio", options: [], multiSelect: false, allowCustomAnswer: true, required: true }, + { id: "count", options: [], multiSelect: false, allowCustomAnswer: true, required: true }, + { + id: "scopes", + options: [ + { label: "read", description: "read", value: "read" }, + { label: "write", description: "write", value: " write " }, + ], + multiSelect: true, + allowCustomAnswer: false, + required: true, + }, + { id: "code", options: [], multiSelect: false, allowCustomAnswer: true, required: true }, + { + id: "optional", + options: [], + multiSelect: false, + allowCustomAnswer: true, + required: false, + }, + ], + ); + const answers: Record = { + choice: [" opaque "], + legacy: [""], + approved: ["true"], + ratio: ["1.25"], + count: ["3"], + scopes: ["read", " write "], + code: "OK", + ignored: "Never return undeclared properties", + ...testCase.overrides, + }; + if ("missing" in testCase && typeof testCase.missing === "string") { + delete answers[testCase.missing]; + } + yield* runtime.respondToRuntimeRequest({ requestId: pending.turnItem.requestId, answers }); + assert.deepEqual( + yield* Deferred.await(response), + testCase.accept + ? { + action: "accept", + content: { + choice: " opaque ", + legacy: "", + approved: testCase.expectedApproved ?? true, + ratio: 1.25, + count: testCase.expectedCount ?? 3, + scopes: testCase.expectedScopes ?? ["read", " write "], + code: "OK", + }, + } + : { action: "decline" }, + ); + const terminal = Option.getOrThrow( + yield* runtime.events.pipe( + Stream.filter((event) => event.type === "turn.terminal"), + Stream.runHead, + ), + ); + assert.equal(terminal.type, "turn.terminal"); + }).pipe(Effect.provide(testLayer), Effect.scoped), + ); + it.live("auto-approves tagged MCP elicitations under full-access policy", () => Effect.gen(function* () { const childProcessSpawner = yield* ChildProcessSpawner.ChildProcessSpawner; diff --git a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts index e5ae85129e2f..9dbc51584a67 100644 --- a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts +++ b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts @@ -1105,20 +1105,147 @@ function selectAutoApprovedPermissionOption( ); } +function elicitationOptions( + property: Record, +): OrchestrationV2UserInputQuestion["options"] | undefined { + // Decline unsupported constraints before opening a form. Agent-provided + // regular expressions must never run on the server's event loop. + if ( + property.type === "string" && + (typeof property.pattern === "string" || typeof property.format === "string") + ) + return undefined; + if (property.type === "boolean") { + return [ + { label: "true", description: "Yes", value: "true" }, + { label: "false", description: "No", value: "false" }, + ]; + } + if (property.type === "number" || property.type === "integer") return []; + const choices = property.type === "array" ? unknownRecord(property.items) : property; + if ((property.type !== "string" && property.type !== "array") || choices?.type !== "string") + return undefined; + + const enumValues = choices.enum; + if ( + enumValues != null && + (!Array.isArray(enumValues) || + enumValues.length === 0 || + !enumValues.every((value) => typeof value === "string")) + ) + return undefined; + if (choices.oneOf != null) { + if (!Array.isArray(choices.oneOf) || choices.oneOf.length === 0) return undefined; + const options: Array = []; + for (const [index, value] of choices.oneOf.entries()) { + const option = unknownRecord(value); + if (typeof option?.const !== "string") return undefined; + if (Array.isArray(enumValues) && !enumValues.includes(option.const)) continue; + const label = nonEmptyText(option.title, `Option ${index + 1}`); + options.push({ + label, + description: nonEmptyText(option.description, label), + value: option.const, + }); + } + return options.length > 0 ? options : undefined; + } + if (!Array.isArray(enumValues)) return property.type === "array" ? undefined : []; + return enumValues.map((value, index) => { + const label = nonEmptyText(value, `Option ${index + 1}`); + return { label, description: label, value }; + }); +} + +/** Validate integrality before floating-point rounding can erase fractional digits. */ +function elicitationIntegerTextIsExact(value: string): boolean { + const [mantissa = "", exponent = "0"] = value.trim().split(/[eE]/); + const fractionLength = mantissa.split(".")[1]?.length ?? 0; + const digits = mantissa.replace(/[-.]/g, ""); + const significantDigits = digits.replace(/0+$/, ""); + return ( + significantDigits.length === 0 || + Number(exponent) >= fractionLength - (digits.length - significantDigits.length) + ); +} + function elicitationContent( answers: ProviderUserInputAnswers, - allowedKeys: ReadonlySet, -): Record { - const content: Record = {}; - for (const [key, value] of Object.entries(answers)) { - if (!allowedKeys.has(key)) continue; - if (typeof value === "string" || typeof value === "number" || typeof value === "boolean") { - content[key] = value; - } else if (Array.isArray(value)) { - content[key] = value.filter((entry): entry is string => typeof entry === "string"); + properties: Record, + required: ReadonlySet, +): Record | undefined { + const content: Array<[string, EffectAcpSchema.ElicitationContentValue]> = []; + for (const id of required) { + if (!Object.hasOwn(properties, id) || !Object.hasOwn(answers, id)) return undefined; + } + for (const [id, property] of Object.entries(properties)) { + if (!Object.hasOwn(answers, id)) continue; + const record = unknownRecord(property); + if (record === undefined) return undefined; + const options = elicitationOptions(record); + if (options === undefined) return undefined; + const answer = answers[id]; + if (record.type === "array") { + if ( + !Array.isArray(answer) || + !answer.every( + (value) => typeof value === "string" && options.some((option) => option.value === value), + ) || + (typeof record.minItems === "number" && answer.length < record.minItems) || + (typeof record.maxItems === "number" && answer.length > record.maxItems) + ) + return undefined; + content.push([id, answer]); + continue; + } + // Some clients wrap single selections in arrays. Never collapse a multi-answer value. + if (Array.isArray(answer) && answer.length !== 1) return undefined; + const value = Array.isArray(answer) ? answer[0] : answer; + switch (record.type) { + case "string": { + if (typeof value !== "string") return undefined; + const length = Array.from(value).length; + if ( + (options.length > 0 && !options.some((option) => option.value === value)) || + (typeof record.minLength === "number" && length < record.minLength) || + (typeof record.maxLength === "number" && length > record.maxLength) + ) + return undefined; + content.push([id, value]); + break; + } + case "boolean": { + if (typeof value === "boolean") content.push([id, value]); + else if (value === "true" || value === "false") content.push([id, value === "true"]); + else return undefined; + break; + } + case "number": + case "integer": { + const number = + typeof value === "number" + ? value + : typeof value === "string" && + /^-?(?:0|[1-9]\d*)(?:\.\d+)?(?:[eE][+-]?\d+)?$/.test(value.trim()) + ? Number(value) + : Number.NaN; + if ( + !Number.isFinite(number) || + (record.type === "integer" && + (!Number.isSafeInteger(number) || + (typeof value === "string" && !elicitationIntegerTextIsExact(value)))) || + (typeof record.minimum === "number" && number < record.minimum) || + (typeof record.maximum === "number" && number > record.maximum) + ) + return undefined; + content.push([id, number]); + break; + } + default: + return undefined; } } - return content; + return Object.fromEntries(content); } interface ActiveTextSegment { @@ -5831,31 +5958,38 @@ export function makeAcpAdapterV2( } const requestedSchema = unknownRecord(params.requestedSchema); const properties = unknownRecord(requestedSchema?.properties) ?? {}; + const required = new Set( + Array.isArray(requestedSchema?.required) + ? requestedSchema.required.filter((id): id is string => typeof id === "string") + : [], + ); const elicitationScopeId = "sessionId" in params ? params.sessionId : `request:${params.requestId}`; - const questions = Object.entries(properties).map( - ([id, property], index): OrchestrationV2UserInputQuestion => { - const record = unknownRecord(property); - const enumValues = Array.isArray(record?.enum) - ? record.enum.filter((value): value is string => typeof value === "string") - : []; - const options = - enumValues.length > 0 - ? enumValues.map((value) => ({ label: value, description: value })) - : record?.type === "boolean" - ? [ - { label: "true", description: "Yes" }, - { label: "false", description: "No" }, - ] - : []; - return { - id, - header: nonEmptyText(record?.title, `Question ${index + 1}`), - question: nonEmptyText(record?.description, params.message), - options, - }; - }, - ); + const questions: OrchestrationV2UserInputQuestion[] = []; + for (const [id, property] of Object.entries(properties)) { + const record = unknownRecord(property); + const options = record === undefined ? undefined : elicitationOptions(record); + if (record === undefined || options === undefined || id.trim().length === 0) { + return { action: "decline" } as const; + } + questions.push({ + id, + header: nonEmptyText(record.title, `Question ${questions.length + 1}`), + question: nonEmptyText(record.description, params.message), + options, + multiSelect: record.type === "array", + allowCustomAnswer: options.length === 0, + required: required.has(id), + ...(record.type === "array" + ? { + minSelections: typeof record.minItems === "number" ? record.minItems : 0, + ...(typeof record.maxItems === "number" + ? { maxSelections: record.maxItems } + : {}), + } + : {}), + }); + } const userInput = yield* requestUserInputWithAdmission( handlerGeneration, Effect.gen(function* () { @@ -5872,16 +6006,16 @@ export function makeAcpAdapterV2( }), transportRequestId, ); + const content = + userInput.answers === null + ? undefined + : elicitationContent(userInput.answers, properties, required); const response = userInput.answers === null ? ({ action: "cancel" } as const) - : ({ - action: "accept", - content: elicitationContent( - userInput.answers, - new Set(Object.keys(properties)), - ), - } as const); + : content === undefined + ? ({ action: "decline" } as const) + : ({ action: "accept", content } as const); yield* userInput.acknowledgeNativeResponse; return response; }), diff --git a/apps/web/src/components/chat/ChatComposer.tsx b/apps/web/src/components/chat/ChatComposer.tsx index 36b2b2a6a271..358a9ab8fc77 100644 --- a/apps/web/src/components/chat/ChatComposer.tsx +++ b/apps/web/src/components/chat/ChatComposer.tsx @@ -1556,6 +1556,7 @@ export interface ChatComposerProps { id: string; multiSelect?: boolean | undefined; allowCustomAnswer?: boolean | undefined; + required?: boolean | undefined; } | null; } | null; activePendingResolvedAnswers: Record | null; @@ -6686,7 +6687,8 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) onDismiss={onDismissActivePendingUserInput} /> {!isChoiceOnlyPendingQuestion || - activePendingProgress?.activeQuestion?.multiSelect ? ( + activePendingProgress?.activeQuestion?.multiSelect || + activePendingProgress?.activeQuestion?.required === false ? (
) : null} - {activePendingProgress?.activeQuestion?.multiSelect ? ( + {activePendingProgress?.activeQuestion?.multiSelect || + activePendingProgress?.activeQuestion?.required === false ? ( 0; + const minSelections = + activeQuestion.minSelections ?? (activeQuestion.maxSelections === 0 ? 0 : 1); + const maxSelections = activeQuestion.maxSelections; + const selectionHint = + activeQuestion.minSelections === undefined && maxSelections === undefined + ? "Select one or more options." + : maxSelections === 0 + ? "Leave all options unselected." + : maxSelections === minSelections + ? `Select ${minSelections} option${minSelections === 1 ? "" : "s"}.` + : maxSelections !== undefined + ? minSelections === 0 + ? `Select up to ${maxSelections} option${maxSelections === 1 ? "" : "s"}.` + : `Select ${minSelections} to ${maxSelections} options.` + : minSelections === 0 + ? "Select any number of options." + : `Select at least ${minSelections} option${minSelections === 1 ? "" : "s"}.`; return (

{activeQuestion.question}

- {activeQuestion.multiSelect ? ( -

Select one or more options.

+ {activeQuestion.multiSelect || activeQuestion.required === false ? ( +

+ {activeQuestion.multiSelect ? selectionHint : null} + {activeQuestion.required === false + ? `${activeQuestion.multiSelect ? " " : ""}You can skip this question.` + : null} +

) : null}
{activeQuestion.options.map((option, index) => { diff --git a/apps/web/src/pendingUserInput.test.ts b/apps/web/src/pendingUserInput.test.ts index 24f1be6fa602..7a79664f3fa9 100644 --- a/apps/web/src/pendingUserInput.test.ts +++ b/apps/web/src/pendingUserInput.test.ts @@ -203,6 +203,121 @@ describe("buildPendingUserInputAnswers", () => { expect(buildPendingUserInputAnswers([singleSelectQuestion], {})).toBeNull(); }); + it("omits unanswered optional questions and permits advancing past them", () => { + const optional = { ...singleSelectQuestion, required: false }; + const drafts = { areas: { selectedOptionValues: ["Server"] } }; + + expect(buildPendingUserInputAnswers([optional, multiSelectQuestion], drafts)).toEqual({ + areas: ["Server"], + }); + expect(buildPendingUserInputAnswers([optional], { scope: { customAnswer: " " } })).toEqual( + {}, + ); + expect( + derivePendingUserInputProgress([optional, multiSelectQuestion], drafts, 0), + ).toMatchObject({ + resolvedAnswer: null, + canAdvance: true, + isComplete: true, + }); + expect( + findFirstUnansweredPendingUserInputQuestionIndex([optional, multiSelectQuestion], {}), + ).toBe(1); + }); + + it.each([ + { attachmentsBlocked: true }, + { attachmentCount: 1, attachmentsBlocked: true }, + { attachmentCount: 1 }, + { customAnswer: "Unlisted answer" }, + { selectedOptionValues: ["unknown"] }, + ])("keeps invalid optional drafts blocking submission: %j", (draft) => { + const question = { ...nativeChoiceQuestion, required: false }; + const drafts = { result: draft }; + + expect(buildPendingUserInputAnswers([question], drafts)).toBeNull(); + expect(derivePendingUserInputProgress([question], drafts, 0)).toMatchObject({ + canAdvance: false, + isComplete: false, + }); + }); + + it.each([0, undefined])( + "submits empty arrays with a zero maximum and minimum %j", + (minSelections) => { + const question = { + ...multiSelectQuestion, + allowCustomAnswer: false, + required: true, + ...(minSelections === undefined ? {} : { minSelections }), + maxSelections: 0, + }; + + expect(resolvePendingUserInputAnswer(question, undefined)).toEqual([]); + expect(buildPendingUserInputAnswers([question], {})).toEqual({ areas: [] }); + expect(derivePendingUserInputProgress([question], {}, 0)).toMatchObject({ + resolvedAnswer: [], + canAdvance: true, + isComplete: true, + }); + expect( + buildPendingUserInputAnswers([question], { areas: { selectedOptionValues: ["Server"] } }), + ).toBeNull(); + expect(buildPendingUserInputAnswers([{ ...question, required: false }], {})).toEqual({}); + }, + ); + + it("requires one selection when no zero maximum is declared", () => { + const question = { ...multiSelectQuestion, required: true, allowCustomAnswer: false }; + + expect(buildPendingUserInputAnswers([question], {})).toBeNull(); + expect(buildPendingUserInputAnswers([{ ...question, maxSelections: 2 }], {})).toBeNull(); + }); + + it("enforces multi-select limits and omits unanswered optional arrays", () => { + const question = { + ...multiSelectQuestion, + options: [...multiSelectQuestion.options, { label: "Mobile", description: "Mobile" }], + allowCustomAnswer: false, + minSelections: 2, + maxSelections: 2, + }; + + expect(buildPendingUserInputAnswers([question], {})).toBeNull(); + expect( + buildPendingUserInputAnswers([question], { areas: { selectedOptionValues: ["Server"] } }), + ).toBeNull(); + expect( + buildPendingUserInputAnswers([question], { + areas: { selectedOptionValues: ["Server", "Web"] }, + }), + ).toEqual({ areas: ["Server", "Web"] }); + expect( + buildPendingUserInputAnswers([question], { + areas: { selectedOptionValues: ["Server", "Web", "Mobile"] }, + }), + ).toBeNull(); + expect(buildPendingUserInputAnswers([{ ...question, required: false }], {})).toEqual({}); + expect( + buildPendingUserInputAnswers([{ ...question, required: false }], { + areas: { selectedOptionValues: ["Server"] }, + }), + ).toBeNull(); + expect( + buildPendingUserInputAnswers([{ ...question, required: false }], { + areas: { selectedOptionValues: ["unknown"] }, + }), + ).toBeNull(); + expect( + buildPendingUserInputAnswers([{ ...question, required: false, minSelections: 0 }], { + areas: { selectedOptionValues: ["unknown"] }, + }), + ).toBeNull(); + expect( + buildPendingUserInputAnswers([multiSelectQuestion], { areas: { attachmentCount: 1 } }), + ).toEqual({ areas: "" }); + }); + it.each([" first\t", ""])("preserves the exact selected option value %j", (value) => { const question = { ...nativeChoiceQuestion, diff --git a/apps/web/src/pendingUserInput.ts b/apps/web/src/pendingUserInput.ts index 538909d13d69..e5db28f9ec47 100644 --- a/apps/web/src/pendingUserInput.ts +++ b/apps/web/src/pendingUserInput.ts @@ -54,11 +54,27 @@ export function resolvePendingUserInputAnswer( (value) => question.options.some((option) => (option.value ?? option.label) === value), ); if (question.multiSelect) { - return selectedOptionValues.length > 0 - ? selectedOptionValues - : question.allowCustomAnswer !== false && (draft?.attachmentCount ?? 0) > 0 + if ( + selectedOptionValues.length === 0 && + ((draft?.selectedOptionValues?.length ?? 0) > 0 || + normalizeDraftAnswer(draft?.customAnswer) !== null || + (question.allowCustomAnswer === false && (draft?.attachmentCount ?? 0) > 0)) + ) + return null; + if ( + selectedOptionValues.length < + (question.minSelections ?? (question.maxSelections === 0 ? 0 : 1)) || + (question.maxSelections !== undefined && selectedOptionValues.length > question.maxSelections) + ) { + return selectedOptionValues.length === 0 && + question.minSelections === undefined && + question.maxSelections === undefined && + question.allowCustomAnswer !== false && + (draft?.attachmentCount ?? 0) > 0 ? "" : null; + } + return selectedOptionValues; } return ( @@ -136,7 +152,16 @@ export function buildPendingUserInputAnswers( const answers: Record = {}; for (const question of questions) { - const answer = resolvePendingUserInputAnswer(question, draftAnswers[question.id]); + const draft = draftAnswers[question.id]; + if ( + question.required === false && + !draft?.attachmentsBlocked && + normalizeDraftAnswer(draft?.customAnswer) === null && + (draft?.selectedOptionValues?.length ?? 0) === 0 && + (draft?.attachmentCount ?? 0) === 0 + ) + continue; + const answer = resolvePendingUserInputAnswer(question, draft); if (answer === null) { return null; } @@ -162,7 +187,7 @@ export function findFirstUnansweredPendingUserInputQuestionIndex( draftAnswers: Record, ): number { const unansweredIndex = questions.findIndex( - (question) => !resolvePendingUserInputAnswer(question, draftAnswers[question.id]), + (question) => buildPendingUserInputAnswers([question], draftAnswers) === null, ); return unansweredIndex === -1 ? Math.max(questions.length - 1, 0) : unansweredIndex; @@ -197,6 +222,8 @@ export function derivePendingUserInputProgress( answeredQuestionCount, isLastQuestion, isComplete: buildPendingUserInputAnswers(questions, draftAnswers) !== null, - canAdvance: resolvedAnswer !== null, + canAdvance: + activeQuestion !== null && + buildPendingUserInputAnswers([activeQuestion], draftAnswers) !== null, }; } diff --git a/packages/contracts/src/orchestrationV2.ts b/packages/contracts/src/orchestrationV2.ts index 675faa4b7f4f..f0933f54d891 100644 --- a/packages/contracts/src/orchestrationV2.ts +++ b/packages/contracts/src/orchestrationV2.ts @@ -1105,6 +1105,8 @@ export const OrchestrationV2UserInputQuestion = Schema.Struct({ multiSelect: Schema.optional(Schema.Boolean), allowCustomAnswer: Schema.optional(Schema.Boolean), required: Schema.optional(Schema.Boolean), + minSelections: Schema.optional(NonNegativeInt), + maxSelections: Schema.optional(NonNegativeInt), }); export type OrchestrationV2UserInputQuestion = typeof OrchestrationV2UserInputQuestion.Type; diff --git a/packages/contracts/src/providerRuntime.ts b/packages/contracts/src/providerRuntime.ts index 5645d230a47a..f5f8491f294a 100644 --- a/packages/contracts/src/providerRuntime.ts +++ b/packages/contracts/src/providerRuntime.ts @@ -491,6 +491,9 @@ export const UserInputQuestion = Schema.Struct({ question: TrimmedNonEmptyStringSchema, options: Schema.Array(UserInputQuestionOption), allowCustomAnswer: Schema.optional(Schema.Boolean), + required: Schema.optional(Schema.Boolean), + minSelections: Schema.optional(NonNegativeInt), + maxSelections: Schema.optional(NonNegativeInt), multiSelect: Schema.optional(Schema.Boolean).pipe( Schema.withConstructorDefault(Effect.succeed(false)), ),