From bdc9cdd9b67914b28e580ed97ed9188af1686388 Mon Sep 17 00:00:00 2001 From: Tal Zaccai Date: Tue, 28 Jul 2026 17:30:41 -0700 Subject: [PATCH 1/7] Generate real REST fetch handlers for OpenAPI-discovered agents Adds a buildRestHandler generator (the REST twin of the existing CLI handler generator) so agents scaffolded from a JSON OpenAPI 3 spec via parseOpenApiSpec produce an executeAction that makes a real HTTP call instead of returning a not-yet-implemented stub. - discoveryHandler.ts: capture ApiSurface.baseUrl (absolute/relative/ server-variable resolution, guarded, http/https only) and DiscoveredParameter.in; merge path-item-level params with operation-level params (op wins); skip \ path items/params (v1 inline-only scope). - restHandlerTemplate.ts + restHandler.template: per-action switch cases split params by .in (path/query/body; header/cookie unsupported v1), slash-normalized URL joining that preserves the server path, camelCase<->wire-name bracket-access fallback, scalar query/body serialization, 30s timeout, truncated/pretty-printed response handling. - scaffolderHandler.ts: guarded routing to buildRestHandler right after the CLI-detection block, only for schema-grammar/external-api patterns with a resolved baseUrl and HTTP actions; other patterns (websocket-bridge, native-platform, ...) keep their own builders. Tests (test/*.spec.ts, run via \ pm test\ / \ sx --test\): extraction+merge+baseUrl resolution from a checked-in OpenAPI fixture, approval-spread preservation of baseUrl/in, routing guard behavior, tsc type-check of generated output, and executeAction correctness against a local node:http server (GET path param, GET query param, POST JSON body). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ts/packages/agents/onboarding/package.json | 3 + .../src/discovery/discoveryHandler.ts | 210 ++++++++++++---- .../src/scaffolder/restHandler.template | 105 ++++++++ .../src/scaffolder/restHandlerTemplate.ts | 201 ++++++++++++++++ .../src/scaffolder/scaffolderHandler.ts | 24 +- .../test/apiSurfacePreservation.spec.ts | 105 ++++++++ .../openapi/book-api-absolute-server.json | 25 ++ .../test/fixtures/openapi/book-api.json | 85 +++++++ .../onboarding/test/openApiExtraction.spec.ts | 226 ++++++++++++++++++ .../test/restHandlerCompile.spec.ts | 143 +++++++++++ .../test/restHandlerExecution.spec.ts | 212 ++++++++++++++++ .../onboarding/test/restRouting.spec.ts | 98 ++++++++ ts/pnpm-lock.yaml | 3 + 13 files changed, 1390 insertions(+), 50 deletions(-) create mode 100644 ts/packages/agents/onboarding/src/scaffolder/restHandler.template create mode 100644 ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts create mode 100644 ts/packages/agents/onboarding/test/apiSurfacePreservation.spec.ts create mode 100644 ts/packages/agents/onboarding/test/fixtures/openapi/book-api-absolute-server.json create mode 100644 ts/packages/agents/onboarding/test/fixtures/openapi/book-api.json create mode 100644 ts/packages/agents/onboarding/test/openApiExtraction.spec.ts create mode 100644 ts/packages/agents/onboarding/test/restHandlerCompile.spec.ts create mode 100644 ts/packages/agents/onboarding/test/restHandlerExecution.spec.ts create mode 100644 ts/packages/agents/onboarding/test/restRouting.spec.ts diff --git a/ts/packages/agents/onboarding/package.json b/ts/packages/agents/onboarding/package.json index 1bdd298e07..8cdc77c489 100644 --- a/ts/packages/agents/onboarding/package.json +++ b/ts/packages/agents/onboarding/package.json @@ -42,6 +42,8 @@ "clean": "rimraf --glob dist *.tsbuildinfo *.done.build.log", "prettier": "prettier --check . --ignore-path ../../../.prettierignore", "prettier:fix": "prettier --write . --ignore-path ../../../.prettierignore", + "test": "npm run test:local", + "test:local": "tsx --test test/*.spec.ts", "tsc": "tsc -b" }, "dependencies": { @@ -64,6 +66,7 @@ "copyfiles": "^2.4.1", "prettier": "^3.5.3", "rimraf": "^6.0.1", + "tsx": "^4.21.0", "typescript": "~5.4.5", "ws": "^8.21.1" }, diff --git a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts index 062cfa4c24..3f9bdbd628 100644 --- a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts +++ b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts @@ -49,6 +49,9 @@ export type DiscoveredParameter = { type: string; description?: string; required?: boolean; + // Where the parameter lives on the wire (OpenAPI `in`, or "body" for a + // request-body property). Only set by the parseOpenApiSpec arm. + in?: "path" | "query" | "header" | "cookie" | "body"; }; export type DiscoveredEntity = { @@ -66,6 +69,10 @@ export type ApiSurface = { approved?: boolean; approvedAt?: string; approvedActions?: string[]; + // Absolute http/https base URL resolved from the OpenAPI spec's + // `servers[0].url` (parseOpenApiSpec arm only). Unset when it cannot be + // resolved to a well-formed http/https URL (see resolveOpenApiBaseUrl). + baseUrl?: string; }; // TypeChat translator for structured CLI help extraction @@ -359,64 +366,91 @@ function isInternalAction(name: string): boolean { return false; } -async function handleParseOpenApiSpec( - integrationName: string, +// Resolve an OpenAPI 3 spec's base URL to an absolute http/https URL, or +// return undefined when it cannot be resolved deterministically (Swagger 2 +// host/basePath/schemes, malformed/local, or missing server variable +// defaults are all left unset — the REST handler generator then falls back +// to the stub handler rather than emitting a malformed request). +export function resolveOpenApiBaseUrl( + spec: any, specSource: string, -): Promise { - const state = await loadState(integrationName); - if (!state) { - return { - error: `Integration "${integrationName}" not found. Run startOnboarding first.`, - }; + fetchedUrl?: string, +): string | undefined { + const specIsHttp = /^https?:\/\//i.test(specSource); + // Prefer the post-redirect URL (only meaningful when we actually fetched + // over http/https); otherwise fall back to the original specSource. + const specLocation = specIsHttp ? (fetchedUrl ?? specSource) : undefined; + + const rawServerUrl: string | undefined = spec?.servers?.[0]?.url; + + if (!rawServerUrl) { + // No `servers` entry — fall back to the origin of the spec's own + // location, but only when that location is itself http/https. + if (!specLocation) return undefined; + try { + return new URL(specLocation).origin; + } catch { + return undefined; + } } - await updatePhase(integrationName, "discovery", { status: "in-progress" }); + // Substitute server variables ({var}) from servers[0].variables[var].default. + // If any referenced variable lacks a default, bail out rather than emit + // a malformed URL containing a literal "{var}". + const variables = spec.servers[0].variables ?? {}; + let substituted = rawServerUrl; + const varNames = [...rawServerUrl.matchAll(/\{([^}]+)\}/g)].map( + (m) => m[1], + ); + for (const varName of varNames) { + const def = variables?.[varName]?.default; + if (def === undefined || def === null) return undefined; + substituted = substituted.replaceAll(`{${varName}}`, String(def)); + } - // Fetch the spec (URL or file path) - let specContent: string; - try { - if ( - specSource.startsWith("http://") || - specSource.startsWith("https://") - ) { - const response = await fetch(specSource); - if (!response.ok) { - return { - error: `Failed to fetch spec: ${response.status} ${response.statusText}`, - }; - } - specContent = await response.text(); - } else { - const fs = await import("fs/promises"); - specContent = await fs.readFile(specSource, "utf-8"); + if (/^https?:\/\//i.test(substituted)) { + // Absolute — use as-is (preserving any path component). + try { + const u = new URL(substituted); + // Strip a trailing slash only; keep the rest of the path intact. + return u.toString().replace(/\/$/, ""); + } catch { + return undefined; } - } catch (err: any) { - return { - error: `Failed to read spec from ${specSource}: ${err?.message ?? err}`, - }; } - let spec: any; + // Relative server URL (e.g. "/v3") — resolve against the spec's own + // fetched location, only when that location is http/https. + if (!specLocation) return undefined; try { - spec = JSON.parse(specContent); + return new URL(substituted, specLocation).toString().replace(/\/$/, ""); } catch { - try { - // Try YAML if JSON fails (basic line parsing) - return { - error: "YAML specs not yet supported — please provide a JSON OpenAPI spec.", - }; - } catch { - return { error: "Could not parse spec as JSON or YAML." }; - } + return undefined; } +} - // Extract actions from OpenAPI paths +// Extract discovered actions from a parsed OpenAPI 3 (or Swagger 2 — +// unsupported, will simply yield no actions since `spec.paths` items won't +// have the shape below) spec document. Pure/synchronous so it can be unit +// tested without going through the workspace-state-backed action handler. +export function extractOpenApiActions( + spec: any, + specSource: string, +): DiscoveredAction[] { const actions: DiscoveredAction[] = []; const paths = spec.paths ?? {}; for (const [pathStr, pathItem] of Object.entries(paths) as [ string, any, ][]) { + // Deterministic inline-OpenAPI-3 subset only — a `$ref`'d path item + // (shared path referencing a component) is out of scope for v1. + if (pathItem?.$ref) continue; + + const pathLevelParams: any[] = Array.isArray(pathItem?.parameters) + ? pathItem.parameters + : []; + for (const method of [ "get", "post", @@ -435,14 +469,29 @@ async function handleParseOpenApiSpec( (_: string, c: string) => c.toUpperCase(), ); - const parameters: DiscoveredParameter[] = (op.parameters ?? []).map( - (p: any) => ({ - name: p.name, - type: p.schema?.type ?? "string", - description: p.description, - required: p.required ?? false, - }), - ); + // Merge path-level parameters (shared across all operations on + // this path) with operation-level parameters, keyed by + // (name, in) — operation-level entries override path-level ones. + const opLevelParams: any[] = Array.isArray(op.parameters) + ? op.parameters + : []; + const mergedByKey = new Map(); + for (const p of [...pathLevelParams, ...opLevelParams]) { + // Inline params only — skip `$ref`'d parameters (out of + // scope for v1 deterministic generation). + if (!p || p.$ref || !p.name) continue; + mergedByKey.set(`${p.in ?? "query"}:${p.name}`, p); + } + + const parameters: DiscoveredParameter[] = Array.from( + mergedByKey.values(), + ).map((p: any) => ({ + name: p.name, + type: p.schema?.type ?? "string", + description: p.description, + required: p.required ?? false, + in: p.in, + })); // Also include request body fields as parameters const requestBody = @@ -457,6 +506,7 @@ async function handleParseOpenApiSpec( description: propSchema.description, required: requestBody.required?.includes(propName) ?? false, + in: "body", }); } } @@ -474,12 +524,74 @@ async function handleParseOpenApiSpec( }); } } + return actions; +} + +async function handleParseOpenApiSpec( + integrationName: string, + specSource: string, +): Promise { + const state = await loadState(integrationName); + if (!state) { + return { + error: `Integration "${integrationName}" not found. Run startOnboarding first.`, + }; + } + + await updatePhase(integrationName, "discovery", { status: "in-progress" }); + + // Fetch the spec (URL or file path) + let specContent: string; + // Post-redirect URL when specSource is fetched over http/https; used to + // resolve a relative `servers[0].url` against the *actual* spec location. + let fetchedUrl: string | undefined; + try { + if ( + specSource.startsWith("http://") || + specSource.startsWith("https://") + ) { + const response = await fetch(specSource); + if (!response.ok) { + return { + error: `Failed to fetch spec: ${response.status} ${response.statusText}`, + }; + } + fetchedUrl = response.url || specSource; + specContent = await response.text(); + } else { + const fs = await import("fs/promises"); + specContent = await fs.readFile(specSource, "utf-8"); + } + } catch (err: any) { + return { + error: `Failed to read spec from ${specSource}: ${err?.message ?? err}`, + }; + } + + let spec: any; + try { + spec = JSON.parse(specContent); + } catch { + try { + // Try YAML if JSON fails (basic line parsing) + return { + error: "YAML specs not yet supported — please provide a JSON OpenAPI spec.", + }; + } catch { + return { error: "Could not parse spec as JSON or YAML." }; + } + } + + // Extract actions from OpenAPI paths + const actions: DiscoveredAction[] = extractOpenApiActions(spec, specSource); + const resolvedBaseUrl = resolveOpenApiBaseUrl(spec, specSource, fetchedUrl); const surface: ApiSurface = { integrationName, discoveredAt: new Date().toISOString(), source: specSource, actions, + ...(resolvedBaseUrl !== undefined ? { baseUrl: resolvedBaseUrl } : {}), }; await writeArtifactJson( diff --git a/ts/packages/agents/onboarding/src/scaffolder/restHandler.template b/ts/packages/agents/onboarding/src/scaffolder/restHandler.template new file mode 100644 index 0000000000..fa80f92816 --- /dev/null +++ b/ts/packages/agents/onboarding/src/scaffolder/restHandler.template @@ -0,0 +1,105 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +// +// Auto-generated REST handler for {{NAME}} +// +// Generated deterministically from an OpenAPI 3 spec (parseOpenApiSpec arm). +// v1 limitations: no auth (no Authorization/api-key headers), scalar-only +// query/path params (no array/object/style/explode), header/cookie params +// are dropped when optional and rejected when required, DELETE requests +// never carry a body. + +import { + ActionContext, + AppAgent, + TypeAgentAction, +} from "@typeagent/agent-sdk"; +import { + createActionResultFromTextDisplay, +} from "@typeagent/agent-sdk/helpers/action"; +import { {{PASCAL_NAME}}Actions } from "./{{NAME}}Schema.js"; + +const BASE_URL = {{BASE_URL}}; +const MAX_RESPONSE_CHARS = 4000; + +function truncate(text: string): string { + return text.length > MAX_RESPONSE_CHARS + ? text.slice(0, MAX_RESPONSE_CHARS) + "\n…(truncated)" + : text; +} + +function formatResponseBody(text: string): string { + try { + return truncate(JSON.stringify(JSON.parse(text), null, 2)); + } catch { + return truncate(text); + } +} + +async function callRest( + method: string, + path: string, + query: Record, + body: Record | undefined, +): Promise { + const trimmedBase = BASE_URL.replace(/\/$/, ""); + const normalizedPath = path.startsWith("/") ? path : `/${path}`; + const url = new URL(`${trimmedBase}${normalizedPath}`); + for (const [key, value] of Object.entries(query)) { + if (value === undefined || value === null) continue; + url.searchParams.set(key, String(value)); + } + + const headers: Record = { Accept: "application/json" }; + let requestBody: string | undefined; + if (body !== undefined) { + headers["Content-Type"] = "application/json"; + requestBody = JSON.stringify(body); + } + + const res = await fetch(url, { + method, + headers, + body: requestBody, + signal: AbortSignal.timeout(30_000), + }); + const text = await res.text(); + if (!res.ok) { + throw new Error(`${res.status} ${res.statusText}: ${truncate(text)}`); + } + return formatResponseBody(text); +} + +async function runAction( + action: TypeAgentAction<{{PASCAL_NAME}}Actions>, +): Promise { + const parameters = action.parameters as Record; + switch (action.actionName) { +{{SWITCH_CASES}} + default: + // Cast rather than reading `action.actionName` directly: when + // the switch above exhaustively covers every literal in the + // {{PASCAL_NAME}}Actions union, TS narrows `action` to `never` + // here, and `never` has no properties. + throw new Error( + `Unknown action: ${(action as { actionName: string }).actionName}`, + ); + } +} + +export function instantiate(): AppAgent { + return { + executeAction: async ( + action: TypeAgentAction<{{PASCAL_NAME}}Actions>, + context: ActionContext<{{PASCAL_NAME}}Actions>, + ) => { + try { + const output = await runAction(action); + return createActionResultFromTextDisplay(output); + } catch (e: unknown) { + const msg = e instanceof Error ? e.message : String(e); + return createActionResultFromTextDisplay(`Error: ${msg}`); + } + }, + }; +} diff --git a/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts b/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts new file mode 100644 index 0000000000..d30f18a3b4 --- /dev/null +++ b/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts @@ -0,0 +1,201 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// REST handler template generator — the fetch-based twin of +// cliHandlerTemplate.ts. Produces a complete TypeScript action handler that +// issues real HTTP requests against a base URL captured from an OpenAPI 3 +// spec's `servers[0].url` (see discoveryHandler.ts's parseOpenApiSpec arm). +// +// Called by scaffolderHandler when the API surface has a resolved +// `baseUrl` and HTTP-method actions with a `path` (parseOpenApiSpec arm +// only — crawlDocUrl's LLM-guessed method/path is out of scope). +// +// v1 limitations (see restHandler.template header comment too): +// - scalar path/query params only (no arrays/objects, no style/explode) +// - header/cookie params are unsupported: required ones fail the action, +// optional ones are silently dropped +// - no auth (no Authorization / api-key headers) +// - DELETE requests never carry a body +// - `$ref`-based params/bodies/path-items are skipped upstream in +// discoveryHandler.ts, so they never reach this generator + +import type { DiscoveredAction } from "../discovery/discoveryHandler.js"; +import fs from "fs/promises"; +import path from "path"; +import { fileURLToPath } from "url"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); + +// Resolve template from src/ relative to the package root. +// At runtime __dirname is dist/scaffolder/, so go up two levels to package root. +function templatePath(): string { + return path.resolve(__dirname, "../../src/scaffolder/restHandler.template"); +} + +const HTTP_METHODS_WITH_HANDLER = new Set([ + "GET", + "POST", + "PUT", + "PATCH", + "DELETE", +]); + +// Deterministic wire-name -> camelCase transform. Mirrors the transform +// SchemaGen's LLM prompt asks the model to apply to parameter names +// (schemaGenHandler.ts), so `book_id` / `book-id` on the wire becomes the +// `bookId` the LLM is expected to populate in `action.parameters`. Exotic +// LLM renames outside this transform are a known v1 limitation. +function toCamel(wireName: string): string { + return wireName.replace(/[-_]+([A-Za-z0-9])/g, (_, c: string) => + c.toUpperCase(), + ); +} + +// Bracket-access expression reading a parameter by its camelCase runtime +// name, falling back to the original wire name in case the LLM (or a +// simple/no-op transform) preserved it verbatim. +function paramValueExpr(wireName: string): string { + const camel = toCamel(wireName); + if (camel === wireName) { + return `parameters[${JSON.stringify(wireName)}]`; + } + return `parameters[${JSON.stringify(camel)}] ?? parameters[${JSON.stringify(wireName)}]`; +} + +// Builds a JS expression for the request path: literal segments are +// JSON.stringify'd string literals, `{param}` placeholders become +// `encodeURIComponent(String())`, all joined with `+`. This +// avoids template-literal escaping concerns entirely (no backtick/`${` +// collision risk from spec-provided literal text). +function buildPathExpr( + pathTemplate: string, + pathParamNames: Set, +): string { + const parts: string[] = []; + let lastIndex = 0; + const re = /\{([^}]+)\}/g; + let m: RegExpExecArray | null; + while ((m = re.exec(pathTemplate)) !== null) { + const literal = pathTemplate.slice(lastIndex, m.index); + if (literal) parts.push(JSON.stringify(literal)); + const paramName = m[1]; + if (pathParamNames.has(paramName)) { + parts.push( + `encodeURIComponent(String(${paramValueExpr(paramName)}))`, + ); + } else { + // Path placeholder with no captured parameter (shouldn't + // normally happen for an inline spec) — leave the literal + // placeholder text in place rather than throw at generation time. + parts.push(JSON.stringify(m[0])); + } + lastIndex = re.lastIndex; + } + const tail = pathTemplate.slice(lastIndex); + if (tail || parts.length === 0) parts.push(JSON.stringify(tail)); + return parts.join(" + "); +} + +function buildSwitchCases(actions: DiscoveredAction[]): string { + const cases: string[] = []; + for (const action of actions) { + const method = (action.method ?? "GET").toUpperCase(); + const pathTemplate = action.path ?? "/"; + const pathParamNames = new Set( + [...pathTemplate.matchAll(/\{([^}]+)\}/g)].map((m) => m[1]), + ); + + const params = action.parameters ?? []; + const bodyParams = params.filter((p) => p.in === "body"); + const headerCookieParams = params.filter( + (p) => p.in === "header" || p.in === "cookie", + ); + const queryParams = params.filter( + (p) => + p.in === "query" || + (p.in === undefined && + !pathParamNames.has(p.name) && + !bodyParams.includes(p)), + ); + + const requiredUnsupported = headerCookieParams.find((p) => p.required); + if (requiredUnsupported) { + cases.push( + ` case ${JSON.stringify(action.name)}:\n` + + ` throw new Error(${JSON.stringify( + `Required parameter "${requiredUnsupported.name}" (${requiredUnsupported.in}) is not supported by the generated REST handler in v1.`, + )});`, + ); + continue; + } + // Optional header/cookie params are silently dropped (v1 limitation). + + const lines: string[] = []; + lines.push(` case ${JSON.stringify(action.name)}: {`); + lines.push( + ` const path = ${buildPathExpr(pathTemplate, pathParamNames)};`, + ); + lines.push(` const query: Record = {};`); + for (const p of queryParams) { + lines.push( + ` query[${JSON.stringify(p.name)}] = ${paramValueExpr(p.name)};`, + ); + } + if (method !== "DELETE" && bodyParams.length > 0) { + lines.push(` const body: Record = {`); + for (const p of bodyParams) { + lines.push( + ` [${JSON.stringify(p.name)}]: ${paramValueExpr(p.name)},`, + ); + } + lines.push(` };`); + lines.push( + ` return await callRest(${JSON.stringify(method)}, path, query, body);`, + ); + } else { + lines.push( + ` return await callRest(${JSON.stringify(method)}, path, query, undefined);`, + ); + } + lines.push(` }`); + cases.push(lines.join("\n")); + } + return cases.join("\n"); +} + +/** + * Returns the subset of `actions` this generator can actually handle: + * actions from the parseOpenApiSpec arm with a recognized HTTP method and a + * `path`. Used by scaffolderHandler to decide whether to route to the REST + * generator at all. + */ +export function filterRestActions( + actions: DiscoveredAction[] | undefined, +): DiscoveredAction[] { + if (!actions) return []; + return actions.filter( + (a) => + !!a.path && + !!a.method && + HTTP_METHODS_WITH_HANDLER.has(a.method.toUpperCase()), + ); +} + +export async function buildRestHandler( + name: string, + pascalName: string, + baseUrl: string, + actions: DiscoveredAction[], +): Promise { + const tpl = await fs.readFile(templatePath(), "utf-8"); + const switchCases = buildSwitchCases(actions); + // Use function-form replacers: string-form replace treats "$" + // sequences in the replacement as special (e.g. "$&", "$1"), which a + // generated base URL or switch-case body could plausibly contain. + return tpl + .replace(/\{\{NAME\}\}/g, () => name) + .replace(/\{\{PASCAL_NAME\}\}/g, () => pascalName) + .replace(/\{\{BASE_URL\}\}/g, () => JSON.stringify(baseUrl)) + .replace(/\{\{SWITCH_CASES\}\}/g, () => switchCases); +} diff --git a/ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts b/ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts index c554b9fd5c..0e03ef583d 100644 --- a/ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts +++ b/ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts @@ -21,6 +21,7 @@ import { } from "../lib/workspace.js"; import type { ApiSurface } from "../discovery/discoveryHandler.js"; import { buildCliHandler } from "./cliHandlerTemplate.js"; +import { buildRestHandler, filterRestActions } from "./restHandlerTemplate.js"; import { loadTemplate } from "./templateLoader.js"; import { generateAgentKeywordFiles } from "./agentKeywordFiles.js"; import type { KeywordSchemaTarget } from "./agentKeywordFiles.js"; @@ -726,7 +727,7 @@ function buildManifest( return manifest; } -async function buildHandler( +export async function buildHandler( name: string, pascalName: string, pattern: AgentPattern = "schema-grammar", @@ -741,6 +742,27 @@ async function buildHandler( return await buildCliHandler(name, pascalName, cliCommand, cliActions); } + // If discovery data has a resolved OpenAPI base URL, generate a real + // fetch-based REST handler for the patterns that model a direct REST + // integration (schema-grammar is the default when a caller doesn't + // specify a pattern). Explicit non-REST patterns (websocket-bridge, + // native-platform, etc.) keep their own dedicated builders even if an + // apiSurface happens to be attached. + if ( + apiSurface?.baseUrl && + (pattern === "schema-grammar" || pattern === "external-api") + ) { + const restActions = filterRestActions(apiSurface.actions); + if (restActions.length > 0) { + return await buildRestHandler( + name, + pascalName, + apiSurface.baseUrl, + restActions, + ); + } + } + switch (pattern) { case "external-api": return buildExternalApiHandler(name, pascalName); diff --git a/ts/packages/agents/onboarding/test/apiSurfacePreservation.spec.ts b/ts/packages/agents/onboarding/test/apiSurfacePreservation.spec.ts new file mode 100644 index 0000000000..eae742af79 --- /dev/null +++ b/ts/packages/agents/onboarding/test/apiSurfacePreservation.spec.ts @@ -0,0 +1,105 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Verifies CHANGE 1/2 survive the full discovery -> approval round trip +// (parseOpenApiSpec writes api-surface.json, approveApiSurface re-reads and +// re-writes it via `{...surface, ...}`): baseUrl and per-parameter `in` +// must still be present in the approved artifact. Exercises the real +// executeDiscoveryAction handler end-to-end against the real per-integration +// workspace on disk (~/.typeagent/onboarding/), cleaned up afterward. + +import { test, after } from "node:test"; +import assert from "node:assert/strict"; +import fs from "fs/promises"; +import path from "path"; +import os from "os"; +import { fileURLToPath } from "url"; +import { executeDiscoveryAction } from "../src/discovery/discoveryHandler.js"; +import type { ApiSurface } from "../src/discovery/discoveryHandler.js"; +import { createWorkspace, readArtifactJson } from "../src/lib/workspace.js"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); + +const INTEGRATION_NAME = `test-book-api-${process.pid}-${Date.now()}`; + +function getWorkspaceDir(): string { + return path.join( + os.homedir(), + ".typeagent", + "onboarding", + INTEGRATION_NAME, + ); +} + +after(async () => { + await fs.rm(getWorkspaceDir(), { recursive: true, force: true }); +}); + +test("baseUrl and parameter `in` survive parseOpenApiSpec -> approveApiSurface", async () => { + await createWorkspace({ integrationName: INTEGRATION_NAME }); + + const fixturePath = path.resolve( + __dirname, + "fixtures/openapi/book-api-absolute-server.json", + ); + + const parseResult = await executeDiscoveryAction( + { + actionName: "parseOpenApiSpec", + parameters: { + integrationName: INTEGRATION_NAME, + specSource: fixturePath, + }, + } as any, + {} as any, + ); + assert.ok( + !("error" in (parseResult as any)), + `parseOpenApiSpec failed: ${JSON.stringify(parseResult)}`, + ); + + const parsedSurface = await readArtifactJson( + INTEGRATION_NAME, + "discovery", + "api-surface.json", + ); + assert.equal(parsedSurface?.baseUrl, "https://api.example.com/v1"); + const getBook = parsedSurface?.actions.find((a) => a.name === "getBook"); + assert.equal( + getBook?.parameters?.find((p) => p.name === "book_id")?.in, + "path", + ); + + const approveResult = await executeDiscoveryAction( + { + actionName: "approveApiSurface", + parameters: { integrationName: INTEGRATION_NAME }, + } as any, + {} as any, + ); + assert.ok( + !("error" in (approveResult as any)), + `approveApiSurface failed: ${JSON.stringify(approveResult)}`, + ); + + const approvedSurface = await readArtifactJson( + INTEGRATION_NAME, + "discovery", + "api-surface.json", + ); + assert.equal( + approvedSurface?.baseUrl, + "https://api.example.com/v1", + "baseUrl must survive the approval spread", + ); + const approvedGetBook = approvedSurface?.actions.find( + (a) => a.name === "getBook", + ); + assert.equal( + approvedGetBook?.parameters?.find((p) => p.name === "book_id")?.in, + "path", + "parameter `in` must survive the approval spread", + ); + assert.equal(approvedSurface?.approved, true); +}); diff --git a/ts/packages/agents/onboarding/test/fixtures/openapi/book-api-absolute-server.json b/ts/packages/agents/onboarding/test/fixtures/openapi/book-api-absolute-server.json new file mode 100644 index 0000000000..f48918064f --- /dev/null +++ b/ts/packages/agents/onboarding/test/fixtures/openapi/book-api-absolute-server.json @@ -0,0 +1,25 @@ +{ + "openapi": "3.0.0", + "info": { + "title": "Book API", + "version": "1.0.0" + }, + "servers": [{ "url": "https://api.example.com/v1" }], + "paths": { + "/books/{book_id}": { + "parameters": [ + { + "name": "book_id", + "in": "path", + "required": true, + "schema": { "type": "string" } + } + ], + "get": { + "operationId": "get_book", + "summary": "Get a book", + "responses": { "200": { "description": "ok" } } + } + } + } +} diff --git a/ts/packages/agents/onboarding/test/fixtures/openapi/book-api.json b/ts/packages/agents/onboarding/test/fixtures/openapi/book-api.json new file mode 100644 index 0000000000..d7101e6e25 --- /dev/null +++ b/ts/packages/agents/onboarding/test/fixtures/openapi/book-api.json @@ -0,0 +1,85 @@ +{ + "openapi": "3.0.0", + "info": { + "title": "Book API", + "version": "1.0.0" + }, + "servers": [{ "url": "/v3" }], + "paths": { + "/books/{book_id}": { + "parameters": [ + { + "name": "book_id", + "in": "path", + "required": true, + "schema": { "type": "string" } + } + ], + "get": { + "operationId": "get_book", + "summary": "Get a book", + "parameters": [ + { + "name": "include_reviews", + "in": "query", + "required": false, + "schema": { "type": "boolean" } + } + ], + "responses": { "200": { "description": "ok" } } + }, + "put": { + "operationId": "update_book", + "summary": "Update a book", + "requestBody": { + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "title": { "type": "string" } + }, + "required": ["title"] + } + } + } + }, + "responses": { "200": { "description": "ok" } } + } + }, + "/books": { + "get": { + "operationId": "search_books", + "summary": "Search books", + "parameters": [ + { + "name": "limit", + "in": "query", + "required": false, + "schema": { "type": "integer" } + } + ], + "responses": { "200": { "description": "ok" } } + }, + "post": { + "operationId": "create_book", + "summary": "Create a book", + "requestBody": { + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "title": { "type": "string" }, + "author": { "type": "string" } + }, + "required": ["title"] + } + } + } + }, + "responses": { "201": { "description": "created" } } + } + } + } +} diff --git a/ts/packages/agents/onboarding/test/openApiExtraction.spec.ts b/ts/packages/agents/onboarding/test/openApiExtraction.spec.ts new file mode 100644 index 0000000000..dace0b28d4 --- /dev/null +++ b/ts/packages/agents/onboarding/test/openApiExtraction.spec.ts @@ -0,0 +1,226 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Deterministic unit tests for the parseOpenApiSpec arm's extraction and +// base-URL resolution logic (CHANGE 1 & CHANGE 2 of the REST-handler- +// generation feature). + +import { test, describe } from "node:test"; +import assert from "node:assert/strict"; +import fs from "fs/promises"; +import path from "path"; +import { fileURLToPath } from "url"; +import { + extractOpenApiActions, + resolveOpenApiBaseUrl, +} from "../src/discovery/discoveryHandler.js"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); + +async function loadBookApiSpec(): Promise { + const raw = await fs.readFile( + path.resolve(__dirname, "fixtures/openapi/book-api.json"), + "utf-8", + ); + return JSON.parse(raw); +} + +describe("extractOpenApiActions", () => { + test("merges path-item-level params with operation-level params", async () => { + const spec = await loadBookApiSpec(); + const actions = extractOpenApiActions( + spec, + "https://api.example.com/openapi.json", + ); + + const getBook = actions.find((a) => a.name === "getBook"); + assert.ok(getBook, "expected a getBook action"); + assert.equal(getBook!.method, "GET"); + assert.equal(getBook!.path, "/books/{book_id}"); + const getBookParamNames = getBook!.parameters?.map((p) => p.name); + assert.ok(getBookParamNames?.includes("book_id")); + assert.ok(getBookParamNames?.includes("include_reviews")); + + const bookIdParam = getBook!.parameters?.find( + (p) => p.name === "book_id", + ); + assert.equal(bookIdParam?.in, "path"); + assert.equal(bookIdParam?.required, true); + + const includeReviewsParam = getBook!.parameters?.find( + (p) => p.name === "include_reviews", + ); + assert.equal(includeReviewsParam?.in, "query"); + }); + + test("operation-level param with the same (name, in) overrides the path-level one", () => { + const spec = { + openapi: "3.0.0", + paths: { + "/items/{id}": { + parameters: [ + { + name: "id", + in: "path", + required: true, + description: "path-level", + schema: { type: "string" }, + }, + ], + get: { + operationId: "get_item", + parameters: [ + { + name: "id", + in: "path", + required: true, + description: "operation-level override", + schema: { type: "string" }, + }, + ], + }, + }, + }, + }; + const actions = extractOpenApiActions(spec, "spec.json"); + const idParam = actions[0].parameters?.find((p) => p.name === "id"); + assert.equal(idParam?.description, "operation-level override"); + // Only one merged entry, not two. + assert.equal( + actions[0].parameters?.filter((p) => p.name === "id").length, + 1, + ); + }); + + test("captures request body properties with in: body", () => { + const spec = { + openapi: "3.0.0", + paths: { + "/books/{book_id}": { + put: { + operationId: "update_book", + requestBody: { + content: { + "application/json": { + schema: { + type: "object", + properties: { + title: { type: "string" }, + }, + required: ["title"], + }, + }, + }, + }, + }, + }, + }, + }; + const actions = extractOpenApiActions(spec, "spec.json"); + const titleParam = actions[0].parameters?.find( + (p) => p.name === "title", + ); + assert.equal(titleParam?.in, "body"); + assert.equal(titleParam?.required, true); + }); + + test("skips a $ref'd path item entirely (inline-only v1 scope)", () => { + const spec = { + openapi: "3.0.0", + paths: { + "/shared": { $ref: "#/components/pathItems/Shared" }, + "/real": { get: { operationId: "get_real" } }, + }, + }; + const actions = extractOpenApiActions(spec, "spec.json"); + assert.equal(actions.length, 1); + assert.equal(actions[0].name, "getReal"); + }); + + test("skips $ref'd parameters", () => { + const spec = { + openapi: "3.0.0", + paths: { + "/x": { + get: { + operationId: "get_x", + parameters: [{ $ref: "#/components/parameters/Foo" }], + }, + }, + }, + }; + const actions = extractOpenApiActions(spec, "spec.json"); + assert.equal(actions[0].parameters?.length, 0); + }); +}); + +describe("resolveOpenApiBaseUrl", () => { + test("resolves a relative servers[0].url against the fetched spec location, preserving path", async () => { + const spec = await loadBookApiSpec(); + const baseUrl = resolveOpenApiBaseUrl( + spec, + "https://api.example.com/openapi.json", + "https://api.example.com/openapi.json", + ); + assert.equal(baseUrl, "https://api.example.com/v3"); + }); + + test("preserves an absolute server URL's path component", () => { + const spec = { servers: [{ url: "https://api.example.com/v1" }] }; + const baseUrl = resolveOpenApiBaseUrl( + spec, + "https://api.example.com/openapi.json", + ); + assert.equal(baseUrl, "https://api.example.com/v1"); + }); + + test("substitutes server variables from their default values", () => { + const spec = { + servers: [ + { + url: "https://{host}.example.com/{basePath}", + variables: { + host: { default: "api" }, + basePath: { default: "v2" }, + }, + }, + ], + }; + const baseUrl = resolveOpenApiBaseUrl(spec, "spec.json"); + assert.equal(baseUrl, "https://api.example.com/v2"); + }); + + test("leaves baseUrl unset when a server variable has no default", () => { + const spec = { + servers: [ + { + url: "https://{host}.example.com", + variables: {}, + }, + ], + }; + const baseUrl = resolveOpenApiBaseUrl(spec, "spec.json"); + assert.equal(baseUrl, undefined); + }); + + test("falls back to the fetched spec's origin when servers is absent", () => { + const baseUrl = resolveOpenApiBaseUrl( + {}, + "https://api.example.com/openapi.json", + "https://api.example.com/openapi.json", + ); + assert.equal(baseUrl, "https://api.example.com"); + }); + + test("leaves baseUrl unset for a local file spec source with a relative servers url", () => { + const spec = { servers: [{ url: "/v3" }] }; + const baseUrl = resolveOpenApiBaseUrl(spec, "./local-spec.json"); + assert.equal(baseUrl, undefined); + }); + + test("leaves baseUrl unset for a local file spec source with no servers entry", () => { + const baseUrl = resolveOpenApiBaseUrl({}, "./local-spec.json"); + assert.equal(baseUrl, undefined); + }); +}); diff --git a/ts/packages/agents/onboarding/test/restHandlerCompile.spec.ts b/ts/packages/agents/onboarding/test/restHandlerCompile.spec.ts new file mode 100644 index 0000000000..f648032bc0 --- /dev/null +++ b/ts/packages/agents/onboarding/test/restHandlerCompile.spec.ts @@ -0,0 +1,143 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Verifies the generator's output actually type-checks with `tsc` (CHANGE 3 +// acceptance criterion 2d). Writes a generated handler + a matching schema +// stub to a scratch folder nested inside the onboarding package (so +// module resolution finds the workspace's @typeagent/agent-sdk via the +// package's own node_modules symlink), then runs `tsc --noEmit` on it. + +import { test, after } from "node:test"; +import assert from "node:assert/strict"; +import { execFile } from "child_process"; +import { promisify } from "util"; +import fs from "fs/promises"; +import path from "path"; +import { fileURLToPath } from "url"; +import { buildRestHandler } from "../src/scaffolder/restHandlerTemplate.js"; +import type { DiscoveredAction } from "../src/discovery/discoveryHandler.js"; + +const execFileAsync = promisify(execFile); +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); + +const SCRATCH_DIR = path.resolve(__dirname, ".tmp-rest-handler-compile"); + +const actions: DiscoveredAction[] = [ + { + name: "getBook", + description: "Get a book", + method: "GET", + path: "/books/{book_id}", + parameters: [ + { name: "book_id", type: "string", required: true, in: "path" }, + { + name: "include_reviews", + type: "boolean", + required: false, + in: "query", + }, + ], + }, + { + name: "createBook", + description: "Create a book", + method: "POST", + path: "/books", + parameters: [ + { name: "title", type: "string", required: true, in: "body" }, + { name: "author", type: "string", required: false, in: "body" }, + ], + }, + { + name: "deleteBook", + description: "Delete a book", + method: "DELETE", + path: "/books/{book_id}", + parameters: [ + { name: "book_id", type: "string", required: true, in: "path" }, + ], + }, +]; + +function buildSchemaStub(pascalName: string): string { + const members = actions + .map( + (a) => + ` | {\n` + + ` actionName: ${JSON.stringify(a.name)};\n` + + ` parameters: Record;\n` + + ` }`, + ) + .join("\n"); + return ( + `// Copyright (c) Microsoft Corporation.\n` + + `// Licensed under the MIT License.\n\n` + + `export type ${pascalName}Actions =\n${members};\n` + ); +} + +after(async () => { + await fs.rm(SCRATCH_DIR, { recursive: true, force: true }); +}); + +test("buildRestHandler output type-checks with tsc", async () => { + await fs.rm(SCRATCH_DIR, { recursive: true, force: true }); + await fs.mkdir(SCRATCH_DIR, { recursive: true }); + + const name = "bookApi"; + const pascalName = "BookApi"; + const handlerSource = await buildRestHandler( + name, + pascalName, + "https://api.example.com/v1", + actions, + ); + + await fs.writeFile( + path.join(SCRATCH_DIR, `${name}ActionHandler.ts`), + handlerSource, + "utf-8", + ); + await fs.writeFile( + path.join(SCRATCH_DIR, `${name}Schema.ts`), + buildSchemaStub(pascalName), + "utf-8", + ); + await fs.writeFile( + path.join(SCRATCH_DIR, "tsconfig.json"), + JSON.stringify( + { + compilerOptions: { + target: "es2021", + lib: ["es2021"], + module: "node16", + moduleResolution: "node16", + types: ["node"], + esModuleInterop: true, + skipLibCheck: true, + strict: true, + noEmit: true, + }, + include: ["*.ts"], + }, + null, + 2, + ), + "utf-8", + ); + + const tscBin = path.resolve( + __dirname, + "../node_modules/typescript/bin/tsc", + ); + try { + await execFileAsync(process.execPath, [tscBin, "-p", SCRATCH_DIR], { + cwd: SCRATCH_DIR, + }); + } catch (err: any) { + assert.fail( + `Generated REST handler failed to type-check:\n${err.stdout ?? err.message}`, + ); + } +}); diff --git a/ts/packages/agents/onboarding/test/restHandlerExecution.spec.ts b/ts/packages/agents/onboarding/test/restHandlerExecution.spec.ts new file mode 100644 index 0000000000..b8e4e53653 --- /dev/null +++ b/ts/packages/agents/onboarding/test/restHandlerExecution.spec.ts @@ -0,0 +1,212 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Deterministic, offline execution test (CHANGE 3 acceptance criterion 2e): +// generates a REST handler with buildRestHandler, points it at a local +// node:http server on an ephemeral port, and exercises GET-with-path-param, +// GET-with-query, and POST-with-JSON-body — asserting the server observed +// the expected method/URL/body and that the handler surfaced the server's +// response back through createActionResultFromTextDisplay. + +import { test, after } from "node:test"; +import assert from "node:assert/strict"; +import fs from "fs/promises"; +import path from "path"; +import { fileURLToPath, pathToFileURL } from "url"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { buildRestHandler } from "../src/scaffolder/restHandlerTemplate.js"; +import type { DiscoveredAction } from "../src/discovery/discoveryHandler.js"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); +const SCRATCH_DIR = path.resolve(__dirname, ".tmp-rest-handler-exec"); + +after(async () => { + await fs.rm(SCRATCH_DIR, { recursive: true, force: true }); +}); + +type ObservedRequest = { + method?: string; + url?: string; + body: string; +}; + +async function startServer( + handleRequest: ( + req: http.IncomingMessage, + res: http.ServerResponse, + body: string, + ) => void, +): Promise<{ baseUrl: string; close: () => Promise }> { + const server = http.createServer((req, res) => { + const chunks: Buffer[] = []; + req.on("data", (c) => chunks.push(c)); + req.on("end", () => { + handleRequest(req, res, Buffer.concat(chunks).toString("utf-8")); + }); + }); + await new Promise((resolve) => + server.listen(0, "127.0.0.1", resolve), + ); + const { port } = server.address() as AddressInfo; + return { + baseUrl: `http://127.0.0.1:${port}`, + close: () => + new Promise((resolve) => server.close(() => resolve())), + }; +} + +async function loadHandler( + baseUrl: string, + actions: DiscoveredAction[], +): Promise<{ instantiate: () => { executeAction: Function } }> { + await fs.rm(SCRATCH_DIR, { recursive: true, force: true }); + await fs.mkdir(SCRATCH_DIR, { recursive: true }); + const source = await buildRestHandler( + "bookApi", + "BookApi", + baseUrl, + actions, + ); + const handlerPath = path.join(SCRATCH_DIR, "bookApiActionHandler.ts"); + await fs.writeFile(handlerPath, source, "utf-8"); + // The generated handler imports "./bookApiSchema.js" purely for a type, + // which is erased at emit time — an empty type-only stub is enough for + // tsx's on-the-fly transpilation (no type-checking at runtime). + await fs.writeFile( + path.join(SCRATCH_DIR, "bookApiSchema.ts"), + "export type BookApiActions = { actionName: string; parameters: Record };\n", + "utf-8", + ); + // Cache-bust: each test loads a freshly written file at the same path. + // pathToFileURL is required on Windows, where raw absolute paths + // (e.g. "D:/...") are not valid ESM specifiers. + const fileUrl = pathToFileURL(handlerPath); + fileUrl.search = `t=${Date.now()}-${Math.random()}`; + const mod = await import(fileUrl.href); + return mod; +} + +test("GET with a path parameter hits the right URL and returns the server's JSON", async () => { + const observed: ObservedRequest[] = []; + const { baseUrl, close } = await startServer((req, res, body) => { + observed.push({ method: req.method, url: req.url, body }); + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ id: "42", title: "Dune" })); + }); + try { + const actions: DiscoveredAction[] = [ + { + name: "getBook", + method: "GET", + path: "/books/{book_id}", + parameters: [ + { + name: "book_id", + type: "string", + required: true, + in: "path", + }, + ], + }, + ]; + const { instantiate } = await loadHandler(baseUrl, actions); + const agent = instantiate(); + const result: any = await agent.executeAction( + { + actionName: "getBook", + parameters: { bookId: "42" }, + }, + {} as any, + ); + + assert.equal(observed.length, 1); + assert.equal(observed[0].method, "GET"); + assert.equal(observed[0].url, "/books/42"); + assert.match(result.displayContent as string, /"title": "Dune"/); + } finally { + await close(); + } +}); + +test("GET with a query parameter is sent as a query string", async () => { + const observed: ObservedRequest[] = []; + const { baseUrl, close } = await startServer((req, res, body) => { + observed.push({ method: req.method, url: req.url, body }); + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ results: [] })); + }); + try { + const actions: DiscoveredAction[] = [ + { + name: "searchBooks", + method: "GET", + path: "/books", + parameters: [ + { + name: "limit", + type: "integer", + required: false, + in: "query", + }, + ], + }, + ]; + const { instantiate } = await loadHandler(baseUrl, actions); + const agent = instantiate(); + await agent.executeAction( + { actionName: "searchBooks", parameters: { limit: 5 } }, + {} as any, + ); + + assert.equal(observed.length, 1); + assert.equal(observed[0].method, "GET"); + assert.equal(observed[0].url, "/books?limit=5"); + } finally { + await close(); + } +}); + +test("POST sends a JSON body and the server's method/body are observed correctly", async () => { + const observed: ObservedRequest[] = []; + const { baseUrl, close } = await startServer((req, res, body) => { + observed.push({ method: req.method, url: req.url, body }); + res.writeHead(201, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ id: "99" })); + }); + try { + const actions: DiscoveredAction[] = [ + { + name: "createBook", + method: "POST", + path: "/books", + parameters: [ + { + name: "title", + type: "string", + required: true, + in: "body", + }, + ], + }, + ]; + const { instantiate } = await loadHandler(baseUrl, actions); + const agent = instantiate(); + const result: any = await agent.executeAction( + { + actionName: "createBook", + parameters: { title: "Foundation" }, + }, + {} as any, + ); + + assert.equal(observed.length, 1); + assert.equal(observed[0].method, "POST"); + assert.equal(observed[0].url, "/books"); + assert.deepEqual(JSON.parse(observed[0].body), { title: "Foundation" }); + assert.match(result.displayContent as string, /"id": "99"/); + } finally { + await close(); + } +}); diff --git a/ts/packages/agents/onboarding/test/restRouting.spec.ts b/ts/packages/agents/onboarding/test/restRouting.spec.ts new file mode 100644 index 0000000000..8aa75f89c4 --- /dev/null +++ b/ts/packages/agents/onboarding/test/restRouting.spec.ts @@ -0,0 +1,98 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Tests for CHANGE 4 — guarded routing of the REST handler generator in +// scaffolderHandler.ts's buildHandler. REST generation must only kick in +// when an apiSurface has a resolved baseUrl AND the pattern is one that +// models a direct REST integration (schema-grammar default, external-api); +// explicit non-REST patterns must keep their own dedicated builders even +// when an apiSurface happens to be attached. + +import { test, describe } from "node:test"; +import assert from "node:assert/strict"; +import { buildHandler } from "../src/scaffolder/scaffolderHandler.js"; +import type { ApiSurface } from "../src/discovery/discoveryHandler.js"; + +function makeSurface(baseUrl?: string): ApiSurface { + return { + integrationName: "bookApi", + discoveredAt: new Date().toISOString(), + source: "https://api.example.com/openapi.json", + actions: [ + { + name: "getBook", + description: "Get a book", + method: "GET", + path: "/books/{book_id}", + parameters: [ + { + name: "book_id", + type: "string", + required: true, + in: "path", + }, + ], + sourceUrl: "https://api.example.com/openapi.json", + }, + ], + ...(baseUrl !== undefined ? { baseUrl } : {}), + }; +} + +describe("buildHandler REST routing", () => { + test("routes to the REST generator for the default (schema-grammar) pattern when baseUrl is set", async () => { + const output = await buildHandler( + "bookApi", + "BookApi", + "schema-grammar", + makeSurface("https://api.example.com/v3"), + ); + assert.match(output, /callRest\(/); + assert.match(output, /"getBook"/); + }); + + test("routes to the REST generator for the external-api pattern when baseUrl is set", async () => { + const output = await buildHandler( + "bookApi", + "BookApi", + "external-api", + makeSurface("https://api.example.com/v3"), + ); + assert.match(output, /callRest\(/); + }); + + test("does NOT route to REST for websocket-bridge even when baseUrl is set", async () => { + const output = await buildHandler( + "bookApi", + "BookApi", + "websocket-bridge", + makeSurface("https://api.example.com/v3"), + ); + assert.doesNotMatch(output, /callRest\(/); + }); + + test("does NOT route to REST for native-platform even when baseUrl is set", async () => { + const output = await buildHandler( + "bookApi", + "BookApi", + "native-platform", + makeSurface("https://api.example.com/v3"), + ); + assert.doesNotMatch(output, /callRest\(/); + }); + + test("falls back to the schema-grammar stub when no baseUrl is present", async () => { + const output = await buildHandler( + "bookApi", + "BookApi", + "schema-grammar", + makeSurface(undefined), + ); + assert.doesNotMatch(output, /callRest\(/); + }); + + test("falls back to the schema-grammar stub when there is no apiSurface at all", async () => { + const output = await buildHandler("bookApi", "BookApi"); + assert.doesNotMatch(output, /callRest\(/); + }); +}); diff --git a/ts/pnpm-lock.yaml b/ts/pnpm-lock.yaml index 15c833a57b..7369980e03 100644 --- a/ts/pnpm-lock.yaml +++ b/ts/pnpm-lock.yaml @@ -2992,6 +2992,9 @@ importers: rimraf: specifier: ^6.0.1 version: 6.0.1 + tsx: + specifier: ^4.21.0 + version: 4.21.0 typescript: specifier: ~5.4.5 version: 5.4.5 From 1c2add8d2bae96ecda68cf3524ef0a54e438d904 Mon Sep 17 00:00:00 2001 From: Tal Zaccai Date: Tue, 28 Jul 2026 18:06:11 -0700 Subject: [PATCH 2/7] Fix REST handler review issues: $ref path params, GET body, non-http scheme leak Addresses code review feedback on the parseOpenApiSpec REST handler generator: - Fix 1 (BLOCKING): resolve local #/components/parameters/* $refs during discovery merge instead of dropping them, so path params factored into components (e.g. GitHub-style specs) are no longer silently lost and substituted as literal "undefined" at runtime. Added a defense-in-depth check in filterRestActions that excludes any action whose path template still has an unresolved {placeholder} with no matching path param. - Fix 2 (WARNING): gate request-body emission on a positive method allowlist (POST/PUT/PATCH) instead of "!== DELETE", so a GET/HEAD action with a requestBody never emits a body (which fetch() rejects at runtime). - Fix 3 (hardening): re-validate the resolved protocol is http/https after resolving a relative servers[0].url, so an absolute non-http(s) scheme (ws://, ftp://, ...) can no longer leak through as a baseUrl. Added regression tests for all three fixes (openApiExtraction.spec.ts, restHandlerCodegenFixes.spec.ts) plus a new Tier-A deterministic end-to-end test (restHandlerE2EBooks.spec.ts) that drives parseOpenApiSpec -> filterRestActions -> buildRestHandler -> tsc compile -> real execution against a local node:http server, covering a $ref-based path param and /v2 base-path preservation together. Full suite: 30/30 node:test passing, tsc -b clean, prettier applied. Also manually ran the full LLM-driven pipeline (discovery -> phraseGen -> schemaGen -> grammarGen -> scaffold) against a books OpenAPI fixture with a $ref path param, pointed at a local HTTP server with a /v2 base path (Tier B, scratch driver, not committed): the generated handler correctly performed GET /v2/books/book-42 (base path preserved, no "undefined") and returned the real server response through createActionResultFromTextDisplay. --- .../src/discovery/discoveryHandler.ts | 43 ++- .../src/scaffolder/restHandlerTemplate.ts | 48 ++- .../onboarding/test/openApiExtraction.spec.ts | 84 ++++- .../test/restHandlerCodegenFixes.spec.ts | 112 +++++++ .../test/restHandlerE2EBooks.spec.ts | 296 ++++++++++++++++++ 5 files changed, 571 insertions(+), 12 deletions(-) create mode 100644 ts/packages/agents/onboarding/test/restHandlerCodegenFixes.spec.ts create mode 100644 ts/packages/agents/onboarding/test/restHandlerE2EBooks.spec.ts diff --git a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts index 3f9bdbd628..d05bcab2ee 100644 --- a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts +++ b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts @@ -423,12 +423,37 @@ export function resolveOpenApiBaseUrl( // fetched location, only when that location is http/https. if (!specLocation) return undefined; try { - return new URL(substituted, specLocation).toString().replace(/\/$/, ""); + const resolved = new URL(substituted, specLocation); + // `substituted` might itself be an absolute non-http(s) URL (e.g. + // "ws://...", "ftp://...") that didn't match the earlier + // http(s)-prefix check; guard the http/https-only guarantee here + // too rather than relying solely on the regex test above. + if (resolved.protocol !== "http:" && resolved.protocol !== "https:") { + return undefined; + } + return resolved.toString().replace(/\/$/, ""); } catch { return undefined; } } +// Resolve a local `#/components/parameters/` JSON-pointer reference +// against the (already-parsed) spec document. Only this narrow, common +// shape is supported (deterministic inline-OpenAPI-3 subset, v1) — any +// other pointer shape (external file refs, `#/definitions/...` from +// Swagger 2, deeper paths) returns undefined and the caller treats the +// parameter as unresolvable. +function resolveLocalParameterRef(spec: any, ref: string): any | undefined { + const match = /^#\/components\/parameters\/([^/]+)$/.exec(ref); + if (!match) return undefined; + const resolved = spec?.components?.parameters?.[match[1]]; + // A resolved entry could itself be a `$ref` (unlikely, but guard against + // infinite loops / unsupported indirection) — only accept a fully + // inline parameter object. + if (!resolved || resolved.$ref) return undefined; + return resolved; +} + // Extract discovered actions from a parsed OpenAPI 3 (or Swagger 2 — // unsupported, will simply yield no actions since `spec.paths` items won't // have the shape below) spec document. Pure/synchronous so it can be unit @@ -476,9 +501,19 @@ export function extractOpenApiActions( ? op.parameters : []; const mergedByKey = new Map(); - for (const p of [...pathLevelParams, ...opLevelParams]) { - // Inline params only — skip `$ref`'d parameters (out of - // scope for v1 deterministic generation). + for (const raw of [...pathLevelParams, ...opLevelParams]) { + if (!raw) continue; + // Resolve local `#/components/parameters/*` refs inline — + // these are extremely common for path params in real-world + // specs (e.g. the GitHub REST spec references nearly every + // path parameter this way), so treating them as "$ref, + // therefore skip" would silently drop the parameter and + // leave the generated handler substituting `undefined` into + // the URL. Any other `$ref` shape (external files, deeper + // pointers) stays unresolved/out of v1 scope and is skipped. + const p = raw.$ref + ? resolveLocalParameterRef(spec, raw.$ref) + : raw; if (!p || p.$ref || !p.name) continue; mergedByKey.set(`${p.in ?? "query"}:${p.name}`, p); } diff --git a/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts b/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts index d30f18a3b4..f322525179 100644 --- a/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts +++ b/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts @@ -142,7 +142,10 @@ function buildSwitchCases(actions: DiscoveredAction[]): string { ` query[${JSON.stringify(p.name)}] = ${paramValueExpr(p.name)};`, ); } - if (method !== "DELETE" && bodyParams.length > 0) { + if ( + ["POST", "PUT", "PATCH"].includes(method) && + bodyParams.length > 0 + ) { lines.push(` const body: Record = {`); for (const p of bodyParams) { lines.push( @@ -170,16 +173,47 @@ function buildSwitchCases(actions: DiscoveredAction[]): string { * `path`. Used by scaffolderHandler to decide whether to route to the REST * generator at all. */ +/** + * Returns the subset of `actions` this generator can actually handle: + * actions from the parseOpenApiSpec arm with a recognized HTTP method and a + * `path`. Used by scaffolderHandler to decide whether to route to the REST + * generator at all. + * + * Also excludes any action whose path template contains a `{placeholder}` + * that has no corresponding resolved (non-`$ref`) path parameter — this can + * happen if a path param used an unsupported `$ref` shape (e.g. an external + * file reference) that discoveryHandler.ts's local-ref resolution couldn't + * follow. Emitting a handler for such an action would substitute the + * literal string "undefined" into the request URL at runtime; safer to + * fall back to the stub handler for that action than to silently build a + * broken request. + */ export function filterRestActions( actions: DiscoveredAction[] | undefined, ): DiscoveredAction[] { if (!actions) return []; - return actions.filter( - (a) => - !!a.path && - !!a.method && - HTTP_METHODS_WITH_HANDLER.has(a.method.toUpperCase()), - ); + return actions.filter((a) => { + if ( + !a.path || + !a.method || + !HTTP_METHODS_WITH_HANDLER.has(a.method.toUpperCase()) + ) { + return false; + } + const pathParamNames = new Set( + [...a.path.matchAll(/\{([^}]+)\}/g)].map((m) => m[1]), + ); + if (pathParamNames.size === 0) return true; + const resolvedPathParamNames = new Set( + (a.parameters ?? []) + .filter((p) => p.in === "path") + .map((p) => p.name), + ); + for (const placeholder of pathParamNames) { + if (!resolvedPathParamNames.has(placeholder)) return false; + } + return true; + }); } export async function buildRestHandler( diff --git a/ts/packages/agents/onboarding/test/openApiExtraction.spec.ts b/ts/packages/agents/onboarding/test/openApiExtraction.spec.ts index dace0b28d4..0817e5a93f 100644 --- a/ts/packages/agents/onboarding/test/openApiExtraction.spec.ts +++ b/ts/packages/agents/onboarding/test/openApiExtraction.spec.ts @@ -138,7 +138,7 @@ describe("extractOpenApiActions", () => { assert.equal(actions[0].name, "getReal"); }); - test("skips $ref'd parameters", () => { + test("skips an unresolvable $ref'd parameter (no matching components.parameters entry)", () => { const spec = { openapi: "3.0.0", paths: { @@ -153,6 +153,78 @@ describe("extractOpenApiActions", () => { const actions = extractOpenApiActions(spec, "spec.json"); assert.equal(actions[0].parameters?.length, 0); }); + + test("resolves a local #/components/parameters/* $ref path parameter inline (Fix 1 regression)", () => { + const spec = { + openapi: "3.0.0", + components: { + parameters: { + BookId: { + name: "bookId", + in: "path", + required: true, + schema: { type: "string" }, + }, + }, + }, + paths: { + "/books/{bookId}": { + get: { + operationId: "get_book", + parameters: [ + { $ref: "#/components/parameters/BookId" }, + ], + }, + }, + }, + }; + const actions = extractOpenApiActions(spec, "spec.json"); + assert.equal(actions.length, 1); + const bookIdParam = actions[0].parameters?.find( + (p) => p.name === "bookId", + ); + assert.ok( + bookIdParam, + "expected the $ref'd path parameter to be resolved and merged, not dropped", + ); + assert.equal(bookIdParam?.in, "path"); + assert.equal(bookIdParam?.required, true); + }); + + test("path-level $ref parameter resolves and merges the same as an inline path-level parameter", () => { + const spec = { + openapi: "3.0.0", + components: { + parameters: { + BookId: { + name: "bookId", + in: "path", + required: true, + schema: { type: "string" }, + }, + }, + }, + paths: { + "/books/{bookId}": { + parameters: [{ $ref: "#/components/parameters/BookId" }], + get: { operationId: "get_book" }, + delete: { operationId: "delete_book" }, + }, + }, + }; + const actions = extractOpenApiActions(spec, "spec.json"); + assert.equal(actions.length, 2); + for (const action of actions) { + const bookIdParam = action.parameters?.find( + (p) => p.name === "bookId", + ); + assert.ok( + bookIdParam, + `expected ${action.name} to carry the path-level $ref'd bookId parameter`, + ); + assert.equal(bookIdParam?.in, "path"); + } + }); }); describe("resolveOpenApiBaseUrl", () => { @@ -223,4 +295,14 @@ describe("resolveOpenApiBaseUrl", () => { const baseUrl = resolveOpenApiBaseUrl({}, "./local-spec.json"); assert.equal(baseUrl, undefined); }); + + test("leaves baseUrl unset when servers[0].url is an absolute non-http(s) scheme (Fix 3 regression)", () => { + const spec = { servers: [{ url: "ws://api.example.com/v1" }] }; + const baseUrl = resolveOpenApiBaseUrl( + spec, + "https://api.example.com/openapi.json", + "https://api.example.com/openapi.json", + ); + assert.equal(baseUrl, undefined); + }); }); diff --git a/ts/packages/agents/onboarding/test/restHandlerCodegenFixes.spec.ts b/ts/packages/agents/onboarding/test/restHandlerCodegenFixes.spec.ts new file mode 100644 index 0000000000..d9de7c6ea0 --- /dev/null +++ b/ts/packages/agents/onboarding/test/restHandlerCodegenFixes.spec.ts @@ -0,0 +1,112 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Targeted regression tests for two review fixes on buildRestHandler's +// generated source text (fast, no tsc/network/server needed): +// - Fix 1: a path parameter resolved from a local `#/components/parameters` +// $ref (already deref'd into DiscoveredAction.parameters by +// discoveryHandler.ts) must be substituted into the path expression via +// the actual parameter lookup, not silently degrade to a literal +// "undefined" segment. +// - Fix 2: a GET (or HEAD) action with body-typed parameters must never +// emit a `body` argument to callRest, since fetch throws for +// GET/HEAD requests carrying a body. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { buildRestHandler } from "../src/scaffolder/restHandlerTemplate.js"; +import type { DiscoveredAction } from "../src/discovery/discoveryHandler.js"; + +test("path parameter (as resolved from a local $ref by discoveryHandler) is substituted, never left as a literal placeholder or undefined", async () => { + // Shape identical to what extractOpenApiActions now produces once it + // resolves a `{"$ref": "#/components/parameters/BookId"}` path + // parameter inline. + const actions: DiscoveredAction[] = [ + { + name: "getBook", + method: "GET", + path: "/books/{bookId}", + parameters: [ + { + name: "bookId", + type: "string", + required: true, + in: "path", + }, + ], + }, + ]; + const source = await buildRestHandler( + "bookApi", + "BookApi", + "https://api.example.com/v1", + actions, + ); + + // Must read the actual parameter (bracket-access lookup), not emit the + // raw placeholder text or a bare `undefined` literal into the path. + assert.match(source, /parameters\["bookId"\]/); + assert.doesNotMatch(source, /"\{bookId\}"/); + assert.doesNotMatch(source, /\+\s*undefined\s*\+/); +}); + +test("GET action with body-typed parameters never emits a body (Fix 2 regression)", async () => { + const actions: DiscoveredAction[] = [ + { + name: "searchBooks", + method: "GET", + path: "/books/search", + parameters: [ + { + name: "query", + type: "string", + required: true, + in: "body", + }, + ], + }, + ]; + const source = await buildRestHandler( + "bookApi", + "BookApi", + "https://api.example.com/v1", + actions, + ); + + const caseMatch = /case "searchBooks": \{([\s\S]*?)\n {8}\}/.exec(source); + assert.ok(caseMatch, "expected a generated case for searchBooks"); + const caseBody = caseMatch![1]; + assert.doesNotMatch( + caseBody, + /const body/, + "GET actions must never construct a request body", + ); + assert.match( + caseBody, + /callRest\("GET", path, query, undefined\)/, + "GET actions must call callRest with an undefined body argument", + ); +}); + +test("HEAD-like semantics aside, DELETE with body-typed parameters still never emits a body", async () => { + const actions: DiscoveredAction[] = [ + { + name: "deleteBook", + method: "DELETE", + path: "/books/{bookId}", + parameters: [ + { name: "bookId", type: "string", required: true, in: "path" }, + { name: "reason", type: "string", required: false, in: "body" }, + ], + }, + ]; + const source = await buildRestHandler( + "bookApi", + "BookApi", + "https://api.example.com/v1", + actions, + ); + const caseMatch = /case "deleteBook": \{([\s\S]*?)\n {8}\}/.exec(source); + assert.ok(caseMatch, "expected a generated case for deleteBook"); + assert.doesNotMatch(caseMatch![1], /const body/); +}); diff --git a/ts/packages/agents/onboarding/test/restHandlerE2EBooks.spec.ts b/ts/packages/agents/onboarding/test/restHandlerE2EBooks.spec.ts new file mode 100644 index 0000000000..2d310b650d --- /dev/null +++ b/ts/packages/agents/onboarding/test/restHandlerE2EBooks.spec.ts @@ -0,0 +1,296 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Tier-A end-to-end onboarding pipeline check (offline/deterministic): runs +// the full parseOpenApiSpec discovery pipeline (including a $ref-resolved +// path parameter — the Fix 1 regression scenario) against an OpenAPI 3 +// "books" spec whose `servers[0].url` is an absolute URL pointing at a +// local node:http server WITH a base path ("/v2", to also exercise +// base-path preservation), then scaffolds a REST handler from the +// resulting ApiSurface, type-checks it, and executes it against that same +// local server for a GET-with-path-param, a GET-with-query, and a +// POST-with-JSON-body action. Asserts the server observed the correct +// method/URL/body for each request (base path preserved, $ref path param +// substituted — never "undefined" — query encoded, JSON body sent) and +// that real response data flows back through the handler's +// createActionResultFromTextDisplay result. + +import { test, after } from "node:test"; +import assert from "node:assert/strict"; +import { execFile } from "child_process"; +import { promisify } from "util"; +import fs from "fs/promises"; +import path from "path"; +import { fileURLToPath, pathToFileURL } from "url"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { + extractOpenApiActions, + resolveOpenApiBaseUrl, +} from "../src/discovery/discoveryHandler.js"; +import { + buildRestHandler, + filterRestActions, +} from "../src/scaffolder/restHandlerTemplate.js"; + +const execFileAsync = promisify(execFile); +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); +const SCRATCH_DIR = path.resolve(__dirname, ".tmp-rest-e2e-books"); + +after(async () => { + await fs.rm(SCRATCH_DIR, { recursive: true, force: true }); +}); + +type ObservedRequest = { method?: string; url?: string; body: string }; + +async function startBooksServer(): Promise<{ + baseOrigin: string; + observed: ObservedRequest[]; + close: () => Promise; +}> { + const observed: ObservedRequest[] = []; + const server = http.createServer((req, res) => { + const chunks: Buffer[] = []; + req.on("data", (c) => chunks.push(c)); + req.on("end", () => { + const body = Buffer.concat(chunks).toString("utf-8"); + observed.push({ method: req.method, url: req.url, body }); + + if (req.method === "GET" && req.url?.startsWith("/v2/books/")) { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ id: "book-42", title: "Dune" })); + return; + } + if (req.method === "GET" && req.url?.startsWith("/v2/books")) { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ results: ["Dune", "Foundation"] })); + return; + } + if (req.method === "POST" && req.url === "/v2/books") { + res.writeHead(201, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ id: "book-99", created: true })); + return; + } + res.writeHead(404); + res.end("not found"); + }); + }); + await new Promise((resolve) => + server.listen(0, "127.0.0.1", resolve), + ); + const { port } = server.address() as AddressInfo; + return { + baseOrigin: `http://127.0.0.1:${port}`, + observed, + close: () => + new Promise((resolve) => server.close(() => resolve())), + }; +} + +function buildBooksSpec(baseOrigin: string) { + return { + openapi: "3.0.0", + info: { title: "Books API", version: "1.0.0" }, + // Absolute server URL WITH a base path ("/v2") — exercises base-path + // preservation end to end, independent of the (local, non-http) + // spec source. + servers: [{ url: `${baseOrigin}/v2` }], + components: { + parameters: { + BookId: { + name: "bookId", + in: "path", + required: true, + schema: { type: "string" }, + }, + }, + }, + paths: { + "/books/{bookId}": { + // Path-level $ref parameter — the Fix 1 regression scenario: + // must resolve inline rather than silently drop, or the + // generated handler would substitute the literal string + // "undefined" into the request URL. + parameters: [{ $ref: "#/components/parameters/BookId" }], + get: { + operationId: "get_book", + summary: "Get a book by id", + responses: { "200": { description: "ok" } }, + }, + }, + "/books": { + get: { + operationId: "search_books", + summary: "Search books", + parameters: [ + { + name: "limit", + in: "query", + required: false, + schema: { type: "integer" }, + }, + ], + responses: { "200": { description: "ok" } }, + }, + post: { + operationId: "create_book", + summary: "Create a book", + requestBody: { + content: { + "application/json": { + schema: { + type: "object", + properties: { + title: { type: "string" }, + }, + required: ["title"], + }, + }, + }, + }, + responses: { "201": { description: "created" } }, + }, + }, + }, + }; +} + +test("Tier A: parseOpenApiSpec discovery + buildRestHandler scaffolding + real local-HTTP execution for a books API", async (t) => { + const { baseOrigin, observed, close } = await startBooksServer(); + t.after(() => close()); + + // --- Discovery phase (parseOpenApiSpec arm) --- + const specSource = "./local-books-spec.json"; // local, non-http source + const spec = buildBooksSpec(baseOrigin); + + const baseUrl = resolveOpenApiBaseUrl(spec, specSource, specSource); + assert.equal( + baseUrl, + `${baseOrigin}/v2`, + "expected the absolute server URL (with its /v2 base path) to be captured verbatim", + ); + + const actions = extractOpenApiActions(spec, specSource); + assert.equal(actions.length, 3); + + const getBook = actions.find((a) => a.name === "getBook"); + assert.ok(getBook, "expected a getBook action"); + const bookIdParam = getBook!.parameters?.find((p) => p.name === "bookId"); + assert.ok( + bookIdParam, + "expected the $ref'd path-level bookId parameter to have been resolved and merged onto get_book", + ); + assert.equal(bookIdParam?.in, "path"); + assert.equal(bookIdParam?.required, true); + + const searchBooks = actions.find((a) => a.name === "searchBooks"); + const limitParam = searchBooks?.parameters?.find((p) => p.name === "limit"); + assert.equal(limitParam?.in, "query"); + + const createBook = actions.find((a) => a.name === "createBook"); + const titleParam = createBook?.parameters?.find((p) => p.name === "title"); + assert.equal(titleParam?.in, "body"); + + // --- Scaffolding phase (buildRestHandler) --- + const restActions = filterRestActions(actions); + assert.equal( + restActions.length, + 3, + "all three actions should be eligible for REST generation (the $ref path param must have resolved, or getBook would have been excluded by the Fix-1 safety net)", + ); + + await fs.rm(SCRATCH_DIR, { recursive: true, force: true }); + await fs.mkdir(SCRATCH_DIR, { recursive: true }); + const handlerSource = await buildRestHandler( + "booksApi", + "BooksApi", + baseUrl!, + restActions, + ); + const handlerPath = path.join(SCRATCH_DIR, "booksApiActionHandler.ts"); + await fs.writeFile(handlerPath, handlerSource, "utf-8"); + await fs.writeFile( + path.join(SCRATCH_DIR, "booksApiSchema.ts"), + "export type BooksApiActions = { actionName: string; parameters: Record };\n", + "utf-8", + ); + + // --- Compile check --- + await fs.writeFile( + path.join(SCRATCH_DIR, "tsconfig.json"), + JSON.stringify( + { + compilerOptions: { + target: "es2021", + lib: ["es2021"], + module: "node16", + moduleResolution: "node16", + types: ["node"], + esModuleInterop: true, + skipLibCheck: true, + strict: true, + noEmit: true, + }, + include: ["*.ts"], + }, + null, + 2, + ), + "utf-8", + ); + const tscBin = path.resolve( + __dirname, + "../node_modules/typescript/bin/tsc", + ); + try { + await execFileAsync(process.execPath, [tscBin, "-p", SCRATCH_DIR], { + cwd: SCRATCH_DIR, + }); + } catch (err: any) { + assert.fail( + `Generated books REST handler failed to type-check:\n${err.stdout ?? err.message}`, + ); + } + + // --- Execution phase: real HTTP calls against the local server --- + const fileUrl = pathToFileURL(handlerPath); + fileUrl.search = `t=${Date.now()}-${Math.random()}`; + const mod = await import(fileUrl.href); + const agent = mod.instantiate(); + + // 1. GET with a $ref-resolved path param. + const getResult: any = await agent.executeAction( + { actionName: "getBook", parameters: { bookId: "book-42" } }, + {} as any, + ); + assert.equal(observed.length, 1); + assert.equal(observed[0].method, "GET"); + assert.equal( + observed[0].url, + "/v2/books/book-42", + "expected the /v2 base path preserved and the $ref-resolved bookId substituted (not 'undefined')", + ); + assert.doesNotMatch(observed[0].url ?? "", /undefined/); + assert.match(getResult.displayContent as string, /"title": "Dune"/); + + // 2. GET with a query param. + await agent.executeAction( + { actionName: "searchBooks", parameters: { limit: 2 } }, + {} as any, + ); + assert.equal(observed.length, 2); + assert.equal(observed[1].method, "GET"); + assert.equal(observed[1].url, "/v2/books?limit=2"); + + // 3. POST with a JSON body. + const postResult: any = await agent.executeAction( + { actionName: "createBook", parameters: { title: "Foundation" } }, + {} as any, + ); + assert.equal(observed.length, 3); + assert.equal(observed[2].method, "POST"); + assert.equal(observed[2].url, "/v2/books"); + assert.deepEqual(JSON.parse(observed[2].body), { title: "Foundation" }); + assert.match(postResult.displayContent as string, /"created": true/); +}); From 2f2eb397ef731ca48f2b557f1ed43d277bc1518a Mon Sep 17 00:00:00 2001 From: Tal Zaccai Date: Wed, 29 Jul 2026 17:05:09 -0700 Subject: [PATCH 3/7] onboarding: tighten REST generator code comments Deduplicate the filterRestActions doc block and trim comments in the REST handler generator and OpenAPI discovery path to describe code behavior concisely, without cross-file/external examples. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/discovery/discoveryHandler.ts | 13 ++--- .../src/scaffolder/restHandlerTemplate.ts | 52 ++++++------------- 2 files changed, 20 insertions(+), 45 deletions(-) diff --git a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts index d05bcab2ee..08dd8e2b0c 100644 --- a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts +++ b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts @@ -503,14 +503,11 @@ export function extractOpenApiActions( const mergedByKey = new Map(); for (const raw of [...pathLevelParams, ...opLevelParams]) { if (!raw) continue; - // Resolve local `#/components/parameters/*` refs inline — - // these are extremely common for path params in real-world - // specs (e.g. the GitHub REST spec references nearly every - // path parameter this way), so treating them as "$ref, - // therefore skip" would silently drop the parameter and - // leave the generated handler substituting `undefined` into - // the URL. Any other `$ref` shape (external files, deeper - // pointers) stays unresolved/out of v1 scope and is skipped. + // Resolve local `#/components/parameters/*` refs inline so a + // referenced parameter isn't dropped (which would leave the + // generated handler substituting `undefined` into the URL). + // Any other `$ref` shape (external files, deeper pointers) + // stays unresolved and is skipped. const p = raw.$ref ? resolveLocalParameterRef(spec, raw.$ref) : raw; diff --git a/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts b/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts index f322525179..111880669a 100644 --- a/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts +++ b/ts/packages/agents/onboarding/src/scaffolder/restHandlerTemplate.ts @@ -1,23 +1,17 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -// REST handler template generator — the fetch-based twin of -// cliHandlerTemplate.ts. Produces a complete TypeScript action handler that -// issues real HTTP requests against a base URL captured from an OpenAPI 3 -// spec's `servers[0].url` (see discoveryHandler.ts's parseOpenApiSpec arm). +// Generates a fetch-based TypeScript action handler that issues HTTP requests +// against a base URL resolved from an OpenAPI 3 spec. Called by +// scaffolderHandler when the API surface has a resolved `baseUrl` and actions +// carrying an HTTP method and `path`. // -// Called by scaffolderHandler when the API surface has a resolved -// `baseUrl` and HTTP-method actions with a `path` (parseOpenApiSpec arm -// only — crawlDocUrl's LLM-guessed method/path is out of scope). -// -// v1 limitations (see restHandler.template header comment too): +// v1 limitations: // - scalar path/query params only (no arrays/objects, no style/explode) -// - header/cookie params are unsupported: required ones fail the action, -// optional ones are silently dropped +// - header/cookie params: required ones fail the action, optional ones are +// dropped // - no auth (no Authorization / api-key headers) // - DELETE requests never carry a body -// - `$ref`-based params/bodies/path-items are skipped upstream in -// discoveryHandler.ts, so they never reach this generator import type { DiscoveredAction } from "../discovery/discoveryHandler.js"; import fs from "fs/promises"; @@ -41,11 +35,9 @@ const HTTP_METHODS_WITH_HANDLER = new Set([ "DELETE", ]); -// Deterministic wire-name -> camelCase transform. Mirrors the transform -// SchemaGen's LLM prompt asks the model to apply to parameter names -// (schemaGenHandler.ts), so `book_id` / `book-id` on the wire becomes the -// `bookId` the LLM is expected to populate in `action.parameters`. Exotic -// LLM renames outside this transform are a known v1 limitation. +// Deterministic wire-name -> camelCase transform, matching how parameter +// names are renamed during SchemaGen, so `book_id`/`book-id` on the wire maps +// to the `bookId` populated in `action.parameters`. function toCamel(wireName: string): string { return wireName.replace(/[-_]+([A-Za-z0-9])/g, (_, c: string) => c.toUpperCase(), @@ -168,25 +160,11 @@ function buildSwitchCases(actions: DiscoveredAction[]): string { } /** - * Returns the subset of `actions` this generator can actually handle: - * actions from the parseOpenApiSpec arm with a recognized HTTP method and a - * `path`. Used by scaffolderHandler to decide whether to route to the REST - * generator at all. - */ -/** - * Returns the subset of `actions` this generator can actually handle: - * actions from the parseOpenApiSpec arm with a recognized HTTP method and a - * `path`. Used by scaffolderHandler to decide whether to route to the REST - * generator at all. - * - * Also excludes any action whose path template contains a `{placeholder}` - * that has no corresponding resolved (non-`$ref`) path parameter — this can - * happen if a path param used an unsupported `$ref` shape (e.g. an external - * file reference) that discoveryHandler.ts's local-ref resolution couldn't - * follow. Emitting a handler for such an action would substitute the - * literal string "undefined" into the request URL at runtime; safer to - * fall back to the stub handler for that action than to silently build a - * broken request. + * Returns the subset of `actions` this generator can handle: those with a + * recognized HTTP method and a `path` whose every `{placeholder}` has a + * resolved path parameter. An action with an unresolved placeholder is + * excluded so the caller falls back to the stub handler instead of emitting a + * request with a literal "undefined" in the URL. */ export function filterRestActions( actions: DiscoveredAction[] | undefined, From dd9cc3ad5e0bd0353aef4fc5e316a71d33eadb85 Mon Sep 17 00:00:00 2001 From: Tal Zaccai Date: Wed, 29 Jul 2026 21:22:25 -0700 Subject: [PATCH 4/7] Fix REST handler template body under exactOptionalPropertyTypes The scaffolded callRest helper passed body: requestBody (string | undefined) directly to fetch, which fails TS2379 under the repo's exactOptionalPropertyTypes, so a scaffolded REST agent would not compile in-workspace. Only set fetch's body when a request body exists. Adds a Fix 3 codegen regression test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/scaffolder/restHandler.template | 2 +- .../test/restHandlerCodegenFixes.spec.ts | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/ts/packages/agents/onboarding/src/scaffolder/restHandler.template b/ts/packages/agents/onboarding/src/scaffolder/restHandler.template index fa80f92816..fb038dcc36 100644 --- a/ts/packages/agents/onboarding/src/scaffolder/restHandler.template +++ b/ts/packages/agents/onboarding/src/scaffolder/restHandler.template @@ -60,8 +60,8 @@ async function callRest( const res = await fetch(url, { method, headers, - body: requestBody, signal: AbortSignal.timeout(30_000), + ...(requestBody !== undefined ? { body: requestBody } : {}), }); const text = await res.text(); if (!res.ok) { diff --git a/ts/packages/agents/onboarding/test/restHandlerCodegenFixes.spec.ts b/ts/packages/agents/onboarding/test/restHandlerCodegenFixes.spec.ts index d9de7c6ea0..72d745b3ed 100644 --- a/ts/packages/agents/onboarding/test/restHandlerCodegenFixes.spec.ts +++ b/ts/packages/agents/onboarding/test/restHandlerCodegenFixes.spec.ts @@ -11,6 +11,11 @@ // - Fix 2: a GET (or HEAD) action with body-typed parameters must never // emit a `body` argument to callRest, since fetch throws for // GET/HEAD requests carrying a body. +// - Fix 3: the callRest helper must only set fetch's `body` when a request +// body exists (conditional spread), never `body: requestBody` where +// requestBody is `string | undefined` — that fails TS2379 under the +// repo's `exactOptionalPropertyTypes`, so a scaffolded REST agent would +// not compile in-workspace. import { test } from "node:test"; import assert from "node:assert/strict"; @@ -110,3 +115,31 @@ test("HEAD-like semantics aside, DELETE with body-typed parameters still never e assert.ok(caseMatch, "expected a generated case for deleteBook"); assert.doesNotMatch(caseMatch![1], /const body/); }); + +test("callRest sets fetch's body only when present, never body: requestBody (Fix 3 regression)", async () => { + const actions: DiscoveredAction[] = [ + { + name: "getBook", + method: "GET", + path: "/books/{bookId}", + parameters: [ + { name: "bookId", type: "string", required: true, in: "path" }, + ], + }, + ]; + const source = await buildRestHandler( + "bookApi", + "BookApi", + "https://api.example.com/v1", + actions, + ); + + // Must NOT unconditionally pass `body: requestBody` (requestBody is + // `string | undefined`) — that fails TS2379 under exactOptionalPropertyTypes. + assert.doesNotMatch(source, /\n\s*body: requestBody,/); + // Must conditionally include body only when a request body exists. + assert.match( + source, + /\.\.\.\(requestBody !== undefined \? \{ body: requestBody \} : \{\}\)/, + ); +}); From 0d0236538c3cdafd038bb654d489e61834befeb4 Mon Sep 17 00:00:00 2001 From: Copilot App <223556219+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 12:35:33 -0700 Subject: [PATCH 5/7] Refactor extractOpenApiActions to satisfy complexity ratchet Extract mergeOperationParameters, extractBodyParameters, and buildActionForOperation helpers so extractOpenApiActions is just its two loops plus a push, bringing it under the CC 25 / Cog 30 CI gate. Behavior-preserving: identical output for local parameter \ resolution, path+op param merge (op overriding path), and request-body props as in: body. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/discovery/discoveryHandler.ts | 161 ++++++++++-------- 1 file changed, 86 insertions(+), 75 deletions(-) diff --git a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts index 08dd8e2b0c..6fb51384a5 100644 --- a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts +++ b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts @@ -454,6 +454,82 @@ function resolveLocalParameterRef(spec: any, ref: string): any | undefined { return resolved; } +// Merge path-level and operation-level parameters, keyed by (in, name), with +// operation-level entries overriding path-level ones. Local +// `#/components/parameters/*` refs are resolved inline so a referenced +// parameter isn't dropped; any other `$ref` shape (external files, deeper +// pointers) stays unresolved and is skipped. +function mergeOperationParameters( + spec: any, + pathLevelParams: any[], + op: any, +): DiscoveredParameter[] { + const opLevelParams: any[] = Array.isArray(op.parameters) + ? op.parameters + : []; + const mergedByKey = new Map(); + for (const raw of [...pathLevelParams, ...opLevelParams]) { + if (!raw) continue; + const p = raw.$ref ? resolveLocalParameterRef(spec, raw.$ref) : raw; + if (!p || p.$ref || !p.name) continue; + mergedByKey.set(`${p.in ?? "query"}:${p.name}`, p); + } + return Array.from(mergedByKey.values()).map((p: any) => ({ + name: p.name, + type: p.schema?.type ?? "string", + description: p.description, + required: p.required ?? false, + in: p.in, + })); +} + +// Map an operation's JSON request-body properties to `in: "body"` parameters. +function extractBodyParameters(op: any): DiscoveredParameter[] { + const requestBody = op.requestBody?.content?.["application/json"]?.schema; + if (!requestBody?.properties) return []; + return (Object.entries(requestBody.properties) as [string, any][]).map( + ([propName, propSchema]): DiscoveredParameter => ({ + name: propName, + type: propSchema.type ?? "string", + description: propSchema.description, + required: requestBody.required?.includes(propName) ?? false, + in: "body", + }), + ); +} + +// Build a single DiscoveredAction from one path/method/operation. +function buildActionForOperation( + spec: any, + pathStr: string, + pathLevelParams: any[], + method: string, + op: any, + specSource: string, +): DiscoveredAction { + const name = + op.operationId ?? `${method}${pathStr.replace(/[^a-zA-Z0-9]/g, "_")}`; + const camelName = name.replace(/_([a-z])/g, (_: string, c: string) => + c.toUpperCase(), + ); + return { + name: camelName, + description: + op.summary ?? + op.description ?? + `${method.toUpperCase()} ${pathStr}`, + method: method.toUpperCase(), + path: pathStr, + parameters: [ + ...mergeOperationParameters(spec, pathLevelParams, op), + ...extractBodyParameters(op), + ], + sourceUrl: specSource, + }; +} + +const OPENAPI_HTTP_METHODS = ["get", "post", "put", "patch", "delete"] as const; + // Extract discovered actions from a parsed OpenAPI 3 (or Swagger 2 — // unsupported, will simply yield no actions since `spec.paths` items won't // have the shape below) spec document. Pure/synchronous so it can be unit @@ -476,84 +552,19 @@ export function extractOpenApiActions( ? pathItem.parameters : []; - for (const method of [ - "get", - "post", - "put", - "patch", - "delete", - ] as const) { + for (const method of OPENAPI_HTTP_METHODS) { const op = pathItem?.[method]; if (!op) continue; - - const name = - op.operationId ?? - `${method}${pathStr.replace(/[^a-zA-Z0-9]/g, "_")}`; - const camelName = name.replace( - /_([a-z])/g, - (_: string, c: string) => c.toUpperCase(), + actions.push( + buildActionForOperation( + spec, + pathStr, + pathLevelParams, + method, + op, + specSource, + ), ); - - // Merge path-level parameters (shared across all operations on - // this path) with operation-level parameters, keyed by - // (name, in) — operation-level entries override path-level ones. - const opLevelParams: any[] = Array.isArray(op.parameters) - ? op.parameters - : []; - const mergedByKey = new Map(); - for (const raw of [...pathLevelParams, ...opLevelParams]) { - if (!raw) continue; - // Resolve local `#/components/parameters/*` refs inline so a - // referenced parameter isn't dropped (which would leave the - // generated handler substituting `undefined` into the URL). - // Any other `$ref` shape (external files, deeper pointers) - // stays unresolved and is skipped. - const p = raw.$ref - ? resolveLocalParameterRef(spec, raw.$ref) - : raw; - if (!p || p.$ref || !p.name) continue; - mergedByKey.set(`${p.in ?? "query"}:${p.name}`, p); - } - - const parameters: DiscoveredParameter[] = Array.from( - mergedByKey.values(), - ).map((p: any) => ({ - name: p.name, - type: p.schema?.type ?? "string", - description: p.description, - required: p.required ?? false, - in: p.in, - })); - - // Also include request body fields as parameters - const requestBody = - op.requestBody?.content?.["application/json"]?.schema; - if (requestBody?.properties) { - for (const [propName, propSchema] of Object.entries( - requestBody.properties, - ) as [string, any][]) { - parameters.push({ - name: propName, - type: propSchema.type ?? "string", - description: propSchema.description, - required: - requestBody.required?.includes(propName) ?? false, - in: "body", - }); - } - } - - actions.push({ - name: camelName, - description: - op.summary ?? - op.description ?? - `${method.toUpperCase()} ${pathStr}`, - method: method.toUpperCase(), - path: pathStr, - parameters, - sourceUrl: specSource, - }); } } return actions; From 693af7026c2afc070b31c27ccf0c91898860226b Mon Sep 17 00:00:00 2001 From: Copilot App <223556219+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 12:58:59 -0700 Subject: [PATCH 6/7] Type the OpenAPI spec model to clear the lint ratchet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a minimal structural OpenApi* type model (spec, path item, operation, parameter, schema, server) and thread it through resolveOpenApiBaseUrl, resolveLocalParameterRef, mergeOperationParameters, extractBodyParameters, buildActionForOperation, extractOpenApiActions, and the parsed-spec local in handleParseOpenApiSpec. Removes the OpenAPI extraction any usage (file no-explicit-any 21 -> 3, back under the base of 7) without disabling lint or undoing the helper extraction. Behavior is unchanged — the types describe the same parsed-JSON shape the code already read. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/discovery/discoveryHandler.ts | 127 +++++++++++++----- 1 file changed, 92 insertions(+), 35 deletions(-) diff --git a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts index 6fb51384a5..f82d03a073 100644 --- a/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts +++ b/ts/packages/agents/onboarding/src/discovery/discoveryHandler.ts @@ -75,6 +75,54 @@ export type ApiSurface = { baseUrl?: string; }; +// Minimal structural model of the subset of an OpenAPI 3 document this handler +// reads. Deliberately partial (every field optional) since specs are parsed +// from untrusted JSON — the extraction code guards each access rather than +// assuming a valid/complete document. +type OpenApiRef = { $ref?: string }; +type OpenApiSchema = { + type?: string; + description?: string; + properties?: Record; + required?: string[]; +} & OpenApiRef; +type OpenApiParameter = { + name?: string; + in?: string; + description?: string; + required?: boolean; + schema?: OpenApiSchema; +} & OpenApiRef; +type OpenApiRequestBody = { + content?: Record; +}; +type OpenApiOperation = { + operationId?: string; + summary?: string; + description?: string; + parameters?: OpenApiParameter[]; + requestBody?: OpenApiRequestBody; +}; +type OpenApiPathItem = { + parameters?: OpenApiParameter[]; + get?: OpenApiOperation; + post?: OpenApiOperation; + put?: OpenApiOperation; + patch?: OpenApiOperation; + delete?: OpenApiOperation; +} & OpenApiRef; +type OpenApiServer = { + url?: string; + variables?: Record; +}; +type OpenApiSpec = { + openapi?: string; + swagger?: string; + paths?: Record; + components?: { parameters?: Record }; + servers?: OpenApiServer[]; +}; + // TypeChat translator for structured CLI help extraction function createCliDiscoveryTranslator(): TypeChatJsonTranslator { const model = getDiscoveryModel(); @@ -372,7 +420,7 @@ function isInternalAction(name: string): boolean { // defaults are all left unset — the REST handler generator then falls back // to the stub handler rather than emitting a malformed request). export function resolveOpenApiBaseUrl( - spec: any, + spec: OpenApiSpec, specSource: string, fetchedUrl?: string, ): string | undefined { @@ -397,7 +445,7 @@ export function resolveOpenApiBaseUrl( // Substitute server variables ({var}) from servers[0].variables[var].default. // If any referenced variable lacks a default, bail out rather than emit // a malformed URL containing a literal "{var}". - const variables = spec.servers[0].variables ?? {}; + const variables = spec.servers?.[0]?.variables ?? {}; let substituted = rawServerUrl; const varNames = [...rawServerUrl.matchAll(/\{([^}]+)\}/g)].map( (m) => m[1], @@ -443,7 +491,10 @@ export function resolveOpenApiBaseUrl( // other pointer shape (external file refs, `#/definitions/...` from // Swagger 2, deeper paths) returns undefined and the caller treats the // parameter as unresolvable. -function resolveLocalParameterRef(spec: any, ref: string): any | undefined { +function resolveLocalParameterRef( + spec: OpenApiSpec, + ref: string, +): OpenApiParameter | undefined { const match = /^#\/components\/parameters\/([^/]+)$/.exec(ref); if (!match) return undefined; const resolved = spec?.components?.parameters?.[match[1]]; @@ -460,51 +511,58 @@ function resolveLocalParameterRef(spec: any, ref: string): any | undefined { // parameter isn't dropped; any other `$ref` shape (external files, deeper // pointers) stays unresolved and is skipped. function mergeOperationParameters( - spec: any, - pathLevelParams: any[], - op: any, + spec: OpenApiSpec, + pathLevelParams: OpenApiParameter[], + op: OpenApiOperation, ): DiscoveredParameter[] { - const opLevelParams: any[] = Array.isArray(op.parameters) + const opLevelParams: OpenApiParameter[] = Array.isArray(op.parameters) ? op.parameters : []; - const mergedByKey = new Map(); + const mergedByKey = new Map(); for (const raw of [...pathLevelParams, ...opLevelParams]) { if (!raw) continue; const p = raw.$ref ? resolveLocalParameterRef(spec, raw.$ref) : raw; if (!p || p.$ref || !p.name) continue; mergedByKey.set(`${p.in ?? "query"}:${p.name}`, p); } - return Array.from(mergedByKey.values()).map((p: any) => ({ - name: p.name, - type: p.schema?.type ?? "string", - description: p.description, - required: p.required ?? false, - in: p.in, - })); + return Array.from(mergedByKey.values()).map( + (p): DiscoveredParameter => + ({ + name: p.name, + type: p.schema?.type ?? "string", + description: p.description, + required: p.required ?? false, + in: p.in, + // The guard loop above guarantees `name` is set and `in` is a + // valid OpenAPI location; assert the mapping since the source + // fields are optional/loosely typed on the parsed spec. + }) as DiscoveredParameter, + ); } // Map an operation's JSON request-body properties to `in: "body"` parameters. -function extractBodyParameters(op: any): DiscoveredParameter[] { +function extractBodyParameters(op: OpenApiOperation): DiscoveredParameter[] { const requestBody = op.requestBody?.content?.["application/json"]?.schema; if (!requestBody?.properties) return []; - return (Object.entries(requestBody.properties) as [string, any][]).map( - ([propName, propSchema]): DiscoveredParameter => ({ - name: propName, - type: propSchema.type ?? "string", - description: propSchema.description, - required: requestBody.required?.includes(propName) ?? false, - in: "body", - }), + return Object.entries(requestBody.properties).map( + ([propName, propSchema]): DiscoveredParameter => + ({ + name: propName, + type: propSchema.type ?? "string", + description: propSchema.description, + required: requestBody.required?.includes(propName) ?? false, + in: "body", + }) as DiscoveredParameter, ); } // Build a single DiscoveredAction from one path/method/operation. function buildActionForOperation( - spec: any, + spec: OpenApiSpec, pathStr: string, - pathLevelParams: any[], + pathLevelParams: OpenApiParameter[], method: string, - op: any, + op: OpenApiOperation, specSource: string, ): DiscoveredAction { const name = @@ -535,20 +593,19 @@ const OPENAPI_HTTP_METHODS = ["get", "post", "put", "patch", "delete"] as const; // have the shape below) spec document. Pure/synchronous so it can be unit // tested without going through the workspace-state-backed action handler. export function extractOpenApiActions( - spec: any, + spec: OpenApiSpec, specSource: string, ): DiscoveredAction[] { const actions: DiscoveredAction[] = []; - const paths = spec.paths ?? {}; - for (const [pathStr, pathItem] of Object.entries(paths) as [ - string, - any, - ][]) { + const paths: Record = spec.paths ?? {}; + for (const [pathStr, pathItem] of Object.entries(paths)) { // Deterministic inline-OpenAPI-3 subset only — a `$ref`'d path item // (shared path referencing a component) is out of scope for v1. if (pathItem?.$ref) continue; - const pathLevelParams: any[] = Array.isArray(pathItem?.parameters) + const pathLevelParams: OpenApiParameter[] = Array.isArray( + pathItem?.parameters, + ) ? pathItem.parameters : []; @@ -611,7 +668,7 @@ async function handleParseOpenApiSpec( }; } - let spec: any; + let spec: OpenApiSpec; try { spec = JSON.parse(specContent); } catch { From 8503913d58426d8094f0cfb3b91da7438452e341 Mon Sep 17 00:00:00 2001 From: Copilot App <223556219+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:08:57 -0700 Subject: [PATCH 7/7] Centralize REST-handler response truncation into agent-sdk helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add DEFAULT_MAX_RESPONSE_CHARS and a reusable truncateText(text, maxChars?) to agent-sdk's action helpers, and have the generated REST handler template import truncateText instead of inlining its own MAX_RESPONSE_CHARS constant and truncate() function. Behavior is unchanged (same 4000-char cap and '…(truncated)' marker). Full model-aware sizing is intentionally deferred to the host/dispatcher, which knows the selected model; standalone codegen has no model context. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../agentSdk/src/helpers/actionHelpers.ts | 16 ++++++++++++++++ .../src/scaffolder/restHandler.template | 14 ++++---------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/ts/packages/agentSdk/src/helpers/actionHelpers.ts b/ts/packages/agentSdk/src/helpers/actionHelpers.ts index 14d0224150..21f305f94a 100644 --- a/ts/packages/agentSdk/src/helpers/actionHelpers.ts +++ b/ts/packages/agentSdk/src/helpers/actionHelpers.ts @@ -459,3 +459,19 @@ export function actionResultToString(actionResult: ActionResult): string { return fields.join("\n"); } } + +// Default cap on characters of a tool/action response body echoed back to the +// dispatcher. Conservative v1 default; not yet model-aware — a model-derived +// budget belongs in the host where the selected model is known. Callers should +// reference this instead of inlining a magic number. +export const DEFAULT_MAX_RESPONSE_CHARS = 4000; + +// Truncate `text` to at most `maxChars`, appending a marker when clipped. +export function truncateText( + text: string, + maxChars: number = DEFAULT_MAX_RESPONSE_CHARS, +): string { + return text.length > maxChars + ? text.slice(0, maxChars) + "\n…(truncated)" + : text; +} diff --git a/ts/packages/agents/onboarding/src/scaffolder/restHandler.template b/ts/packages/agents/onboarding/src/scaffolder/restHandler.template index fb038dcc36..352e8bfc81 100644 --- a/ts/packages/agents/onboarding/src/scaffolder/restHandler.template +++ b/ts/packages/agents/onboarding/src/scaffolder/restHandler.template @@ -16,23 +16,17 @@ import { } from "@typeagent/agent-sdk"; import { createActionResultFromTextDisplay, + truncateText, } from "@typeagent/agent-sdk/helpers/action"; import { {{PASCAL_NAME}}Actions } from "./{{NAME}}Schema.js"; const BASE_URL = {{BASE_URL}}; -const MAX_RESPONSE_CHARS = 4000; - -function truncate(text: string): string { - return text.length > MAX_RESPONSE_CHARS - ? text.slice(0, MAX_RESPONSE_CHARS) + "\n…(truncated)" - : text; -} function formatResponseBody(text: string): string { try { - return truncate(JSON.stringify(JSON.parse(text), null, 2)); + return truncateText(JSON.stringify(JSON.parse(text), null, 2)); } catch { - return truncate(text); + return truncateText(text); } } @@ -65,7 +59,7 @@ async function callRest( }); const text = await res.text(); if (!res.ok) { - throw new Error(`${res.status} ${res.statusText}: ${truncate(text)}`); + throw new Error(`${res.status} ${res.statusText}: ${truncateText(text)}`); } return formatResponseBody(text); }