From 325c827322a270fbe25d8d372678935639b93287 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg Date: Thu, 27 Aug 2026 18:11:41 +0200 Subject: [PATCH] test(mcp): run the tools against a real PostgREST, not a fake supabase (#1983) All 100 files in extensions/general/mcp-server/__tests__ fake supabase. query-journal.test.ts says out loud that its query chain is "exercised by the live MCP smoke test", and no such test exists in CI. So the PostgREST grammar of 157 tools, every .select() column string, every resource embed, every or=(...) form, is gated by nothing and fails first in production. pg-real cannot cover this: it holds a pg Pool and writes SQL, and none of that grammar is resolved by Postgres. It is resolved by PostgREST at request time. Adds a tool-pg vitest project, a docker-compose stack, a reset script that replays every migration the way the pg-real CI job does, and a CI job. The first sweep covers 74 read tools and finds no malformed query, across 87 real requests. That number is honest rather than impressive: with an empty argument set many tools bail before querying. Per-tool fixtures are what deepen it, and this harness is what makes writing them worth the effort. Includes a self-test that injects a bad column and asserts the harness detects it. That is not ceremony. It caught this file passing green while exercising nothing, twice: once locally where supabase-js prefixes /rest/v1 onto a bare PostgREST that does not serve it, and once on CI where Node 20 has no native WebSocket, so every client construction threw and was swallowed by the per-tool catch as a domain refusal. The client is now built once outside that catch, the proof-of-life assertion counts real requests instead of being trivially satisfiable, and realtime gets an inert transport. Also excludes .next from all three vitest projects. These projects override vitest's default excludes, so a local `npm run build` leaves a traced copy of the repo that gets collected as a second set of test files. Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) --- .github/workflows/test-pg-real.yml | 97 +++++++++++++ DECISIONS.md | 1 + package.json | 4 +- scripts/tool-pg/reset.sh | 82 +++++++++++ tests/tool-pg/client.ts | 105 ++++++++++++++ tests/tool-pg/docker-compose.yml | 48 +++++++ tests/tool-pg/query-grammar.tool.test.ts | 171 +++++++++++++++++++++++ tests/tool-pg/setup.ts | 35 +++++ vitest.config.ts | 51 ++++++- 9 files changed, 587 insertions(+), 7 deletions(-) create mode 100755 scripts/tool-pg/reset.sh create mode 100644 tests/tool-pg/client.ts create mode 100644 tests/tool-pg/docker-compose.yml create mode 100644 tests/tool-pg/query-grammar.tool.test.ts create mode 100644 tests/tool-pg/setup.ts diff --git a/.github/workflows/test-pg-real.yml b/.github/workflows/test-pg-real.yml index 4a2fb211..d119bab4 100644 --- a/.github/workflows/test-pg-real.yml +++ b/.github/workflows/test-pg-real.yml @@ -200,3 +200,100 @@ jobs: done - run: npm run test:pg + + tool-pg: + # MCP tools driven through a REAL supabase-js client against a REAL + # PostgREST. This is NOT a duplicate of the pg-real job: that one holds a + # `pg` Pool and writes SQL, which cannot see the half of a tool that + # PostgREST resolves (the .select() column strings, the resource embeds, + # the or=(...) grammar). Before this job, all 100 files in + # extensions/general/mcp-server/__tests__ faked supabase and nothing in CI + # exercised that surface. + runs-on: ubuntu-latest + + services: + postgres: + image: supabase/postgres:15.8.1.060 + env: + POSTGRES_PASSWORD: postgres + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U postgres" + --health-interval 5s + --health-timeout 5s + --health-retries 20 + + env: + DATABASE_URL: postgresql://postgres:postgres@localhost:5432/postgres + TOOL_PG_DATABASE_URL: postgresql://postgres:postgres@localhost:5432/postgres + TOOL_PG_REST_URL: http://127.0.0.1:3000 + PGPASSWORD: postgres + + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + + - uses: actions/setup-node@249970729cb0ef3589644e2896645e5dc5ba9c38 # v6 + with: + node-version: 20 + + - run: npm ci + + - name: Install psql client + run: sudo apt-get update && sudo apt-get install -y --no-install-recommends postgresql-client + + - name: Bootstrap storage schema + run: psql "$DATABASE_URL" -v ON_ERROR_STOP=1 -q -f tests/pg/bootstrap.sql + + - name: Default privileges for the supabase roles + # Without these PostgREST answers every request with 42501, because the + # grants have to exist before the migrations create ~400 tables. + run: | + psql "$DATABASE_URL" -v ON_ERROR_STOP=1 -q -c " + GRANT USAGE ON SCHEMA public TO postgres, anon, authenticated, service_role; + ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON TABLES TO postgres, anon, authenticated, service_role; + ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON ROUTINES TO postgres, anon, authenticated, service_role; + ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON SEQUENCES TO postgres, anon, authenticated, service_role; + " + + - name: Apply migrations + run: | + set -euo pipefail + shopt -s nullglob + files=(supabase/migrations/*.sql) + if [ ${#files[@]} -eq 0 ]; then echo "No migration files found"; exit 1; fi + for f in "${files[@]}"; do + psql "$DATABASE_URL" -v ON_ERROR_STOP=1 -q -f "$f" + done + + - name: Grant on everything the migrations created + run: | + psql "$DATABASE_URL" -v ON_ERROR_STOP=1 -q -c " + GRANT ALL ON ALL TABLES IN SCHEMA public TO postgres, anon, authenticated, service_role; + GRANT ALL ON ALL ROUTINES IN SCHEMA public TO postgres, anon, authenticated, service_role; + GRANT ALL ON ALL SEQUENCES IN SCHEMA public TO postgres, anon, authenticated, service_role; + " + + - name: Start PostgREST + # Deliberately `docker run --network host` rather than a service + # container. Service containers on a non-containerized job are reachable + # from the runner on localhost, but NOT from each other by name, and + # PostgREST has to reach Postgres. Host networking sidesteps that. + run: | + docker run -d --name postgrest --network host \ + -e PGRST_DB_URI="postgres://postgres:postgres@127.0.0.1:5432/postgres" \ + -e PGRST_DB_SCHEMAS=public \ + -e PGRST_DB_ANON_ROLE=anon \ + -e PGRST_JWT_SECRET="super-secret-jwt-token-with-at-least-32-characters-long" \ + -e PGRST_DB_MAX_ROWS=100000 \ + -e PGRST_SERVER_PORT=3000 \ + postgrest/postgrest:v12.2.3 + for _ in $(seq 1 60); do + if curl -sf -o /dev/null "http://127.0.0.1:3000/" ; then break; fi + sleep 1 + done + curl -sf -o /dev/null "http://127.0.0.1:3000/" || (docker logs postgrest && exit 1) + + - run: npm run test:tools diff --git a/DECISIONS.md b/DECISIONS.md index 60bd8307..c23eaa07 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1298,4 +1298,5 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-08-27] New `unlinked_documents` category on the Accounted://attention resource, backed by lib/documents/unlinked-documents.ts. The whole design is the mime ALLOW-LIST, and the naive predicate is a trap: "current version, no journal_entry_id, referenced by none of the eight linking tables" returns 15 806 rows on prod, of which 11 309 are application/json and every single one is named psd2-response__pN.json, the archived PSD2 bank-API responses the integration stores as evidence of each fetch. Those are unlinked BY DESIGN; surfacing them would hand an agent 11 309 items of work it must not action, which is worse than showing nothing. Measured 2026-08-27: application/json was 11 309 of 11 309 psd2, and pdf/png/jpeg/heic were 0 of 4 495, so the split is clean. Chose an allow-list of underlag-shaped mime types over excluding known-bad filenames, so a future machine-payload format (XML, CSV, an audit bundle) stays out by default instead of leaking until someone notices. Real remaining surface: 4 497 documents across 210 companies, median 3 per company, 481 in the preceding week, and NOT agent-specific (2 374 upload_source=api vs 1 623 file_upload from the web UI). Two-pass fetch mirroring fetchPurchasesWithoutUnderlag: indexed column filter, then eight reference lookups that run only when candidates exist, so the common case costs one query. Scan cap is 300 and is set by URL LENGTH, not table size: each candidate id is echoed through eight .in(column, ids) lookups at ~38 bytes per UUID, and a cap in the thousands would exceed the gateway limit, fail the lookups, and the "claims nothing" fallback would turn every candidate into a false positive. A failing lookup is deliberately treated as "claims nothing" (can only ADD a row) rather than dropping the category, so one misbehaving table cannot hide real work. UnlinkedDocument is a type alias not an interface: the resource assigns it into samples: Record[] and an interface has no implicit index signature; vitest does not typecheck so this only fails in npm run build. [2026-08-27] NOT fixed, and recorded so the next person does not act on an inflated number: the agent-facing readers (resources/attention.ts, resources/recent-activity.ts) still test booked-ness with a raw journal_entry_id null check instead of the canonical isTransactionBooked, which misses the bulk-book (transaction_voucher_links) and multi-allocation (invoice_payments / supplier_invoice_payments) cases. Real scale measured on prod 2026-08-27: 4 transactions, in 1 company, out of 567 column-filtered unbooked, all 4 via transaction_voucher_links and 0 via either payments table. Worth fixing as hygiene, but it is a 4-row problem and doing it properly in attention.ts needs the same two-pass treatment plus a decision about count semantics for a tenant with thousands of unbooked rows, so it does not belong bolted onto this change. [2026-08-27] Klarmarkera (markPeriodClosedExternally) gets an undo, reopenExternallyClosedPeriod, allowed only while the closed state still comes from klarmarkera (closed_externally set, no closing entry): that close was a person's control decision without a bokslutsverifikat, so reversing it strands nothing, whereas a closePeriod close keeps its closing entry and stays irreversible here. The reopen clears the lock too, because the reason to reopen is to change the period's contents (Forsslund Systems 2026-08-27: five imported years klarmarkerade, then the prior-year SIE turned out wrong; replace refused the closed year, unlock refused the closed state, no way back). Audit_log row plus period.unlocked event; the MCP staged-op surface (lock/unlock) does not get a reopen op yet, follow-up. +[2026-08-27] New `tool-pg` vitest project: MCP tools driven through a REAL supabase-js client against a REAL PostgREST (tests/tool-pg/, scripts/tool-pg/reset.sh, `npm run tools:pg:reset` + `npm run test:tools`, plus a tool-pg CI job). NOT a duplicate of pg-real: that project holds a `pg` Pool and writes SQL, which structurally cannot see the half of a tool that PostgREST resolves at request time (the `.select()` column strings, the resource embeds, the `or=(...)` grammar, `.contains()` operand types). Before this, all 100 files in extensions/general/mcp-server/__tests__ faked supabase and query-journal.test.ts deferred its query chain to "the live MCP smoke test", which does not exist in CI: the PostgREST grammar of 157 tools was gated by nothing. Three findings worth keeping. (1) supabase-js hard-codes a `/rest/v1` prefix that a bare PostgREST does not serve, so the first version of the harness 404'd all 55 sweep queries, the tools reported the empty response as "Database error: undefined", and the suite passed GREEN while exercising nothing; fixed with a URL-rewriting `global.fetch` in createToolPgClient, and a permanent self-test now injects a bad column and asserts the harness detects 42703, so a green sweep means something. (2) Errors are captured at the TRANSPORT, not from the thrown Error: the tools wrap failures in their own prose and lose the payload, so a real 42703 arrives as an unclassifiable string. (3) The reset recreates the CONTAINER rather than dropping schemas: `storage` is owned by supabase_storage_admin so `DROP SCHEMA storage` fails as postgres, and dropping only `public` leaves the storage RLS policies migration 20240101000024 creates unconditionally, aborting the next replay partway and leaving a half-migrated database that looks like a migration bug. CI runs PostgREST via `docker run --network host` rather than a service container, because service containers on a non-containerized job are reachable from the runner but not from each other by name. Current coverage is honest and partial: 74 read tools, 87 real requests, 0 malformed queries, 4 failures all 22P02 from the empty argument set. Per-tool argument fixtures are what deepen it, and the harness is the thing that makes writing them worthwhile. [2026-08-27] Added `npm run check:types`, a typecheck ratchet (scripts/checks/no-new-type-errors.mjs + typecheck-baseline.json), wired into the core-build `checks` job next to check:lint. Reason: `npm test` does NOT typecheck. Vitest transpiles and discards types, so a type error passes all 18 000 tests and only surfaces in `npm run build` minutes later; that happened TWICE on 2026-08-27 (a widened errorKind union in the MCP server that lib/events/types.ts still contradicted, and an `interface` that would not assign into `Record[]` because interfaces have no implicit index signature). It is not merely a faster copy of the build job: `tsc --noEmit` also covers `__tests__` files, which the Next.js build never compiles, and that is where all 539 baseline errors live. Baseline is keyed per FILE, deliberately unlike the per-RULE lint ratchet: the legacy errors are concentrated in a handful of old test files and TS2322 is common enough that a code-keyed budget would silently absorb a real regression somewhere else, whereas per-file trips the moment a previously-clean file gains an error. Verified the gate actually fires by introducing a deliberate `const x: number = 'str'` and watching it fail with the exact location, then restoring. Cost measured: 36 s cold (what CI pays, since tsconfig.tsbuildinfo is gitignored) and 4.4 s warm locally via the existing `incremental: true`. The script sets NODE_OPTIONS=--max-old-space-size=8192 because a bare tsc dies with "Ineffective mark-compacts near heap limit" on this graph after about two minutes, which reads like a hang rather than a misconfiguration; it also detects that OOM string and exits 2 with a "raise HEAP_MB" message rather than silently reporting zero errors. NOT changed: Definition of Done item 1 still says only lint + test. CI enforcement is the stronger mechanism and does not need the policy edit; adding it to DoD is a founder call. diff --git a/package.json b/package.json index b1efad04..72cae94c 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,9 @@ "check:lint": "node scripts/checks/no-new-lint-errors.mjs", "check:types": "node scripts/checks/no-new-type-errors.mjs", "test": "vitest run --project unit", - "test:pg": "vitest run --project pg-real" + "test:pg": "vitest run --project pg-real", + "tools:pg:reset": "bash scripts/tool-pg/reset.sh", + "test:tools": "TOOL_PG_REST_URL=${TOOL_PG_REST_URL:-http://127.0.0.1:54330} vitest run --project tool-pg" }, "dependencies": { "@ai-sdk/openai-compatible": "2.0.69", diff --git a/scripts/tool-pg/reset.sh b/scripts/tool-pg/reset.sh new file mode 100755 index 00000000..b7c28c00 --- /dev/null +++ b/scripts/tool-pg/reset.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# Rebuild the MCP tool-integration database from scratch. +# +# Mirrors the pg-real CI job step for step (bootstrap.sql, then every migration +# in filename order with ON_ERROR_STOP), so a schema that passes there passes +# here. Two additions: +# +# * The container is recreated rather than the schemas dropped. Dropping is the +# obvious approach and it does not work: `storage` is owned by +# supabase_storage_admin, so `DROP SCHEMA storage` fails as postgres, and +# dropping only `public` leaves the storage RLS policies that migration +# 20240101000024 creates unconditionally, which aborts the next replay +# partway through and leaves a half-migrated database that looks like a +# migration bug. A fresh volume costs about fifteen seconds and removes the +# entire class. +# +# * PostgREST caches the schema at boot, so a freshly-migrated database is +# invisible to it until it is told to look again. +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +COMPOSE_FILE="$REPO_ROOT/tests/tool-pg/docker-compose.yml" + +compose() { docker compose -f "$COMPOSE_FILE" "$@"; } + +psql_run() { + compose exec -T postgres \ + psql "postgresql://postgres:postgres@localhost:5432/postgres" -v ON_ERROR_STOP=1 -q "$@" +} + +echo "==> recreating containers with a fresh volume" +compose down -v --remove-orphans >/dev/null 2>&1 || true +compose up -d --wait >/dev/null + +echo "==> waiting for postgres" +for _ in $(seq 1 90); do + if compose exec -T postgres pg_isready -U postgres >/dev/null 2>&1; then break; fi + sleep 1 +done + +echo "==> bootstrap storage schema" +psql_run -f - < "$REPO_ROOT/tests/pg/bootstrap.sql" >/dev/null 2>&1 + +# The image grants these at init, but DEFAULT PRIVILEGES are what make the +# grants apply to the ~400 tables the migrations are about to create. Without +# them PostgREST answers every request with 42501 "permission denied". +echo "==> default privileges for the supabase roles" +psql_run -c " + GRANT USAGE ON SCHEMA public TO postgres, anon, authenticated, service_role; + ALTER DEFAULT PRIVILEGES IN SCHEMA public + GRANT ALL ON TABLES TO postgres, anon, authenticated, service_role; + ALTER DEFAULT PRIVILEGES IN SCHEMA public + GRANT ALL ON ROUTINES TO postgres, anon, authenticated, service_role; + ALTER DEFAULT PRIVILEGES IN SCHEMA public + GRANT ALL ON SEQUENCES TO postgres, anon, authenticated, service_role; +" >/dev/null + +echo "==> applying migrations" +count=0 +for f in "$REPO_ROOT"/supabase/migrations/*.sql; do + if ! psql_run -f - < "$f" >/dev/null 2>/tmp/tool-pg-migrate.err; then + echo "FAILED on $(basename "$f")" >&2 + tail -20 /tmp/tool-pg-migrate.err >&2 + exit 1 + fi + count=$((count + 1)) + if [ $((count % 200)) -eq 0 ]; then echo " ... $count migrations applied"; fi +done +echo "==> $count migrations applied" + +echo "==> granting on everything the migrations created" +psql_run -c " + GRANT ALL ON ALL TABLES IN SCHEMA public TO postgres, anon, authenticated, service_role; + GRANT ALL ON ALL ROUTINES IN SCHEMA public TO postgres, anon, authenticated, service_role; + GRANT ALL ON ALL SEQUENCES IN SCHEMA public TO postgres, anon, authenticated, service_role; +" >/dev/null + +echo "==> reloading PostgREST schema cache" +psql_run -c "NOTIFY pgrst, 'reload schema';" >/dev/null +sleep 3 + +echo "==> ready" diff --git a/tests/tool-pg/client.ts b/tests/tool-pg/client.ts new file mode 100644 index 00000000..60381548 --- /dev/null +++ b/tests/tool-pg/client.ts @@ -0,0 +1,105 @@ +/** + * A REAL supabase-js client, pointed at a real PostgREST, over a real Postgres + * with every migration replayed. + * + * Why this is not the same thing as the pg-real suite: those tests hold a `pg` + * Pool and write SQL. The MCP tools do not write SQL. They call + * `supabase.from('x').select('a, b:c(d)')`, and the string inside `.select()` + * is parsed by PostgREST, not by Postgres. A misspelled column, a resource + * embed whose foreign key does not exist, an `or=(...)` whose grammar is + * slightly off, a `.contains()` against a non-jsonb column: every one of those + * is a runtime 400 from PostgREST that a mocked client answers cheerfully and a + * SQL test never reaches. + */ +import { createHmac } from 'node:crypto' +import { createClient, type SupabaseClient } from '@supabase/supabase-js' + +/** Must match PGRST_JWT_SECRET in tests/tool-pg/docker-compose.yml. */ +export const TOOL_PG_JWT_SECRET = + 'super-secret-jwt-token-with-at-least-32-characters-long' + +export const TOOL_PG_REST_URL = process.env.TOOL_PG_REST_URL ?? 'http://127.0.0.1:54330' +export const TOOL_PG_DATABASE_URL = + process.env.TOOL_PG_DATABASE_URL ?? 'postgresql://postgres:postgres@127.0.0.1:54329/postgres' + +function base64url(input: Buffer | string): string { + return Buffer.from(input) + .toString('base64') + .replace(/\+/g, '-') + .replace(/\//g, '_') + .replace(/=+$/, '') +} + +/** + * Hand-rolled HS256 rather than a JWT library: this repo is AGPL and audits its + * dependency surface, and a signed JWT is three base64url segments and one + * HMAC. Not worth a dependency, and definitely not worth one that ships only to + * tests. + */ +export function signServiceRoleJwt(secret = TOOL_PG_JWT_SECRET): string { + const header = base64url(JSON.stringify({ alg: 'HS256', typ: 'JWT' })) + const payload = base64url( + JSON.stringify({ + role: 'service_role', + iss: 'tool-pg', + // Fixed far-future expiry: the harness is disposable and a clock-derived + // value would make an otherwise deterministic suite time-dependent. + exp: 4102444800, + }), + ) + const signature = base64url( + createHmac('sha256', secret).update(`${header}.${payload}`).digest(), + ) + return `${header}.${payload}.${signature}` +} + +/** + * Service-role client, which is what the MCP server actually uses: + * `createServiceClientNoCookies()` on the API-key path. RLS is therefore NOT + * the thing under test here; the query grammar is. Tenant isolation on this + * surface comes from explicit `.eq('company_id', ...)` discipline, and a tool + * that forgets it is exactly the kind of bug these tests can catch. + */ +/** + * `createClient` eagerly constructs a RealtimeClient, which resolves a + * WebSocket implementation and throws "native WebSocket not found" on Node 20. + * CI runs Node 20; local machines may not, which is exactly the kind of + * difference that turns into a green local run and a red CI one. + * + * Nothing here subscribes to realtime, and RealtimeClient only RESOLVES the + * constructor rather than instantiating it, so handing it an inert class is + * enough. Bumping the job to Node 22 would work too, but it would make this the + * only job in the repo on a different runtime for a feature it never uses. + */ +class UnusedRealtimeTransport { + constructor() { + throw new Error('tool-pg: realtime is not used by these tests') + } +} + +export function createToolPgClient(): SupabaseClient { + const key = signServiceRoleJwt() + return createClient(TOOL_PG_REST_URL, key, { + auth: { persistSession: false, autoRefreshToken: false }, + db: { schema: 'public' }, + realtime: { transport: UnusedRealtimeTransport as never }, + global: { + headers: { apikey: key }, + // supabase-js hard-codes a `/rest/v1` prefix onto every PostgREST + // request, because that is where Supabase's own gateway mounts it. A + // bare PostgREST serves at the root, so without this rewrite every + // query 404s. + // + // That is not a hypothetical: the first version of this harness omitted + // it, all 55 sweep queries 404d, the tools reported the empty response + // as "Database error: undefined", and the suite passed green while + // exercising nothing at all. The self-test in query-grammar.tool.test.ts + // exists to make that failure mode loud. + fetch: (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url + const rewritten = url.replace('/rest/v1/', '/').replace(/\/rest\/v1$/, '') + return fetch(rewritten, init) + }, + }, + }) +} diff --git a/tests/tool-pg/docker-compose.yml b/tests/tool-pg/docker-compose.yml new file mode 100644 index 00000000..9e271399 --- /dev/null +++ b/tests/tool-pg/docker-compose.yml @@ -0,0 +1,48 @@ +# Postgres + PostgREST for the MCP tool integration tests. +# +# The existing pg-real suite talks to Postgres through a raw `pg` Pool, which +# is the right shape for testing triggers and RLS in SQL. It is the wrong shape +# for testing the MCP tools, because every one of them queries through +# supabase-js, and supabase-js speaks PostgREST rather than SQL. The embeds, +# the `or=(col.is.null,col.not.in.(...))` forms, the `.contains()` filters and +# the column names in `.select()` strings are all resolved by PostgREST at +# request time, so a raw-SQL test cannot see any of them. +# +# That is the gap this stack exists to close: as of 2026-08-27 all 100 files in +# extensions/general/mcp-server/__tests__ fake supabase, and query-journal.test.ts +# says out loud that its query chain is "exercised by the live MCP smoke test", +# which does not exist in CI. +services: + postgres: + # Same image and tag as the pg-real CI job, deliberately: it ships the auth + # schema, auth.uid(), the anon/authenticated/service_role roles and the + # extensions this repo's migrations need. Plain postgres:15 would not. + image: supabase/postgres:15.8.1.060 + environment: + POSTGRES_PASSWORD: postgres + ports: + - '54329:5432' + healthcheck: + test: ['CMD-SHELL', 'pg_isready -U postgres'] + interval: 3s + timeout: 5s + retries: 30 + + postgrest: + image: postgrest/postgrest:v12.2.3 + depends_on: + postgres: + condition: service_healthy + environment: + PGRST_DB_URI: postgres://postgres:postgres@postgres:5432/postgres + PGRST_DB_SCHEMAS: public + PGRST_DB_ANON_ROLE: anon + # Must match TOOL_PG_JWT_SECRET in tests/tool-pg/env.ts. Length matters: + # PostgREST refuses a secret shorter than 32 bytes. + PGRST_JWT_SECRET: super-secret-jwt-token-with-at-least-32-characters-long + # The tools call RPCs and read a lot of rows; the default 1000-row cap + # would silently truncate and make a passing test meaningless. + PGRST_DB_MAX_ROWS: '100000' + PGRST_DB_POOL: '10' + ports: + - '54330:3000' diff --git a/tests/tool-pg/query-grammar.tool.test.ts b/tests/tool-pg/query-grammar.tool.test.ts new file mode 100644 index 00000000..7abf74cd --- /dev/null +++ b/tests/tool-pg/query-grammar.tool.test.ts @@ -0,0 +1,171 @@ +/** + * Every read-only MCP tool, run once against a real PostgREST. + * + * ## What this catches that nothing else does + * + * A tool's query is half TypeScript and half a string that PostgREST parses at + * request time. `select('a, b:c(d)')` names columns and a resource embed; + * `or('a.is.null,b.not.in.(1,2)')` is a grammar; `.contains()` requires a jsonb + * or array column. All of it is resolved by PostgREST, none of it by the type + * system, and a mocked client answers every one of them cheerfully. + * + * As of 2026-08-27 all 100 files in extensions/general/mcp-server/__tests__ + * fake supabase; query-journal.test.ts states that its query chain is + * "exercised by the live MCP smoke test", and no such test exists in CI. So + * this class of bug (a 42703 undefined column, a PGRST200 missing relationship) + * reaches production and shows up as an agent-reported failure. + * + * ## What a failure here means, and what it does not + * + * This asserts ONLY that the query is well-formed against the real schema. A + * tool that legitimately refuses because a required argument is missing, or + * because the seeded company has no data, is not a failure: those are filtered + * out below. The bar is deliberately narrow so the suite stays honest. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import { tools } from '@/extensions/general/mcp-server/server' +import { seedCompany } from '@/tests/pg/fixtures' +import { createToolPgClient, TOOL_PG_REST_URL } from './client' + +/** + * Postgres and PostgREST error codes that mean "this query is malformed", as + * opposed to "this request was refused for a domain reason". + */ +const MALFORMED_QUERY_CODES = new Set([ + '42703', // undefined_column + '42P01', // undefined_table + '42883', // undefined_function + '42P10', // invalid_column_reference + 'PGRST100', // parse error in the query-string specifier + 'PGRST200', // requested embed has no relationship + 'PGRST202', // requested function not found in the schema cache +]) + +/** + * Errors are captured at the TRANSPORT, not from the thrown Error. + * + * This matters more than it sounds. Sweeping the tools and inspecting what they + * throw looks equivalent and is not: the tools wrap failures in their own prose + * and lose the payload doing it, so a real 42703 arrives as the string + * "Database error: undefined" and every classifier downstream sees nothing. The + * first version of this file passed for exactly that reason while 55 real + * queries were failing underneath it. + * + * Intercepting fetch sees what PostgREST actually said, regardless of how the + * calling tool chose to report it. + */ +interface CapturedFailure { + tool: string + status: number + code: string | null + message: string + url: string +} + +const captured: CapturedFailure[] = [] +/** Every request that reached PostgREST, failed or not. */ +let requestCount = 0 +let currentTool = '(none)' +const originalFetch = globalThis.fetch +const REST_HOST = TOOL_PG_REST_URL.replace(/^https?:\/\//, '') + +beforeAll(() => { + globalThis.fetch = (async (...args: Parameters) => { + const response = await originalFetch(...args) + const url = String(args[0]) + if (url.includes(REST_HOST)) requestCount += 1 + if (!response.ok && url.includes(REST_HOST)) { + let code: string | null = null + let message = '' + try { + const body = (await response.clone().json()) as { code?: string; message?: string } + code = typeof body.code === 'string' ? body.code : null + message = typeof body.message === 'string' ? body.message : JSON.stringify(body) + } catch { + message = await response.clone().text() + } + captured.push({ tool: currentTool, status: response.status, code, message, url }) + } + return response + }) as typeof fetch +}) + +afterAll(() => { + globalThis.fetch = originalFetch +}) + +const readTools = tools.filter((t) => t.annotations.readOnlyHint === true) + +let companyId: string +let userId: string +let client: ReturnType + +beforeAll(async () => { + // Constructed ONCE, here, on purpose. Building it inside the sweep loop puts + // it inside the per-tool try/catch, so a client that cannot be constructed at + // all is swallowed 74 times as a "domain refusal" and the suite reports a + // clean sweep having issued zero queries. That is not hypothetical: it is + // what this file did on CI, where Node 20 could not give supabase-js a + // WebSocket and every createClient threw. + client = createToolPgClient() + const seeded = await seedCompany() + companyId = seeded.companyId + userId = seeded.userId +}, 30_000) + +describe('MCP read tools against real PostgREST', () => { + it('has a non-trivial number of read tools to sweep', () => { + // Guards against the filter silently matching nothing after a refactor, + // which would turn this whole file into a no-op that always passes. + expect(readTools.length).toBeGreaterThan(50) + }) + + it('detects a malformed query, so that a green sweep means something', async () => { + // The self-test that the previous version of this file needed and did not + // have. A harness that cannot see a failure reports none, and the sweep + // below would then pass forever while the surface it guards rotted. + currentTool = '(self-test)' + const before = captured.length + await client.from('transactions').select('no_such_column_exists').limit(1) + currentTool = '(none)' + + const detected = captured.slice(before).filter((f) => f.code && MALFORMED_QUERY_CODES.has(f.code)) + expect(detected.length, 'a deliberately bad column was not detected').toBeGreaterThan(0) + expect(detected[0].code).toBe('42703') + + // Drop the deliberate failure so it cannot pollute the sweep assertion. + captured.length = before + }, 30_000) + + it('issues no malformed query', async () => { + for (const tool of readTools) { + currentTool = tool.name + try { + await tool.execute({ __keyScopes: [] }, companyId, userId, client as never, { + type: 'api_key', + }) + } catch { + // A domain refusal (missing argument, no data, scope, capability) is + // not what this file is about. Only what PostgREST said counts, and + // that was captured at the transport above. + } + } + currentTool = '(none)' + + // Proof of life, and it has to be a real number. A sweep that issues + // nothing reports no failures and passes, which is how this file lied + // twice: once locally when every query 404d on the /rest/v1 prefix, and + // once on CI when every client construction threw. Both times the assertion + // here was satisfiable without a single request being made. + // + // 50 is well under the 87 observed on 2026-08-27 and well above anything a + // broken harness produces, which is zero. + expect(requestCount, 'the sweep barely reached PostgREST').toBeGreaterThan(50) + + const malformed = captured + .filter((f) => f.code && MALFORMED_QUERY_CODES.has(f.code)) + .map((f) => `${f.tool}: ${f.code} ${f.message}`) + + expect([...new Set(malformed)]).toEqual([]) + }, 300_000) +}) diff --git a/tests/tool-pg/setup.ts b/tests/tool-pg/setup.ts new file mode 100644 index 00000000..63ace48c --- /dev/null +++ b/tests/tool-pg/setup.ts @@ -0,0 +1,35 @@ +/** + * Vitest setup for the MCP tool integration project. + * + * Fails loudly and early if the stack is not up, because the alternative is a + * suite that silently passes against nothing. `scripts/tool-pg/reset.sh` brings + * it up and replays every migration. + */ +import { beforeAll } from 'vitest' +import { TOOL_PG_REST_URL, TOOL_PG_DATABASE_URL, signServiceRoleJwt } from './client' + +// tests/pg/fixtures.ts builds its rows through a `pg` Pool keyed on +// DATABASE_URL. Pointing that at the same database lets this project reuse +// seedCompany / insertCompany / insertPostedJournalEntry verbatim instead of +// growing a second, divergent set of fixtures. +process.env.DATABASE_URL ??= TOOL_PG_DATABASE_URL + +beforeAll(async () => { + const jwt = signServiceRoleJwt() + let response: Response + try { + response = await fetch(`${TOOL_PG_REST_URL}/companies?select=id&limit=1`, { + headers: { apikey: jwt, Authorization: `Bearer ${jwt}` }, + }) + } catch (err) { + throw new Error( + `tool-pg: PostgREST unreachable at ${TOOL_PG_REST_URL}. Run: npm run tools:pg:reset\n${String(err)}`, + ) + } + if (!response.ok) { + throw new Error( + `tool-pg: PostgREST answered ${response.status} for a trivial read: ${await response.text()}\n` + + 'The schema is probably missing or stale. Run: npm run tools:pg:reset', + ) + } +}, 30_000) diff --git a/vitest.config.ts b/vitest.config.ts index 5a02c730..fc03cd21 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -18,7 +18,20 @@ const unitProject = { include: ['**/*.test.ts'], // `.claude/worktrees/*` are ephemeral agent checkouts whose `@/*` imports // resolve back to this root: never part of the suite. - exclude: ['**/node_modules/**', '**/*.pg.test.ts', '**/.claude/**'], + // Two additions to the exclude list, both learned the hard way: + // * `*.tool.test.ts` also matches `**/*.test.ts`, so without it the unit + // project runs the PostgREST suite with no stack up. + // * `.next/standalone` is a traced COPY of the repo left by a local + // build, so `npm run build && npm test` collects those files a second + // time. Only two match here today, but the duplication is silent and + // grows with the build's tracing. + exclude: [ + '**/node_modules/**', + '**/*.pg.test.ts', + '**/*.tool.test.ts', + '**/.claude/**', + '**/.next/**', + ], }, } @@ -29,7 +42,9 @@ const pgRealProject = { globals: true, environment: 'node' as const, include: ['**/*.pg.test.ts'], - exclude: ['**/node_modules/**', '**/.claude/**'], + // `.next/standalone` holds a traced copy of these files after a local + // build; 128 of them match this project's glob. + exclude: ['**/node_modules/**', '**/.claude/**', '**/.next/**'], setupFiles: ['tests/pg/setup.ts'], // One-connection-at-a-time to avoid cross-file DB contention. fileParallelism: false, @@ -37,12 +52,36 @@ const pgRealProject = { }, } +// MCP tools queried through a REAL supabase-js client against a REAL +// PostgREST. Separate from pg-real because that project holds a `pg` Pool and +// writes SQL, which cannot see the half of a tool that PostgREST resolves: the +// `.select()` column strings, the resource embeds, the or=(...) grammar. +const toolPgProject = { + resolve: { alias }, + test: { + name: 'tool-pg', + globals: true, + environment: 'node' as const, + include: ['**/*.tool.test.ts'], + // See the unit project's note: `.next/standalone` is a build artifact that + // would otherwise be collected as a second copy of this suite. + exclude: ['**/node_modules/**', '**/.claude/**', '**/.next/**'], + setupFiles: ['tests/tool-pg/setup.ts'], + // The suite shares one database; parallel files would race on seeded rows. + fileParallelism: false, + testTimeout: 30000, + }, +} + // Only register the pg-real project when DATABASE_URL is set. Local devs // running a bare `vitest run` would otherwise hit the schema sanity check -// against a non-existent DB. `npm run test:pg` is the opt-in entry point. -const projects = process.env.DATABASE_URL - ? [unitProject, pgRealProject] - : [unitProject] +// against a non-existent DB. `npm run test:pg` is the opt-in entry point, and +// `npm run test:tools` is the equivalent for tool-pg. +const projects = [ + unitProject, + ...(process.env.DATABASE_URL ? [pgRealProject] : []), + ...(process.env.TOOL_PG_REST_URL ? [toolPgProject] : []), +] export default defineConfig({ resolve: { alias },