From bcf7e10e6e3b3cac2d0368d500dd8695518368e9 Mon Sep 17 00:00:00 2001 From: murashit Date: Fri, 26 Jun 2026 17:29:05 +0900 Subject: [PATCH] Move chat server request protocol behind app-server boundary --- biome.jsonc | 7 +- ...-app-server-protocol-boundary-imports.grit | 6 +- src/app-server/protocol/server-requests.ts | 394 +++++++++++++----- .../inbound => app-server}/route-scope.ts | 0 .../server-requests.ts} | 22 +- .../chat/app-server/inbound/handler.ts | 14 +- .../inbound/notification-routing.ts | 2 +- .../generated-import-boundary.test.ts | 2 +- .../chat/protocol/inbound/routing.test.ts | 2 +- .../server-requests/mcp-elicitation.test.ts | 63 +++ tests/scripts/grit-policy.test.mjs | 27 +- 11 files changed, 394 insertions(+), 145 deletions(-) rename src/{features/chat/app-server/inbound => app-server}/route-scope.ts (100%) rename src/{features/chat/app-server/inbound/server-requests/adapter.ts => app-server/server-requests.ts} (96%) diff --git a/biome.jsonc b/biome.jsonc index c6ce4419..1811121b 100644 --- a/biome.jsonc +++ b/biome.jsonc @@ -17,12 +17,7 @@ }, { "path": "./scripts/lint/no-generated-app-server-boundary-imports.grit", - "includes": [ - "**/src/**/*.ts", - "**/src/**/*.tsx", - "!**/src/app-server/connection/**", - "!**/src/app-server/protocol/server-requests.ts" - ] + "includes": ["**/src/**/*.ts", "**/src/**/*.tsx", "!**/src/app-server/connection/**"] }, // Shared source layering boundaries. diff --git a/scripts/lint/no-app-server-protocol-boundary-imports.grit b/scripts/lint/no-app-server-protocol-boundary-imports.grit index d67676c8..4cdec994 100644 --- a/scripts/lint/no-app-server-protocol-boundary-imports.grit +++ b/scripts/lint/no-app-server-protocol-boundary-imports.grit @@ -16,11 +16,7 @@ js_module_reference() as $stmt where { not { $filename <: r".*/src/features/chat/app-server/mappers/message-stream/turn-items\.ts$", $source <: r"^[\"'](?:(?:\.\./)+app-server/protocol/turn|src/app-server/protocol/turn)(?:/.*)?[\"']$" - }, - not { - $filename <: r".*/src/features/chat/app-server/inbound/server-requests/.*", - $source <: r"^[\"'](?:(?:\.\./)+app-server/protocol/server-requests|src/app-server/protocol/server-requests)(?:/.*)?[\"']$" } }, - register_diagnostic(span=$stmt, message="Source modules outside root src/app-server must use domain models and app-server services instead of app-server protocol modules. Chat turn-item conversion may consume turn protocol, and chat request handling may consume server request protocol at their app-server boundaries; feature state and UI must use Panel-owned models.", severity="error") + register_diagnostic(span=$stmt, message="Source modules outside root src/app-server must use domain models and app-server services instead of app-server protocol modules. Chat turn-item conversion may consume turn protocol at its app-server boundary; feature state and UI must use Panel-owned models.", severity="error") } diff --git a/src/app-server/protocol/server-requests.ts b/src/app-server/protocol/server-requests.ts index 0cd153bd..6d2c432c 100644 --- a/src/app-server/protocol/server-requests.ts +++ b/src/app-server/protocol/server-requests.ts @@ -10,24 +10,103 @@ import type { PendingMcpElicitationField, PendingMcpElicitationOption, PendingUserInput, + PendingUserInputQuestion, } from "../../domain/pending-requests/model"; -import type { ServerRequest } from "../../generated/app-server/ServerRequest"; import { pathRelativeToRoot } from "../../shared/path/file-paths"; import { jsonPreview } from "../../shared/text/preview"; -type AppServerRequestByMethod = Extract; - interface AppServerGrantedPermissionProfile { network?: unknown; fileSystem?: unknown; } type SimpleApprovalDecision = "accept" | "acceptForSession" | "decline" | "cancel"; -type CommandApprovalRequest = AppServerRequestByMethod<"item/commandExecution/requestApproval">; -type CommandApprovalParams = CommandApprovalRequest["params"]; -type CommandApprovalDecision = NonNullable[number]; -type FileChangeApprovalParams = AppServerRequestByMethod<"item/fileChange/requestApproval">["params"]; -type PermissionsApprovalParams = AppServerRequestByMethod<"item/permissions/requestApproval">["params"]; +type CommandApprovalDecision = + | SimpleApprovalDecision + | { acceptWithExecpolicyAmendment: unknown } + | { applyNetworkPolicyAmendment: { network_policy_amendment: { action?: unknown; host?: unknown } } }; + +type AppServerRequest = { + method: string; + id: PendingApproval["requestId"]; + params: unknown; +}; + +interface CommandApprovalParams { + command?: unknown; + cwd?: unknown; + turnId?: unknown; + reason?: unknown; + networkApprovalContext?: unknown; + commandActions?: unknown; + additionalPermissions?: unknown; + proposedExecpolicyAmendment?: unknown; + proposedNetworkPolicyAmendments?: unknown; + availableDecisions?: unknown; +} + +interface FileChangeApprovalParams { + turnId?: unknown; + reason?: unknown; + grantRoot?: unknown; +} + +interface PermissionsApprovalParams { + cwd?: unknown; + turnId?: unknown; + reason?: unknown; + environmentId?: unknown; + permissions?: unknown; +} + +type AppServerMcpElicitationPrimitiveSchema = + | { + type: "boolean"; + title?: unknown; + description?: unknown; + default?: unknown; + } + | { + type: "number" | "integer"; + title?: unknown; + description?: unknown; + default?: unknown; + minimum?: unknown; + maximum?: unknown; + } + | { + type: "array"; + title?: unknown; + description?: unknown; + default?: unknown; + minItems?: unknown; + maxItems?: unknown; + items?: unknown; + } + | { + type: "string"; + title?: unknown; + description?: unknown; + default?: unknown; + format?: unknown; + minLength?: unknown; + maxLength?: unknown; + oneOf?: unknown; + enum?: unknown; + enumNames?: unknown; + }; + +interface NormalizedMcpElicitationParams { + threadId: string; + turnId: string | null; + serverName: string; + mode: "form" | "url"; + message: string; + meta: unknown; + requestedSchema?: unknown; + url: string; + elicitationId: string; +} export type AppServerApprovalResponse = | { decision: CommandApprovalDecision } @@ -37,27 +116,20 @@ export interface AppServerUserInputResponse { answers: Record; } -type AppServerMcpElicitationRequest = AppServerRequestByMethod<"mcpServer/elicitation/request">; -type AppServerMcpElicitationFormParams = Extract; -type AppServerMcpElicitationSchema = NonNullable; -type AppServerMcpElicitationPrimitiveSchema = NonNullable< - Extract["properties"][string] ->; - export interface AppServerMcpElicitationResponse { action: McpElicitationAction; content: unknown; _meta: unknown; } -export function appServerApprovalRequest(request: ServerRequest): PendingApproval | null { +export function appServerApprovalRequest(request: AppServerRequest): PendingApproval | null { switch (request.method) { case "item/commandExecution/requestApproval": - return commandApprovalRequest(request.id, request.params); + return commandApprovalRequest(request.id, asRecordOrEmpty(request.params)); case "item/fileChange/requestApproval": - return fileChangeApprovalRequest(request.id, request.params); + return fileChangeApprovalRequest(request.id, asRecordOrEmpty(request.params)); case "item/permissions/requestApproval": - return permissionsApprovalRequest(request.id, request.params); + return permissionsApprovalRequest(request.id, asRecordOrEmpty(request.params)); default: return null; } @@ -68,11 +140,13 @@ export function appServerApprovalResponse(approval: PendingApproval, action: App return approvalResponseForIntent(approval, action); } -export function appServerUserInputRequest(request: ServerRequest): PendingUserInput | null { +export function appServerUserInputRequest(request: AppServerRequest): PendingUserInput | null { if (request.method !== "item/tool/requestUserInput") return null; + const params = pendingUserInputParams(request.params); + if (!params) return null; return { requestId: request.id, - params: request.params, + params, }; } @@ -92,9 +166,10 @@ export function appServerUserInputResponse( }; } -export function appServerMcpElicitationRequest(request: ServerRequest): PendingMcpElicitation | null { +export function appServerMcpElicitationRequest(request: AppServerRequest): PendingMcpElicitation | null { if (request.method !== "mcpServer/elicitation/request") return null; - const params = request.params; + const params = mcpElicitationParams(request.params); + if (!params) return null; if (params.mode === "url") { return { requestId: request.id, @@ -104,7 +179,7 @@ export function appServerMcpElicitationRequest(request: ServerRequest): PendingM serverName: params.serverName, mode: "url", message: params.message, - meta: params._meta, + meta: params.meta, url: params.url, elicitationId: params.elicitationId, }, @@ -118,7 +193,7 @@ export function appServerMcpElicitationRequest(request: ServerRequest): PendingM serverName: params.serverName, mode: "form", message: params.message, - meta: params._meta, + meta: params.meta, fields: mcpElicitationFieldsFromRequestedSchema(params.requestedSchema), }, }; @@ -140,7 +215,7 @@ function commandApprovalRequest(requestId: PendingApproval["requestId"], params: return { requestId, kind: "command", - turnId: params.turnId, + turnId: nullableString(params.turnId), title: "Command approval", summary: approvalSummary(params.reason, params.command, "Command execution requested."), resultSummary: approvalResultSummary(params.reason, params.command, "Command execution requested."), @@ -152,11 +227,11 @@ function commandApprovalRequest(requestId: PendingApproval["requestId"], params: function fileChangeApprovalRequest(requestId: PendingApproval["requestId"], params: FileChangeApprovalParams): PendingApproval { const reason = params.reason ?? null; - const grantRoot = params.grantRoot ?? null; + const grantRoot = nonEmptyString(params.grantRoot); return { requestId, kind: "fileChange", - turnId: params.turnId, + turnId: nullableString(params.turnId), title: "File change approval", summary: approvalSummary(reason, grantRoot ? `grant root: ${grantRoot}` : null, "Allow file changes?"), resultSummary: approvalResultSummary(reason, grantRoot ? `grant root: ${grantRoot}` : null, "Allow file changes?"), @@ -168,6 +243,8 @@ function fileChangeApprovalRequest(requestId: PendingApproval["requestId"], para function permissionsApprovalRequest(requestId: PendingApproval["requestId"], params: PermissionsApprovalParams): PendingApproval { const details: ApprovalDetailRow[] = []; + const cwd = stringValue(params.cwd); + const cwdSummary = cwd ? `cwd: ${cwd}` : null; addOptional(details, "reason", params.reason); addOptional(details, "cwd", params.cwd); addOptional(details, "environment", params.environmentId); @@ -175,10 +252,10 @@ function permissionsApprovalRequest(requestId: PendingApproval["requestId"], par return { requestId, kind: "permission", - turnId: params.turnId, + turnId: nullableString(params.turnId), title: "Permission approval", - summary: approvalSummary(params.reason, `cwd: ${params.cwd}`, "Permission change requested."), - resultSummary: approvalResultSummary(params.reason, `cwd: ${params.cwd}`, "Permission change requested."), + summary: approvalSummary(params.reason, cwdSummary, "Permission change requested."), + resultSummary: approvalResultSummary(params.reason, cwdSummary, "Permission change requested."), details, responses: { accept: { scope: "turn", permissions: grantedPermissions(params.permissions) }, @@ -224,10 +301,11 @@ function fileChangeDecision(intent: ApprovalActionIntent): SimpleApprovalDecisio return "decline"; } -function grantedPermissions(requested: { network?: unknown; fileSystem?: unknown }): AppServerGrantedPermissionProfile { +function grantedPermissions(requested: unknown): AppServerGrantedPermissionProfile { + const profile = asRecordOrNull(requested); const granted: AppServerGrantedPermissionProfile = {}; - if (requested.network) granted.network = requested.network; - if (requested.fileSystem) granted.fileSystem = requested.fileSystem; + if (profile?.["network"]) granted.network = profile["network"]; + if (profile?.["fileSystem"]) granted.fileSystem = profile["fileSystem"]; return granted; } @@ -246,11 +324,12 @@ interface NetworkApprovalContext { function commandApprovalDetails(params: CommandApprovalParams): ApprovalDetailRow[] { const rows: ApprovalDetailRow[] = []; + const cwd = nullableString(params.cwd); addOptional(rows, "reason", params.reason); addOptional(rows, "command", params.command); addOptional(rows, "cwd", params.cwd); addOptional(rows, "network", networkApprovalContextLabel(params.networkApprovalContext)); - rows.push(...commandActionRows(params.commandActions, params.cwd)); + rows.push(...commandActionRows(params.commandActions, cwd)); rows.push(...prefixedPermissionRows("additional", params.additionalPermissions)); addOptional(rows, "future command rule", params.proposedExecpolicyAmendment); addOptional(rows, "future network rules", networkPolicyAmendmentsLabel(params.proposedNetworkPolicyAmendments)); @@ -258,8 +337,9 @@ function commandApprovalDetails(params: CommandApprovalParams): ApprovalDetailRo } function commandApprovalActionOptions(decisions: CommandApprovalParams["availableDecisions"]): PendingApprovalOption[] | null { - if (!decisions || decisions.length === 0) return null; - return decisions.map((decision, index) => { + const availableDecisions = commandApprovalDecisions(decisions); + if (availableDecisions.length === 0) return null; + return availableDecisions.map((decision, index) => { const intent = commandDecisionIntent(decision); return { id: `approval-option:${String(index)}:${commandDecisionKey(decision)}`, @@ -274,6 +354,20 @@ function commandApprovalActionOptions(decisions: CommandApprovalParams["availabl }); } +function commandApprovalDecisions(value: unknown): CommandApprovalDecision[] { + if (!Array.isArray(value)) return []; + return value.filter(isCommandApprovalDecision); +} + +function isCommandApprovalDecision(value: unknown): value is CommandApprovalDecision { + if (typeof value === "string") return true; + const decision = asRecordOrNull(value); + if (!decision) return false; + if ("acceptWithExecpolicyAmendment" in decision) return true; + const networkDecision = asRecordOrNull(decision["applyNetworkPolicyAmendment"]); + return Boolean(asRecordOrNull(networkDecision?.["network_policy_amendment"])); +} + function commandDecisionIntent(decision: CommandApprovalDecision): ApprovalActionIntent { if (typeof decision === "string") return simpleCommandDecisionIntent(decision); if ("acceptWithExecpolicyAmendment" in decision) return "accept-session"; @@ -313,7 +407,7 @@ function commandDecisionKey(decision: CommandApprovalDecision): string { if ("applyNetworkPolicyAmendment" in decision) { const amendment = decision.applyNetworkPolicyAmendment.network_policy_amendment; const host = amendment.host; - return `applyNetworkPolicyAmendment:${amendment.action}:${typeof host === "string" ? host : ""}`; + return `applyNetworkPolicyAmendment:${stringValue(amendment.action)}:${typeof host === "string" ? host : ""}`; } return "unknown"; } @@ -462,29 +556,91 @@ function nonEmptyString(value: unknown): string | null { return typeof value === "string" && value.trim().length > 0 ? value.trim() : null; } +function nullableString(value: unknown): string | null { + return typeof value === "string" ? value : null; +} + function asRecordOrNull(value: unknown): Record | null { return value && typeof value === "object" ? (value as Record) : null; } +function asRecordOrEmpty(value: unknown): Record { + return asRecordOrNull(value) ?? {}; +} + +function pendingUserInputParams(value: unknown): PendingUserInput["params"] | null { + const params = asRecordOrNull(value); + const questions = params?.["questions"]; + if (!params || !Array.isArray(questions)) return null; + return { + threadId: stringValue(params["threadId"]), + turnId: stringValue(params["turnId"]), + itemId: stringValue(params["itemId"]), + questions: questions.map(pendingUserInputQuestion), + autoResolutionMs: numberOrNull(params["autoResolutionMs"]), + }; +} + +function pendingUserInputQuestion(value: unknown): PendingUserInputQuestion { + const question = asRecordOrEmpty(value); + const options = question["options"]; + return { + id: stringValue(question["id"]), + header: stringValue(question["header"]), + question: stringValue(question["question"]), + isOther: question["isOther"] === true, + isSecret: question["isSecret"] === true, + options: Array.isArray(options) + ? options.map((option) => { + const record = asRecordOrEmpty(option); + return { label: stringValue(record["label"]), description: stringValue(record["description"]) }; + }) + : null, + }; +} + +function mcpElicitationParams(value: unknown): NormalizedMcpElicitationParams | null { + const params = asRecordOrNull(value); + if (!params) return null; + const mode = params["mode"]; + if (mode !== "form" && mode !== "url") return null; + return { + threadId: stringValue(params["threadId"]), + turnId: nullableString(params["turnId"]), + serverName: stringValue(params["serverName"]), + mode, + message: stringValue(params["message"]), + meta: params["_meta"] ?? null, + requestedSchema: params["requestedSchema"], + url: stringValue(params["url"]), + elicitationId: stringValue(params["elicitationId"]), + }; +} + +function numberOrNull(value: unknown): number | null { + return typeof value === "number" && Number.isFinite(value) ? value : null; +} + function mcpElicitationFieldsFromRequestedSchema(schema: unknown): PendingMcpElicitationField[] { - if (!isMcpElicitationObjectSchema(schema)) return []; - return mcpElicitationFieldsFromSchema(schema.properties, new Set(schema.required ?? [])); -} - -function isMcpElicitationObjectSchema(schema: unknown): schema is Extract { - return Boolean(schema && typeof schema === "object" && "properties" in schema && typeof schema.properties === "object"); -} - -function mcpElicitationFieldsFromSchema( - properties: Record, - required: ReadonlySet, -): PendingMcpElicitationField[] { - return Object.entries(properties).flatMap(([id, schema]) => { - if (!schema) return []; - return [mcpElicitationFieldFromSchema(id, schema, required.has(id))]; + const record = asRecordOrNull(schema); + const properties = asRecordOrNull(record?.["properties"]); + if (!properties) return []; + const required = new Set( + Array.isArray(record?.["required"]) ? record["required"].filter((item): item is string => typeof item === "string") : [], + ); + return Object.entries(properties).flatMap(([id, fieldSchema]) => { + if (!isMcpElicitationPrimitiveSchema(fieldSchema)) return []; + return [mcpElicitationFieldFromSchema(id, fieldSchema, required.has(id))]; }); } +function isMcpElicitationPrimitiveSchema(schema: unknown): schema is AppServerMcpElicitationPrimitiveSchema { + const record = asRecordOrNull(schema); + if (!record) return false; + const type = record["type"]; + return type === "boolean" || type === "number" || type === "integer" || type === "array" || type === "string"; +} + function mcpElicitationFieldFromSchema( id: string, schema: AppServerMcpElicitationPrimitiveSchema, @@ -492,68 +648,100 @@ function mcpElicitationFieldFromSchema( ): PendingMcpElicitationField { const base = { id, - title: schema.title ?? id, - description: schema.description ?? null, + title: nonEmptyString(schema.title) ?? id, + description: nonEmptyString(schema.description), required, }; - if (schema.type === "boolean") { - return { ...base, type: "boolean", defaultValue: schema.default ?? false }; + switch (schema.type) { + case "boolean": + return { ...base, type: "boolean", defaultValue: schema.default === true }; + case "number": + case "integer": + return { + ...base, + type: schema.type, + minimum: numberOrNull(schema.minimum), + maximum: numberOrNull(schema.maximum), + defaultValue: numberOrNull(schema.default), + }; + case "array": + return { + ...base, + type: "multi-select", + options: multiSelectOptions(schema), + minItems: bigintToNumberOrNull(schema.minItems), + maxItems: bigintToNumberOrNull(schema.maxItems), + defaultValue: stringArrayOrEmpty(schema.default), + }; + case "string": { + const options = singleSelectOptions(schema); + if (options.length > 0) { + return { + ...base, + type: "single-select", + options, + defaultValue: typeof schema.default === "string" ? schema.default : (options[0]?.value ?? ""), + }; + } + return { + ...base, + type: "string", + format: nullableString(schema.format), + minLength: numberOrNull(schema.minLength), + maxLength: numberOrNull(schema.maxLength), + defaultValue: typeof schema.default === "string" ? schema.default : "", + }; + } } - if (schema.type === "number" || schema.type === "integer") { - return { - ...base, - type: schema.type, - minimum: schema.minimum ?? null, - maximum: schema.maximum ?? null, - defaultValue: schema.default ?? null, - }; - } - if (schema.type === "array") { - return { - ...base, - type: "multi-select", - options: multiSelectOptions(schema), - minItems: bigintToNumberOrNull(schema.minItems), - maxItems: bigintToNumberOrNull(schema.maxItems), - defaultValue: schema.default ?? [], - }; - } - if ("oneOf" in schema || "enum" in schema) { - const options = singleSelectOptions(schema); - return { - ...base, - type: "single-select", - options, - defaultValue: schema.default ?? options[0]?.value ?? "", - }; - } - const stringSchema = schema as Extract; - return { - ...base, - type: "string", - format: "format" in stringSchema ? (stringSchema.format ?? null) : null, - minLength: "minLength" in stringSchema ? (stringSchema.minLength ?? null) : null, - maxLength: "maxLength" in stringSchema ? (stringSchema.maxLength ?? null) : null, - defaultValue: typeof stringSchema.default === "string" ? stringSchema.default : "", - }; } function singleSelectOptions(schema: Extract): PendingMcpElicitationOption[] { - if ("oneOf" in schema) return schema.oneOf.map((option) => ({ value: option.const, label: option.title })); - if ("enum" in schema) { - const enumNames = "enumNames" in schema ? schema.enumNames : undefined; - if (enumNames) return schema.enum.map((value, index) => ({ value, label: enumNames[index] ?? value })); - return schema.enum.map((value) => ({ value, label: value })); + if (Array.isArray(schema.oneOf)) { + const options = schema.oneOf.flatMap((option) => { + const selected = selectOption(option); + return selected ? [selected] : []; + }); + if (options.length > 0) return options; } - return []; + return enumOptions(schema.enum, schema.enumNames); } function multiSelectOptions(schema: Extract): PendingMcpElicitationOption[] { - if ("anyOf" in schema.items) return schema.items.anyOf.map((option) => ({ value: option.const, label: option.title })); - return schema.items.enum.map((value) => ({ value, label: value })); + const items = asRecordOrNull(schema.items); + const anyOf = items?.["anyOf"]; + if (Array.isArray(anyOf)) { + const options = anyOf.flatMap((option) => { + const selected = selectOption(option); + return selected ? [selected] : []; + }); + if (options.length > 0) return options; + } + return enumOptions(items?.["enum"], null); } -function bigintToNumberOrNull(value: bigint | number | undefined): number | null { +function stringArrayOrEmpty(value: unknown): readonly string[] { + if (!Array.isArray(value)) return []; + if (!value.every((item) => typeof item === "string")) return []; + return value; +} + +function enumOptions(values: unknown, labels: unknown): PendingMcpElicitationOption[] { + if (!Array.isArray(values)) return []; + const labelValues = Array.isArray(labels) ? labels : []; + return values.flatMap((value, index) => { + if (typeof value !== "string") return []; + return [{ value, label: nonEmptyString(labelValues[index]) ?? value }]; + }); +} + +function selectOption(value: unknown): PendingMcpElicitationOption | null { + const record = asRecordOrNull(value); + const optionValue = nullableString(record?.["const"]); + if (optionValue === null) return null; + return { value: optionValue, label: nonEmptyString(record?.["title"]) ?? optionValue }; +} + +function bigintToNumberOrNull(value: unknown): number | null { if (typeof value === "bigint") return Number(value); return typeof value === "number" && Number.isFinite(value) ? value : null; } diff --git a/src/features/chat/app-server/inbound/route-scope.ts b/src/app-server/route-scope.ts similarity index 100% rename from src/features/chat/app-server/inbound/route-scope.ts rename to src/app-server/route-scope.ts diff --git a/src/features/chat/app-server/inbound/server-requests/adapter.ts b/src/app-server/server-requests.ts similarity index 96% rename from src/features/chat/app-server/inbound/server-requests/adapter.ts rename to src/app-server/server-requests.ts index 42deeeef..bb849d59 100644 --- a/src/features/chat/app-server/inbound/server-requests/adapter.ts +++ b/src/app-server/server-requests.ts @@ -1,12 +1,3 @@ -import type { ServerRequest } from "../../../../../app-server/connection/rpc-messages"; -import { - appServerApprovalRequest, - appServerApprovalResponse, - appServerMcpElicitationRequest, - appServerMcpElicitationResponse, - appServerUserInputRequest, - appServerUserInputResponse, -} from "../../../../../app-server/protocol/server-requests"; import type { ApprovalAction, McpElicitationAction, @@ -14,14 +5,23 @@ import type { PendingApproval, PendingMcpElicitation, PendingUserInput, -} from "../../../../../domain/pending-requests/model"; +} from "../domain/pending-requests/model"; +import type { ServerRequest } from "./connection/rpc-messages"; +import { + appServerApprovalRequest, + appServerApprovalResponse, + appServerMcpElicitationRequest, + appServerMcpElicitationResponse, + appServerUserInputRequest, + appServerUserInputResponse, +} from "./protocol/server-requests"; import { type ActiveRouteScope, fallbackMessageScope, isMessageScopeInActiveRouteScope, isTurnScopedMessageForIdleActiveThread, type MessageScope, -} from "../route-scope"; +} from "./route-scope"; export type ServerRequestRoute = | { kind: "approval"; request: ServerRequest; approval: PendingApproval } diff --git a/src/features/chat/app-server/inbound/handler.ts b/src/features/chat/app-server/inbound/handler.ts index 327d76cb..7377146d 100644 --- a/src/features/chat/app-server/inbound/handler.ts +++ b/src/features/chat/app-server/inbound/handler.ts @@ -1,4 +1,11 @@ import type { RequestId, ServerNotification, ServerRequest } from "../../../../app-server/connection/rpc-messages"; +import { + routeServerRequest, + serverRequestApprovalResponse, + serverRequestCurrentTimeResponse, + serverRequestMcpElicitationResponse, + serverRequestUserInputResponse, +} from "../../../../app-server/server-requests"; import { type ApprovalAction, contentForPendingMcpElicitation, @@ -24,13 +31,6 @@ import { import type { AppServerResourceEvent } from "../actions/metadata"; import { classifyAppServerLog } from "./app-server-logs"; import { type ChatNotificationEffect, planChatNotification } from "./notification-plan"; -import { - routeServerRequest, - serverRequestApprovalResponse, - serverRequestCurrentTimeResponse, - serverRequestMcpElicitationResponse, - serverRequestUserInputResponse, -} from "./server-requests/adapter"; function cannotSendApprovalResponseMessage(): string { return "Could not send approval response because Codex app-server is not connected."; diff --git a/src/features/chat/app-server/inbound/notification-routing.ts b/src/features/chat/app-server/inbound/notification-routing.ts index df53c757..1bb2bd00 100644 --- a/src/features/chat/app-server/inbound/notification-routing.ts +++ b/src/features/chat/app-server/inbound/notification-routing.ts @@ -5,7 +5,7 @@ import { isMessageScopeInActiveRouteScope, isTurnScopedMessageForIdleActiveThread, type MessageScope, -} from "./route-scope"; +} from "../../../../app-server/route-scope"; type ServerNotificationMethod = ServerNotification["method"]; type RoutedNotification = Extract; diff --git a/tests/app-server/generated-import-boundary.test.ts b/tests/app-server/generated-import-boundary.test.ts index 95fd7236..38c81d45 100644 --- a/tests/app-server/generated-import-boundary.test.ts +++ b/tests/app-server/generated-import-boundary.test.ts @@ -63,5 +63,5 @@ function slashPath(value: string): string { } function allowsGeneratedAppServerImport(relativePath: string): boolean { - return relativePath.startsWith("src/app-server/connection/") || relativePath === "src/app-server/protocol/server-requests.ts"; + return relativePath.startsWith("src/app-server/connection/"); } diff --git a/tests/features/chat/protocol/inbound/routing.test.ts b/tests/features/chat/protocol/inbound/routing.test.ts index 13b6be6f..3e48e36d 100644 --- a/tests/features/chat/protocol/inbound/routing.test.ts +++ b/tests/features/chat/protocol/inbound/routing.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import type { ServerNotification, ServerRequest } from "../../../../../src/app-server/connection/rpc-messages"; +import { routeServerRequest } from "../../../../../src/app-server/server-requests"; import { PLANNED_SERVER_NOTIFICATION_METHODS_BY_ROUTE_KIND, planChatNotification, @@ -8,7 +9,6 @@ import { ROUTED_SERVER_NOTIFICATION_METHODS_BY_ROUTE_KIND, routeServerNotification, } from "../../../../../src/features/chat/app-server/inbound/notification-routing"; -import { routeServerRequest } from "../../../../../src/features/chat/app-server/inbound/server-requests/adapter"; import { chatStateFixture, chatStateWith } from "../../support/state"; const activeScope = { activeThreadId: "thread-active", activeTurnId: "turn-active" }; diff --git a/tests/features/chat/protocol/server-requests/mcp-elicitation.test.ts b/tests/features/chat/protocol/server-requests/mcp-elicitation.test.ts index 825bb292..de7c39d4 100644 --- a/tests/features/chat/protocol/server-requests/mcp-elicitation.test.ts +++ b/tests/features/chat/protocol/server-requests/mcp-elicitation.test.ts @@ -90,6 +90,37 @@ describe("MCP elicitation request model", () => { }); expect(contentForPendingMcpElicitation(input, new Map())).toBeNull(); }); + + it("normalizes malformed schema fields without leaking invalid values", () => { + const input = expectPresent(toPendingMcpElicitation(malformedSchemaRequest())); + if (input.params.mode !== "form") throw new Error("Expected form mode"); + + expect(input.params.fields.map((field) => field.id)).toEqual(["badDefault", "brokenSelect", "enumSelect", "labels"]); + expect(input.params.fields.find((field) => field.id === "badDefault")).toMatchObject({ + type: "boolean", + defaultValue: false, + }); + expect(input.params.fields.find((field) => field.id === "brokenSelect")).toMatchObject({ + type: "string", + format: null, + defaultValue: "", + }); + expect(input.params.fields.find((field) => field.id === "enumSelect")).toMatchObject({ + type: "single-select", + options: [ + { value: "low", label: "Low" }, + { value: "high", label: "High" }, + ], + }); + expect(input.params.fields.find((field) => field.id === "labels")).toMatchObject({ + type: "multi-select", + defaultValue: [], + options: [ + { value: "bug", label: "Bug" }, + { value: "docs", label: "docs" }, + ], + }); + }); }); function formRequest(): ServerRequest { @@ -155,3 +186,35 @@ function urlRequest(): ServerRequest { }, }; } + +function malformedSchemaRequest(): ServerRequest { + return { + id: 44, + method: "mcpServer/elicitation/request", + params: { + threadId: "thread", + turnId: null, + serverName: "github", + mode: "form", + _meta: null, + message: "Provide issue details", + requestedSchema: { + type: "object", + properties: { + unsupported: { type: "object", title: "Unsupported" }, + badDefault: { type: "boolean", title: "Bad default", default: "yes" }, + brokenSelect: { type: "string", title: "Broken select", oneOf: { const: "low", title: "Low" }, format: 1 }, + enumSelect: { type: "string", title: "Enum select", enum: ["low", 1, "high"], enumNames: ["Low", "One", "High"] }, + labels: { + type: "array", + title: "Labels", + items: { + anyOf: [{ const: "bug", title: "Bug" }, { const: 1, title: "One" }, { const: "docs" }], + }, + default: ["bug", 1], + }, + }, + }, + }, + } as unknown as ServerRequest; +} diff --git a/tests/scripts/grit-policy.test.mjs b/tests/scripts/grit-policy.test.mjs index b41c66e6..f11e180d 100644 --- a/tests/scripts/grit-policy.test.mjs +++ b/tests/scripts/grit-policy.test.mjs @@ -15,7 +15,7 @@ const projectPluginByName = new Map( }), ); const APP_SERVER_PROTOCOL_BOUNDARY_MESSAGE = - "Source modules outside root src/app-server must use domain models and app-server services instead of app-server protocol modules. Chat turn-item conversion may consume turn protocol, and chat request handling may consume server request protocol at their app-server boundaries; feature state and UI must use Panel-owned models."; + "Source modules outside root src/app-server must use domain models and app-server services instead of app-server protocol modules. Chat turn-item conversion may consume turn protocol at its app-server boundary; feature state and UI must use Panel-owned models."; let appServerBoundaryPolicyReportPromise; let renderingAndCssPolicyReportPromise; @@ -314,7 +314,10 @@ export function timestamp(): number { expect(pluginMessages(report, "src/app-server/protocol/turn.ts")).toEqual([ "Keep generated app-server types behind src/app-server adapters; expose Panel-owned models outside raw app-server boundaries.", ]); - expect(pluginDiagnostics(report, "src/app-server/protocol/server-requests.ts")).toEqual([]); + expect(pluginMessages(report, "src/app-server/protocol/server-requests.ts")).toEqual([ + "Keep generated app-server types behind src/app-server adapters; expose Panel-owned models outside raw app-server boundaries.", + "Keep generated app-server types behind src/app-server adapters; expose Panel-owned models outside raw app-server boundaries.", + ]); }); it("keeps TSX files in rendering-owned source folders", async () => { @@ -351,7 +354,8 @@ export function timestamp(): number { APP_SERVER_PROTOCOL_BOUNDARY_MESSAGE, APP_SERVER_PROTOCOL_BOUNDARY_MESSAGE, ]); - expect(pluginMessages(report, "src/features/chat/app-server/inbound/server-requests/adapter.ts")).toEqual([ + expect(pluginMessages(report, "src/features/chat/app-server/inbound/server-request-protocol-leak.ts")).toEqual([ + APP_SERVER_PROTOCOL_BOUNDARY_MESSAGE, APP_SERVER_PROTOCOL_BOUNDARY_MESSAGE, ]); }); @@ -398,10 +402,14 @@ export function timestamp(): number { "Do not import app-server connection internals from this module. Keep connection usage at app-server adapters.", ]); expect(pluginDiagnostics(report, "src/app-server/services/catalog.ts")).toEqual([]); - expect(pluginMessages(report, "src/domain/connection-client.ts")).toEqual([ - "Domain modules must stay pure; outer layers may depend on domain, not the reverse.", - "Do not import app-server connection internals from this module. Keep connection usage at app-server adapters.", - ]); + const domainConnectionMessages = pluginMessages(report, "src/domain/connection-client.ts"); + expect(domainConnectionMessages).toHaveLength(2); + expect(domainConnectionMessages).toEqual( + expect.arrayContaining([ + "Domain modules must stay pure; outer layers may depend on domain, not the reverse.", + "Do not import app-server connection internals from this module. Keep connection usage at app-server adapters.", + ]), + ); expect(pluginMessages(report, "src/shared/connection-client.ts")).toEqual([ "Do not import app-server connection internals from this module. Keep connection usage at app-server adapters.", ]); @@ -661,7 +669,7 @@ export type Item = TurnItem; `.trimStart(), ); await writeFile( - path.join(cwd, "src/features/chat/app-server/inbound/server-requests/adapter.ts"), + path.join(cwd, "src/features/chat/app-server/inbound/server-request-protocol-leak.ts"), ` import { appServerUserInputResponse } from "../../../../../app-server/protocol/server-requests"; @@ -825,7 +833,7 @@ export type AppServerThreadResumeClient = Pick; "src/features/chat/ui/protocol-leak.tsx", "src/features/chat/app-server/inbound/app-server-logs.ts", "src/features/chat/app-server/mappers/message-stream/turn-items.ts", - "src/features/chat/app-server/inbound/server-requests/adapter.ts", + "src/features/chat/app-server/inbound/server-request-protocol-leak.ts", "src/domain/threads/model.ts", "src/domain/threads/format.ts", "src/features/chat/domain/message-stream/selectors.ts", @@ -858,7 +866,6 @@ async function tempBiomeWorkspace(plugins) { await mkdir(path.join(cwd, "src/features/chat/application/threads"), { recursive: true }); await mkdir(path.join(cwd, "src/features/chat/application/pending-requests"), { recursive: true }); await mkdir(path.join(cwd, "src/features/chat/app-server/inbound"), { recursive: true }); - await mkdir(path.join(cwd, "src/features/chat/app-server/inbound/server-requests"), { recursive: true }); await mkdir(path.join(cwd, "src/features/chat/app-server/mappers/message-stream"), { recursive: true }); await mkdir(path.join(cwd, "src/features/chat/host"), { recursive: true }); await mkdir(path.join(cwd, "src/features/chat/panel"), { recursive: true });