fix(coding-agent): restored ask tool timeout
Ensured ask tool timeouts abort stalled UI selectors and return the recommended option instead of hanging. Added regression coverage for selectors that never settle. Fixes #4995
This commit is contained in:
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed the ask tool timeout so it auto-selects the recommended option even when the UI selector does not settle on its own. ([#4995](https://github.com/can1357/oh-my-pi/issues/4995))
|
||||
|
||||
## [16.3.15] - 2026-07-09
|
||||
|
||||
### Changed
|
||||
|
||||
@@ -432,10 +432,16 @@ async function askSingleQuestion(
|
||||
const helpText = navigation
|
||||
? "up/down navigate enter select ←/→ question esc cancel"
|
||||
: "up/down navigate enter select esc cancel";
|
||||
const timeoutController = typeof timeout === "number" && timeout > 0 ? new AbortController() : undefined;
|
||||
const dialogSignal =
|
||||
signal && timeoutController
|
||||
? AbortSignal.any([signal, timeoutController.signal])
|
||||
: (timeoutController?.signal ?? signal);
|
||||
let timeoutId: NodeJS.Timeout | undefined;
|
||||
const dialogOptions = {
|
||||
initialIndex,
|
||||
timeout,
|
||||
signal,
|
||||
signal: dialogSignal,
|
||||
outline: true,
|
||||
onTimeout,
|
||||
helpText,
|
||||
@@ -454,18 +460,33 @@ async function askSingleQuestion(
|
||||
: undefined,
|
||||
};
|
||||
const startMs = Date.now();
|
||||
const choice = signal
|
||||
? await untilAborted(signal, () => ui.select(prompt, optionsToShow, dialogOptions))
|
||||
: await ui.select(prompt, optionsToShow, dialogOptions);
|
||||
if (!timeoutTriggered && choice === undefined && typeof timeout === "number") {
|
||||
// Fallback for UI surfaces that enforce `timeout` without invoking
|
||||
// `onTimeout`: their auto-cancel resolves right at the deadline. A
|
||||
// cancel arriving well past the deadline is a deliberate user Esc on
|
||||
// a surface that kept the dialog open — keep treating it as a cancel.
|
||||
const elapsed = Date.now() - startMs;
|
||||
timeoutTriggered = elapsed >= timeout && elapsed <= timeout + TIMEOUT_DETECTION_TOLERANCE_MS;
|
||||
if (timeoutController && typeof timeout === "number") {
|
||||
timeoutId = setTimeout(() => {
|
||||
timeoutTriggered = true;
|
||||
timeoutController.abort();
|
||||
}, timeout);
|
||||
}
|
||||
try {
|
||||
const choice = dialogSignal
|
||||
? await untilAborted(dialogSignal, () => ui.select(prompt, optionsToShow, dialogOptions))
|
||||
: await ui.select(prompt, optionsToShow, dialogOptions);
|
||||
if (!timeoutTriggered && choice === undefined && typeof timeout === "number") {
|
||||
// Fallback for UI surfaces that enforce `timeout` without invoking
|
||||
// `onTimeout`: their auto-cancel resolves right at the deadline. A
|
||||
// cancel arriving well past the deadline is a deliberate user Esc on
|
||||
// a surface that kept the dialog open — keep treating it as a cancel.
|
||||
const elapsed = Date.now() - startMs;
|
||||
timeoutTriggered = elapsed >= timeout && elapsed <= timeout + TIMEOUT_DETECTION_TOLERANCE_MS;
|
||||
}
|
||||
return { choice, timedOut: timeoutTriggered, navigation: navigationAction };
|
||||
} catch (error) {
|
||||
if (timeoutTriggered && error instanceof Error && error.name === "AbortError") {
|
||||
return { choice: undefined, timedOut: true, navigation: navigationAction };
|
||||
}
|
||||
throw error;
|
||||
} finally {
|
||||
clearTimeout(timeoutId);
|
||||
}
|
||||
return { choice, timedOut: timeoutTriggered, navigation: navigationAction };
|
||||
};
|
||||
|
||||
const promptForCustomInput = async (
|
||||
|
||||
@@ -0,0 +1,91 @@
|
||||
import { afterEach, beforeAll, describe, expect, it, vi } from "bun:test";
|
||||
import type { AgentToolContext, AgentToolResult } from "@oh-my-pi/pi-agent-core";
|
||||
import { getThemeByName, setThemeInstance } from "../src/modes/theme/theme";
|
||||
import type { ToolSession } from "../src/tools";
|
||||
import { AskTool, type AskToolDetails } from "../src/tools/ask";
|
||||
|
||||
type AskExecutionResult = AgentToolResult<AskToolDetails>;
|
||||
|
||||
async function drainMicrotasks(): Promise<void> {
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
}
|
||||
|
||||
function createAskTool(): AskTool {
|
||||
return new AskTool({
|
||||
hasUI: true,
|
||||
settings: {
|
||||
get(key: string): unknown {
|
||||
if (key === "ask.timeout") return 0.01;
|
||||
if (key === "ask.notify") return "off";
|
||||
if (key === "speech.enabled") return false;
|
||||
return undefined;
|
||||
},
|
||||
},
|
||||
getPlanModeState: () => ({ enabled: false }),
|
||||
} as unknown as ToolSession);
|
||||
}
|
||||
|
||||
describe("AskTool timeout", () => {
|
||||
beforeAll(async () => {
|
||||
const loaded = await getThemeByName("dark");
|
||||
if (!loaded) throw new Error("theme unavailable");
|
||||
setThemeInstance(loaded);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("auto-selects the recommended option when the selector does not settle", async () => {
|
||||
vi.useFakeTimers();
|
||||
const select = vi.fn(() => new Promise<string | undefined>(() => {}));
|
||||
const abort = vi.fn();
|
||||
const context = {
|
||||
hasUI: true,
|
||||
ui: {
|
||||
select,
|
||||
editor: vi.fn(),
|
||||
},
|
||||
abort,
|
||||
} as unknown as AgentToolContext;
|
||||
let result: AskExecutionResult | undefined;
|
||||
let rejection: unknown;
|
||||
|
||||
void createAskTool()
|
||||
.execute(
|
||||
"ask-timeout",
|
||||
{
|
||||
questions: [
|
||||
{
|
||||
id: "db",
|
||||
question: "Which database?",
|
||||
options: [{ label: "SQLite" }, { label: "Postgres" }],
|
||||
recommended: 1,
|
||||
},
|
||||
],
|
||||
},
|
||||
undefined,
|
||||
undefined,
|
||||
context,
|
||||
)
|
||||
.then(
|
||||
value => {
|
||||
result = value;
|
||||
},
|
||||
error => {
|
||||
rejection = error;
|
||||
},
|
||||
);
|
||||
|
||||
await drainMicrotasks();
|
||||
vi.advanceTimersByTime(10);
|
||||
await drainMicrotasks();
|
||||
|
||||
expect(rejection).toBeUndefined();
|
||||
expect(result?.details?.selectedOptions).toEqual(["Postgres"]);
|
||||
expect(result?.details?.timedOut).toBe(true);
|
||||
expect(abort).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user