diff --git a/CLAUDE.md b/CLAUDE.md index 6102f28..2ffa81d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -9,7 +9,7 @@ A Model Context Protocol (MCP) server that exposes the Kosli API to LLM clients ## Architecture in one minute - **`src/catalog.json`** is the source of truth for available API actions. It's generated by `scripts/generate-catalog.ts` (which fetches `https://app.kosli.com/api/v2/openapi.json`) and committed to the repo. Regenerate with `npm run generate-catalog` when Kosli's API changes. The generator inlines `$ref` schema references (`scripts/resolve-refs.ts`) — the catalog doesn't ship the spec's components section, so an unresolved `$ref` would be a dangling pointer the LLM can't follow. Output is written with a width-capped compact formatter (`scripts/format-json.ts`): structures that fit in 100 columns are inlined, larger ones expand. -- **`src/index.ts`** registers exactly three MCP tools. Don't add more tools per endpoint — keep the catalog-driven design. +- **`src/server.ts`** registers exactly three MCP tools; `src/index.ts` is the bin that builds one and connects stdio. Don't add more tools per endpoint — keep the catalog-driven design. - **`src/tools/search-actions.ts`** does a cheap client-side scored keyword search over `catalog.searchText`. No index, no embedding — deliberately simple. - **`src/tools/execute-action.ts`** + **`src/client/kosli-client.ts`** build the URL from path params, put remaining params into query string (GET/DELETE) or JSON body (other methods), and return the JSON response (or a structured error object — it does not throw on non-2xx). Both execute tools support an optional `fields` array that filters response objects to only the requested keys via `pickFields` before returning — this is critical for token efficiency. - **`src/config.ts`** loads env vars. `KOSLI_API_TOKEN` is preferred; `KOSLI_API_KEY` is a backwards-compat fallback. @@ -26,7 +26,10 @@ A Model Context Protocol (MCP) server that exposes the Kosli API to LLM clients - Commit messages and PR titles follow [Conventional Commits](https://www.conventionalcommits.org/) format (e.g. `feat:`, `fix:`, `docs:`, `ci:`, `refactor:`, `test:`, `chore:`). - ESM only (`"type": "module"`, `NodeNext` resolution). Relative imports must use `.js` extensions even though the source is `.ts`. - TypeScript `strict: true`. Don't weaken it. -- The `org` path parameter falls back to `config.org` (from `KOSLI_ORG`). Preserve this in `KosliClient.buildUrl`. +- There is no configured org. Every action whose path contains `{org}` must be told one per call, so no request lands in an organization the caller did not name. `KosliClient.buildUrl` treats `org` as an ordinary path parameter; don't reintroduce a fallback. +- A per-call org is decided in one place, `resolveOrg` in `src/tools/execute-action.ts`. Registration lives in `createServer` (`src/server.ts`) so a test can drive the real tools over an in-memory transport; `index.ts` is only the bin that connects stdio. Which parameter counts as the org is decided once in `src/org.ts`, because three places have to agree: the check in `resolveOrg`, the strip in `search_actions`, and the key `executeAction` writes the resolved org under. `buildUrl` is generic and reads whatever key the parameter is named. The `org` tool input and `params.org` obey the same rules: trimmed, only a string names one, `null` means not supplied, blank rejected, an org on a non-org-scoped action rejected, and two different names rejected rather than resolved. The body is not a third channel: a body naming `org` is refused by that rule. +- A request body may not name any of the action's own parameters. `unwrapBodyParam` flattens a body over the top level, so such a field would overwrite the caller's value and redirect the request while the approval prompt still showed what they typed. `test/catalog.test.ts` pins that no catalog body declares one. +- `search_actions` strips the `org` path parameter from what it returns. The server supplies it, and advertising it as required invites the model to put it in `params` instead of the tool's own `org` input. `PUT /user/{org}` is exempt: there the org is what the call writes, so the caller has to choose it. - Errors from the Kosli API are returned as `{ error: true, status, statusText, message }` — not thrown. Tools stringify whatever they get. Keep this contract; the LLM handles the error object. - Responses are serialized with `JSON.stringify(result)` (compact, no pretty-printing) to minimize token usage. Don't revert to pretty-printing. - All API requests include `User-Agent: kosli-mcp-server/` for server-side tracking, with the version coming from `VERSION` in `src/version.ts`. Preserve the header and keep the version in it — the backend uses it to tell releases apart. diff --git a/README.md b/README.md index a449694..0b9e0e7 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,6 @@ The server reads configuration from environment variables: | Variable | Required | Default | Notes | |----------|----------|---------|-------| | `KOSLI_API_TOKEN` | yes | — | Preferred. `KOSLI_API_KEY` is accepted as a fallback. | -| `KOSLI_ORG` | yes | — | Default org used when a path param `org` is not supplied. | | `KOSLI_BASE_URL` | no | `https://app.kosli.com` | EU (default), US (`https://app.us.kosli.com`), or your single-tenant endpoint. | ## Wire up to an MCP client @@ -44,13 +43,12 @@ Run this from your project directory (or use `--scope user` for global): ```bash claude mcp add kosli \ -e KOSLI_API_TOKEN=your-token \ - -e KOSLI_ORG=your-org \ -- npx -y @kosli/mcp-server ``` ### Claude Desktop (Desktop Extension) -Download the latest `.mcpb` file from [Releases](https://github.com/kosli-dev/mcp-server/releases), then drag it into Claude Desktop or double-click to install. Claude Desktop will prompt you for your API token and organization. This is the recommended method for Claude Desktop as secrets are stored in the OS keychain rather than in a plain-text config file. +Download the latest `.mcpb` file from [Releases](https://github.com/kosli-dev/mcp-server/releases), then drag it into Claude Desktop or double-click to install. Claude Desktop will prompt you for your API token. This is the recommended method for Claude Desktop as secrets are stored in the OS keychain rather than in a plain-text config file. > [!NOTE] > When installing from a `.mcpb` file, Claude Desktop shows a warning that the extension has not been verified by Anthropic. This is expected for any extension installed from a file rather than from the built-in directory. Sideloaded extensions also do not auto-update — you'll need to download and reinstall new versions manually. Both of these limitations go away once the extension is listed in Anthropic's [Connectors Directory](https://claude.com/docs/connectors/building/submission). @@ -66,8 +64,7 @@ Alternatively, add the following to your `claude_desktop_config.json` (Settings "command": "npx", "args": ["-y", "@kosli/mcp-server"], "env": { - "KOSLI_API_TOKEN": "your-token", - "KOSLI_ORG": "your-org" + "KOSLI_API_TOKEN": "your-token" } } } @@ -76,7 +73,7 @@ Alternatively, add the following to your `claude_desktop_config.json` (Settings ### Other MCP clients -The server communicates over stdio. Point any MCP-compatible client at the package via `npx -y @kosli/mcp-server` and set the `KOSLI_API_TOKEN` and `KOSLI_ORG` environment variables. +The server communicates over stdio. Point any MCP-compatible client at the package via `npx -y @kosli/mcp-server` and set the `KOSLI_API_TOKEN` environment variable. ### Local checkout @@ -89,8 +86,7 @@ If you're running from a local checkout instead: "command": "node", "args": ["/absolute/path/to/mcp-server/dist/index.js"], "env": { - "KOSLI_API_TOKEN": "your-token", - "KOSLI_ORG": "your-org" + "KOSLI_API_TOKEN": "your-token" } } } @@ -107,7 +103,43 @@ Typical LLM flow: > [!IMPORTANT] > `execute_write_action` creates, modifies, and deletes real resources in your Kosli organization. MCP clients gate these calls behind an approval prompt — read the action ID and parameters before approving. An LLM may select the wrong action, or the right action with the wrong parameters, and approval is the only checkpoint before the call is made. Treat deletions and anything touching service accounts or API keys with particular care. -The `org` path parameter defaults to `KOSLI_ORG` if not supplied. For `GET`/`DELETE`, non-path params become query parameters; for other methods they become the JSON body. +For `GET`/`DELETE`, non-path params become query parameters; for other methods they become the JSON body. + +### Naming the organization + +Both execute tools take an `org`, so one running server reaches any organization +your token has access to: + +```json +{ "actionId": "list_environments", "org": "cyber-dojo", "fields": ["name"] } +``` + +It is required for every action whose path contains `{org}`, which is all but +four of them. There is no configured default: a call that names no org is +refused rather than sent to one nobody chose. + +`KOSLI_ORG` used to supply that default and has been removed. A config that +still sets it starts fine, the value is ignored, and the server says so on +stderr. + +An org can also arrive as `params.org`. The two must agree: two different names +are rejected rather than resolved, since the same tools perform writes. + +A write's request body may not name any of the action's own parameters, `org` +included. Such a field would overwrite what you supplied and send the request +somewhere else, while the approval prompt still showed your value. + +The org must be one name, given as a non-empty string. Anything else is rejected +rather than coerced, because coercing invents a name: `["cyber-dojo", +"kosli-public"]` would request the org `cyber-dojo,kosli-public`. + +Four actions are not organization-scoped: `get_user_default_org`, +`list_system_attestation_types`, and the two `/schemas/...` actions. Passing an +org to one of those is rejected rather than ignored. + +An org your token cannot reach returns the usual error object with the API's +status, `403` for both a non-member and a non-existent org. Kosli staff accounts +can read any org, so a successful read does not prove a write would be allowed. Both execute tools accept an optional `fields` array to request only specific top-level fields from each object in the response. This dramatically reduces response size and token usage: @@ -141,7 +173,9 @@ npm run pack:mcpb # build a .mcpb bundle for Claude Desktop ``` src/ - index.ts # MCP server entry point (stdio transport) + index.ts # bin: builds a server and connects stdio + org.ts # which parameter is the org, shared by three places + server.ts # registers the three MCP tools config.ts # env-var loading types.ts # shared types (CatalogEntry, Config, …) catalog.json # generated action catalog diff --git a/docs/future/cli-mcp.md b/docs/future/cli-mcp.md index f9e7a1e..9230ba9 100644 --- a/docs/future/cli-mcp.md +++ b/docs/future/cli-mcp.md @@ -66,4 +66,4 @@ Until any of that is real, the API MCP (plus the multipart gap above, if it beco ## What explicitly doesn't carry over - The OpenAPI catalog generator — CLI commands don't correspond 1:1 to API endpoints (one CLI command often calls several endpoints plus local work). -- The `org` fallback from env — the CLI has its own profile logic; don't double-configure. +- Any notion of a configured org — the MCP server requires one per call, and the CLI has its own profile logic. diff --git a/manifest.json b/manifest.json index 86e271c..099c1ed 100644 --- a/manifest.json +++ b/manifest.json @@ -26,7 +26,6 @@ "args": ["${__dirname}/dist/index.js"], "env": { "KOSLI_API_TOKEN": "${user_config.api_token}", - "KOSLI_ORG": "${user_config.org}", "KOSLI_BASE_URL": "${user_config.base_url}" } } @@ -54,12 +53,6 @@ "sensitive": true, "required": true }, - "org": { - "type": "string", - "title": "Organization", - "description": "Your Kosli organization name", - "required": true - }, "base_url": { "type": "string", "title": "Base URL", diff --git a/src/client/kosli-client.ts b/src/client/kosli-client.ts index 8ef81ce..38f19b9 100644 --- a/src/client/kosli-client.ts +++ b/src/client/kosli-client.ts @@ -126,12 +126,7 @@ export class KosliClient { for (const param of entry.parameters) { if (param.in !== "path") continue; - let value: unknown; - if (param.name === "org") { - value = params.org ?? this.config.org; - } else { - value = params[param.name]; - } + const value = params[param.name]; if (value === undefined && param.required) { return { diff --git a/src/config.ts b/src/config.ts index 74f92a1..ac4c8b4 100644 --- a/src/config.ts +++ b/src/config.ts @@ -6,9 +6,12 @@ export function loadConfig(): Config { throw new Error("KOSLI_API_TOKEN (or KOSLI_API_KEY) environment variable is required"); } - const org = process.env.KOSLI_ORG; - if (!org) { - throw new Error("KOSLI_ORG environment variable is required"); + // Removed in favour of naming the org per call. Say so rather than letting a + // stale config turn into a refusal the user cannot connect to anything. + if (process.env.KOSLI_ORG) { + console.error( + "KOSLI_ORG is set but no longer used: the org is named per call now, and this value is ignored.", + ); } const baseUrl = process.env.KOSLI_BASE_URL || "https://app.kosli.com"; @@ -24,5 +27,5 @@ export function loadConfig(): Config { ); } - return { apiKey, org, baseUrl }; + return { apiKey, baseUrl }; } diff --git a/src/index.ts b/src/index.ts index 2f3a35b..76ef4b8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,107 +1,11 @@ #!/usr/bin/env node -import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; -import { z } from "zod"; import { loadConfig } from "./config.js"; -import { searchActions } from "./tools/search-actions.js"; -import { executeAction } from "./tools/execute-action.js"; -import catalog from "./catalog.json" with { type: "json" }; -import hints from "./hints.json" with { type: "json" }; -import type { CatalogEntry, ActionHints } from "./types.js"; -import { VERSION } from "./version.js"; - -const entries = catalog as CatalogEntry[]; -const config = loadConfig(); - -const server = new McpServer({ - name: "kosli", - version: VERSION, -}); - -server.registerTool( - "search_actions", - { - title: "Search actions", - description: `Search for available Kosli API actions by natural-language query. Returns matching actions with their IDs, descriptions, and parameter schemas. Use this to discover what actions are available before calling execute_read_action or execute_write_action. The configured Kosli org is "${config.org}".`, - annotations: { - readOnlyHint: true, - }, - inputSchema: { - query: z.string().describe("Natural-language search query (e.g. 'list environments', 'get trail', 'search artifacts')"), - limit: z.number().optional().default(10).describe("Maximum number of results to return"), - }, - }, - async ({ query, limit }) => { - const results = searchActions(entries, query, limit, hints as ActionHints); - return { - content: [ - { - type: "text" as const, - text: JSON.stringify(results), - }, - ], - }; - }, -); - -server.registerTool( - "execute_read_action", - { - title: "Execute read action", - description: `Execute a read-only (GET) Kosli API action by its ID with the given parameters. Use search_actions first to find the action ID and required parameters. The 'org' parameter defaults to "${config.org}" — you do not need to supply it unless querying a different organization.`, - annotations: { - readOnlyHint: true, - }, - inputSchema: { - actionId: z.string().describe("The action ID from search_actions results"), - params: z.record(z.string(), z.unknown()).optional().default({}).describe("Parameters for the action (path params or query params)"), - fields: z.array(z.string()).optional().describe("Only include these fields in each object of the response. Dramatically reduces response size. Example: [\"name\",\"compliant\",\"fingerprint\",\"reasons_for_incompliance\"]"), - }, - }, - async ({ actionId, params, fields }) => { - const result = await executeAction(entries, config, actionId, params, fields, undefined, "GET"); - return { - content: [ - { - type: "text" as const, - text: JSON.stringify(result), - }, - ], - }; - }, -); - -server.registerTool( - "execute_write_action", - { - title: "Execute write action", - description: `Execute a write (POST, PUT, PATCH, DELETE) Kosli API action by its ID with the given parameters. Use search_actions first to find the action ID and required parameters. The 'org' parameter defaults to "${config.org}" — you do not need to supply it unless querying a different organization.`, - annotations: { - destructiveHint: true, - readOnlyHint: false, - }, - inputSchema: { - actionId: z.string().describe("The action ID from search_actions results"), - params: z.record(z.string(), z.unknown()).optional().default({}).describe("Parameters for the action (path params, query params, or body)"), - fields: z.array(z.string()).optional().describe("Only include these fields in each object of the response. Dramatically reduces response size."), - }, - }, - async ({ actionId, params, fields }) => { - const result = await executeAction(entries, config, actionId, params, fields, undefined, "WRITE"); - return { - content: [ - { - type: "text" as const, - text: JSON.stringify(result), - }, - ], - }; - }, -); +import { createServer } from "./server.js"; async function main() { - const transport = new StdioServerTransport(); - await server.connect(transport); + const server = createServer(loadConfig()); + await server.connect(new StdioServerTransport()); } main().catch((err) => { diff --git a/src/org.ts b/src/org.ts new file mode 100644 index 0000000..1b8126c --- /dev/null +++ b/src/org.ts @@ -0,0 +1,27 @@ +import type { ActionParam, CatalogEntry } from "./types.js"; + +/** + * Three places have to agree on which parameter the org is: the check in + * resolveOrg, the strip in search-actions, and the key executeAction writes the + * resolved value under. buildUrl is generic and reads whatever key the + * parameter is named. Widen this and forget that key and the call dies with + * "Missing required path parameter", which is why it is decided once here. + */ +export function isOrgPathParam(param: ActionParam): boolean { + return param.name === "org" && param.in === "path"; +} + +export function takesOrg(entry: CatalogEntry): boolean { + return entry.parameters.some(isOrgPathParam); +} + +/** + * `PUT /user/{org}` sets the caller's default org, so its `{org}` is the thing + * being written, not the scope it is written in. It is the one action whose org + * the caller must choose, so search must keep advertising it. Matched on path + * rather than id: the id has already changed once (`put_user_default_org` to + * `set_user_default_org`) while the path stayed put. + */ +export function orgIsTheArgument(entry: CatalogEntry): boolean { + return entry.path === "/user/{org}"; +} diff --git a/src/server.ts b/src/server.ts new file mode 100644 index 0000000..5af24b0 --- /dev/null +++ b/src/server.ts @@ -0,0 +1,102 @@ +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { z } from "zod"; +import { searchActions } from "./tools/search-actions.js"; +import { executeAction, type ToolMode } from "./tools/execute-action.js"; +import catalog from "./catalog.json" with { type: "json" }; +import hints from "./hints.json" with { type: "json" }; +import type { CatalogEntry, ActionHints, Config } from "./types.js"; +import { VERSION } from "./version.js"; + +const entries = catalog as CatalogEntry[]; + +/** Builds the server with its three tools registered. */ +export function createServer(config: Config) { + const ORG_INPUT = z.string().nullable().optional().describe( + 'Kosli organization to run this call against, e.g. "cyber-dojo". Required for every action whose path contains {org}, which is all but four of them. There is no default: a call that names no org is refused rather than sent somewhere you did not choose.', + ); + + const run = (mode: ToolMode) => + async ({ actionId, params, fields, org }: { + actionId: string; + params: Record; + fields?: string[]; + org?: string | null; + }) => ({ + content: [{ + type: "text" as const, + text: JSON.stringify( + await executeAction(entries, config, actionId, params, { fields, mode, org }), + ), + }], + }); + + const server = new McpServer({ + name: "kosli", + version: VERSION, + }); + + server.registerTool( + "search_actions", + { + title: "Search actions", + description: "Search for available Kosli API actions by natural-language query. Returns matching actions with their IDs, descriptions, and parameter schemas. Use this to discover what actions are available before calling execute_read_action or execute_write_action.", + annotations: { + readOnlyHint: true, + }, + inputSchema: { + query: z.string().describe("Natural-language search query (e.g. 'list environments', 'get trail', 'search artifacts')"), + limit: z.number().optional().default(10).describe("Maximum number of results to return"), + }, + }, + async ({ query, limit }) => { + const results = searchActions(entries, query, limit, hints as ActionHints); + return { + content: [ + { + type: "text" as const, + text: JSON.stringify(results), + }, + ], + }; + }, + ); + + server.registerTool( + "execute_read_action", + { + title: "Execute read action", + description: "Execute a read-only (GET) Kosli API action by its ID with the given parameters. Use search_actions first to find the action ID and required parameters. Set 'org' to say which organization the call runs against; there is no default.", + annotations: { + readOnlyHint: true, + }, + inputSchema: { + actionId: z.string().describe("The action ID from search_actions results"), + org: ORG_INPUT, + params: z.record(z.string(), z.unknown()).optional().default({}).describe("Parameters for the action (path params or query params)"), + fields: z.array(z.string()).optional().describe("Only include these fields in each object of the response. Dramatically reduces response size. Example: [\"name\",\"compliant\",\"fingerprint\",\"reasons_for_incompliance\"]"), + }, + }, + run("GET"), + ); + + server.registerTool( + "execute_write_action", + { + title: "Execute write action", + description: "Execute a write (POST, PUT, PATCH, DELETE) Kosli API action by its ID with the given parameters. Use search_actions first to find the action ID and required parameters. Set 'org' to say which organization the write lands in; there is no default, so check it before approving.", + annotations: { + destructiveHint: true, + readOnlyHint: false, + }, + inputSchema: { + actionId: z.string().describe("The action ID from search_actions results"), + org: ORG_INPUT, + params: z.record(z.string(), z.unknown()).optional().default({}).describe("Parameters for the action (path params, query params, or body)"), + fields: z.array(z.string()).optional().describe("Only include these fields in each object of the response. Dramatically reduces response size."), + }, + }, + run("WRITE"), + ); + + return server; +} diff --git a/src/tools/execute-action.ts b/src/tools/execute-action.ts index ce16dce..dd2d96e 100644 --- a/src/tools/execute-action.ts +++ b/src/tools/execute-action.ts @@ -1,5 +1,6 @@ import type { CatalogEntry, Config } from "../types.js"; import { KosliClient } from "../client/kosli-client.js"; +import { takesOrg } from "../org.js"; type FetchFn = typeof globalThis.fetch; @@ -38,22 +39,108 @@ export type ToolMode = "GET" | "WRITE"; * when it's unambiguous: the entry takes a request body, nothing it declares * is genuinely called "body", and every sibling key is a declared parameter. */ +/** + * The params to send, or the reason the call cannot be made. The payload is + * nested rather than returned bare: it is caller-supplied JSON, so a field of + * its own named `error` would otherwise read as a failure and cancel the call. + */ +interface Unwrapped { + params: Record; + collision?: string; +} + function unwrapBodyParam( entry: CatalogEntry, params: Record, -): Record { +): Unwrapped { const body = params.body; - if (body === null || typeof body !== "object" || Array.isArray(body)) return params; - if (!entry.requestBody) return params; - if (entry.parameters.some((p) => p.name === "body")) return params; + if (body === null || typeof body !== "object" || Array.isArray(body)) return { params }; + if (!entry.requestBody) return { params }; + if (entry.parameters.some((p) => p.name === "body")) return { params }; const schema = entry.requestBody[0]?.schema; const properties = schema?.properties; - if (properties && typeof properties === "object" && "body" in properties) return params; + if (properties && typeof properties === "object" && "body" in properties) return { params }; const declared = new Set(entry.parameters.map((p) => p.name)); + + // A body field sharing a name with one of the action's own parameters would + // overwrite what the caller put at the top level and send the request + // somewhere else, while the approval prompt still showed the caller's value. + // No catalog body declares such a field, so a collision is always a mistake. + const collisions = Object.keys(body as Record).filter((k) => declared.has(k)); + if (collisions.length > 0) { + const names = collisions.map((c) => `"${c}"`); + const listed = names.length > 1 + ? `${names.slice(0, -1).join(", ")} and ${names[names.length - 1]}` + : names[0]; + const [are, them, parameters] = collisions.length > 1 + ? ["are", "them", "parameters"] + : ["is", "it", "a parameter"]; + return { + params, + collision: `The request body names ${listed}, which ${are} also ${parameters} of "${entry.id}". Supply ${them} once, outside the body.`, + }; + } + const siblings = Object.keys(params).filter((k) => k !== "body"); - if (!siblings.every((k) => declared.has(k))) return params; + if (!siblings.every((k) => declared.has(k))) return { params }; const { body: _unwrapped, ...rest } = params; - return { ...rest, ...(body as Record) }; + return { params: { ...rest, ...(body as Record) } }; +} + +type OrgResolution = { error: string } | { org?: string }; + +/** Trimmed, or `""` when supplied but unusable; `undefined` when not supplied. */ +function normalizeOrg(value: unknown): string | undefined { + if (value === undefined || value === null) return undefined; + // Coercion would invent a name: ["a", "b"] reads as the org "a,b", and an org + // id as an org called "1234". Both would then pass as a single name. + if (typeof value !== "string") return ""; + return value.trim(); +} + +/** + * The one place the target org is decided. A call can name one two ways, as the + * tool input or as `params.org`, and they must agree, because this same path + * performs writes. There is no fallback: an org-scoped action with no org named + * is refused. + */ +function resolveOrg( + entry: CatalogEntry, + org: string | null | undefined, + params: Record, +): OrgResolution { + const named = [...new Set( + [org, params.org].map(normalizeOrg).filter((o) => o !== undefined), + )]; + + if (!takesOrg(entry)) { + return named.length === 0 + ? {} + : { error: `Action "${entry.id}" is not organization-scoped — it takes no org. Retry with no org in the org parameter and none in params.org.` }; + } + + if (named.includes("")) { + return { error: "The org must be a single organization name, given as a non-empty string. Check the org parameter and params.org." }; + } + + if (named.length > 1) { + const quoted = named.map((o) => `"${o}"`).join(" and "); + return { error: `Conflicting orgs in one call: ${quoted}. The org parameter and params.org must agree — supply just one.` }; + } + + if (named.length === 0) { + return { error: `Action "${entry.id}" runs against one organization and none was named. Set the org parameter. There is no default, so that no call lands somewhere the caller did not choose.` }; + } + + return { org: named[0] }; +} + +export interface ExecuteOptions { + fields?: string[]; + fetchFn?: FetchFn; + mode?: ToolMode; + /** `null` counts as not supplied, the same as `params.org`. */ + org?: string | null; } export async function executeAction( @@ -61,9 +148,7 @@ export async function executeAction( config: Config, actionId: string, params: Record, - fields?: string[], - fetchFn: FetchFn = globalThis.fetch, - mode?: ToolMode, + { fields, fetchFn = globalThis.fetch, mode, org }: ExecuteOptions = {}, ): Promise { const entry = catalog.find((e) => e.id === actionId); if (!entry) { @@ -84,8 +169,23 @@ export async function executeAction( }; } + // The org is read from `params` as the caller wrote it. The unwrap cannot + // introduce another one, because a body naming a declared parameter is + // refused above rather than flattened over the top level. + const { params: unwrapped, collision } = unwrapBodyParam(entry, params); + if (collision) return { error: true, message: collision }; + + const resolved = resolveOrg(entry, org, params); + if ("error" in resolved) return { error: true, message: resolved.error }; + + // The key here has to match what org.ts calls the org, since buildUrl looks + // the path segment up by parameter name. + const withOrg = { ...unwrapped }; + if (resolved.org === undefined) delete withOrg.org; + else withOrg.org = resolved.org; + const client = new KosliClient(config, fetchFn); - const result = await client.execute(entry, unwrapBodyParam(entry, params)); + const result = await client.execute(entry, withOrg); // Never strip error shapes — pickFields would reduce them to {} // and hide the failure from the LLM. diff --git a/src/tools/search-actions.ts b/src/tools/search-actions.ts index 193fff7..6575d16 100644 --- a/src/tools/search-actions.ts +++ b/src/tools/search-actions.ts @@ -1,4 +1,5 @@ import type { ActionHint, ActionHints, CatalogEntry } from "../types.js"; +import { isOrgPathParam, orgIsTheArgument } from "../org.js"; export interface SearchResult { id: string; @@ -47,7 +48,12 @@ export function searchActions( path: entry.path, summary: entry.summary, tags: entry.tags, - parameters: entry.parameters, + // The tool's own `org` input carries it, so advertising it here as well + // only invites the model to put it in `params`. Except where the org is + // the thing being written, and belongs with the action's own arguments. + parameters: orgIsTheArgument(entry) + ? entry.parameters + : entry.parameters.filter((p) => !isOrgPathParam(p)), requestBody: entry.requestBody, }; const hint = hints?.[entry.id]; diff --git a/src/types.ts b/src/types.ts index 59303e9..2022023 100644 --- a/src/types.ts +++ b/src/types.ts @@ -27,7 +27,6 @@ export interface CatalogEntry { export interface Config { apiKey: string; - org: string; baseUrl: string; } diff --git a/test/catalog.test.ts b/test/catalog.test.ts index 6342b46..cf50989 100644 --- a/test/catalog.test.ts +++ b/test/catalog.test.ts @@ -17,4 +17,56 @@ describe("catalog.json", () => { const ids = catalog.map((entry) => entry.id); expect(new Set(ids).size).toBe(ids.length); }); + + // The README names these four. Update both together when the spec moves. + it("has exactly the four documented actions that take no org", () => { + const withoutOrg = catalog + .filter((entry) => !entry.parameters.some((p) => p.name === "org" && p.in === "path")) + .map((entry) => entry.id) + .sort(); + + expect(withoutOrg).toEqual([ + "get_environment_policy_schema_v1", + "get_flow_template_schema_v1", + "get_user_default_org", + "list_system_attestation_types", + ]); + }); + + it("never declares org as a query or header parameter", () => { + const offenders = catalog + .filter((entry) => entry.parameters.some((p) => p.name === "org" && p.in !== "path")) + .map((entry) => entry.id); + + expect(offenders).toEqual([]); + }); + + // A write's request body is flattened over the top level, so a body field + // sharing a name with one of the action's own parameters would overwrite the + // caller's value. executeAction refuses such a call, which is only safe while + // no legitimate body declares one. Matched against the whole schema rather + // than its top-level properties, so a field behind an allOf still counts: a + // hit means read the catalog diff. + it("never names one of an action's own parameters anywhere in its request body schema", () => { + const offenders = catalog.flatMap((entry) => { + const schema = JSON.stringify((entry.requestBody ?? []).map((body) => body.schema ?? {})); + return entry.parameters + .filter((p) => schema.includes(`"${p.name}"`)) + .map((p) => `${entry.id}.${p.name}`); + }); + + expect(offenders).toEqual([]); + }); + + // The blunt half of the same guard: "org" anywhere in a body schema, however + // it is nested, is worth a human look even when it is not a top-level field. + it("never mentions org in a request body schema", () => { + const offenders = catalog + .filter((entry) => + entry.requestBody?.some((body) => JSON.stringify(body.schema ?? {}).includes('"org"')), + ) + .map((entry) => entry.id); + + expect(offenders).toEqual([]); + }); }); diff --git a/test/client/kosli-client.test.ts b/test/client/kosli-client.test.ts index 51f523a..58b7f1c 100644 --- a/test/client/kosli-client.test.ts +++ b/test/client/kosli-client.test.ts @@ -5,9 +5,7 @@ import type { CatalogEntry, Config } from "../../src/types.js"; const config: Config = { apiKey: "test-api-key", - org: "test-org", baseUrl: "https://app.kosli.com", - readOnly: true, }; const listEnvEntry: CatalogEntry = { @@ -89,14 +87,14 @@ describe("KosliClient", () => { client = new KosliClient(config, mockFetch); }); - it("builds correct URL with org auto-injected", async () => { + it("fills the org path segment from params", async () => { mockFetch.mockResolvedValue({ ok: true, status: 200, json: () => Promise.resolve({ environments: [] }), }); - await client.execute(listEnvEntry, {}); + await client.execute(listEnvEntry, { org: "test-org" }); expect(mockFetch).toHaveBeenCalledWith( "https://app.kosli.com/api/v2/environments/test-org", @@ -118,6 +116,7 @@ describe("KosliClient", () => { }); await client.execute(getTrailEntry, { + org: "test-org", flow_name: "my-flow", trail_name: "my-trail", }); @@ -128,19 +127,16 @@ describe("KosliClient", () => { ); }); - it("allows org override via params", async () => { - mockFetch.mockResolvedValue({ - ok: true, - status: 200, - json: () => Promise.resolve({ environments: [] }), - }); + // The client knows nothing about orgs: executeAction refuses a call that names + // none, and if that check were ever bypassed this must fail rather than pick. + it("has no org of its own to fall back on", async () => { + const mockFetch = vi.fn(); + const client = new KosliClient(config, mockFetch); - await client.execute(listEnvEntry, { org: "other-org" }); + const result = await client.execute(listEnvEntry, {}); - expect(mockFetch).toHaveBeenCalledWith( - "https://app.kosli.com/api/v2/environments/other-org", - expect.anything(), - ); + expect(result).toMatchObject({ error: true, message: expect.stringContaining("Missing required path parameter: org") }); + expect(mockFetch).not.toHaveBeenCalled(); }); it("returns structured error on non-OK response", async () => { @@ -151,7 +147,7 @@ describe("KosliClient", () => { json: () => Promise.resolve({ message: "Environment not found" }), }); - const result = await client.execute(listEnvEntry, {}); + const result = await client.execute(listEnvEntry, { org: "test-org" }); expect(result).toEqual({ error: true, @@ -164,7 +160,7 @@ describe("KosliClient", () => { it("returns structured error on network failure without leaking raw error detail", async () => { mockFetch.mockRejectedValue(new Error("connect ECONNREFUSED 10.0.0.5:443")); - const result = await client.execute(listEnvEntry, {}); + const result = await client.execute(listEnvEntry, { org: "test-org" }); expect(result).toEqual({ error: true, @@ -177,7 +173,7 @@ describe("KosliClient", () => { }); it("returns structured error on missing required path param (does not throw)", async () => { - const result = await client.execute(getTrailEntry, { flow_name: "my-flow" }); + const result = await client.execute(getTrailEntry, { org: "test-org", flow_name: "my-flow" }); expect(result).toEqual({ error: true, @@ -197,6 +193,7 @@ describe("KosliClient", () => { }); await client.execute(putPolicyEntry, { + org: "test-org", name: "provenance", type: "env", policy_file: { @@ -236,6 +233,7 @@ describe("KosliClient", () => { }); await client.execute(putPolicyEntry, { + org: "test-org", policy_file: { filename: "x.txt", content: "hello" }, }); @@ -251,6 +249,7 @@ describe("KosliClient", () => { }); await client.execute(putPolicyEntry, { + org: "test-org", metadata: { owner: "security-team" }, }); @@ -265,7 +264,7 @@ describe("KosliClient", () => { json: () => Promise.resolve({}), }); - await client.execute(createFlowJsonEntry, { name: "my-flow" }); + await client.execute(createFlowJsonEntry, { org: "test-org", name: "my-flow" }); const [, init] = mockFetch.mock.calls[0]; expect(init.body).toBe('{"name":"my-flow"}'); diff --git a/test/config.test.ts b/test/config.test.ts index df040e2..bc204b8 100644 --- a/test/config.test.ts +++ b/test/config.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { loadConfig } from "../src/config.js"; describe("loadConfig", () => { @@ -15,18 +15,15 @@ describe("loadConfig", () => { it("loads valid config from env vars", () => { delete process.env.KOSLI_API_TOKEN; process.env.KOSLI_API_KEY = "test-key"; - process.env.KOSLI_ORG = "test-org"; const config = loadConfig(); expect(config.apiKey).toBe("test-key"); - expect(config.org).toBe("test-org"); expect(config.baseUrl).toBe("https://app.kosli.com"); }); it("uses custom base URL when provided", () => { process.env.KOSLI_API_KEY = "test-key"; - process.env.KOSLI_ORG = "test-org"; process.env.KOSLI_BASE_URL = "https://staging.kosli.com"; const config = loadConfig(); @@ -37,7 +34,6 @@ describe("loadConfig", () => { it("prefers KOSLI_API_TOKEN over KOSLI_API_KEY", () => { process.env.KOSLI_API_TOKEN = "token-value"; process.env.KOSLI_API_KEY = "key-value"; - process.env.KOSLI_ORG = "test-org"; const config = loadConfig(); @@ -47,7 +43,6 @@ describe("loadConfig", () => { it("falls back to KOSLI_API_KEY when KOSLI_API_TOKEN is missing", () => { delete process.env.KOSLI_API_TOKEN; process.env.KOSLI_API_KEY = "key-value"; - process.env.KOSLI_ORG = "test-org"; const config = loadConfig(); @@ -55,23 +50,25 @@ describe("loadConfig", () => { }); it("throws when both KOSLI_API_TOKEN and KOSLI_API_KEY are missing", () => { - process.env.KOSLI_ORG = "test-org"; delete process.env.KOSLI_API_KEY; delete process.env.KOSLI_API_TOKEN; expect(() => loadConfig()).toThrow("KOSLI_API_TOKEN"); }); - it("throws when KOSLI_ORG is missing", () => { + it("says so when a stale KOSLI_ORG is still set", () => { process.env.KOSLI_API_KEY = "test-key"; - delete process.env.KOSLI_ORG; + process.env.KOSLI_ORG = "left-over"; + const warn = vi.spyOn(console, "error").mockImplementation(() => {}); - expect(() => loadConfig()).toThrow("KOSLI_ORG"); + loadConfig(); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining("KOSLI_ORG is set but no longer used")); + warn.mockRestore(); }); it("throws when KOSLI_BASE_URL is not https", () => { process.env.KOSLI_API_KEY = "test-key"; - process.env.KOSLI_ORG = "test-org"; process.env.KOSLI_BASE_URL = "http://evil.example.com"; expect(() => loadConfig()).toThrow(/https/); @@ -79,7 +76,6 @@ describe("loadConfig", () => { it("throws when KOSLI_BASE_URL is not a valid URL", () => { process.env.KOSLI_API_KEY = "test-key"; - process.env.KOSLI_ORG = "test-org"; process.env.KOSLI_BASE_URL = "not a url"; expect(() => loadConfig()).toThrow(/not a valid URL/); diff --git a/test/fixtures/catalog-subset.json b/test/fixtures/catalog-subset.json index fb3bdd1..37547e9 100644 --- a/test/fixtures/catalog-subset.json +++ b/test/fixtures/catalog-subset.json @@ -125,5 +125,46 @@ { "name": "body", "required": true, "description": "Request body (multipart/form-data)" } ], "searchText": "create or update policy create or update a policy in an organization. put policies org policies" + }, + { + "id": "get_user_default_org", + "method": "GET", + "path": "/user/default-org", + "summary": "Get default organization", + "description": "Get the default org for the current user.", + "tags": ["User"], + "parameters": [], + "requestBody": null, + "searchText": "get default organization get the default org for the current user. get user default-org user" + }, + { + "id": "set_user_default_org", + "method": "PUT", + "path": "/user/{org}", + "summary": "Set default organization", + "description": "Set a default org for the current user.", + "tags": ["User"], + "parameters": [ + { "name": "org", "in": "path", "required": true, "description": "" } + ], + "requestBody": null, + "searchText": "set default organization set a default org for the current user. put user org user" + }, + { + "id": "create_artifact", + "method": "POST", + "path": "/artifacts/{org}/{flow_name}", + "summary": "Report artifact", + "description": "Report an artifact to a flow.", + "tags": ["Artifacts"], + "parameters": [ + { "name": "org", "in": "path", "required": true, "description": "" }, + { "name": "flow_name", "in": "path", "required": true, "description": "" } + ], + "requestBody": [ + { "name": "body", "required": true, "description": "Request body (application/json)", + "schema": { "type": "object", "properties": { "fingerprint": { "type": "string" }, "trail_name": { "type": "string" } } } } + ], + "searchText": "report artifact post artifacts org flow_name artifacts" } ] diff --git a/test/org.test.ts b/test/org.test.ts new file mode 100644 index 0000000..00982dc --- /dev/null +++ b/test/org.test.ts @@ -0,0 +1,26 @@ +import { describe, it, expect } from "vitest"; +import { isOrgPathParam, orgIsTheArgument, takesOrg } from "../src/org.js"; +import type { CatalogEntry } from "../src/types.js"; + +const entry = (over: Partial): CatalogEntry => ({ + id: "x", method: "GET", path: "/x/{org}", summary: "", description: "", + tags: [], parameters: [], requestBody: null, searchText: "", ...over, +}); + +describe("org predicates", () => { + it("counts only a path parameter named org", () => { + expect(isOrgPathParam({ name: "org", in: "path", required: true, description: "" })).toBe(true); + expect(isOrgPathParam({ name: "org", in: "query", required: false, description: "" })).toBe(false); + expect(isOrgPathParam({ name: "flow", in: "path", required: true, description: "" })).toBe(false); + }); + + it("reads org-scope off the parameters, as buildUrl does", () => { + expect(takesOrg(entry({ parameters: [{ name: "org", in: "path", required: true, description: "" }] }))).toBe(true); + expect(takesOrg(entry({ parameters: [] }))).toBe(false); + }); + + it("singles out the action that writes an org rather than running in one", () => { + expect(orgIsTheArgument(entry({ path: "/user/{org}" }))).toBe(true); + expect(orgIsTheArgument(entry({ path: "/environments/{org}" }))).toBe(false); + }); +}); diff --git a/test/server.test.ts b/test/server.test.ts new file mode 100644 index 0000000..2b62a9b --- /dev/null +++ b/test/server.test.ts @@ -0,0 +1,123 @@ +import { describe, it, expect, vi, beforeAll, afterEach } from "vitest"; +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { createServer } from "../src/server.js"; +import type { Config } from "../src/types.js"; + +// The registered tools are what a client actually sees, so this covers the +// wiring: a schema that no longer admits `org`, or a handler wired to the wrong +// mode, is invisible to a test of executeAction alone. +const config: Config = { + apiKey: "test-key", + baseUrl: "https://app.kosli.com", +}; + +const fetchMock = vi.fn(); +let client: Client; + +beforeAll(async () => { + vi.stubGlobal("fetch", fetchMock); + client = new Client({ name: "test", version: "0" }); + const [clientSide, serverSide] = InMemoryTransport.createLinkedPair(); + await Promise.all([createServer(config).connect(serverSide), client.connect(clientSide)]); +}); + +afterEach(() => fetchMock.mockReset()); + +function respondOk() { + fetchMock.mockResolvedValue({ ok: true, status: 200, json: () => Promise.resolve([]) }); +} + +describe("createServer", () => { + it("offers org on both execute tools and on neither other input", async () => { + const { tools } = await client.listTools(); + const inputs = Object.fromEntries( + tools.map((t) => [t.name, Object.keys(t.inputSchema.properties ?? {})]), + ); + + expect(inputs.execute_read_action).toContain("org"); + expect(inputs.execute_write_action).toContain("org"); + expect(inputs.search_actions).not.toContain("org"); + }); + + it("sends the org a client supplies", async () => { + respondOk(); + + await client.callTool({ + name: "execute_read_action", + arguments: { actionId: "list_environments", org: "cyber-dojo" }, + }); + + expect(fetchMock).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + // params carries every path segment and the whole request body, so a handler + // that forwarded everything except params would still pass the org tests. + it("passes params through to the request", async () => { + respondOk(); + + await client.callTool({ + name: "execute_read_action", + arguments: { + actionId: "get_environment", + org: "cyber-dojo", + params: { env_name: "aws-prod" }, + }, + }); + + expect(fetchMock).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo/aws-prod", + expect.anything(), + ); + }); + + it("refuses a call that names no org, rather than choosing one", async () => { + const result = await client.callTool({ + name: "execute_read_action", + arguments: { actionId: "list_environments" }, + }); + + expect(JSON.stringify(result.content)).toContain("none was named"); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("treats a null org as not supplied, rather than rejecting it at the schema", async () => { + respondOk(); + + const result = await client.callTool({ + name: "execute_read_action", + arguments: { actionId: "list_environments", org: null }, + }); + + expect(result.isError).toBeFalsy(); + expect(JSON.stringify(result.content)).toContain("none was named"); + }); + + it("passes fields through, and returns the result as compact JSON", async () => { + fetchMock.mockResolvedValue({ + ok: true, + status: 200, + json: () => Promise.resolve([{ name: "prod", type: "ECS" }]), + }); + + const result = await client.callTool({ + name: "execute_read_action", + arguments: { actionId: "list_environments", org: "cyber-dojo", fields: ["name"] }, + }); + + expect(result.content).toEqual([{ type: "text", text: '[{"name":"prod"}]' }]); + }); + + it("keeps writes out of the read tool", async () => { + const result = await client.callTool({ + name: "execute_read_action", + arguments: { actionId: "create_control", org: "cyber-dojo" }, + }); + + expect(JSON.stringify(result.content)).toContain("Use execute_write_action instead"); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); diff --git a/test/tools/execute-action.test.ts b/test/tools/execute-action.test.ts index ed9f2ce..97971e7 100644 --- a/test/tools/execute-action.test.ts +++ b/test/tools/execute-action.test.ts @@ -6,7 +6,6 @@ import type { CatalogEntry, Config } from "../../src/types.js"; const entries = catalog as CatalogEntry[]; const config: Config = { apiKey: "test-key", - org: "test-org", baseUrl: "https://app.kosli.com", }; @@ -18,7 +17,7 @@ describe("executeAction", () => { json: () => Promise.resolve({ environments: ["prod", "staging"] }), }); - const result = await executeAction(entries, config, "list_environments", {}, undefined, mockFetch); + const result = await executeAction(entries, config, "list_environments", { org: "test-org" }, { fetchFn: mockFetch }); expect(result).toEqual({ environments: ["prod", "staging"] }); expect(mockFetch).toHaveBeenCalledOnce(); @@ -27,7 +26,7 @@ describe("executeAction", () => { it("returns error for unknown action ID", async () => { const mockFetch = vi.fn(); - const result = await executeAction(entries, config, "nonexistent_action", {}, undefined, mockFetch); + const result = await executeAction(entries, config, "nonexistent_action", {}, { fetchFn: mockFetch }); expect(result).toEqual({ error: true, @@ -43,10 +42,8 @@ describe("executeAction", () => { json: () => Promise.resolve({ trail: { name: "v1.0" } }), }); - await executeAction(entries, config, "get_trail", { - flow_name: "my-flow", - trail_name: "v1.0", - }, undefined, mockFetch); + await executeAction(entries, config, "get_trail", { flow_name: "my-flow", + trail_name: "v1.0", org: "test-org" }, { fetchFn: mockFetch }); expect(mockFetch).toHaveBeenCalledWith( expect.stringContaining("/trails/test-org/my-flow/v1.0"), @@ -64,11 +61,7 @@ describe("executeAction", () => { ]), }); - const result = await executeAction( - entries, config, "list_environments", {}, - ["name", "type"], - mockFetch, - ); + const result = await executeAction(entries, config, "list_environments", { org: "test-org" }, { fields: ["name", "type"], fetchFn: mockFetch }); expect(result).toEqual([ { name: "prod", type: "ECS" }, @@ -83,7 +76,7 @@ describe("executeAction", () => { json: () => Promise.resolve({ name: "prod", type: "ECS", tags: {} }), }); - const result = await executeAction(entries, config, "list_environments", {}, undefined, mockFetch); + const result = await executeAction(entries, config, "list_environments", { org: "test-org" }, { fetchFn: mockFetch }); expect(result).toEqual({ name: "prod", type: "ECS", tags: {} }); }); @@ -91,7 +84,7 @@ describe("executeAction", () => { it("rejects write action in GET mode", async () => { const mockFetch = vi.fn(); - const result = await executeAction(entries, config, "create_or_update_policy", {}, undefined, mockFetch, "GET"); + const result = await executeAction(entries, config, "create_or_update_policy", { org: "test-org" }, { fetchFn: mockFetch, mode: "GET" }); expect(result).toEqual({ error: true, @@ -103,7 +96,7 @@ describe("executeAction", () => { it("rejects read action in WRITE mode", async () => { const mockFetch = vi.fn(); - const result = await executeAction(entries, config, "list_environments", {}, undefined, mockFetch, "WRITE"); + const result = await executeAction(entries, config, "list_environments", { org: "test-org" }, { fetchFn: mockFetch, mode: "WRITE" }); expect(result).toEqual({ error: true, @@ -119,7 +112,7 @@ describe("executeAction", () => { json: () => Promise.resolve({ environments: [] }), }); - const result = await executeAction(entries, config, "list_environments", {}, undefined, mockFetch, "GET"); + const result = await executeAction(entries, config, "list_environments", { org: "test-org" }, { fetchFn: mockFetch, mode: "GET" }); expect(result).toEqual({ environments: [] }); expect(mockFetch).toHaveBeenCalledOnce(); @@ -132,7 +125,7 @@ describe("executeAction", () => { json: () => Promise.resolve({ policy: "created" }), }); - const result = await executeAction(entries, config, "create_or_update_policy", {}, undefined, mockFetch, "WRITE"); + const result = await executeAction(entries, config, "create_or_update_policy", { org: "test-org" }, { fetchFn: mockFetch, mode: "WRITE" }); expect(result).toEqual({ policy: "created" }); expect(mockFetch).toHaveBeenCalledOnce(); @@ -146,11 +139,7 @@ describe("executeAction", () => { json: () => Promise.resolve({ message: "Environment not found" }), }); - const result = await executeAction( - entries, config, "list_environments", {}, - ["name", "type"], - mockFetch, - ); + const result = await executeAction(entries, config, "list_environments", { org: "test-org" }, { fields: ["name", "type"], fetchFn: mockFetch }); expect(result).toEqual({ error: true, @@ -213,7 +202,7 @@ describe("request body unwrapping", () => { it("unwraps params nested under a body key for JSON write actions", async () => { const mockFetch = mockFetchOk(); - await executeAction(entries, config, "create_control", { body: jsonBody }, undefined, mockFetch, "WRITE"); + await executeAction(entries, config, "create_control", { body: jsonBody, org: "test-org" }, { fetchFn: mockFetch, mode: "WRITE" }); expect(mockFetch).toHaveBeenCalledWith( expect.stringContaining("/controls/test-org"), @@ -224,7 +213,7 @@ describe("request body unwrapping", () => { it("still accepts body fields spread at the top level", async () => { const mockFetch = mockFetchOk(); - await executeAction(entries, config, "create_control", { ...jsonBody }, undefined, mockFetch, "WRITE"); + await executeAction(entries, config, "create_control", { ...jsonBody, org: "test-org" }, { fetchFn: mockFetch, mode: "WRITE" }); expect(mockFetch).toHaveBeenCalledWith( expect.stringContaining("/controls/test-org"), @@ -235,7 +224,7 @@ describe("request body unwrapping", () => { it("unwraps when path params accompany the body key", async () => { const mockFetch = mockFetchOk(); - await executeAction(entries, config, "create_control", { org: "other-org", body: jsonBody }, undefined, mockFetch, "WRITE"); + await executeAction(entries, config, "create_control", { org: "other-org", body: jsonBody }, { fetchFn: mockFetch, mode: "WRITE" }); expect(mockFetch).toHaveBeenCalledWith( expect.stringContaining("/controls/other-org"), @@ -246,7 +235,7 @@ describe("request body unwrapping", () => { it("does not unwrap when an undeclared sibling key is present", async () => { const mockFetch = mockFetchOk(); - await executeAction(entries, config, "create_control", { body: jsonBody, extra: 1 }, undefined, mockFetch, "WRITE"); + await executeAction(entries, config, "create_control", { body: jsonBody, extra: 1, org: "test-org" }, { fetchFn: mockFetch, mode: "WRITE" }); expect(mockFetch).toHaveBeenCalledWith( expect.anything(), @@ -273,7 +262,7 @@ describe("request body unwrapping", () => { }; const mockFetch = mockFetchOk(); - await executeAction([entry], config, "post_with_body_prop", { body: { body: "text" } }, undefined, mockFetch, "WRITE"); + await executeAction([entry], config, "post_with_body_prop", { body: { body: "text" }, org: "test-org" }, { fetchFn: mockFetch, mode: "WRITE" }); expect(mockFetch).toHaveBeenCalledWith( expect.anything(), @@ -284,7 +273,7 @@ describe("request body unwrapping", () => { it("leaves non-object body values alone", async () => { const mockFetch = mockFetchOk(); - await executeAction(entries, config, "create_control", { body: "not-an-object" }, undefined, mockFetch, "WRITE"); + await executeAction(entries, config, "create_control", { body: "not-an-object", org: "test-org" }, { fetchFn: mockFetch, mode: "WRITE" }); expect(mockFetch).toHaveBeenCalledWith( expect.anything(), @@ -292,3 +281,339 @@ describe("request body unwrapping", () => { ); }); }); + +describe("org selection", () => { + function mockFetchOk(body: unknown = { ok: true }) { + return vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: () => Promise.resolve(body), + }); + } + + it("targets the org given as the org parameter", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "list_environments", {}, { fetchFn: mockFetch, mode: "GET", org: "cyber-dojo" }); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("still accepts an org supplied inside params", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "list_environments", { org: "cyber-dojo" }, { fetchFn: mockFetch, mode: "GET" }); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("accepts the same org in both places", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "list_environments", { org: "cyber-dojo" }, { fetchFn: mockFetch, mode: "GET", org: "cyber-dojo" }); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("rejects conflicting orgs without calling the API", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction(entries, config, "list_environments", { org: "kosli-public" }, { fetchFn: mockFetch, mode: "GET", org: "cyber-dojo" }); + + expect(result).toEqual({ + error: true, + message: + 'Conflicting orgs in one call: "cyber-dojo" and "kosli-public". The org parameter and params.org must agree — supply just one.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("rejects a blank org rather than building a URL with a missing segment", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction(entries, config, "list_environments", {}, { fetchFn: mockFetch, mode: "GET", org: " " }); + + expect(result).toEqual({ + error: true, + message: + "The org must be a single organization name, given as a non-empty string. Check the org parameter and params.org.", + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("rejects an org on an action that is not organization-scoped", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction(entries, config, "get_user_default_org", {}, { fetchFn: mockFetch, mode: "GET", org: "cyber-dojo" }); + + expect(result).toEqual({ + error: true, + message: + 'Action "get_user_default_org" is not organization-scoped — it takes no org. Retry with no org in the org parameter and none in params.org.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("calls a non-org-scoped action when no org is given", async () => { + const mockFetch = mockFetchOk({ default_org_name: "test-org" }); + + const result = await executeAction(entries, config, "get_user_default_org", {}, { fetchFn: mockFetch, mode: "GET" }); + + expect(result).toEqual({ default_org_name: "test-org" }); + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/user/default-org", + expect.anything(), + ); + }); + + it("trims a padded org rather than encoding the padding into the path", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "list_environments", {}, { fetchFn: mockFetch, mode: "GET", org: " cyber-dojo " }); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("applies the same trimming to an org supplied inside params", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "list_environments", { org: " cyber-dojo " }, { fetchFn: mockFetch, mode: "GET" }); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/cyber-dojo", + expect.anything(), + ); + }); + + it("rejects a blank org supplied inside params, rather than dropping the path segment", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction(entries, config, "list_environments", { org: "" }, { fetchFn: mockFetch, mode: "GET" }); + + expect(result).toEqual({ + error: true, + message: + "The org must be a single organization name, given as a non-empty string. Check the org parameter and params.org.", + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("refuses an org-scoped call that names no org, rather than choosing one", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "list_environments", {}, + { fetchFn: mockFetch, mode: "GET" }, + ); + + expect(result).toEqual({ + error: true, + message: + 'Action "list_environments" runs against one organization and none was named. Set the org parameter. There is no default, so that no call lands somewhere the caller did not choose.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("treats a null org as naming none at all", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "list_environments", { org: null }, + { fetchFn: mockFetch, mode: "GET", org: null }, + ); + + expect(result).toEqual({ + error: true, + message: + 'Action "list_environments" runs against one organization and none was named. Set the org parameter. There is no default, so that no call lands somewhere the caller did not choose.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it.each([ + ["a list of orgs", ["cyber-dojo", "kosli-public"]], + ["a number, which could name a real org", 1234], + ])("rejects %s rather than coercing it into the path", async (_label, org) => { + const mockFetch = vi.fn(); + + const result = await executeAction(entries, config, "list_environments", { org }, { fetchFn: mockFetch, mode: "GET" }); + + expect(result).toEqual({ + error: true, + message: + "The org must be a single organization name, given as a non-empty string. Check the org parameter and params.org.", + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("reports the unusable value, not a disagreement, when a real org is named too", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction(entries, config, "list_environments", { org: [] }, { fetchFn: mockFetch, mode: "GET", org: "cyber-dojo" }); + + expect(result).toEqual({ + error: true, + message: + "The org must be a single organization name, given as a non-empty string. Check the org parameter and params.org.", + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("rejects an org supplied only inside params for a non-org-scoped action", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction(entries, config, "get_user_default_org", { org: "cyber-dojo" }, { fetchFn: mockFetch, mode: "GET" }); + + expect(result).toEqual({ + error: true, + message: + 'Action "get_user_default_org" is not organization-scoped — it takes no org. Retry with no org in the org parameter and none in params.org.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it.each([ + ["with only the body naming it", {}], + ["with an undeclared sibling blocking the unwrap", { extra: 1 }], + ["with the org also named at the top level", { org: "cyber-dojo" }], + ])("refuses an org inside a write's request body, %s", async (_label, extra) => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "create_control", + { ...extra, body: { org: "other-org", identifier: "ctrl-1" } }, + { fetchFn: mockFetch, mode: "WRITE" }, + ); + + expect(result).toEqual({ + error: true, + message: + 'The request body names "org", which is also a parameter of "create_control". Supply it once, outside the body.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + // Not org-specific: a body field named after any declared parameter would + // redirect the request while the approval prompt showed the caller's value. + it("refuses a body field that collides with any other declared parameter", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "create_artifact", + { flow_name: "caller-flow", body: { flow_name: "body-flow", fingerprint: "f" } }, + { fetchFn: mockFetch, mode: "WRITE", org: "cyber-dojo" }, + ); + + expect(result).toEqual({ + error: true, + message: + 'The request body names "flow_name", which is also a parameter of "create_artifact". Supply it once, outside the body.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("names every colliding field, not just the first", async () => { + const mockFetch = vi.fn(); + + const result = await executeAction( + entries, config, "create_artifact", + { body: { org: "o", flow_name: "f", fingerprint: "x" } }, + { fetchFn: mockFetch, mode: "WRITE", org: "cyber-dojo" }, + ); + + expect(result).toEqual({ + error: true, + message: + 'The request body names "org" and "flow_name", which are also parameters of "create_artifact". Supply them once, outside the body.', + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("sends a request body that happens to contain a field called error", async () => { + const mockFetch = mockFetchOk({ created: true }); + + await executeAction( + entries, config, "create_control", + { body: { identifier: "ctrl-1", error: "not ours" } }, + { fetchFn: mockFetch, mode: "WRITE", org: "cyber-dojo" }, + ); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/controls/cyber-dojo", + expect.objectContaining({ body: JSON.stringify({ identifier: "ctrl-1", error: "not ours" }) }), + ); + }); + + it("targets the org on a multipart write without adding it as a form field", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "create_or_update_policy", { body: { name: "policy-1" } }, { fetchFn: mockFetch, mode: "WRITE", org: "cyber-dojo" }); + + const [url, init] = mockFetch.mock.calls[0]; + expect(url).toBe("https://app.kosli.com/api/v2/policies/cyber-dojo"); + expect([...(init.body as FormData).keys()]).toEqual(["name"]); + }); + + // KosliClient's JSON-body branch counts keys without filtering undefined, so + // an always-present org key would turn a bodyless write into one sending {}. + it("sends no body for a write on an action that takes no org", async () => { + const bodyless: CatalogEntry = { + id: "bodyless_write", method: "POST", path: "/user/ping", + summary: "", description: "", tags: [], parameters: [], requestBody: null, + searchText: "bodyless write", + }; + const mockFetch = mockFetchOk(); + + await executeAction([bodyless], config, "bodyless_write", {}, { fetchFn: mockFetch, mode: "WRITE" }); + + const [, init] = mockFetch.mock.calls[0]; + expect(init.body).toBeUndefined(); + }); + + it("drops a null org instead of forwarding it as a query parameter", async () => { + const mockFetch = mockFetchOk({ default_org_name: "test-org" }); + + await executeAction(entries, config, "get_user_default_org", { org: null }, { fetchFn: mockFetch, mode: "GET" }); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/user/default-org", + expect.anything(), + ); + }); + + // buildUrl's encodeURIComponent is the only thing keeping this one segment. + it("encodes an org rather than letting it rewrite the path", async () => { + const mockFetch = mockFetchOk(); + + await executeAction(entries, config, "list_environments", {}, { fetchFn: mockFetch, mode: "GET", org: "../../user/default-org" }); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/environments/..%2F..%2Fuser%2Fdefault-org", + expect.anything(), + ); + }); + + it("applies to writes, and the org does not leak into the request body", async () => { + const mockFetch = mockFetchOk({ created: true }); + const body = { identifier: "ctrl-1", name: "Control 1" }; + + await executeAction(entries, config, "create_control", { body }, { fetchFn: mockFetch, mode: "WRITE", org: "cyber-dojo" }); + + expect(mockFetch).toHaveBeenCalledWith( + "https://app.kosli.com/api/v2/controls/cyber-dojo", + expect.objectContaining({ body: JSON.stringify(body) }), + ); + }); +}); diff --git a/test/tools/search-actions.test.ts b/test/tools/search-actions.test.ts index f0bbb1e..bf61170 100644 --- a/test/tools/search-actions.test.ts +++ b/test/tools/search-actions.test.ts @@ -64,6 +64,24 @@ describe("searchActions", () => { }); }); + // The tool's own `org` input carries it. Advertising it here as well tells + // the model to put it in `params`. + it("does not advertise the org path parameter", () => { + const results = searchActions(entries, "get environment"); + + const env = results.find((r) => r.id === "get_environment"); + expect(env!.parameters.map((p) => p.name)).toEqual(["env_name"]); + }); + + // PUT /user/{org} writes the org rather than running in it, so the caller has + // to choose it and search has to keep showing it. + it("keeps the org parameter where the org is the thing being written", () => { + const results = searchActions(entries, "set default organization"); + + const setDefault = results.find((r) => r.id === "set_user_default_org"); + expect(setDefault!.parameters.map((p) => p.name)).toEqual(["org"]); + }); + it("omits hints field when no hint exists for an action", () => { const results = searchActions(entries, "environments", 10, hints as ActionHints);