diff --git a/mcp/CLAUDE.md b/mcp/CLAUDE.md index 5a252deb12..f958218e2b 100644 --- a/mcp/CLAUDE.md +++ b/mcp/CLAUDE.md @@ -107,11 +107,19 @@ must never arrive in bulk. more than one paragraph to describe, split it. 4. **Do a broader debug pass after the feature is assembled** — run the full suite, the e2e client test, and MCP Inspector against `wrangler dev`. This pass is *in addition to* - step 1, never a replacement for it. -5. **Verify claims about the upstream API against the spec or the Rust source, not memory.** + step 1, never a replacement for it. It must include things unit tests cannot show: boot + the Worker for real (`wrangler dev` + curl), `wrangler deploy --dry-run` for every target, + and `pnpm install --frozen-lockfile` (what CI runs). +5. **A guardrail test must prove it fires.** Any test that exists to catch a bad edit (config + invariants, import boundaries, environment-name isolation) needs a case that feeds it the + bad edit and asserts the failure. A guardrail that has only ever passed is unproven — + express the check as a pure function and test it against mutated input. +6. **"Small unit" means one thing, then run.** One module and its test, then the checks; not + five files and then the checks. If a batch passes first time, that was luck, not method. +7. **Verify claims about the upstream API against the spec or the Rust source, not memory.** The published spec is https://storyteller-docs.netlify.app/api.json; the handlers are under `crates/service/web/storyteller_web/src/http_server/`. Read-only reference — see constraints. -6. **When something is uncertain, say so in the PR description** and pin it with a test that +8. **When something is uncertain, say so in the PR description** and pin it with a test that documents the assumed behaviour, so a wrong assumption fails loudly later instead of silently. ## Architecture diff --git a/mcp/fake-upstream/package.json b/mcp/fake-upstream/package.json index 4761f0aed7..e18f80117b 100644 --- a/mcp/fake-upstream/package.json +++ b/mcp/fake-upstream/package.json @@ -6,7 +6,7 @@ "type": "module", "scripts": { "dev": "wrangler dev --port 12345", - "deploy:preview": "wrangler deploy --env preview" + "deploy": "wrangler deploy" }, "dependencies": { "hono": "4.13.4" diff --git a/mcp/fake-upstream/wrangler.toml b/mcp/fake-upstream/wrangler.toml index 3fe8232c2f..4ec258e1b9 100644 --- a/mcp/fake-upstream/wrangler.toml +++ b/mcp/fake-upstream/wrangler.toml @@ -1,10 +1,9 @@ # Fake of the allowlisted Artcraft API routes. A separate Worker that the MCP server never # imports; it exists so local and preview environments never touch api.storyteller.ai. # See mcp/CLAUDE.md → "fake-upstream — a fake of the API, not of this server". +# +# One deploy target only (workers.dev, used by preview), so no [env.*] blocks. name = "artcraft-api-fake" main = "src/index.ts" compatibility_date = "2026-08-01" - -[env.preview] -name = "artcraft-api-fake" diff --git a/mcp/test/env-isolation.test.ts b/mcp/test/env-isolation.test.ts index 35d497c474..d71f902c8a 100644 --- a/mcp/test/env-isolation.test.ts +++ b/mcp/test/env-isolation.test.ts @@ -1,5 +1,7 @@ import { describe, expect, it } from "vitest"; +import { environmentNameOffenders } from "./helpers/environment-name-offenders"; + /** * `MCP_ENVIRONMENT` exists so the Worker can assert its configuration at startup. It must not * become a behaviour switch. The only file allowed to mention it is src/config.ts; anything @@ -20,21 +22,36 @@ describe("environment-name isolation", () => { expect(Object.keys(sources)).toContain("../src/config.ts"); }); - it("references MCP_ENVIRONMENT only in src/config.ts", () => { - const offenders = Object.entries(sources) - .filter(([path]) => !ALLOWED_FILES.includes(path)) - .filter(([, source]) => source.includes("MCP_ENVIRONMENT")) - .map(([path]) => path); - expect(offenders).toEqual([]); + it("finds no offenders in the committed tree", () => { + expect(environmentNameOffenders(sources, ALLOWED_FILES)).toEqual({ + readsBinding: [], + branches: [], + }); }); +}); - it("never branches on the environment name outside src/config.ts", () => { - const offenders = Object.entries(sources) - .filter(([path]) => !ALLOWED_FILES.includes(path)) - .filter(([, source]) => - /environment\s*(===|!==|==|!=)\s*["'](local|preview|production)["']/.test(source), - ) - .map(([path]) => path); - expect(offenders).toEqual([]); +describe("environment-name checks fire on an offender", () => { + it("catches a file that reads the binding", () => { + const result = environmentNameOffenders( + { "../src/leak.ts": 'const isProd = env.MCP_ENVIRONMENT === "production";' }, + ALLOWED_FILES, + ); + expect(result.readsBinding).toEqual(["../src/leak.ts"]); + }); + + it("catches a file that branches on the environment name", () => { + const result = environmentNameOffenders( + { "../src/leak.ts": 'if (config.environment !== "production") { grantFreeCredits(); }' }, + ALLOWED_FILES, + ); + expect(result.branches).toEqual(["../src/leak.ts"]); + }); + + it("still allows config.ts itself", () => { + const result = environmentNameOffenders( + { "../src/config.ts": "MCP_ENVIRONMENT: z.enum([...])" }, + ALLOWED_FILES, + ); + expect(result).toEqual({ readsBinding: [], branches: [] }); }); }); diff --git a/mcp/test/helpers/environment-mapping.ts b/mcp/test/helpers/environment-mapping.ts new file mode 100644 index 0000000000..b0215f69de --- /dev/null +++ b/mcp/test/helpers/environment-mapping.ts @@ -0,0 +1,79 @@ +import { PRODUCTION_UPSTREAM_API_HOST } from "../../src/config"; + +/** + * The wrangler.toml invariants, as a pure function over the parsed file so the tests can prove + * they fire against a broken configuration, not only that they pass against the real one. + */ + +export interface EnvBlock { + vars?: Record; + kv_namespaces?: { binding: string; id: string }[]; +} + +export interface WranglerConfig extends EnvBlock { + name: string; + main: string; + env?: Record; +} + +const PREVIEW_HOST_PATTERN = /^https:\/\/[a-z0-9.-]+\.workers\.dev$/; +const LOCAL_HOST_PATTERN = /^http:\/\/(localhost|127\.0\.0\.1)(:\d+)?$/; + +/** Returns human-readable violations; an empty array means the mapping is sound. */ +export function environmentMappingViolations(config: WranglerConfig): string[] { + const violations: string[] = []; + const envNames = Object.keys(config.env ?? {}).sort(); + if (envNames.join(",") !== "preview,production") { + violations.push( + `expected exactly the environments preview,production; got ${envNames.join(",")}`, + ); + } + + const blocks: Record = { + local: config, + preview: config.env?.preview, + production: config.env?.production, + }; + + for (const [name, block] of Object.entries(blocks)) { + if (!block) { + violations.push(`${name}: block missing`); + continue; + } + if (block.vars?.MCP_ENVIRONMENT !== name) { + violations.push(`${name}: MCP_ENVIRONMENT must be "${name}"`); + } + const host = block.vars?.UPSTREAM_API_HOST; + if (typeof host !== "string") { + violations.push(`${name}: UPSTREAM_API_HOST missing`); + continue; + } + if (name === "production" && host !== PRODUCTION_UPSTREAM_API_HOST) { + violations.push(`production: must use ${PRODUCTION_UPSTREAM_API_HOST}, got ${host}`); + } + if (name !== "production" && host === PRODUCTION_UPSTREAM_API_HOST) { + violations.push(`${name}: must never point at the production API`); + } + if (name === "preview" && !PREVIEW_HOST_PATTERN.test(host)) { + violations.push(`preview: must use a workers.dev fake upstream, got ${host}`); + } + if (name === "local" && !LOCAL_HOST_PATTERN.test(host)) { + violations.push(`local: must use a localhost upstream, got ${host}`); + } + } + + const kvIds = Object.entries(blocks).map(([name, block]) => { + const kv = block?.kv_namespaces?.find((namespace) => namespace.binding === "OAUTH_KV"); + if (!kv) violations.push(`${name}: OAUTH_KV binding missing`); + return kv?.id; + }); + if (new Set(kvIds).size !== kvIds.length) { + violations.push("OAUTH_KV namespace ids must differ between environments"); + } + + if (config.main !== "src/index.ts") { + violations.push(`main must be src/index.ts, got ${config.main}`); + } + + return violations; +} diff --git a/mcp/test/helpers/environment-name-offenders.ts b/mcp/test/helpers/environment-name-offenders.ts new file mode 100644 index 0000000000..31901be186 --- /dev/null +++ b/mcp/test/helpers/environment-name-offenders.ts @@ -0,0 +1,22 @@ +/** + * Finds server source files that read the environment name or branch on it. Pure so the test + * can prove it catches an offender, not only that the current tree has none. + */ + +const ENVIRONMENT_BRANCH_PATTERN = + /environment\s*(===|!==|==|!=)\s*["'](local|preview|production)["']/; + +export function environmentNameOffenders( + sources: Record, + allowedPaths: readonly string[], +): { readsBinding: string[]; branches: string[] } { + const candidates = Object.entries(sources).filter(([path]) => !allowedPaths.includes(path)); + return { + readsBinding: candidates + .filter(([, source]) => source.includes("MCP_ENVIRONMENT")) + .map(([path]) => path), + branches: candidates + .filter(([, source]) => ENVIRONMENT_BRANCH_PATTERN.test(source)) + .map(([path]) => path), + }; +} diff --git a/mcp/test/index-misconfiguration.test.ts b/mcp/test/index-misconfiguration.test.ts new file mode 100644 index 0000000000..ad82d5c620 --- /dev/null +++ b/mcp/test/index-misconfiguration.test.ts @@ -0,0 +1,68 @@ +import { createExecutionContext, waitOnExecutionContext } from "cloudflare:test"; +import { env } from "cloudflare:workers"; +import { describe, expect, it, vi } from "vitest"; + +/** + * The Worker entry memoizes its app per isolate. These tests re-import the module for a fresh + * isolate-equivalent so each scenario starts from an empty cache. + */ +async function freshWorker() { + vi.resetModules(); + const module = await import("../src/index"); + return module.default; +} + +function bindings(overrides: Partial>): Cloudflare.Env { + return { ...env, ...overrides } as unknown as Cloudflare.Env; +} + +async function fetchHealth( + worker: Awaited>, + workerEnv: Cloudflare.Env, +) { + const ctx = createExecutionContext(); + const response = await worker.fetch(new Request("https://mcp.test/healthz"), workerEnv, ctx); + await waitOnExecutionContext(ctx); + return response; +} + +describe("worker entry with misconfigured bindings", () => { + it("fails with a 500 that names the mismatch", async () => { + const worker = await freshWorker(); + const response = await fetchHealth( + worker, + bindings({ MCP_ENVIRONMENT: "production", UPSTREAM_API_HOST: "http://localhost:12345" }), + ); + expect(response.status).toBe(500); + expect(response.headers.get("content-type")).toMatch(/^text\/plain/); + expect(await response.text()).toBe( + "Worker misconfigured: production must use https://api.storyteller.ai, got http://localhost:12345", + ); + }); + + it("keeps failing on subsequent requests (the failure is memoized, not retried)", async () => { + const worker = await freshWorker(); + const bad = bindings({ + MCP_ENVIRONMENT: "preview", + UPSTREAM_API_HOST: "https://api.storyteller.ai", + }); + const first = await fetchHealth(worker, bad); + const second = await fetchHealth(worker, bad); + expect(first.status).toBe(500); + expect(second.status).toBe(500); + expect(await second.text()).toMatch(/preview must never point at the production API/); + }); + + it("reports missing bindings rather than crashing", async () => { + const worker = await freshWorker(); + const response = await fetchHealth(worker, bindings({ UPSTREAM_API_HOST: undefined })); + expect(response.status).toBe(500); + expect(await response.text()).toMatch(/invalid Worker bindings: UPSTREAM_API_HOST/); + }); + + it("serves normally when the bindings are sound", async () => { + const worker = await freshWorker(); + const response = await fetchHealth(worker, env); + expect(response.status).toBe(200); + }); +}); diff --git a/mcp/test/wrangler-config.test.ts b/mcp/test/wrangler-config.test.ts index b0093e8219..9fc0100d3d 100644 --- a/mcp/test/wrangler-config.test.ts +++ b/mcp/test/wrangler-config.test.ts @@ -3,67 +3,114 @@ import { describe, expect, it } from "vitest"; import { PRODUCTION_UPSTREAM_API_HOST } from "../src/config"; import wranglerToml from "../wrangler.toml?raw"; +import { + environmentMappingViolations, + type EnvBlock, + type WranglerConfig, +} from "./helpers/environment-mapping"; /** * The deployment configuration is part of the security boundary: it decides which upstream - * each environment may reach. These tests pin the mapping so a wrangler.toml edit that points - * preview at production (or production anywhere else) fails CI before it can deploy. + * each environment may reach. The first block pins the real file; the second proves the + * checks actually fire, by mutating a copy the way a careless edit would. */ -interface EnvBlock { - vars?: Record; - kv_namespaces?: { binding: string; id: string }[]; +const REAL_CONFIG = parse(wranglerToml) as unknown as WranglerConfig; + +function mutated(mutate: (config: WranglerConfig) => void): WranglerConfig { + const copy = structuredClone(REAL_CONFIG); + mutate(copy); + return copy; } -interface WranglerConfig extends EnvBlock { - name: string; - main: string; - env?: Record; +function setVar(block: EnvBlock, key: string, value: string): void { + block.vars = { ...block.vars, [key]: value }; } -const config = parse(wranglerToml) as unknown as WranglerConfig; -const envs = { - local: config, - preview: config.env?.preview, - production: config.env?.production, -}; +function envBlock(config: WranglerConfig, name: "preview" | "production"): EnvBlock { + const block = config.env?.[name]; + if (!block) throw new Error(`wrangler.toml has no [env.${name}] block`); + return block; +} describe("wrangler.toml environment mapping", () => { - it("declares exactly the three environments", () => { - expect(Object.keys(config.env ?? {}).sort()).toEqual(["preview", "production"]); - expect(envs.preview).toBeDefined(); - expect(envs.production).toBeDefined(); + it("is sound as committed", () => { + expect(environmentMappingViolations(REAL_CONFIG)).toEqual([]); }); - it("names each environment in MCP_ENVIRONMENT", () => { - expect(envs.local.vars?.MCP_ENVIRONMENT).toBe("local"); - expect(envs.preview?.vars?.MCP_ENVIRONMENT).toBe("preview"); - expect(envs.production?.vars?.MCP_ENVIRONMENT).toBe("production"); + it("commits the expected literal hosts", () => { + expect(REAL_CONFIG.env?.production?.vars?.UPSTREAM_API_HOST).toBe(PRODUCTION_UPSTREAM_API_HOST); + expect(REAL_CONFIG.env?.preview?.vars?.UPSTREAM_API_HOST).toMatch(/\.workers\.dev$/); + expect(REAL_CONFIG.vars?.UPSTREAM_API_HOST).toMatch(/^http:\/\/localhost/); + }); +}); + +describe("wrangler.toml checks fire on a broken mapping", () => { + it("catches preview pointed at production", () => { + const config = mutated((c) => { + setVar(envBlock(c, "preview"), "UPSTREAM_API_HOST", PRODUCTION_UPSTREAM_API_HOST); + }); + expect(environmentMappingViolations(config)).toEqual([ + "preview: must never point at the production API", + `preview: must use a workers.dev fake upstream, got ${PRODUCTION_UPSTREAM_API_HOST}`, + ]); }); - it("points production at the real API, and nothing else at it", () => { - expect(envs.production?.vars?.UPSTREAM_API_HOST).toBe(PRODUCTION_UPSTREAM_API_HOST); - expect(envs.preview?.vars?.UPSTREAM_API_HOST).not.toBe(PRODUCTION_UPSTREAM_API_HOST); - expect(envs.local.vars?.UPSTREAM_API_HOST).not.toBe(PRODUCTION_UPSTREAM_API_HOST); + it("catches production pointed at a fake", () => { + const config = mutated((c) => { + setVar( + envBlock(c, "production"), + "UPSTREAM_API_HOST", + "https://artcraft-api-fake.workers.dev", + ); + }); + expect(environmentMappingViolations(config)).toEqual([ + "production: must use https://api.storyteller.ai, got https://artcraft-api-fake.workers.dev", + ]); }); - it("points preview at a workers.dev fake and local at localhost", () => { - expect(envs.preview?.vars?.UPSTREAM_API_HOST).toMatch(/^https:\/\/[a-z0-9.-]+\.workers\.dev$/); - expect(envs.local.vars?.UPSTREAM_API_HOST).toMatch( - /^http:\/\/(localhost|127\.0\.0\.1)(:\d+)?$/, + it("catches local pointed at production", () => { + const config = mutated((c) => { + setVar(c, "UPSTREAM_API_HOST", PRODUCTION_UPSTREAM_API_HOST); + }); + expect(environmentMappingViolations(config)).toContain( + "local: must never point at the production API", ); }); - it("gives every environment its own OAUTH_KV namespace", () => { - const ids = [envs.local, envs.preview, envs.production].map((block) => { - const kv = block?.kv_namespaces?.find((namespace) => namespace.binding === "OAUTH_KV"); - expect(kv).toBeDefined(); - return kv?.id; + it("catches a mislabelled environment", () => { + const config = mutated((c) => { + setVar(envBlock(c, "preview"), "MCP_ENVIRONMENT", "production"); }); - expect(new Set(ids).size).toBe(ids.length); + expect(environmentMappingViolations(config)).toEqual([ + 'preview: MCP_ENVIRONMENT must be "preview"', + ]); }); - it("uses the same entry point for every environment", () => { - expect(config.main).toBe("src/index.ts"); + it("catches a shared KV namespace", () => { + const config = mutated((c) => { + envBlock(c, "preview").kv_namespaces = structuredClone( + envBlock(c, "production").kv_namespaces ?? [], + ); + }); + expect(environmentMappingViolations(config)).toEqual([ + "OAUTH_KV namespace ids must differ between environments", + ]); + }); + + it("catches an extra environment such as staging", () => { + const config = mutated((c) => { + c.env = { ...c.env, staging: structuredClone(envBlock(c, "preview")) }; + }); + expect(environmentMappingViolations(config)[0]).toMatch(/expected exactly the environments/); + }); + + it("catches a changed entry point", () => { + const config = mutated((c) => { + c.main = "src/other.ts"; + }); + expect(environmentMappingViolations(config)).toEqual([ + "main must be src/index.ts, got src/other.ts", + ]); }); });