diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index 1f84f853..6e7236b8 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -29,7 +29,9 @@ type AcpBackedMcpElicitationParams = Extract< { mode: "form" } | { mode: "url" } >; -const USER_INPUT_OTHER_FIELD_SUFFIX = "__other"; +const USER_INPUT_NOTE_FIELD_SUFFIX = "_note"; +const USER_INPUT_OTHER_OPTION = "None of the above"; +const USER_INPUT_NOTE_PREFIX = "user_note: "; function normalizeElicitationSchema(value: unknown): acp.ElicitationSchema { const normalized = normalizeElicitationSchemaValue(value); @@ -109,17 +111,14 @@ function elicitationResponseMeta( return Object.keys(meta).length === 0 ? null : meta; } -function userInputOtherFieldId(questionId: string, questionIds: Set): string { - const base = `${questionId}${USER_INPUT_OTHER_FIELD_SUFFIX}`; - if (!questionIds.has(base)) { - return base; - } - +function userInputNoteFieldId(questionId: string, questionIds: ReadonlySet): string { + const base = `${questionId}${USER_INPUT_NOTE_FIELD_SUFFIX}`; + let fieldId = base; let index = 1; - while (questionIds.has(`${base}${index}`)) { - index += 1; + while (questionIds.has(fieldId)) { + fieldId = `${base}${index++}`; } - return `${base}${index}`; + return fieldId; } function userInputResponseValue( @@ -400,8 +399,8 @@ export class CodexElicitationHandler implements ElicitationHandler { const hasOptions = options.length > 0; const hasOtherAnswer = question.isOther && hasOptions; const base = { - title: question.header || question.id, - description: question.question, + title: question.question || question.header || question.id, + ...(question.header ? { description: question.header } : {}), _meta: { codex: { isOther: question.isOther, @@ -409,32 +408,36 @@ export class CodexElicitationHandler implements ElicitationHandler { }, }, }; - if (!hasOtherAnswer) { - required.push(question.id); - } + required.push(question.id); properties[question.id] = hasOptions ? { ...base, type: "string", - oneOf: options.map(option => ({ - const: option.label, - title: option.label, - description: option.description, - })), + oneOf: [ + ...options.map(option => ({ + const: option.label, + title: option.label, + ...(option.description ? { description: option.description } : {}), + })), + ...(hasOtherAnswer && !options.some(option => option.label === USER_INPUT_OTHER_OPTION) ? [{ + const: USER_INPUT_OTHER_OPTION, + title: USER_INPUT_OTHER_OPTION, + description: "Provide a different answer in the note field.", + }] : []), + ], } : { ...base, type: "string", }; if (hasOtherAnswer) { - properties[userInputOtherFieldId(question.id, questionIds)] = { + properties[userInputNoteFieldId(question.id, questionIds)] = { type: "string", - title: "Other", - description: "Type your own answer instead of choosing an option above.", + title: "Additional answer or note", _meta: { codex: { questionId: question.id, - isOtherAnswer: true, + role: "user_note", isSecret: question.isSecret, }, }, @@ -442,14 +445,11 @@ export class CodexElicitationHandler implements ElicitationHandler { } } - const firstQuestion = params.questions[0]; return { sessionId: params.threadId, toolCallId: params.itemId, mode: "form", - message: params.questions.length === 1 && firstQuestion - ? firstQuestion.question - : "Input requested", + message: "Codex needs your input to continue.", requestedSchema: { type: "object", properties, @@ -523,17 +523,23 @@ export class CodexElicitationHandler implements ElicitationHandler { const content = contentRecord(response.content); const questionIds = new Set(params.questions.map(question => question.id)); for (const question of params.questions) { - const value = question.isOther && question.options != null && question.options.length > 0 - ? userInputResponseValue(content, userInputOtherFieldId(question.id, questionIds)) - ?? userInputResponseValue(content, question.id) - : userInputResponseValue(content, question.id); - if (value === undefined) { + const answerValues: string[] = []; + const value = userInputResponseValue(content, question.id); + if (value !== undefined) { + answerValues.push(...(Array.isArray(value) ? value.map(String) : [String(value)])); + } + if (question.isOther && question.options != null && question.options.length > 0) { + const note = userInputResponseValue(content, userInputNoteFieldId(question.id, questionIds)); + if (note !== undefined) { + const notes = Array.isArray(note) ? note : [note]; + answerValues.push(...notes.map(item => `${USER_INPUT_NOTE_PREFIX}${String(item).trim()}`)); + } + } + if (answerValues.length === 0) { continue; } answers[question.id] = { - answers: Array.isArray(value) - ? value.map(String) - : [String(value)], + answers: answerValues, }; } return { answers }; diff --git a/src/__tests__/CodexACPAgent/data/elicitation-user-input-existing-other.json b/src/__tests__/CodexACPAgent/data/elicitation-user-input-existing-other.json new file mode 100644 index 00000000..ecbbcd22 --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/elicitation-user-input-existing-other.json @@ -0,0 +1,58 @@ +{ + "method": "createElicitation", + "args": [ + { + "sessionId": "test-session-id", + "toolCallId": "request-user-input-1", + "mode": "form", + "message": "Codex needs your input to continue.", + "requestedSchema": { + "type": "object", + "properties": { + "next_step": { + "title": "What should I do next?", + "description": "Next step", + "_meta": { + "codex": { + "isOther": true, + "isSecret": false + } + }, + "type": "string", + "oneOf": [ + { + "const": "Run tests", + "title": "Run tests", + "description": "Run the focused test suite." + }, + { + "const": "None of the above", + "title": "None of the above", + "description": "Use a different approach." + } + ] + }, + "next_step_note": { + "type": "string", + "title": "Additional answer or note", + "_meta": { + "codex": { + "questionId": "next_step", + "role": "user_note", + "isSecret": false + } + } + } + }, + "required": [ + "next_step" + ] + }, + "_meta": { + "codex": { + "autoResolutionMs": null + } + } + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/elicitation-user-input-note-collision-after.json b/src/__tests__/CodexACPAgent/data/elicitation-user-input-note-collision-after.json new file mode 100644 index 00000000..9ad0ed87 --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/elicitation-user-input-note-collision-after.json @@ -0,0 +1,87 @@ +{ + "method": "createElicitation", + "args": [ + { + "sessionId": "test-session-id", + "toolCallId": "request-user-input-1", + "mode": "form", + "message": "Codex needs your input to continue.", + "requestedSchema": { + "type": "object", + "properties": { + "choice": { + "title": "What should I do next?", + "description": "Next step", + "_meta": { + "codex": { + "isOther": true, + "isSecret": true + } + }, + "type": "string", + "oneOf": [ + { + "const": "Run tests", + "title": "Run tests", + "description": "Run the focused test suite." + }, + { + "const": "Stop", + "title": "Stop", + "description": "Stop and report current status." + }, + { + "const": "None of the above", + "title": "None of the above", + "description": "Provide a different answer in the note field." + } + ] + }, + "choice_note2": { + "type": "string", + "title": "Additional answer or note", + "_meta": { + "codex": { + "questionId": "choice", + "role": "user_note", + "isSecret": true + } + } + }, + "choice_note": { + "title": "Which constraints should I follow?", + "description": "Constraints", + "_meta": { + "codex": { + "isOther": false, + "isSecret": false + } + }, + "type": "string" + }, + "choice_note1": { + "title": "What private context should I consider?", + "description": "Private", + "_meta": { + "codex": { + "isOther": false, + "isSecret": true + } + }, + "type": "string" + } + }, + "required": [ + "choice", + "choice_note", + "choice_note1" + ] + }, + "_meta": { + "codex": { + "autoResolutionMs": null + } + } + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/elicitation-user-input-note-collision-before.json b/src/__tests__/CodexACPAgent/data/elicitation-user-input-note-collision-before.json new file mode 100644 index 00000000..ae234c3c --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/elicitation-user-input-note-collision-before.json @@ -0,0 +1,87 @@ +{ + "method": "createElicitation", + "args": [ + { + "sessionId": "test-session-id", + "toolCallId": "request-user-input-1", + "mode": "form", + "message": "Codex needs your input to continue.", + "requestedSchema": { + "type": "object", + "properties": { + "choice_note": { + "title": "Which constraints should I follow?", + "description": "Constraints", + "_meta": { + "codex": { + "isOther": false, + "isSecret": false + } + }, + "type": "string" + }, + "choice_note1": { + "title": "What private context should I consider?", + "description": "Private", + "_meta": { + "codex": { + "isOther": false, + "isSecret": true + } + }, + "type": "string" + }, + "choice": { + "title": "What should I do next?", + "description": "Next step", + "_meta": { + "codex": { + "isOther": true, + "isSecret": true + } + }, + "type": "string", + "oneOf": [ + { + "const": "Run tests", + "title": "Run tests", + "description": "Run the focused test suite." + }, + { + "const": "Stop", + "title": "Stop", + "description": "Stop and report current status." + }, + { + "const": "None of the above", + "title": "None of the above", + "description": "Provide a different answer in the note field." + } + ] + }, + "choice_note2": { + "type": "string", + "title": "Additional answer or note", + "_meta": { + "codex": { + "questionId": "choice", + "role": "user_note", + "isSecret": true + } + } + } + }, + "required": [ + "choice_note", + "choice_note1", + "choice" + ] + }, + "_meta": { + "codex": { + "autoResolutionMs": null + } + } + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts index 3c899fa3..b6882667 100644 --- a/src/__tests__/CodexACPAgent/elicitation-events.test.ts +++ b/src/__tests__/CodexACPAgent/elicitation-events.test.ts @@ -900,47 +900,70 @@ describe('Elicitation Events', () => { }, }); - const [elicitationEvent] = fixture.getAcpConnectionEvents(['_meta']); - expect(elicitationEvent).toMatchObject({ + expect(fixture.getAcpConnectionEvents([])).toEqual([{ method: 'createElicitation', args: [{ sessionId, toolCallId: 'request-user-input-1', mode: 'form', - message: 'Input requested', + message: 'Codex needs your input to continue.', requestedSchema: { type: 'object', - required: ['notes'], + properties: { + next_step: { + type: 'string', + title: 'What should I do next?', + description: 'Next step', + oneOf: [ + { const: 'Run tests', title: 'Run tests', description: 'Run the focused test suite.' }, + { const: 'Stop', title: 'Stop', description: 'Stop and report current status.' }, + { + const: 'None of the above', + title: 'None of the above', + description: 'Provide a different answer in the note field.', + }, + ], + _meta: { + codex: { isOther: true, isSecret: false }, + }, + }, + next_step_note: { + type: 'string', + title: 'Additional answer or note', + _meta: { + codex: { questionId: 'next_step', role: 'user_note', isSecret: false }, + }, + }, + notes: { + type: 'string', + title: 'Any extra instructions?', + description: 'Notes', + _meta: { + codex: { isOther: false, isSecret: false }, + }, + }, + }, + required: ['next_step', 'notes'], + }, + _meta: { + codex: { autoResolutionMs: 60000 }, }, }], - }); - expect(elicitationEvent!.args[0].requestedSchema.properties.next_step.oneOf).toEqual([ - { const: 'Run tests', title: 'Run tests', description: 'Run the focused test suite.' }, - { const: 'Stop', title: 'Stop', description: 'Stop and report current status.' }, - ]); - expect(elicitationEvent!.args[0].requestedSchema.properties.next_step__other).toMatchObject({ - type: 'string', - title: 'Other', - }); - expect(elicitationEvent!.args[0].requestedSchema.properties.notes).toMatchObject({ - type: 'string', - title: 'Notes', - description: 'Any extra instructions?', - }); + }]); completeTurn(); await promptPromise; }); - it('should prefer free-form Other answers over fixed choices', async () => { + it('should return the Other choice and its note using Codex answer conventions', async () => { const { promptPromise, completeTurn } = await setupSessionWithPendingPromptAndCapabilities({ elicitation: { form: {} }, }); fixture.setElicitationResponse({ action: 'accept', content: { - next_step: 'Run tests', - next_step__other: 'Inspect flaky logs', + next_step: 'None of the above', + next_step_note: 'Inspect flaky logs', }, }); @@ -966,7 +989,7 @@ describe('Elicitation Events', () => { const response = await fixture.sendServerRequest('item/tool/requestUserInput', params); expect(response).toEqual({ answers: { - next_step: { answers: ['Inspect flaky logs'] }, + next_step: { answers: ['None of the above', 'user_note: Inspect flaky logs'] }, }, }); @@ -974,6 +997,120 @@ describe('Elicitation Events', () => { await promptPromise; }); + it.each(['before', 'after'] as const)( + 'should preserve questions with note field IDs when they appear %s the choice', + async (order) => { + const { promptPromise, completeTurn } = await setupSessionWithPendingPromptAndCapabilities({ + elicitation: { form: {} }, + }); + fixture.setElicitationResponse({ + action: 'accept', + content: { + choice: 'Run tests', + choice_note: 'Follow project conventions', + choice_note1: 'Private context', + choice_note2: ' Run the focused suite first ', + }, + }); + + const choice: ToolRequestUserInputParams['questions'][number] = { + id: 'choice', + header: 'Next step', + question: 'What should I do next?', + isOther: true, + isSecret: true, + options: [ + { label: 'Run tests', description: 'Run the focused test suite.' }, + { label: 'Stop', description: 'Stop and report current status.' }, + ], + }; + const otherQuestions: ToolRequestUserInputParams['questions'] = [ + { + id: 'choice_note', + header: 'Constraints', + question: 'Which constraints should I follow?', + isOther: false, + isSecret: false, + options: null, + }, + { + id: 'choice_note1', + header: 'Private', + question: 'What private context should I consider?', + isOther: false, + isSecret: true, + options: null, + }, + ]; + const params: ToolRequestUserInputParams = { + threadId: sessionId, + turnId: 'turn-1', + itemId: 'request-user-input-1', + autoResolutionMs: null, + isBlocking: true, + questions: order === 'before' ? [...otherQuestions, choice] : [choice, ...otherQuestions], + }; + + const response = await fixture.sendServerRequest('item/tool/requestUserInput', params); + expect(response).toEqual({ + answers: { + choice: { answers: ['Run tests', 'user_note: Run the focused suite first'] }, + choice_note: { answers: ['Follow project conventions'] }, + choice_note1: { answers: ['Private context'] }, + }, + }); + await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( + `data/elicitation-user-input-note-collision-${order}.json`, + ); + + completeTurn(); + await promptPromise; + }, + ); + + it('should keep an existing None of the above choice selectable without duplicating it', async () => { + const { promptPromise, completeTurn } = await setupSessionWithPendingPromptAndCapabilities({ + elicitation: { form: {} }, + }); + fixture.setElicitationResponse({ + action: 'accept', + content: { next_step: 'None of the above' }, + }); + + const params: ToolRequestUserInputParams = { + threadId: sessionId, + turnId: 'turn-1', + itemId: 'request-user-input-1', + autoResolutionMs: null, + isBlocking: true, + questions: [{ + id: 'next_step', + header: 'Next step', + question: 'What should I do next?', + isOther: true, + isSecret: false, + options: [ + { label: 'Run tests', description: 'Run the focused test suite.' }, + { label: 'None of the above', description: 'Use a different approach.' }, + ], + }], + }; + + const response = await fixture.sendServerRequest('item/tool/requestUserInput', params); + expect(response).toEqual({ + answers: { next_step: { answers: ['None of the above'] } }, + }); + const [elicitationEvent] = fixture.getAcpConnectionEvents([]); + const options = elicitationEvent!.args[0].requestedSchema.properties.next_step.oneOf; + expect(options.filter((option: { const: string }) => option.const === 'None of the above')).toHaveLength(1); + await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( + 'data/elicitation-user-input-existing-other.json', + ); + + completeTurn(); + await promptPromise; + }); + it('should auto-resolve request_user_input when the client does not answer in time', async () => { const { promptPromise, completeTurn } = await setupSessionWithPendingPromptAndCapabilities({ elicitation: { form: {} },