From 4a51a8284d8ef95bc3645d42b50e28733bd1f002 Mon Sep 17 00:00:00 2001 From: Manas Srivastava Date: Thu, 11 Jun 2026 01:46:04 +0530 Subject: [PATCH] fix(mcp): set isError on tool failures + surface request_id/retry/capabilities/ignored_fields, correct anon-stack TTL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live-cohort dogfood found error-contract + rendering gaps that hurt agent integration. Five fixes in one PR: FIX 1 (P1) — tool failures now set isError:true. Every API-side failure (402/404/409/503/501) was returned as a NORMAL successful CallToolResult whose text merely said "instanode.dev error (...)", so a host could not distinguish failure from success. Per the MCP spec a tool-execution failure is reported IN the result via isError:true. Added a shared errorResult(err) helper that wraps formatError and sets the flag; all ~29 tool catch blocks now return errorResult(err) instead of textResult(formatError(err)). SUCCESS results never set it. A 402/403 upgrade/permission gate IS a failure (the op did not happen) → isError:true, with the agent_action text preserved. FIX 2 (P1) — error text now includes request_id (always) + retry_after_seconds (when present). The raw envelope carried both for support correlation / backoff; formatError dropped them. ApiError now carries requestId + retryAfterSeconds, lifted off the envelope in a single shared apiErrorFromEnvelope() used by both request and requestMultipart. FIX 3 (P2) — get_capabilities now renders resource_count_limit + RPO/RTO (it already rendered backups), matching its tool description's promise. FIX 4 (P2) — corrected the anon-STACK TTL prose from 24h to 6h (the real value; api PR #214 anonymousStackTTL) across src + README. The anon RESOURCE TTL (db/cache/etc.) is correctly left at 24h. FIX 5 (P3) — provision responses now echo ignored_fields (api #283) via a shared appendIgnoredFields() on the 7 resource-provision tools, so an agent learns its hallucinated params were dropped. Tests: new test/error-contract-unit.test.ts drives isError through the real registered-tool dispatch path (handler), asserts request_id in rendered text, capabilities render, ignored_fields render, and a no-"24h"-stack-TTL prose assertion; client-unit covers the envelope helper's request_id/retry branches. 495/495 pass; client.js 100% line coverage. Added the new test to the CI test list + the coverage.yml lcov list for the 100%-patch diff-cover gate. Co-Authored-By: Claude Fable 5 --- .github/workflows/coverage.yml | 4 +- README.md | 4 +- package.json | 4 +- src/client.ts | 120 +++++++++---- src/index.ts | 159 ++++++++++++++---- test/client-unit.test.ts | 54 ++++++ test/error-contract-unit.test.ts | 280 +++++++++++++++++++++++++++++++ test/mock-api.ts | 22 ++- 8 files changed, 570 insertions(+), 77 deletions(-) create mode 100644 test/error-contract-unit.test.ts diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 86d800b..7a158d5 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -62,7 +62,9 @@ jobs: dist-test/test/tool-coverage.test.js \ dist-test/test/tool-contract.test.js \ dist-test/test/env-regex-unit.test.js \ - dist-test/test/input-hardening-unit.test.js + dist-test/test/input-hardening-unit.test.js \ + dist-test/test/deploy-id-schema-unit.test.js \ + dist-test/test/error-contract-unit.test.js - uses: actions/setup-python@v6 if: github.event_name == 'pull_request' with: diff --git a/README.md b/README.md index 9cf3506..fc9e5bc 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ One tool call per resource type, each returning a drop-in connection string: resolves tokens to connection URLs server-side. - **Multi-service stack** (`create_stack`) → declare 1..N services in an `instant.yaml` manifest, ship them as a bundle in a single MCP call. Anonymous - callers get a 24h-TTL stack with a live URL on + callers get a 6h-TTL stack with a live URL on `*.deployment.instanode.dev` — no card required. Cross-service refs (`service://`) resolve cluster-internally at deploy time. Poll status with `get_stack`. @@ -120,7 +120,7 @@ to reach for this MCP, see . | `create_storage` | `POST /storage/new` — Provision an S3-compatible bucket prefix (DigitalOcean Spaces). Returns endpoint, access keys, prefix + `note`/`upgrade`. `name` required. | | `create_webhook` | `POST /webhook/new` — Provision an inbound webhook receiver URL. Returns `receive_url` + `note`/`upgrade`. `name` required. | | `create_deploy` | `POST /deploy/new` — Upload a base64 gzip tarball (with Dockerfile) and deploy a container. Returns `deploy_id`, `status`, `url`, `build_logs_url`. `name` required. Pass `redeploy: true` (with the SAME `name`) to update an existing deployment IN PLACE (same app_id + URL). Requires `INSTANODE_TOKEN`. | -| `create_stack` | `POST /stacks/new` — Multi-service bundle. Upload an `instant.yaml` manifest plus one base64 gzip tarball per service; returns `stack_id`, per-service URLs, and the 24h-TTL claim block on the anonymous tier. **Anonymous-friendly** (the wedge). `name`, `manifest`, `service_tarballs` required. | +| `create_stack` | `POST /stacks/new` — Multi-service bundle. Upload an `instant.yaml` manifest plus one base64 gzip tarball per service; returns `stack_id`, per-service URLs, and the 6h-TTL claim block on the anonymous tier. **Anonymous-friendly** (the wedge). `name`, `manifest`, `service_tarballs` required. | | `get_stack` | `GET /stacks/{stack_id}` — Poll a stack's per-service status + URLs. Anonymous-friendly. `stack_id` required. | | `list_deployments`| `GET /api/v1/deployments` — List all deployments on the caller's team. Requires `INSTANODE_TOKEN`. | | `get_deployment` | `GET /api/v1/deployments/:id` — Fetch one deployment (poll until `status="running"`). Requires `INSTANODE_TOKEN`. | diff --git a/package.json b/package.json index 1ff7513..1327f64 100644 --- a/package.json +++ b/package.json @@ -46,8 +46,8 @@ "dev": "tsc --watch", "start": "node dist/index.js", "pretest": "tsc && tsc -p tsconfig.test.json", - "test": "node --test --experimental-test-coverage --test-coverage-exclude='dist-test/test/**' --test-coverage-exclude='dist/**' --test-coverage-exclude='node_modules/**' dist-test/test/integration.test.js dist-test/test/live-smoke.test.js dist-test/test/prod-cohort.test.js dist-test/test/client-unit.test.js dist-test/test/index-unit.test.js dist-test/test/tools-unit.test.js dist-test/test/operate-tools-unit.test.js dist-test/test/tool-coverage.test.js dist-test/test/tool-contract.test.js dist-test/test/env-regex-unit.test.js dist-test/test/input-hardening-unit.test.js dist-test/test/deploy-id-schema-unit.test.js", - "test:nocov": "node --test dist-test/test/integration.test.js dist-test/test/live-smoke.test.js dist-test/test/prod-cohort.test.js dist-test/test/client-unit.test.js dist-test/test/index-unit.test.js dist-test/test/tools-unit.test.js dist-test/test/operate-tools-unit.test.js dist-test/test/tool-coverage.test.js dist-test/test/tool-contract.test.js dist-test/test/env-regex-unit.test.js dist-test/test/input-hardening-unit.test.js dist-test/test/deploy-id-schema-unit.test.js", + "test": "node --test --experimental-test-coverage --test-coverage-exclude='dist-test/test/**' --test-coverage-exclude='dist/**' --test-coverage-exclude='node_modules/**' dist-test/test/integration.test.js dist-test/test/live-smoke.test.js dist-test/test/prod-cohort.test.js dist-test/test/client-unit.test.js dist-test/test/index-unit.test.js dist-test/test/tools-unit.test.js dist-test/test/operate-tools-unit.test.js dist-test/test/tool-coverage.test.js dist-test/test/tool-contract.test.js dist-test/test/env-regex-unit.test.js dist-test/test/input-hardening-unit.test.js dist-test/test/deploy-id-schema-unit.test.js dist-test/test/error-contract-unit.test.js", + "test:nocov": "node --test dist-test/test/integration.test.js dist-test/test/live-smoke.test.js dist-test/test/prod-cohort.test.js dist-test/test/client-unit.test.js dist-test/test/index-unit.test.js dist-test/test/tools-unit.test.js dist-test/test/operate-tools-unit.test.js dist-test/test/tool-coverage.test.js dist-test/test/tool-contract.test.js dist-test/test/env-regex-unit.test.js dist-test/test/input-hardening-unit.test.js dist-test/test/deploy-id-schema-unit.test.js dist-test/test/error-contract-unit.test.js", "test:smoke": "bash test.sh", "prepublishOnly": "npm run build" }, diff --git a/src/client.ts b/src/client.ts index 14b0e67..faafb1c 100644 --- a/src/client.ts +++ b/src/client.ts @@ -121,6 +121,14 @@ export interface ProvisionResultBase { upgrade_jwt?: string; expires_at?: string | null; env?: string; + /** + * Unknown request-body fields the api silently dropped (api D7 / #283). When + * an agent sends a hallucinated param (e.g. `region`, `size`) the api ignores + * it and echoes the dropped key(s) here so the caller learns the param had no + * effect. Empty/absent on a clean request. Surfaced verbatim by the provision + * tools so the agent stops sending the dead field. + */ + ignored_fields?: string[]; } export interface DatabaseProvisionResult extends ProvisionResultBase { @@ -597,7 +605,7 @@ export interface StackResult { env?: string; name?: string; services: StackService[]; - /** Anonymous stacks have a 24h TTL; authenticated stacks return empty. */ + /** Anonymous stacks have a 6h TTL; authenticated stacks return empty. */ expires_in?: string; /** Anonymous-tier CTA fields, same semantics as create_*. */ note?: string; @@ -709,6 +717,21 @@ export class ApiError extends Error { * `upgradeURL` is the tier step (claimed → paid). */ readonly claimURL?: string; + /** + * The `request_id` field every API error envelope carries — a support / + * log-correlation id. An agent or user can quote it when reporting a failure + * so the operator can grep it in the api logs. Previously the MCP discarded + * it entirely, so a failure was unattributable. Surfaced verbatim by + * formatError. + */ + readonly requestId?: string; + /** + * The `retry_after_seconds` field present on rate-limit (429) and some + * transient-backpressure envelopes — the number of seconds the api asks the + * caller to wait before retrying. Surfaced so an agent backs off for the + * right duration instead of hammering or guessing. + */ + readonly retryAfterSeconds?: number; constructor( status: number, @@ -716,7 +739,9 @@ export class ApiError extends Error { code?: string, upgradeURL?: string, agentAction?: string, - claimURL?: string + claimURL?: string, + requestId?: string, + retryAfterSeconds?: number ) { super(message); this.name = "ApiError"; @@ -725,9 +750,58 @@ export class ApiError extends Error { this.upgradeURL = upgradeURL; this.agentAction = agentAction; this.claimURL = claimURL; + this.requestId = requestId; + this.retryAfterSeconds = retryAfterSeconds; } } +/** + * Build an ApiError from a parsed non-2xx response body. + * + * Single construction site for both `request` and `requestMultipart` so + * the set of fields lifted off the api's error envelope can never drift between + * the JSON and multipart code paths. The api's error envelope shape is + * `{ ok:false, error, message, agent_action?, upgrade_url?, claim_url?, + * request_id, retry_after_seconds? }` (api/internal/handlers ErrorResponse). + * + * `request_id` is always present on a real api error; `retry_after_seconds` is + * present on 429 and transient-backpressure envelopes. Both are coerced + * defensively (a hostile/garbled body must not throw here) and only forwarded + * when they are the right shape. + */ +function apiErrorFromEnvelope(status: number, data: unknown): ApiError { + const err = (data ?? {}) as { + error?: string; + message?: string; + upgrade_url?: string; + agent_action?: string; + claim_url?: string; + request_id?: string; + retry_after_seconds?: number; + }; + const message = err.message ?? "upstream error"; + const requestId = + typeof err.request_id === "string" && err.request_id.length > 0 + ? err.request_id + : undefined; + const retryAfterSeconds = + typeof err.retry_after_seconds === "number" && + Number.isFinite(err.retry_after_seconds) && + err.retry_after_seconds >= 0 + ? err.retry_after_seconds + : undefined; + return new ApiError( + status, + message, + err.error, + err.upgrade_url, + err.agent_action, + err.claim_url, + requestId, + retryAfterSeconds + ); +} + /** * Validate that a base URL is well-formed and uses http(s). BUG-MCP-040: * `INSTANODE_API_URL=javascript:alert(1)` would otherwise produce mysterious @@ -865,22 +939,7 @@ export class InstantClient { } if (!resp.ok) { - const err = (data ?? {}) as { - error?: string; - message?: string; - upgrade_url?: string; - agent_action?: string; - claim_url?: string; - }; - const message = err.message ?? "upstream error"; - throw new ApiError( - resp.status, - message, - err.error, - err.upgrade_url, - err.agent_action, - err.claim_url - ); + throw apiErrorFromEnvelope(resp.status, data); } // BugBash B16 F1 (regression of task #170 P0-1): empty 2xx bodies used to @@ -944,22 +1003,7 @@ export class InstantClient { } if (!resp.ok) { - const err = (data ?? {}) as { - error?: string; - message?: string; - upgrade_url?: string; - agent_action?: string; - claim_url?: string; - }; - const message = err.message ?? "upstream error"; - throw new ApiError( - resp.status, - message, - err.error, - err.upgrade_url, - err.agent_action, - err.claim_url - ); + throw apiErrorFromEnvelope(resp.status, data); } // Same empty-2xx safe sentinel as request(). See the long comment up @@ -1275,7 +1319,9 @@ export class InstantClient { * * Anonymous-friendly: like /deploy/new the api accepts anonymous callers * (OptionalAuth — openapi.json:157), issuing the stack at the anonymous tier - * with a 24h TTL. This is the CEO wedge: a single MCP call from a cold-start + * with a 6h TTL (anonymousStackTTL — a stack is live compute, so the window + * is tighter than the 24h anonymous RESOURCE TTL; api PR #214). This is the + * CEO wedge: a single MCP call from a cold-start * agent → live bundle URL on *.deployment.instanode.dev, no card, no * dashboard round-trip. * @@ -1318,8 +1364,8 @@ export class InstantClient { form.append(serviceName, blob, `${serviceName}.tar.gz`); } - // /stacks/new is OptionalAuth — anonymous callers are accepted with a 24h - // TTL. Do NOT pass requireAuth here. + // /stacks/new is OptionalAuth — anonymous callers are accepted with a 6h + // TTL (anonymousStackTTL, api PR #214). Do NOT pass requireAuth here. return this.requestMultipart("/stacks/new", form); } diff --git a/src/index.ts b/src/index.ts index 01a9f67..2cab82f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -190,20 +190,63 @@ export function formatError(err: unknown): string { if (err.agentAction && err.agentAction.length > 0) { lines.push("", `Action: ${err.agentAction}`); } + // Surface retry_after_seconds when the api asks the caller to back off + // (429 + transient-backpressure envelopes) so the agent waits the right + // amount of time instead of hammering or guessing. + if (typeof err.retryAfterSeconds === "number" && err.retryAfterSeconds >= 0) { + lines.push(`Retry after: ${err.retryAfterSeconds}s`); + } if (err.upgradeURL && err.upgradeURL.length > 0) { lines.push(`Upgrade: ${err.upgradeURL}`); } if (err.claimURL && err.claimURL.length > 0) { lines.push(`Claim: ${err.claimURL}`); } + // Always last: the support / log-correlation id. An agent or user can quote + // it when reporting the failure so the operator can grep it in the api logs. + if (err.requestId && err.requestId.length > 0) { + lines.push(`Request ID: ${err.requestId}`); + } return lines.join("\n"); } const msg = err instanceof Error ? err.message : String(err); return `instanode.dev error: ${msg}`; } -export function textResult(text: string) { - return { content: [{ type: "text" as const, text }] }; +/** + * Wrap text in a CallToolResult. `isError` defaults to false (a SUCCESS result). + * The MCP spec distinguishes a tool-execution FAILURE from a success by the + * `isError: true` flag on the result — a host/agent reads it to know the + * operation did not happen. Success results MUST NOT set it. Use `errorResult` + * (below) for the failure path so the flag is applied uniformly. + */ +export function textResult(text: string, isError = false) { + const result: { content: { type: "text"; text: string }[]; isError?: boolean } = { + content: [{ type: "text" as const, text }], + }; + if (isError) result.isError = true; + return result; +} + +/** + * The shared failure-rendering path for every tool. Maps a thrown error + * (ApiError / AuthRequiredError / plain Error) to a CallToolResult whose + * `isError` is true, so MCP hosts and agents can distinguish a tool FAILURE + * (the operation did not happen) from a SUCCESS. + * + * Per the MCP spec a tool-execution failure is reported IN the result via + * `isError: true` — not as a protocol-level error. Every api-side failure the + * MCP maps (402 upgrade-required, 403 permission, 404 not-found, 409 conflict, + * 429 rate-limit, 5xx, 501) is a tool failure: the requested operation did not + * complete. A 402/403 "upgrade/permission required" is still a failure (the + * provision/deploy did not happen) — the agent_action text is preserved by + * formatError so the agent still gets the path forward. + * + * Centralising here means all ~40 tool catch blocks get the flag uniformly via + * `return errorResult(err)` instead of `return textResult(formatError(err))`. + */ +export function errorResult(err: unknown) { + return textResult(formatError(err), true); } export function formatLimits(limits: ProvisionLimits | undefined): string[] { @@ -233,6 +276,27 @@ export function appendUpgradeBlock( } } +/** + * Surface `ignored_fields` — unknown request-body keys the api silently dropped + * (api D7 / #283). When an agent sends a hallucinated param (e.g. `region`, + * `size`), the api ignores it and echoes the dropped key(s) so the caller + * learns the param had no effect and stops resending it. No-op on a clean + * request (absent or empty array). Shared across every provision tool. + */ +export function appendIgnoredFields( + lines: string[], + result: { ignored_fields?: string[] } +): void { + const ignored = result.ignored_fields; + if (Array.isArray(ignored) && ignored.length > 0) { + lines.push( + ``, + `Note: the api ignored ${ignored.length} unknown field(s) you sent: ${ignored.join(", ")}. ` + + `They had no effect — remove them or check the tool schema for the correct param names.` + ); + } +} + // The single "name" schema reused by every create_* tool. BugBash B16 F2 // (regression of task #173): mirrors the api's ^[A-Za-z0-9][A-Za-z0-9 _-]*$ // regex via the shared nameSchema, so bad input is rejected at the Zod @@ -431,6 +495,7 @@ Store the connection_url in an env var (DATABASE_URL); do not hardcode it.`, ...formatLimits(result.limits), ]; appendUpgradeBlock(lines, result); + appendIgnoredFields(lines, result); lines.push( ``, `Use directly as DATABASE_URL (add .env to .gitignore):`, @@ -440,7 +505,7 @@ Store the connection_url in an env var (DATABASE_URL); do not hardcode it.`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -500,6 +565,7 @@ The 'name' field is required.`, ...formatLimits(result.limits), ]; appendUpgradeBlock(lines, result); + appendIgnoredFields(lines, result); lines.push( ``, `Use directly as DATABASE_URL (add .env to .gitignore):`, @@ -510,7 +576,7 @@ The 'name' field is required.`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -548,6 +614,7 @@ The 'name' field is required.`, ...formatLimits(result.limits), ]; appendUpgradeBlock(lines, result); + appendIgnoredFields(lines, result); lines.push( ``, `Use directly as REDIS_URL:`, @@ -555,7 +622,7 @@ The 'name' field is required.`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -593,6 +660,7 @@ The 'name' field is required.`, ...formatLimits(result.limits), ]; appendUpgradeBlock(lines, result); + appendIgnoredFields(lines, result); lines.push( ``, `Use directly as MONGODB_URI:`, @@ -600,7 +668,7 @@ The 'name' field is required.`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -637,6 +705,7 @@ The 'name' field is required.`, ...formatLimits(result.limits), ]; appendUpgradeBlock(lines, result); + appendIgnoredFields(lines, result); lines.push( ``, `Use directly as NATS_URL:`, @@ -644,7 +713,7 @@ The 'name' field is required.`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -693,6 +762,7 @@ The 'name' field is required.`, ...formatLimits(result.limits), ]; appendUpgradeBlock(lines, result); + appendIgnoredFields(lines, result); lines.push( ``, `S3-compatible — use with the AWS SDK in any language:`, @@ -702,7 +772,7 @@ The 'name' field is required.`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -740,6 +810,7 @@ delete_resource to tear down on demand.`, ...formatLimits(result.limits), ]; appendUpgradeBlock(lines, result); + appendIgnoredFields(lines, result); lines.push( ``, `Point any provider at the receive_url; GET it to pull stored requests:`, @@ -748,7 +819,7 @@ delete_resource to tear down on demand.`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -897,7 +968,7 @@ a URL the user can click in their browser.`, } return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -937,7 +1008,7 @@ token, tier, status, name, and expiry.`, [`${items.length} resource(s) on this account:`, "", ...rows].join("\n") ); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -981,7 +1052,7 @@ Requires INSTANODE_TOKEN.`, if (result.message) lines.push(`Message: ${result.message}`); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1043,7 +1114,7 @@ agent can route the user to the dashboard instead of guessing.`, ]; return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1256,7 +1327,7 @@ Requires INSTANODE_TOKEN (anonymous tier cannot deploy).`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1276,9 +1347,10 @@ and resource deps (\`needs: [postgres, redis]\` to auto-provision and bind, or \`http://:\` URLs at deploy time. ANONYMOUS-FRIENDLY: no INSTANODE_TOKEN required. Anonymous stacks land at the -anonymous tier with a 24h TTL, rate-limited by /24-subnet fingerprint. The +anonymous tier with a 6h TTL (a stack is live compute, so its window is tighter +than the 24h anonymous RESOURCE TTL), rate-limited by /24-subnet fingerprint. The response carries the same 'note' + 'upgrade' (claim) URL as create_postgres so -the agent can prompt the user to keep the stack past 24h. With INSTANODE_TOKEN +the agent can prompt the user to keep the stack past 6h. With INSTANODE_TOKEN the stack inherits the user's plan tier and is permanent. Multipart shape (the client builds this for you): @@ -1311,7 +1383,7 @@ Each tarball: gzip(tar()) → base64, cap 50 MiB per service (client-enforced). Total request body cap is 200 MB across all services (api). Returns: stack_id, status, tier, env, per-service { name, port, expose, url, -status } (only exposed services get a public URL), expires_in (24h on anon), +status } (only exposed services get a public URL), expires_in (6h on anon), plus the anonymous-tier upgrade fields.`, { name: nameSchema, @@ -1407,7 +1479,7 @@ plus the anonymous-tier upgrade fields.`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1458,7 +1530,7 @@ per-service { name, port, expose, url, status }, expires_in.`, appendUpgradeBlock(lines, result); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1506,7 +1578,7 @@ Requires INSTANODE_TOKEN.`, [`${result.total} deployment(s) on this team:`, "", ...rows].join("\n") ); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1566,7 +1638,7 @@ Requires INSTANODE_TOKEN.`, } return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1654,7 +1726,7 @@ clean "not found" (the api never confirms other teams' deployments).`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1717,7 +1789,7 @@ Requires INSTANODE_TOKEN.`, lines.push(``, `Poll get_deployment({ id: "${appId}" }) until status="running".`); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1747,7 +1819,7 @@ Requires INSTANODE_TOKEN.`, if (result.message) lines.push(`Message: ${result.message}`); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1811,12 +1883,31 @@ its first call. The response is the same for every caller.`, } } lines.push(` deployments: ${fmt(t.deployments_apps)}`); + // resource_count_limit — per-service cap on the NUMBER of active + // resources (distinct from storage/connection caps). The tool + // description promises this; render the per-service counts compactly + // (e.g. "postgres 5, redis 3"). -1 = unlimited. + const counts = t.resource_count_limit ?? {}; + const countSvcs = Object.keys(counts); + if (countSvcs.length > 0) { + const pairs = countSvcs.map((svc) => `${svc} ${fmt(counts[svc])}`); + lines.push(` resource count: ${pairs.join(", ")}`); + } + // backup + RPO/RTO durability promise — the description advertises + // "backup + RPO/RTO promises". Show backup retention + manual-backup + // quota when restore is enabled, then RPO/RTO when promised (0 = not + // promised, e.g. the anonymous/free tiers). if (t.backup_restore_enabled) { lines.push( ` backups: ${t.backup_retention_days}d retention, ` + `${t.manual_backups_per_day}/day manual` ); } + if (t.rpo_minutes > 0 || t.rto_minutes > 0) { + lines.push( + ` durability: RPO ${t.rpo_minutes}m, RTO ${t.rto_minutes}m` + ); + } if (t.annual_discount_percent > 0) { lines.push(` annual: save ${t.annual_discount_percent}%`); } @@ -1827,7 +1918,7 @@ its first call. The response is the same for every caller.`, if (result.contact) lines.push(`Enterprise: ${result.contact}`); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1880,7 +1971,7 @@ Requires INSTANODE_TOKEN.`, ]; return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1927,7 +2018,7 @@ Requires INSTANODE_TOKEN.`, ]; return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -1980,7 +2071,7 @@ team returns a clean 404 (the api never confirms other teams' deployments).`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -2034,7 +2125,7 @@ slug not on your team returns a clean 404. Requires INSTANODE_TOKEN.`, ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -2103,7 +2194,7 @@ no '..' path traversal — the api rejects those).`, ]; return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -2142,7 +2233,7 @@ Requires INSTANODE_TOKEN. A token not on your team returns a clean 404.`, lines.push(``, `Resume with: resume_resource({ id: "${id}" }).`); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -2177,7 +2268,7 @@ team returns a clean 404.`, if (result.message) lines.push(`Message: ${result.message}`); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -2218,7 +2309,7 @@ Requires INSTANODE_TOKEN. A token not on your team returns a clean 404.`, ]; return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); @@ -2259,7 +2350,7 @@ Requires INSTANODE_TOKEN. A deployment id not on your team returns a clean 404.` ); return textResult(lines.join("\n")); } catch (err) { - return textResult(formatError(err)); + return errorResult(err); } } ); diff --git a/test/client-unit.test.ts b/test/client-unit.test.ts index 5008ee9..8a8d72e 100644 --- a/test/client-unit.test.ts +++ b/test/client-unit.test.ts @@ -141,6 +141,60 @@ describe("InstantClient — unit-level branch coverage", () => { ); }); + it("apiErrorFromEnvelope → lifts request_id + retry_after_seconds off a non-2xx envelope", async () => { + stubFetch( + () => + new Response( + JSON.stringify({ + ok: false, + error: "rate_limited", + message: "slow down", + request_id: "req_envelope_42", + retry_after_seconds: 30, + }), + { status: 429, headers: { "content-type": "application/json" } } + ) + ); + const c = new InstantClient({ baseURL: "https://example.test" }); + await assert.rejects( + () => c.createPostgres("db"), + (err: unknown) => { + assert.ok(err instanceof ApiError); + assert.equal((err as ApiError).status, 429); + assert.equal((err as ApiError).requestId, "req_envelope_42"); + assert.equal((err as ApiError).retryAfterSeconds, 30); + return true; + } + ); + }); + + it("apiErrorFromEnvelope → drops a malformed retry_after_seconds + absent request_id (defensive branch)", async () => { + stubFetch( + () => + new Response( + JSON.stringify({ + ok: false, + error: "bad_request", + message: "nope", + // request_id absent, retry_after_seconds the wrong type / negative — + // both must coerce to undefined rather than propagate garbage. + retry_after_seconds: -5, + }), + { status: 400, headers: { "content-type": "application/json" } } + ) + ); + const c = new InstantClient({ baseURL: "https://example.test" }); + await assert.rejects( + () => c.createPostgres("db"), + (err: unknown) => { + assert.ok(err instanceof ApiError); + assert.equal((err as ApiError).requestId, undefined); + assert.equal((err as ApiError).retryAfterSeconds, undefined); + return true; + } + ); + }); + it("redeploy → empty-2xx body resolves to safe sentinel with caller-supplied id", async () => { stubFetch(() => new Response("", { status: 202 })); process.env["INSTANODE_TOKEN"] = "tok_xyz"; diff --git a/test/error-contract-unit.test.ts b/test/error-contract-unit.test.ts new file mode 100644 index 0000000..204e0d6 --- /dev/null +++ b/test/error-contract-unit.test.ts @@ -0,0 +1,280 @@ +/** + * Tests for the MCP error-contract + rendering fixes (live-cohort dogfood). + * + * Five fixes, all driven through the REAL registered-tool dispatch path + * (`server._registeredTools[name].handler(args)`) — the same surface the + * MCP-SDK invokes at the wire boundary, NOT a hand-rolled handler bypass — + * or through the exported pure render helpers (formatError / appendIgnoredFields + * / the capabilities handler), so the assertions reflect what an agent host + * actually receives. + * + * FIX 1 — every API-side failure now sets `isError: true` on the CallToolResult. + * Before this, a 402/404/409/503/501 came back as a NORMAL successful + * tool result whose text merely said "instanode.dev error (...)", so a + * host could not distinguish failure from success. The MCP spec reports + * a tool-execution failure IN the result via `isError: true`. SUCCESS + * results must NOT carry the flag. + * FIX 2 — the rendered error text now includes `request_id` (always) and + * `retry_after_seconds` (when present), which the raw envelope carried + * but the MCP previously dropped. + * FIX 3 — get_capabilities now renders resource-count + backup + RPO/RTO, which + * its description had promised but the body omitted. + * FIX 4 — anon-STACK TTL prose corrected from 24h to 6h (the real value; api + * PR #214). The anon RESOURCE TTL stays 24h. + * FIX 5 — provision responses echo `ignored_fields` (api #283) so an agent + * learns its hallucinated params were dropped. + */ + +import { strict as assert } from "node:assert"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; +import { after, before, describe, it } from "node:test"; + +import { + startMockApi, + VALID_TOKEN, + HOBBY_TOKEN, + MOCK_REQUEST_ID, + type MockApiHandle, +} from "./mock-api.js"; +import { ApiError } from "../src/client.js"; + +// Keep the side-effecting `await server.connect(transport)` off, same flag +// every in-process suite uses. +process.env["INSTANODE_MCP_NO_LISTEN"] = "1"; + +let mock: MockApiHandle; +let server: any; +let formatError: (err: unknown) => string; +let appendIgnoredFields: (lines: string[], r: { ignored_fields?: string[] }) => void; + +// Resolve the registered tool's handler — the SAME function the MCP-SDK +// dispatch (CallToolRequestSchema) invokes after input validation. Driving +// this exercises the real try/catch → errorResult path, not a bypass. +function handlerFor(name: string): (args: any, extra?: any) => Promise { + const reg = (server as any)._registeredTools as Record; + const t = reg[name]; + if (!t) throw new Error(`tool not registered: ${name}`); + return t.handler as any; +} + +function flat(callResult: any): string { + if (!callResult || !callResult.content) return ""; + return callResult.content.map((c: any) => c.text ?? "").join("\n"); +} + +before(async () => { + mock = await startMockApi(); + process.env["INSTANODE_API_URL"] = mock.url; + delete process.env["INSTANODE_TOKEN"]; + const mod: any = await import("../src/index.js"); + server = mod.server; + formatError = mod.formatError; + appendIgnoredFields = mod.appendIgnoredFields; +}); + +after(async () => { + delete process.env["INSTANODE_TOKEN"]; + await mock.close(); +}); + +// ── FIX 1: isError on mapped API failures, absent on success ───────────────── + +describe("FIX 1 — tool failures set isError:true (dispatch path)", () => { + it("404 not_found (delete_resource) → result.isError === true", async () => { + process.env["INSTANODE_TOKEN"] = VALID_TOKEN; + const res = await handlerFor("delete_resource")({ + token: "00000000-0000-0000-0000-000000000000", + }); + delete process.env["INSTANODE_TOKEN"]; + assert.match(flat(res), /instanode\.dev error \(404/); + assert.equal( + res.isError, + true, + "a 404 API failure MUST set isError:true so the host knows the op did not happen" + ); + }); + + it("402 tier_upgrade_required (create_deploy private on hobby) → isError === true, agent_action preserved", async () => { + process.env["INSTANODE_TOKEN"] = HOBBY_TOKEN; + const { gzipSync } = await import("node:zlib"); + const tarball = gzipSync(Buffer.from("FROM scratch\n")).toString("base64"); + // private:true is coupled to a non-empty allowed_ips client-side, so we + // pass one — this lets the request actually reach the api and trip its 402 + // tier-gate (private deploys require Pro+), which is the path we assert. + const res = await handlerFor("create_deploy")({ + name: "u-priv-deploy", + tarball_base64: tarball, + private: true, + allowed_ips: ["203.0.113.42"], + }); + delete process.env["INSTANODE_TOKEN"]; + const text = flat(res); + // A 402 "upgrade required" IS a failure — the deploy did not happen. + assert.equal(res.isError, true, "a 402 upgrade-gate is a tool FAILURE → isError:true"); + // ...but the agent_action path forward is still preserved (FIX 1 caveat). + assert.match(text, /Action:/, "agent_action must survive on the isError result"); + }); + + it("AuthRequiredError (delete_resource, no token) → isError === true", async () => { + delete process.env["INSTANODE_TOKEN"]; + const res = await handlerFor("delete_resource")({ token: "any-token" }); + assert.match(flat(res), /requires authentication/i); + assert.equal(res.isError, true, "an auth-required failure is a tool failure → isError:true"); + }); + + it("SUCCESS results do NOT set isError (anonymous create_postgres)", async () => { + delete process.env["INSTANODE_TOKEN"]; + const res = await handlerFor("create_postgres")({ name: "u-ok-pg" }); + assert.match(flat(res), /Postgres database provisioned\./); + assert.notEqual(res.isError, true, "a SUCCESS result must NOT carry isError:true"); + }); + + it("SUCCESS results do NOT set isError (get_capabilities, no auth)", async () => { + delete process.env["INSTANODE_TOKEN"]; + const res = await handlerFor("get_capabilities")({}); + assert.match(flat(res), /tier\(s\)/); + assert.notEqual(res.isError, true, "a successful capabilities read must NOT set isError"); + }); +}); + +// ── FIX 2: request_id + retry_after_seconds in rendered error text ─────────── + +describe("FIX 2 — request_id + retry_after_seconds surfaced in error text", () => { + it("request_id from the envelope appears in the rendered error (dispatch path)", async () => { + process.env["INSTANODE_TOKEN"] = VALID_TOKEN; + const res = await handlerFor("delete_resource")({ + token: "00000000-0000-0000-0000-000000000000", + }); + delete process.env["INSTANODE_TOKEN"]; + const text = flat(res); + assert.match( + text, + new RegExp(`Request ID: ${MOCK_REQUEST_ID}`), + "the support/correlation request_id must be quotable in the rendered error" + ); + }); + + it("formatError renders request_id for an ApiError that carries one", () => { + const out = formatError( + new ApiError(409, "already claimed", "already_claimed", undefined, undefined, undefined, "req_abc123") + ); + assert.match(out, /Request ID: req_abc123/); + }); + + it("formatError renders retry_after_seconds when the api asks the caller to back off", () => { + const out = formatError( + new ApiError( + 429, + "rate limited", + "rate_limited", + undefined, + undefined, + undefined, + "req_rl_9", + 42 + ) + ); + assert.match(out, /Retry after: 42s/); + assert.match(out, /Request ID: req_rl_9/); + }); + + it("formatError omits retry-after + request-id lines when absent (no empty 'Request ID:')", () => { + const out = formatError(new ApiError(404, "not found", "not_found")); + assert.doesNotMatch(out, /Retry after:/); + assert.doesNotMatch(out, /Request ID:/); + }); +}); + +// ── FIX 3: capabilities renders what its description promises ───────────────── + +describe("FIX 3 — get_capabilities renders resource-count + backup + RPO/RTO", () => { + it("renders resource count, backups, and RPO/RTO for a paid tier", async () => { + delete process.env["INSTANODE_TOKEN"]; + const res = await handlerFor("get_capabilities")({}); + const text = flat(res); + // resource_count_limit — promised by the description, previously omitted. + assert.match(text, /resource count:/, "resource_count_limit must be rendered"); + // backups — restore-enabled paid tiers show retention + manual quota. + assert.match(text, /backups: \d+d retention/, "backup promise must be rendered"); + // RPO/RTO durability promise (the pro/hobby tiers promise these in the mock). + assert.match(text, /durability: RPO \d+m, RTO \d+m/, "RPO/RTO must be rendered"); + }); +}); + +// ── FIX 5: ignored_fields echo ─────────────────────────────────────────────── + +describe("FIX 5 — ignored_fields surfaced when the api drops unknown params", () => { + it("appendIgnoredFields lists dropped fields on a non-empty array", () => { + const lines: string[] = ["Postgres database provisioned."]; + appendIgnoredFields(lines, { ignored_fields: ["region", "size"] }); + const text = lines.join("\n"); + assert.match(text, /ignored 2 unknown field\(s\)/); + assert.match(text, /region, size/); + }); + + it("appendIgnoredFields is a no-op on an empty array", () => { + const lines: string[] = ["x"]; + appendIgnoredFields(lines, { ignored_fields: [] }); + assert.equal(lines.length, 1, "empty ignored_fields must add no lines"); + }); + + it("appendIgnoredFields is a no-op when the field is absent", () => { + const lines: string[] = ["x"]; + appendIgnoredFields(lines, {}); + assert.equal(lines.length, 1, "absent ignored_fields must add no lines"); + }); +}); + +// ── FIX 4: anon-STACK TTL prose is 6h, not 24h ─────────────────────────────── + +describe("FIX 4 — anon-stack TTL prose corrected to 6h", () => { + // This test file compiles to dist-test/test/error-contract-unit.test.js, so + // the repo root is two levels up from the compiled file's dir, NOT one. We + // assert against the canonical TypeScript SOURCE (src/*.ts) + README.md, which + // live only at the repo root — not the compiled dist-test copy. + const here = dirname(fileURLToPath(import.meta.url)); + const repoRoot = join(here, "..", ".."); + + function read(rel: string): string { + return readFileSync(join(repoRoot, rel), "utf8"); + } + + it("create_stack tool description says 6h (not 24h) for the anon stack TTL", () => { + const reg = (server as any)._registeredTools as Record; + const desc = reg["create_stack"]?.description ?? ""; + assert.ok(desc.length > 0, "create_stack must be registered with a description"); + assert.match(desc, /6h TTL/, "anon stack TTL must read 6h"); + // No bare "24h TTL" stack claim — but the description MAY mention the 24h + // RESOURCE TTL for contrast ("tighter than the 24h ... RESOURCE TTL"). + assert.doesNotMatch( + desc, + /anonymous tier with a 24h TTL/, + "the stale '24h TTL' anon-stack claim must be gone" + ); + }); + + it("README anon-stack TTL prose reads 6h, not a 24h-TTL stack", () => { + const readme = read("README.md"); + assert.match(readme, /6h-TTL stack/, "README must describe the anon stack as 6h-TTL"); + assert.doesNotMatch(readme, /24h-TTL stack/, "no stale 24h-TTL stack claim in README"); + }); + + it("no source string pairs 'stack' with a 24h TTL claim", () => { + for (const rel of ["src/index.ts", "src/client.ts", "README.md"]) { + const body = read(rel); + // Reject either ordering of stack + 24h within a short window — the same + // shape the brief's grep checks. A 24h RESOURCE mention near the word + // "stack" in a contrastive sentence is allowed ONLY when it is explicitly + // the RESOURCE TTL; we exclude that phrasing. + const stackTtl = + /stack[^\n]{0,40}24h TTL|24h TTL[^\n]{0,40}stack/i.exec(body); + assert.equal( + stackTtl, + null, + `${rel} still pairs 'stack' with a 24h TTL claim: ${stackTtl?.[0]}` + ); + } + }); +}); diff --git a/test/mock-api.ts b/test/mock-api.ts index a56f73a..6de79a0 100644 --- a/test/mock-api.ts +++ b/test/mock-api.ts @@ -266,6 +266,14 @@ function validateName(name: unknown): { error: string; message: string } | null return null; } +/** + * A fixed request_id the mock stamps on EVERY error envelope, mirroring the + * real api which attaches a request_id to every ErrorResponse for support / + * log correlation. Fixed (not random) so tests can assert the exact value + * round-trips through the MCP-rendered error text. + */ +export const MOCK_REQUEST_ID = "req_mock_0123456789abcdef"; + /** Standard error envelope, matching the real API's shape. */ function errorEnvelope(opts: { error?: string; @@ -273,12 +281,24 @@ function errorEnvelope(opts: { upgrade_url?: string; agent_action?: string; claim_url?: string; + /** Seconds the api asks the caller to wait before retrying (429 / backpressure). */ + retry_after_seconds?: number; }): Record { - const e: Record = { ok: false, message: opts.message }; + // request_id is present on EVERY real api error envelope — stamp it here so + // the MCP's error-correlation surface (formatError "Request ID: ...") is + // exercised end-to-end through every error path. + const e: Record = { + ok: false, + message: opts.message, + request_id: MOCK_REQUEST_ID, + }; if (opts.error) e["error"] = opts.error; if (opts.upgrade_url) e["upgrade_url"] = opts.upgrade_url; if (opts.agent_action) e["agent_action"] = opts.agent_action; if (opts.claim_url) e["claim_url"] = opts.claim_url; + if (typeof opts.retry_after_seconds === "number") { + e["retry_after_seconds"] = opts.retry_after_seconds; + } return e; }