fix(omp): preserve described select options (#3628)

* fix(omp): preserve described select options

* test(omp): assert exact option arrays instead of toMatchObject

toMatchObject ignores extra/missing keys, so the described-select and
malformed-metadata tests passed even with option-index misalignment or
an unfiltered whitespace-only description. Switched to toStrictEqual
on the options array so both are actually caught.
This commit is contained in:
Joe Shull
2026-09-25 10:25:19 +02:00
committed by GitHub
parent a048094da4
commit 90d978ab68
4 changed files with 141 additions and 10 deletions
@@ -7,6 +7,7 @@ import type { OmpAgentMessage } from "./rpc-types.js";
import type { OmpNoTurnScheduler, OmpProviderIdleScheduler } from "./agent.js";
import type { OmpUsagePollScheduler } from "./usage-poller.js";
import { resolveOmpProviderParams } from "./provider-config.js";
import { OmpRuntimeEventSchema } from "./rpc-types.js";
import { OmpHarness } from "./test-utils/omp-harness.js";
const TURN_LIFECYCLE_EVENTS = new Set<AgentStreamEvent["type"]>([
@@ -42,7 +43,6 @@ test("OMP ready timeout defaults to 20 seconds and RPC timeout overrides both",
rpcTimeoutMs: 90_000,
});
});
class ManualIdleScheduler implements OmpProviderIdleScheduler {
private readonly retries: Array<() => void> = [];
private readonly waiters: Array<{ count: number; resolve: () => void }> = [];
@@ -582,6 +582,106 @@ describe("OMP agent client and session", () => {
]);
});
test("maps legacy select options without descriptions and preserves ordinary responses", async () => {
const omp = new OmpHarness();
await omp.start();
omp.emit({
type: "extension_ui_request",
id: "select-legacy",
method: "select",
title: "Choose",
options: ["Approve", "Deny"],
});
expect(omp.pendingPermissions()[0]?.input).toMatchObject({
questions: [{ options: [{ label: "Approve" }, { label: "Deny" }] }],
});
await omp.respondToPermission("select-legacy", {
behavior: "allow",
updatedInput: { answers: { Response: "Deny" } },
});
expect(omp.extensionUiResponses()).toContainEqual({
id: "select-legacy",
response: { value: "Deny" },
});
});
test("maps described and mixed select metadata by option index", async () => {
const omp = new OmpHarness();
await omp.start();
omp.emit({
type: "extension_ui_request",
id: "select-described",
method: "select",
title: "Choose",
options: ["First", "Second", "Third"],
optionDetails: [{ description: "First detail" }, {}, { description: " \t" }],
});
expect(omp.pendingPermissions()[0]?.input?.questions?.[0]?.options).toStrictEqual([
{ label: "First", description: "First detail" },
{ label: "Second" },
{ label: "Third" },
]);
});
test("accepts malformed or misaligned optional metadata and falls back to labels", async () => {
const omp = new OmpHarness();
await omp.start();
const parsed = OmpRuntimeEventSchema.safeParse({
type: "extension_ui_request",
id: "select-malformed",
method: "select",
options: ["First", "Second"],
optionDetails: [{ description: 42 }, { description: "\n\t" }, { description: "extra" }],
});
expect(parsed.success).toBe(true);
if (!parsed.success) throw new Error("Expected malformed metadata event to parse");
omp.emit(parsed.data);
expect(omp.pendingPermissions()[0]?.input?.questions?.[0]?.options).toStrictEqual([
{ label: "First" },
{ label: "Second" },
]);
});
test("preserves combined selection descriptions and freeform sentinel behavior", async () => {
const omp = new OmpHarness();
await omp.start();
omp.emit({
type: "tool_execution_start",
toolCallId: "ask-user-1",
toolName: "ask_user",
args: { allowComment: true, allowFreeform: true, allowMultiple: false },
});
omp.emit({
type: "extension_ui_request",
id: "select-combined",
method: "select",
title: "Choose",
options: ["First", "✏️ Type custom response..."],
optionDetails: [{ description: "First detail" }, { description: "ignored" }],
});
const combinedInput = omp.pendingPermissions()[0]?.input;
expect(combinedInput?.questions?.[0]?.options).toStrictEqual([
{ label: "First", description: "First detail" },
]);
expect(combinedInput).toMatchObject({
questions: [{ allowOther: true }, { header: "Comment" }],
});
await omp.respondToPermission("select-combined", {
behavior: "allow",
updatedInput: { answers: { Response: "custom", Comment: "note" } },
});
expect(omp.extensionUiResponses()).toContainEqual({
id: "select-combined",
response: { value: "✏️ Type custom response..." },
});
});
test("exposes OMP modes and commands through the domain session", async () => {
const omp = new OmpHarness();
omp.queueCommands([{ name: "review", description: "Review changes", source: "skill" }]);
@@ -568,6 +568,24 @@ function getInputQuestionTitle(title: string | undefined, placeholder: string |
return "Optional response";
}
interface OmpSelectOption {
label: string;
description?: string;
}
function readSelectOptions(options: unknown, optionDetails: unknown): OmpSelectOption[] {
const labels = readStringArray(options);
const details = Array.isArray(optionDetails) ? optionDetails : [];
return labels.map((label, index) => {
const detail = details[index];
const description =
isRecord(detail) && typeof detail.description === "string" && detail.description.trim() !== ""
? detail.description
: undefined;
return description === undefined ? { label } : { label, description };
});
}
function readStringArray(value: unknown): string[] {
return Array.isArray(value)
? value.filter((item): item is string => typeof item === "string")
@@ -586,7 +604,7 @@ function mapExtensionUiRequestToPermission(
const label = options.label ?? "OMP";
switch (event.method) {
case "select": {
const selectOptions = readStringArray(event.options);
const selectOptions = readSelectOptions(event.options, event.optionDetails);
if (options.combineOptionalComment) {
return buildCombinedAskUserQuestionPermission(event, {
provider,
@@ -633,7 +651,7 @@ function mapExtensionUiRequestToPermission(
question: [optionalString(event.title), optionalString(event.message)]
.filter(Boolean)
.join("\n\n"),
options: ["Yes", "No"],
options: [{ label: "Yes" }, { label: "No" }],
multiSelect: false,
});
default:
@@ -678,7 +696,7 @@ function buildExtensionUiQuestionPermission(
provider: AgentProvider;
label: string;
question: string;
options: string[];
options: OmpSelectOption[];
multiSelect: boolean;
placeholder?: string;
allowEmpty?: boolean;
@@ -696,7 +714,10 @@ function buildExtensionUiQuestionPermission(
{
question: input.question,
header: QUESTION_RESPONSE_HEADER,
options: input.options.map((label) => ({ label })),
options: input.options.map((option) => ({
label: option.label,
...(option.description === undefined ? {} : { description: option.description }),
})),
multiSelect: input.multiSelect,
...(input.placeholder ? { placeholder: input.placeholder } : {}),
...(input.allowEmpty ? { allowEmpty: true } : {}),
@@ -717,11 +738,13 @@ function buildCombinedAskUserQuestionPermission(
provider: AgentProvider;
label: string;
question: string;
options: string[];
options: OmpSelectOption[];
allowFreeform: boolean;
},
): AgentPermissionRequest {
const visibleOptions = input.options.filter((option) => !isOmpAskUserFreeformOption(option));
const visibleOptions = input.options.filter(
(option) => !isOmpAskUserFreeformOption(option.label),
);
const allowOther = input.allowFreeform || visibleOptions.length !== input.options.length;
return {
id: event.id,
@@ -734,7 +757,10 @@ function buildCombinedAskUserQuestionPermission(
{
question: input.question,
header: QUESTION_RESPONSE_HEADER,
options: visibleOptions.map((label) => ({ label })),
options: visibleOptions.map((option) => ({
label: option.label,
...(option.description === undefined ? {} : { description: option.description }),
})),
multiSelect: false,
...(allowOther ? { allowOther: true } : {}),
},
@@ -753,7 +779,7 @@ function buildCombinedAskUserQuestionPermission(
answerHeader: QUESTION_RESPONSE_HEADER,
commentHeader: QUESTION_COMMENT_HEADER,
combinedAskUser: COMBINED_ASK_USER_METADATA,
selectOptions: visibleOptions,
selectOptions: visibleOptions.map((option) => option.label),
...(allowOther ? { freeformSentinel: OMP_ASK_USER_FREEFORM_SENTINEL } : {}),
},
};
@@ -444,6 +444,7 @@ const OmpExtensionUiRequestSchema = z
title: z.string().optional(),
message: z.string().optional(),
options: z.array(z.string()).optional(),
optionDetails: z.unknown().optional(),
placeholder: z.string().optional(),
url: z.string().optional(),
launchUrl: z.string().optional(),
@@ -19,7 +19,7 @@ import {
type OmpProviderIdleScheduler,
} from "../agent.js";
import type { OmpUsagePollScheduler } from "../usage-poller.js";
import type { OmpAgentMessage, OmpRpcSlashCommand } from "../rpc-types.js";
import type { OmpAgentMessage, OmpRpcSlashCommand, OmpRuntimeEvent } from "../rpc-types.js";
import { FakeOmp } from "./fake-omp.js";
const CWD = "/tmp/paseo-omp-agent-test";
@@ -422,6 +422,10 @@ export class OmpHarness {
this.omp.latestSession().requestToolApproval(input);
}
emit(event: OmpRuntimeEvent): void {
this.omp.latestSession().emit(event);
}
pendingPermissions() {
return this.requireSession().getPendingPermissions();
}