diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e00d0f02d..91f250df5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -186,6 +186,10 @@ - Added `externalThinking` setting for private scratchpad reasoning via the new `think` tool +### Fixed + +- Fixed the ask dialog's multi-select mode dead-ending on Enter: Space toggles options and Enter submits the current selection (matching single-select), instead of Enter silently toggling the focused option with the only submit path hidden behind the Submit tab ([#8252](https://github.com/can1357/oh-my-pi/issues/8252)). + ## [17.2.13] - 2026-08-11 ### Added diff --git a/packages/coding-agent/src/modes/components/ask-dialog.ts b/packages/coding-agent/src/modes/components/ask-dialog.ts index bf4253b3b..f45eae5a9 100644 --- a/packages/coding-agent/src/modes/components/ask-dialog.ts +++ b/packages/coding-agent/src/modes/components/ask-dialog.ts @@ -614,7 +614,9 @@ export class AskDialogComponent implements Component { return `Enter submit · ↑/↓ scroll ·${scroll} ${cancel}`; } const question = this.#questions[this.#currentQuestionIndex()]; - const action = question?.multi ? "Space/Enter toggle · n note" : "Enter select · n note"; + // Enter advances in multi-question dialogs and submits single-question ones. + const enterAction = this.#hasSubmitTab() ? "next" : "submit"; + const action = question?.multi ? `Space toggle · Enter ${enterAction}` : "Enter select · n note"; const tabs = this.#hasSubmitTab() ? " · Tab/←/→" : ""; if (this.#questionCanPage && indicator) { return `${action} · ↑/↓${tabs} · ${cancel} · ${pageKeysLabel()} ${indicator}`; @@ -692,8 +694,14 @@ export class AskDialogComponent implements Component { const option = question.options[rowItem.optionIndex ?? -1]; if (!option) return; if (question.multi) { - // Multi is toggle-only: Enter and Space both toggle, and the - // answer is confirmed from the Submit tab. + if (isEnter) { + // Enter confirms the current selection without toggling the + // focused option; Space toggles. Advances to the next question + // (submitting only for a single-question dialog), matching + // single-select Enter (#8252). + this.#advanceAfterQuestion(); + return; + } if (state.selectedOptions.has(option.label)) { state.selectedOptions.delete(option.label); clearNoteIfRow(state, rowItem.key); diff --git a/packages/coding-agent/src/tools/ask.ts b/packages/coding-agent/src/tools/ask.ts index 0730a207c..2781a24c3 100644 --- a/packages/coding-agent/src/tools/ask.ts +++ b/packages/coding-agent/src/tools/ask.ts @@ -753,7 +753,8 @@ function formatSingleQuestionResponse(result: { : `User added note: ${result.note}`, ); } - return responseParts.length > 0 ? responseParts.join("\n") : "User cancelled the selection"; + if (responseParts.length > 0) return responseParts.join("\n"); + return result.multi ? "User did not select any options" : "User cancelled the selection"; } // ============================================================================= @@ -949,9 +950,15 @@ export class AskTool implements AgentTool { } if (params.questions.length === 1) { const result = results[0]; + // An empty multi-select submission is a valid "select none" + // answer (#8265 review); only a truly empty single-select + // result counts as cancellation. if ( !result || - (!result.timedOut && result.selectedOptions.length === 0 && result.customInput === undefined) + (!result.timedOut && + !result.multi && + result.selectedOptions.length === 0 && + result.customInput === undefined) ) { context.abort(); throw new ToolAbortError("Ask tool was cancelled by the user"); diff --git a/packages/coding-agent/test/modes/components/ask-dialog.test.ts b/packages/coding-agent/test/modes/components/ask-dialog.test.ts index 8d5851ed5..e607450e0 100644 --- a/packages/coding-agent/test/modes/components/ask-dialog.test.ts +++ b/packages/coding-agent/test/modes/components/ask-dialog.test.ts @@ -187,7 +187,7 @@ describe("AskDialogComponent", () => { ]); }); - it("multi-select: Space and Enter both toggle without advancing; Submit tab confirms", () => { + it("multi-select: Space toggles options; Enter submits the current selection", () => { const onSubmit = vi.fn(); const onCancel = vi.fn(); const onPrompt = vi.fn(); @@ -207,22 +207,24 @@ describe("AskDialogComponent", () => { onPrompt, }); - // Space on Option A - toggles without advancing + // Space toggles without submitting. component.handleInput(SPACE); expect(onSubmit).not.toHaveBeenCalled(); - // Down to Option B, Enter - toggles B, still no submit and no movement - component.handleInput(DOWN); - component.handleInput(ENTER); + // Space again toggles the same option back off. + component.handleInput(SPACE); expect(onSubmit).not.toHaveBeenCalled(); - // Tab to the Submit tab (present even for a single multi question), - // Enter confirms the selection. - component.handleInput(TAB); + // Space once more re-selects it. + component.handleInput(SPACE); + expect(onSubmit).not.toHaveBeenCalled(); + + // Enter submits the current selection without toggling the focused + // option (issue #8252). component.handleInput(ENTER); expect(onSubmit).toHaveBeenCalledTimes(1); - expect(onSubmit.mock.calls[0][0].results[0].selectedOptions).toEqual(["Option A", "Option B"]); + expect(onSubmit.mock.calls[0][0].results[0].selectedOptions).toEqual(["Option A"]); }); it("tab-state persistence: answer question 0, Tab forward, Tab back, answer still present", () => { @@ -596,6 +598,69 @@ describe("AskDialogComponent", () => { expect(onSubmit.mock.calls[0][0].results[0].customInput).toBe("custom detail"); }); + it("multi-question, multi-select: Enter on a plain option advances, does not submit", () => { + const onSubmit = vi.fn(); + const onCancel = vi.fn(); + const onPrompt = vi.fn(); + + const questions: ExtensionAskDialogQuestion[] = [ + { + id: "q1", + question: "Choose multiple?", + options: [{ label: "Option A" }, { label: "Option B" }], + multi: true, + }, + { + id: "q2", + question: "Second question?", + options: [{ label: "Option C" }, { label: "Option D" }], + }, + ]; + + const component = new AskDialogComponent(questions, { + onSubmit, + onCancel, + onPrompt, + }); + + // Space toggles Option A; Enter on the plain option row confirms and + // advances to Q2 instead of submitting the whole dialog (#8265 review). + component.handleInput(SPACE); + component.handleInput(ENTER); + expect(onSubmit).not.toHaveBeenCalled(); + + // On Q2: Down to Option D and Enter advances to the Submit tab. + component.handleInput(DOWN); + component.handleInput(ENTER); + expect(onSubmit).not.toHaveBeenCalled(); + + // On the Submit tab Enter submits once with both answers. + component.handleInput(ENTER); + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onSubmit.mock.calls[0][0].results).toEqual([ + { + id: "q1", + question: "Choose multiple?", + options: ["Option A", "Option B"], + multi: true, + selectedOptions: ["Option A"], + customInput: undefined, + note: undefined, + timedOut: undefined, + }, + { + id: "q2", + question: "Second question?", + options: ["Option C", "Option D"], + multi: false, + selectedOptions: ["Option D"], + customInput: undefined, + note: undefined, + timedOut: undefined, + }, + ]); + }); + it("defers a timeout that fires during a pending prompt and honors the resolved custom input", async () => { vi.useFakeTimers(); const deferred = Promise.withResolvers(); @@ -933,7 +998,7 @@ describe("AskDialogComponent", () => { } }); - it("single-question multi-select: Enter toggles instead of submitting", () => { + it("single-question multi-select: Enter submits the current selection immediately", () => { const onSubmit = vi.fn(); const questions: ExtensionAskDialogQuestion[] = [ { @@ -950,42 +1015,34 @@ describe("AskDialogComponent", () => { onPrompt: vi.fn(), }); - // Enter on Option B toggles it — no submit, no tab movement. - component.handleInput(DOWN); - component.handleInput(ENTER); - expect(onSubmit).not.toHaveBeenCalled(); - - // The toggle registered: Submit tab confirms only Option B. - component.handleInput(TAB); - component.handleInput(ENTER); - expect(onSubmit).toHaveBeenCalledTimes(1); - expect(onSubmit.mock.calls[0][0].results[0].selectedOptions).toEqual(["Option B"]); - }); - - it("multi-select: Enter on a checked option toggles it off; empty answer submits from Submit tab", () => { - const onSubmit = vi.fn(); - const questions: ExtensionAskDialogQuestion[] = [ - { - id: "q1", - question: "Choose multiple?", - options: [{ label: "Option A" }, { label: "Option B" }], - multi: true, - }, - ]; - - const component = new AskDialogComponent(questions, { - onSubmit, - onCancel: vi.fn(), - onPrompt: vi.fn(), - }); - - // Space checks Option A, Enter on the same row unchecks it. + // Space selects Option A; Enter submits right away — no need to + // discover the Submit tab (issue #8252). component.handleInput(SPACE); component.handleInput(ENTER); - // Submit tab warns about the unanswered question but still submits. - component.handleInput(TAB); - expect(render(component).toLowerCase()).toContain("unanswered"); + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onSubmit.mock.calls[0][0].results[0].selectedOptions).toEqual(["Option A"]); + }); + + it("multi-select: Enter submits an empty selection instead of dead-ending", () => { + const onSubmit = vi.fn(); + const questions: ExtensionAskDialogQuestion[] = [ + { + id: "q1", + question: "Choose multiple?", + options: [{ label: "Option A" }, { label: "Option B" }], + multi: true, + }, + ]; + + const component = new AskDialogComponent(questions, { + onSubmit, + onCancel: vi.fn(), + onPrompt: vi.fn(), + }); + + // Enter with nothing selected submits the empty selection rather than + // toggling or blocking on the Submit tab. component.handleInput(ENTER); expect(onSubmit).toHaveBeenCalledTimes(1); diff --git a/packages/coding-agent/test/tools/ask.test.ts b/packages/coding-agent/test/tools/ask.test.ts index e1a2c7715..a231a0027 100644 --- a/packages/coding-agent/test/tools/ask.test.ts +++ b/packages/coding-agent/test/tools/ask.test.ts @@ -1607,6 +1607,43 @@ describe("AskTool rich ask dialog", () => { expect(abort).toHaveBeenCalledTimes(1); }); + it("accepts an empty multi-select submission instead of aborting", async () => { + const tool = new AskTool(createSession()); + const abort = vi.fn(); + const askDialog = vi.fn().mockResolvedValue({ + kind: "submit", + results: [ + { + id: "q1", + question: "Choose?", + options: ["A", "B"], + multi: true, + selectedOptions: [], + customInput: undefined, + timedOut: undefined, + }, + ], + }); + const context = createContext({ askDialog, abort }); + + const result = await tool.execute( + "call-empty-multi", + { + questions: [{ id: "q1", question: "Choose?", options: [{ label: "A" }, { label: "B" }], multi: true }], + }, + undefined, + undefined, + context, + ); + + expect(abort).not.toHaveBeenCalled(); + expect(result.details?.selectedOptions).toEqual([]); + expect(result.content[0]?.type).toBe("text"); + if (result.content[0]?.type === "text") { + expect(stripAnsi(result.content[0].text)).toContain("User did not select any options"); + } + }); + it("returns chat redirect result when askDialog returns kind chat", async () => { const tool = new AskTool(createSession()); const abort = vi.fn();