From 61145bc20bedb0badef1e771d713a7672cc927fd Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Thu, 22 Oct 2026 07:54:56 +0000 Subject: [PATCH] feat(mcp): diff and sync, as the CLI calls them scan_drift and apply_fixes become diff and sync. A single tool with a "really write" argument has to be marked destructive, so a client asked before every plan, or read-only while it could write. Two tools let each annotation be true. - diff writes nothing and returns the plan: the statements sync would run, in order, and what it holds back. apply_fixes previewed by default but called its preview "applied". - sync takes the same arguments, so the plan shown is the plan run, and adds prove: a rehearsal on a throwaway copy that refuses to write if the copy would not match the source. The proof is shared with diff --apply --prove, moved out of the command into prove-gate. - list_environments names the environments without their credentials, for clients that show tools but not resources. - create_migration writes what migrate create writes, grants included; it diffed two snapshots of one environment. The file's construction is shared, in migration-file. - Schema and data findings say what differs and what the fix does, in words, where they said "difference detected by @dbdiff/cli". Co-Authored-By: Claude Opus 5.5 --- packages/cli/README.md | 19 +- packages/cli/mcpb/manifest.template.json | 5 +- packages/cli/src/commands/diff.ts | 122 ++---- packages/cli/src/commands/migrate/create.ts | 90 +---- packages/cli/src/dbdiff.ts | 21 +- packages/cli/src/mcp/server.ts | 420 +++++++++++--------- packages/cli/src/migration-file.ts | 106 +++++ packages/cli/src/prove-gate.ts | 101 +++++ packages/cli/test/e2e/mcp-soak.test.ts | 2 +- packages/cli/test/mcp/server-safety.test.ts | 50 +-- packages/cli/test/mcp/server.test.ts | 50 ++- packages/mcp/test/server.test.ts | 25 +- 12 files changed, 587 insertions(+), 424 deletions(-) create mode 100644 packages/cli/src/migration-file.ts create mode 100644 packages/cli/src/prove-gate.ts diff --git a/packages/cli/README.md b/packages/cli/README.md index 8870125..7148a62 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -367,13 +367,18 @@ as `io.github.akalsoftware/supaforge`. The server exposes these tools: -| Tool | Annotation | -| --- | --- | -| `scan_drift` | Read-only | -| `get_check_result` | Read-only | -| `apply_fixes` | Destructive. Previews unless `dryRun: false` | -| `take_snapshot` | Writes files | -| `create_migration` | Writes files | +| Tool | What it does | Annotation | +| --- | --- | --- | +| `list_environments` | The environments in the config, without their credentials | Read-only | +| `diff` | Every difference, with the plan `sync` would run | Read-only | +| `get_check_result` | One check's result from the last `diff` or `sync` | Read-only | +| `sync` | Runs the plan on the target; `prove: true` rehearses it on a copy first | Destructive | +| `create_migration` | Writes the drift as a migration file, as `migrate create` does | Writes files | +| `take_snapshot` | Snapshots an environment to local files | Writes files | + +`diff` and `sync` take the same arguments, so the plan an agent shows you is +the plan that runs. Fixes that drop tables or columns are held back unless +`allowDestructive` is set. It reads `supaforge.config.json` in the directory given by `--project` (or the one it starts in), and refuses a `configPath` from a client unless started diff --git a/packages/cli/mcpb/manifest.template.json b/packages/cli/mcpb/manifest.template.json index e769fd3..cb95ada 100644 --- a/packages/cli/mcpb/manifest.template.json +++ b/packages/cli/mcpb/manifest.template.json @@ -28,8 +28,9 @@ } }, "tools": [ - { "name": "scan_drift", "description": "Compare two environments and report drift, with the SQL to fix it" }, - { "name": "apply_fixes", "description": "Apply fixes from a scan to the target environment" }, + { "name": "list_environments", "description": "List the environments in the config, without their credentials" }, + { "name": "diff", "description": "Compare two environments and report every difference, with the plan sync would run" }, + { "name": "sync", "description": "Make the target match the source by running that plan, optionally proved on a copy first" }, { "name": "take_snapshot", "description": "Snapshot an environment to local files" }, { "name": "create_migration", "description": "Write a migration file for the drift found" }, { "name": "get_check_result", "description": "Read one check's result from the last scan" } diff --git a/packages/cli/src/commands/diff.ts b/packages/cli/src/commands/diff.ts index 048f602..c1df475 100644 --- a/packages/cli/src/commands/diff.ts +++ b/packages/cli/src/commands/diff.ts @@ -4,7 +4,7 @@ import { createDefaultRegistry } from '../checks/index.js' import { scan } from '../scanner.js' import type { ScanProgressEvent } from '../scanner.js' import { renderSummary, renderDetailed, formatSkips } from '../render.js' -import { promote, planWork, type PromoteResult } from '../promote.js' +import { promote, type PromoteResult } from '../promote.js' import type { CheckName, ScanResult } from '../types/drift.js' import { CHECK_NAMES, CHECK_META } from '../types/drift.js' import { ok, warn, dim, cmd, bold } from '../ui.js' @@ -14,9 +14,9 @@ import { formatGitHubAnnotations, computeCiExitCode, formatCiSummary, type FailO import { resolveTableFilter, isFiltered, describeTableFilter } from '../utils/table-filter.js' import { parseFlagList } from '../utils/strings.js' import { isCloneDatabase, sourceLooksLikeCloneOf } from '../branch.js' -import { proveConvergence, residualHeldBack, proofScope, comparedSchemas } from '../prove.js' +import { proveBeforeApply } from '../prove-gate.js' import { summarizeByKind } from '../scoring.js' -import { CLONE_SKIP_FLAGS, DEFAULT_IGNORE_SCHEMAS, schemaDiffIgnores } from '../defaults.js' +import { CLONE_SKIP_FLAGS, DEFAULT_IGNORE_SCHEMAS } from '../defaults.js' import { Preflight } from '../preflight.js' import type { SupaForgeConfig } from '../types/config.js' import { redactUrls, describeFailure } from '../utils/error.js' @@ -438,83 +438,56 @@ export default class Diff extends BaseCommand { const targetEnv = config.environments[config.target!] const dryRun = flags['dry-run'] - // Prove before touching the target. A migration that executes without - // error can still leave the database in a state that is not the source — - // a flattened partition or an index that never reached its partitions - // both apply cleanly. Only replaying it and looking at the result catches - // that, so when --prove is set a failure blocks the apply rather than - // warning after the fact. + // Prove before touching the target — see prove-gate. if (flags.prove && !dryRun) { - const sourceEnv = config.environments[config.source!] - const planned = planWork(scanResult, { - checks, only: parseFlagList(flags.only), - allowDestructive: flags['allow-destructive'], tableFilter, - applyPosture: flags['apply-posture'], - }) - const { replay, unproved } = proofScope(planned.sqlStatements) - const migrationSql = replay.map(s => s.sql).join('\n') - this.log(`\n ${dim('Proving convergence on a throwaway clone…')}`) - if (unproved.length > 0) { + const proof = await proveBeforeApply({ + scanResult, + plan: { + checks, only: parseFlagList(flags.only), + allowDestructive: flags['allow-destructive'], tableFilter, + applyPosture: flags['apply-posture'], + }, + sourceUrl: config.environments[config.source!].dbUrl, + targetUrl: targetEnv.dbUrl, + ignoreSchemas: config.ignoreSchemas, + }) + if (proof.unproved.length > 0) { // Applied all the same; the proof just cannot speak for them. - const byCheck = [...new Set(unproved.map(s => s.check))] - .map(c => `${c} ×${unproved.filter(s => s.check === c).length}`).join(', ') + const byCheck = [...new Set(proof.unproved.map(s => s.check))] + .map(c => `${c} ×${proof.unproved.filter(s => s.check === c).length}`).join(', ') this.log(` ${dim(`Not proved — act outside the proved schemas or on the whole server: ${byCheck}`)}`) } - // Nothing left to replay means nothing to prove. Running it anyway - // compared the whole schema, and refused a cron-only apply over - // tables it was never going to touch. - const proof = replay.length === 0 - ? { converged: false, residual: [], cloneName: '', skipped: 'none of the planned fixes is one the proof can replay' } - : await proveConvergence({ - sourceUrl: sourceEnv.dbUrl, - targetUrl: targetEnv.dbUrl, - migrationSql, - fixes: replay, - // Every schema the schema diff compares, not `public` alone: a - // migration dropping a schema the source lacks runs against it. - schemas: await comparedSchemas(sourceEnv.dbUrl, targetEnv.dbUrl, - schemaDiffIgnores(config.ignoreSchemas ?? DEFAULT_IGNORE_SCHEMAS)), - // The checks this run compares: without the schema check, what the - // target already lacks elsewhere is not this migration's to fix. - checks: scanResult.checks.map(c => c.check), - }) - - // What the replay left out because this server can't hold it, as the - // apply will: expected to stay different, so it counts as held back. - const heldBack = [...heldBackSql(scanResult, planned), ...(proof.notApplied ?? []).map(n => n.sql)] - if (proof.notApplied?.length) { + if (proof.notApplied.length > 0) { this.log(` ${dim(`Left out, as the apply will leave them — this server can't hold them: ${proof.notApplied.map(n => n.issueId).join(', ')}`)}`) } - if (proof.skipped && replay.length === 0) { - // Nothing the proof could replay: every planned fix is listed above - // as not proved, and there is nothing a rehearsal would add. - this.log(` ${dim(`Nothing to prove: ${proof.skipped}.`)}\n`) - } else if (proof.skipped) { - // Could not prove is not the same as failed to converge, so say which — - // but either way nothing is applied. --prove is asked for as a gate, - // and it used to apply anyway, exiting 0, when it could not run. - this.refuseUnproven(`Not proven: ${proof.skipped}`, [], flags.json) - } else if (!proof.converged && residualHeldBack(proof.residual, heldBack).unexplained.length === 0) { - // Everything left over is what this run deliberately holds back. - this.log(` ${ok('Converged')}, apart from what is held back and stays on the target:`) - for (const line of proof.residual.slice(0, 15)) this.log(` ${dim(line)}`) - this.log('') - } else if (!proof.converged) { - const { unexplained } = residualHeldBack(proof.residual, heldBack) - this.refuseUnproven('Migration does not reproduce the source.', unexplained, flags.json) - } else { - if (proof.outOfScope?.length) { - this.log(` ${ok('Converged')} on what this run compares. Outside it, and already different before:`) - for (const line of proof.outOfScope.slice(0, 15)) this.log(` ${dim(line)}`) - if (proof.outOfScope.length > 15) this.log(` ${dim(`…and ${proof.outOfScope.length - 15} more`)}`) + switch (proof.verdict) { + case 'nothing-to-prove': + this.log(` ${dim(`Nothing to prove: ${proof.reason}.`)}\n`) + break + case 'not-proven': + this.refuseUnproven(`Not proven: ${proof.reason}`, [], flags.json) + break + case 'diverged': + this.refuseUnproven('Migration does not reproduce the source.', proof.residual, flags.json) + break + case 'converged-apart-from-held-back': + this.log(` ${ok('Converged')}, apart from what is held back and stays on the target:`) + for (const line of proof.residual.slice(0, 15)) this.log(` ${dim(line)}`) this.log('') - } else { - this.log(` ${ok('Converged')} — the migration reproduces the source exactly.\n`) - } + break + case 'converged': + if (proof.outOfScope.length > 0) { + this.log(` ${ok('Converged')} on what this run compares. Outside it, and already different before:`) + for (const line of proof.outOfScope.slice(0, 15)) this.log(` ${dim(line)}`) + if (proof.outOfScope.length > 15) this.log(` ${dim(`…and ${proof.outOfScope.length - 15} more`)}`) + this.log('') + } else { + this.log(` ${ok('Converged')} — the migration reproduces the source exactly.\n`) + } } - if (proof.data && !proof.skipped) { + if (proof.data) { const { sampled, unchecked, rewritten } = proof.data if (sampled.length > 0) this.log(` ${ok('Rows kept')} in the ${sampled.length} table(s) sampled.`) if (rewritten.length > 0) { @@ -756,15 +729,6 @@ export default class Diff extends BaseCommand { } } -/** The SQL of the fixes this run plans to skip, which the proof did not replay. */ -function heldBackSql(scanResult: ScanResult, planned: { skipped: Array<{ issueId: string }> }): string[] { - const skipped = new Set(planned.skipped.map(s => s.issueId)) - return scanResult.checks.flatMap(c => c.issues) - .filter(i => skipped.has(i.id) && i.sql?.up) - .map(i => i.sql!.up) -} - - /** A preflight entry without the raw error object, which may not serialise. */ function stripError(c: { label: string; passed: boolean; detail?: string; error?: unknown }) { return { label: c.label, passed: c.passed, ...(c.detail ? { detail: c.detail } : {}), diff --git a/packages/cli/src/commands/migrate/create.ts b/packages/cli/src/commands/migrate/create.ts index 6732d1c..6844ae2 100644 --- a/packages/cli/src/commands/migrate/create.ts +++ b/packages/cli/src/commands/migrate/create.ts @@ -1,17 +1,8 @@ -import { writeFile, mkdir } from 'node:fs/promises' -import { join } from 'node:path' import { Flags } from '@oclif/core' import { BaseCommand } from '../../base-command.js' -import { runDbDiff, isDestructiveSql } from '../../dbdiff.js' -import { RolesCheck } from '../../checks/roles.js' -import type { CheckContext } from '../../checks/base.js' -import { pgQuery } from '../../db.js' -import { DEFAULT_IGNORE_SCHEMAS } from '../../defaults.js' +import { writeMigrationFile } from '../../migration-file.js' import { ok, warn, dim, bold, cmd } from '../../ui.js' -import { splitSqlStatements } from '../../utils/sql-split.js' import { truncateSql } from './run.js' -import { slugify } from '../../utils/strings.js' -import { targetFingerprint, writtenAgainstLine } from '../../written-against.js' /** * Generate a new migration file from the current schema drift. @@ -48,7 +39,6 @@ export default class MigrateCreate extends BaseCommand { const sourceEnv = config.environments[config.source!] const targetEnv = config.environments[config.target!] const dir = this.resolveMigrationsDir(config) - const ignoreSchemas = config.ignoreSchemas ?? DEFAULT_IGNORE_SCHEMAS // Preflight: verify both databases are reachable const pre = this.createPreflight('Migrate create preflight checks') @@ -58,69 +48,12 @@ export default class MigrateCreate extends BaseCommand { this.log(`${bold('migrate create')} — diffing ${dim(config.source!)} → ${dim(config.target!)}\n`) - const result = await runDbDiff({ - sourceUrl: sourceEnv.dbUrl, - targetUrl: targetEnv.dbUrl, - type: 'schema', - include: 'both', - ignoreSchemas, - }) - - // The grants on what it makes, written out: the file is replayed where - // default privileges differ — a new project, a preview branch, a local - // reset — and Supabase no longer grants the Data API roles on new tables. - const grants = (await new RolesCheck(pgQuery, { explicit: true }).scan({ - source: sourceEnv, target: targetEnv, config, - } as CheckContext)).filter(i => i.id.startsWith('roles-grant-') && i.sql?.up) - - if (!result.up.trim() && grants.length === 0) { + const { filePath, destructive } = await writeMigrationFile({ config, name: flags.name, dir }) + if (!filePath) { this.log(`${ok('No schema drift detected.')} Nothing to write. ✓`) return } - // Generate timestamped filename - const timestamp = generateTimestamp() - const slug = slugify(flags.name) - const filename = `${timestamp}_${slug}.sql` - - // Said when the file is written, not first when `migrate run` refuses it: - // a diff in which the target has a table the source lacks becomes a - // DROP TABLE here, and the file looks like any other. - const destructive = splitSqlStatements(result.up).filter(isDestructiveSql) - - // Build file content with UP and optional DOWN - // The target as the file was written against it, for migrate run to check. - const digest = await targetFingerprint(targetEnv.dbUrl, ignoreSchemas).catch(() => undefined) - const lines: string[] = [ - `-- Migration: ${flags.name}`, - `-- Generated by supaforge migrate create`, - `-- Source: ${config.source!} → Target: ${config.target!}`, - ...(digest ? [writtenAgainstLine(config.target!, digest)] : []), - ...(destructive.length > 0 - ? [`-- WARNING: ${destructive.length} destructive statement(s) — drops or deletes data. Review before running.`] - : []), - '', - result.up, - ...(grants.length > 0 ? ['', '-- Grants', ...grants.map(i => i.sql!.up)] : []), - ] - - if (result.down.trim() || grants.length > 0) { - lines.push( - '', - '-- ==================== DOWN ====================', - '-- To revert this migration, run the SQL below manually:', - '--', - ...[...grants.map(i => i.sql?.down ?? '').filter(Boolean).reverse(), result.down].join('\n').split('\n').map(l => `-- ${l}`), - ) - } - - const content = lines.join('\n') + '\n' - - // Write file - await mkdir(dir, { recursive: true }) - const filePath = join(dir, filename) - await writeFile(filePath, content, 'utf8') - this.log(`${ok('Created migration:')} ${cmd(filePath)}`) if (destructive.length > 0) { this.log(`\n${warn(`${destructive.length} destructive statement(s) in it`)} ${dim('— they drop or delete data:')}`) @@ -133,22 +66,7 @@ export default class MigrateCreate extends BaseCommand { } } -/** Generate a UTC timestamp in Supabase migration format: YYYYMMDDHHmmss */ -export function generateTimestamp(): string { - const now = new Date() - return [ - now.getUTCFullYear(), - pad(now.getUTCMonth() + 1), - pad(now.getUTCDate()), - pad(now.getUTCHours()), - pad(now.getUTCMinutes()), - pad(now.getUTCSeconds()), - ].join('') -} - -function pad(n: number): string { - return String(n).padStart(2, '0') -} +export { generateTimestamp } from '../../migration-file.js' /** Re-export for backward-compat — uses shared slugify with underscore separator. */ export { slugify } from '../../utils/strings.js' diff --git a/packages/cli/src/dbdiff.ts b/packages/cli/src/dbdiff.ts index 3b33ca4..9a45797 100644 --- a/packages/cli/src/dbdiff.ts +++ b/packages/cli/src/dbdiff.ts @@ -767,7 +767,7 @@ function unfoldedIssues( // The DOWN counterpart is passed so a title can recover a schema the UP // statement does not carry — see routineLabel (issue #47). title: summariseStatement(lead, check, downSql), - description: `${check === 'schema' ? 'Schema' : 'Data'} difference detected by @dbdiff/cli.`, + description: describeDifference(type), sql: { up: upSql, down: downSql }, } }) @@ -848,7 +848,7 @@ function issuesFromUnits( check, severity: DROP_TYPES.includes(type) ? 'critical' : 'warning', title: summariseStatement(lead, check, downSql), - description: `${check === 'schema' ? 'Schema' : 'Data'} difference detected by @dbdiff/cli.`, + description: describeDifference(type), sql: { up: upSql, down: downSql }, ...manual, } @@ -1410,6 +1410,23 @@ function withoutLeadingComments(chunk: string): string { return chunk.trim().replace(/^(?:--[^\n]*(?:\n|$)\s*)+/, '').trim() } +/** + * What a finding means and what its fix does, from the kind of statement that + * leads it. Said in words because an agent relays the description to a person, + * who should not need to read the SQL to know whether it adds or removes. + */ +export function describeDifference(type: string): string { + if (type === 'insert') return 'Rows in the source are missing from the target. The fix inserts them.' + if (type === 'update') return 'Rows differ between source and target. The fix updates the target\'s rows to match.' + if (type === 'delete') return 'The target has rows the source does not. The fix deletes them from the target.' + if (type === 'drop' || type.startsWith('drop-')) { + return 'The target has this and the source does not. The fix drops it from the target, ' + + 'and with it any data it holds.' + } + if (type.startsWith('create')) return 'The source has this and the target does not. The fix creates it on the target.' + return 'This exists on both sides but differs. The fix changes the target to match the source.' +} + export function classifyStatement(sql: string): string { const upper = sql.toUpperCase().trimStart() // Views diff --git a/packages/cli/src/mcp/server.ts b/packages/cli/src/mcp/server.ts index e383b71..f5da751 100644 --- a/packages/cli/src/mcp/server.ts +++ b/packages/cli/src/mcp/server.ts @@ -8,10 +8,11 @@ import { resolveConfig, validateConfig } from '../config.js' import { createDefaultRegistry } from '../checks/index.js' import { scan } from '../scanner.js' import { promote } from '../promote.js' +import { proveBeforeApply } from '../prove-gate.js' import { checkConnection } from '../preflight.js' import { redactUrls, describeFailure } from '../utils/error.js' import { captureSnapshot, unrestorableSchema } from '../snapshot.js' -import { backup } from '../migration.js' +import { writeMigrationFile, migrationsDir } from '../migration-file.js' import { CHECK_NAMES } from '../types/drift.js' import type { ScanResult, CheckName } from '../types/drift.js' import { setLastScanResult, getLastScanResult } from './state.js' @@ -109,177 +110,233 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): // ── Tools ────────────────────────────────────────────────────────────────── + const configPathArg = z + .string() + .optional() + .describe( + 'Absolute path to supaforge.config.json. Rejected unless the server was ' + + 'started with --allow-config-path; otherwise the config in the server\'s ' + + 'own working directory is used.', + ) + + /** The arguments diff and sync share, so the plan diff shows is the one sync runs. */ + const comparisonArgs = { + configPath: configPathArg, + source: z.string().optional().describe('Source environment name; list_environments shows them. Defaults to the config\'s source.'), + target: z.string().optional().describe('Target environment name. Defaults to the config\'s target.'), + checks: z + .array(z.enum(CHECK_NAMES)) + .optional() + .describe('Compare only these checks (e.g. ["schema", "rls"]).'), + skip: z + .array(z.enum(CHECK_NAMES)) + .optional() + .describe('Leave these checks out, as the CLI --skip flag does. Added to checks.exclude from the config.'), + only: z + .array(z.string()) + .optional() + .describe('Fix only these findings, by id from diff (globs allowed, e.g. "rls-*").'), + allowDestructive: z + .boolean() + .optional() + .default(false) + .describe('Include fixes that drop tables or columns. Off by default: they are reported but held back.'), + } + + /** Load the config with the source and target asked for, or say what is wrong with it. */ + const loadComparison = async (args: { configPath?: string; source?: string; target?: string }) => { + const effectiveCwd = resolveCwd(args.configPath) + const raw = JSON.parse(await readFile(resolve(effectiveCwd, 'supaforge.config.json'), 'utf-8')) + const config = resolveConfig({ + ...raw, + ...(args.source ? { source: args.source } : {}), + ...(args.target ? { target: args.target } : {}), + }) + const errors = validateConfig(config) + if (errors.length > 0) return { error: `Config errors:\n${errors.join('\n')}` } + + // Both databases must answer before anything is compared. Without this, an + // unreadable source looked empty to the checks that treated a failed read + // as "nothing there", and the target's webhooks were dropped as extra. + const unreachable: string[] = [] + for (const [label, name] of [['Source', config.source!], ['Target', config.target!]] as const) { + const conn = await checkConnection(config.environments[name].dbUrl) + if (!conn.reachable) unreachable.push(`${label} (${name}) not reachable: ${redactUrls(describeFailure(conn.error))}`) + } + if (unreachable.length > 0) return { error: `Nothing compared.\n${unreachable.join('\n')}` } + return { config, cwd: effectiveCwd } + } + + const textResult = (body: unknown, isError = false) => ({ + content: [{ type: 'text' as const, text: typeof body === 'string' ? body : JSON.stringify(body, null, 2) }], + ...(isError ? { isError: true } : {}), + }) + const failure = (err: unknown) => textResult(`Error: ${err instanceof Error ? err.message : String(err)}`, true) + + /** A check that failed measured nothing, so none of its drift is known. */ + const uncheckedOf = (scanResult: ScanResult) => scanResult.checks + .filter(c => c.status === 'error') + .map(c => ({ check: c.check, error: c.error ?? 'check failed' })) + /** - * scan_drift: Scan for environment drift and return a structured report. + * list_environments: the environments in the config, so an agent can name + * them. Clients that show tools but not resources had no other way to learn + * them short of reading the config file, credentials and all. */ server.registerTool( - 'scan_drift', + 'list_environments', { - // Reads both databases and writes nothing. Without annotations a client - // could not tell this from apply_fixes, so it could not ask for - // confirmation on the one that writes (issue #96). annotations: { readOnlyHint: true, destructiveHint: false, idempotentHint: true }, - description: - 'Scan for drift between Supabase environments and return a structured report. ' + - 'Results include per-check status, issues with SQL fixes, and a health score.', - inputSchema: { - configPath: z - .string() - .optional() - .describe( - 'Absolute path to supaforge.config.json. Rejected unless the server was ' - + 'started with --allow-config-path; otherwise the config in the server\'s ' - + 'own working directory is used.', - ), - source: z - .string() - .optional() - .describe('Source environment name (overrides config default).'), - target: z - .string() - .optional() - .describe('Target environment name (overrides config default).'), - checks: z - .array(z.enum(CHECK_NAMES)) - .optional() - .describe('Limit scan to specific checks (e.g. ["rls", "rls-coverage"]).'), - skip: z - .array(z.enum(CHECK_NAMES)) - .optional() - .describe( - 'Skip specific checks, the equivalent of the CLI --skip flag. ' + - 'Unioned with checks.exclude from the config file. Use this to avoid ' + - 'a slow or failing layer without editing the project config.', - ), - }, + 'List the environments in supaforge.config.json, and which are the default source and target. ' + + 'Says whether each has the Supabase project details the API-based checks need; never returns credentials.', + inputSchema: { configPath: configPathArg }, }, - async ({ configPath, source, target, checks, skip }) => { + async ({ configPath }) => { try { - const effectiveCwd = resolveCwd(configPath) - const raw = JSON.parse(await readFile(resolve(effectiveCwd, 'supaforge.config.json'), 'utf-8')) - const config = resolveConfig({ - ...raw, - ...(source ? { source } : {}), - ...(target ? { target } : {}), + const raw = JSON.parse(await readFile(resolve(resolveCwd(configPath), 'supaforge.config.json'), 'utf-8')) + const config = resolveConfig(raw) + return textResult({ + source: config.source ?? null, + target: config.target ?? null, + environments: Object.entries(config.environments).map(([name, env]) => ({ + name, + database: Boolean(env.dbUrl), + supabaseProject: Boolean(env.projectRef), + accessToken: Boolean(env.accessToken), + })), }) + } catch (err: unknown) { + return failure(err) + } + }, + ) - const errors = validateConfig(config) - if (errors.length > 0) { - return { content: [{ type: 'text' as const, text: `Config errors:\n${errors.join('\n')}` }], isError: true } - } + /** + * diff: what differs, and exactly what sync would run to fix it. + */ + server.registerTool( + 'diff', + { + // Reads both databases and writes nothing, so a client can let it run + // without asking. sync is the one that writes. + annotations: { readOnlyHint: true, destructiveHint: false, idempotentHint: true }, + description: + 'Compare two Supabase environments and report every difference: schema, RLS, roles and grants, ' + + 'auth, storage, Edge Functions, cron and more. Returns the findings with their SQL, a drift score, ' + + 'and the plan: the statements sync would run, in order, and what it would hold back and why. ' + + 'Writes nothing. Show the plan to the user before calling sync.', + inputSchema: comparisonArgs, + }, + async ({ configPath, source, target, checks, skip, only, allowDestructive }) => { + try { + const loaded = await loadComparison({ configPath, source, target }) + if ('error' in loaded) return textResult(loaded.error, true) + const { config } = loaded - const registry = createDefaultRegistry() - const result: ScanResult = await scan(registry, { + const scanResult: ScanResult = await scan(createDefaultRegistry(), { config, checks: checks as CheckName[] | undefined, skip: skip as CheckName[] | undefined, }) - setLastScanResult(result) + setLastScanResult(scanResult) - return { content: [{ type: 'text' as const, text: JSON.stringify(result, null, 2) }] } + const preview = await promote({ + dbUrl: config.environments[config.target!].dbUrl, + sourceUrl: config.environments[config.source!].dbUrl, + scanResult, + checks: checks as string[] | undefined, + only, + allowDestructive, + dryRun: true, + }) + const unchecked = uncheckedOf(scanResult) + return textResult({ + ...scanResult, + plan: { + statements: preview.applied, + heldBack: preview.skipped, + ...(preview.warnings?.length ? { warnings: preview.warnings } : {}), + }, + ...(unchecked.length ? { unchecked } : {}), + }) } catch (err: unknown) { - const msg = err instanceof Error ? err.message : String(err) - return { content: [{ type: 'text' as const, text: `Error: ${msg}` }], isError: true } + return failure(err) } }, ) /** - * apply_fixes: Apply SQL fixes to resolve drift in the target environment. + * sync: make the target match the source. */ server.registerTool( - 'apply_fixes', + 'sync', { - // Writes to the target database, and destructively when - // allowDestructive is set. This is the one a client should confirm. + // Writes to the target database. This is the one a client should confirm. annotations: { readOnlyHint: false, destructiveHint: true, idempotentHint: false }, - description: - 'Apply SQL fixes to resolve drift detected by scan_drift. ' + - 'Previews by default — pass dryRun=false to actually write to the target database.', + 'Make the target environment match the source by running the plan diff shows. WRITES TO THE TARGET ' + + 'DATABASE: call diff first, show the user the plan, and call this only once they agree, with the ' + + 'same arguments. SQL fixes run in one transaction and roll back together if any fails. ' + + 'prove=true first rehearses the plan on a throwaway copy of the target and refuses to write ' + + 'if the result would not match the source.', inputSchema: { - configPath: z - .string() - .optional() - .describe( - 'Absolute path to supaforge.config.json. Rejected unless the server was ' - + 'started with --allow-config-path; otherwise the config in the server\'s ' - + 'own working directory is used.', - ), - source: z.string().optional().describe('Source environment name.'), - target: z.string().optional().describe('Target environment name.'), - checks: z - .array(z.enum(CHECK_NAMES)) - .optional() - .describe('Apply fixes only for specific checks.'), - dryRun: z + ...comparisonArgs, + prove: z .boolean() .optional() - .default(true) + .default(false) .describe( - 'Preview SQL without executing it. Defaults to true: writing is opt-in ' - + 'here as it is everywhere else in supaforge, where it needs --apply. ' - + 'Pass dryRun=false to actually apply the fixes.', + 'Rehearse on a throwaway copy of the target first, and write nothing unless the copy ends up ' + + 'matching the source. Needs permission to create a database on the target\'s server.', ), }, }, - async ({ configPath, source, target, checks, dryRun }) => { + async ({ configPath, source, target, checks, skip, only, allowDestructive, prove }) => { try { - const effectiveCwd = resolveCwd(configPath) - const raw = JSON.parse(await readFile(resolve(effectiveCwd, 'supaforge.config.json'), 'utf-8')) - const config = resolveConfig({ - ...raw, - ...(source ? { source } : {}), - ...(target ? { target } : {}), - }) - - const errors = validateConfig(config) - if (errors.length > 0) { - return { content: [{ type: 'text' as const, text: `Config errors:\n${errors.join('\n')}` }], isError: true } - } + const loaded = await loadComparison({ configPath, source, target }) + if ('error' in loaded) return textResult(loaded.error, true) + const { config } = loaded + const sourceUrl = config.environments[config.source!].dbUrl + const targetUrl = config.environments[config.target!].dbUrl - // Both databases must answer before anything is compared. Without - // this, an unreadable source looked empty to the checks that treated a - // failed read as "nothing there", and the target's webhooks were - // dropped as extra. - const unreachable: string[] = [] - for (const [label, name] of [['Source', config.source!], ['Target', config.target!]] as const) { - const conn = await checkConnection(config.environments[name].dbUrl) - if (!conn.reachable) unreachable.push(`${label} (${name}) not reachable: ${redactUrls(describeFailure(conn.error))}`) - } - if (unreachable.length > 0) { - return { content: [{ type: 'text' as const, text: `Nothing compared or applied.\n${unreachable.join('\n')}` }], isError: true } - } - - const registry = createDefaultRegistry() - const scanResult: ScanResult = await scan(registry, { + const scanResult: ScanResult = await scan(createDefaultRegistry(), { config, checks: checks as CheckName[] | undefined, + skip: skip as CheckName[] | undefined, }) setLastScanResult(scanResult) - const targetEnv = config.environments[config.target!] - const promoteResult = await promote({ - dbUrl: targetEnv.dbUrl, + const proof = prove + ? await proveBeforeApply({ + scanResult, + plan: { checks: checks as string[] | undefined, only, allowDestructive }, + sourceUrl, + targetUrl, + ignoreSchemas: config.ignoreSchemas, + }) + : undefined + if (proof && !proof.proceed) { + return textResult({ written: false, proof, applied: [], skipped: [], errors: [] }, true) + } + + const result = await promote({ + dbUrl: targetUrl, + sourceUrl, scanResult, checks: checks as string[] | undefined, - dryRun, + only, + allowDestructive, }) - // A check that failed measured nothing, so none of its drift was // applied; the call is not a success. - const unchecked = scanResult.checks - .filter(c => c.status === 'error') - .map(c => ({ check: c.check, error: c.error ?? 'check failed' })) - const body = unchecked.length ? { ...promoteResult, unchecked } : promoteResult - return { - content: [{ type: 'text' as const, text: JSON.stringify(body, null, 2) }], - ...(unchecked.length || promoteResult.errors.length ? { isError: true } : {}), - } + const unchecked = uncheckedOf(scanResult) + return textResult( + { ...result, ...(proof ? { proof } : {}), ...(unchecked.length ? { unchecked } : {}) }, + unchecked.length > 0 || result.errors.length > 0, + ) } catch (err: unknown) { - const msg = err instanceof Error ? err.message : String(err) - return { content: [{ type: 'text' as const, text: `Error: ${msg}` }], isError: true } + return failure(err) } }, ) @@ -351,79 +408,48 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): ) /** - * create_migration: Create a migration file from snapshot diff. + * create_migration: write the schema drift as a migration file, as + * `migrate create` does — the same file, grants and all. */ server.registerTool( 'create_migration', { - // Local files only, as with take_snapshot. + // Reads both databases and writes one local file; no database changes. annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: false }, - description: - 'Capture a snapshot and generate a migration file containing the diff against the previous snapshot. ' + - 'Migration files are stored under .supaforge/migrations/.', + 'Write the schema difference between source and target as a migration file, as ' + + '`supaforge migrate create` does: the SQL that brings the target to the source, the grants on ' + + 'what it creates (so it works where new tables are not granted by default), and a DOWN to ' + + 'revert it. Written to the configured migrations directory (supabase/migrations by default). ' + + 'Changes no database.', inputSchema: { - configPath: z - .string() - .optional() - .describe( - 'Absolute path to supaforge.config.json. Rejected unless the server was ' - + 'started with --allow-config-path; otherwise the config in the server\'s ' - + 'own working directory is used.', - ), - environment: z + configPath: configPathArg, + source: comparisonArgs.source, + target: comparisonArgs.target, + name: z .string() - .optional() - .describe('Environment to snapshot (defaults to config source).'), - description: z - .string() - .optional() - .describe('Human-readable description for the migration (e.g. "add-rls-to-orders").'), + .min(1) + .describe('Name for the migration, used in the file name (e.g. "add_rls_to_orders").'), }, }, - async ({ configPath, environment, description }) => { + async ({ configPath, source, target, name }) => { try { - const effectiveCwd = resolveCwd(configPath) - const raw = JSON.parse(await readFile(resolve(effectiveCwd, 'supaforge.config.json'), 'utf-8')) - const config = resolveConfig(raw) - - const envName = environment ?? config.source ?? Object.keys(config.environments)[0] - const env = config.environments[envName] - if (!env) { - return { - content: [{ type: 'text' as const, text: `Environment "${envName}" not found in config.` }], - isError: true, - } - } - - const result = await backup({ - envName, - env, - config, - cwd: effectiveCwd, - description, + const loaded = await loadComparison({ configPath, source, target }) + if ('error' in loaded) return textResult(loaded.error, true) + const { config, cwd: effectiveCwd } = loaded + const { filePath, destructive } = await writeMigrationFile({ + config, name, dir: migrationsDir(config, effectiveCwd), + }) + if (!filePath) return textResult({ written: false, reason: 'No schema drift between source and target.' }) + return textResult({ + written: true, + migrationFile: filePath, + ...(destructive.length > 0 + ? { destructive, warning: 'These statements drop or delete data. migrate run applies them only with --allow-destructive.' } + : {}), }) - - return { - content: [ - { - type: 'text' as const, - text: JSON.stringify( - { - isBaseline: result.isBaseline, - migrationFile: result.migrationFile, - snapshotDir: result.snapshot.dir, - timestamp: result.snapshot.timestamp, - }, - null, - 2, - ), - }, - ], - } } catch (err: unknown) { - const msg = err instanceof Error ? err.message : String(err) - return { content: [{ type: 'text' as const, text: `Error: ${msg}` }], isError: true } + return failure(err) } }, ) @@ -438,7 +464,7 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): annotations: { readOnlyHint: true, destructiveHint: false, idempotentHint: true }, description: - 'Retrieve the result for a specific check from the most recent scan_drift call. ' + + 'Retrieve the result for one check from the most recent diff or sync call. ' + 'Returns issues, status, and duration for the named check.', inputSchema: { check: z.enum(CHECK_NAMES).describe('The check name to retrieve (e.g. "rls-coverage").'), @@ -448,7 +474,7 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): const result = getLastScanResult() if (!result) { return { - content: [{ type: 'text' as const, text: 'No scan result available. Run scan_drift first.' }], + content: [{ type: 'text' as const, text: 'No scan result available. Run diff first.' }], isError: true, } } @@ -490,7 +516,7 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): }, ) - /** Last scan result (updated after each scan_drift or apply_fixes call). */ + /** Last scan result (updated after each diff or sync call). */ server.registerResource( 'last-scan', 'supaforge://last-scan', @@ -506,7 +532,7 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): contents: [ { uri: uri.href, - text: JSON.stringify({ message: 'No scan result yet. Run scan_drift first.' }), + text: JSON.stringify({ message: 'No scan result yet. Run diff first.' }), mimeType: 'application/json', }, ], @@ -565,7 +591,7 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): const lastScan = getLastScanResult() const scanContext = lastScan ? `\n\nLast scan result (${lastScan.timestamp}):\n- Score: ${lastScan.score}\n- Total issues: ${lastScan.summary.total} (${lastScan.summary.critical} critical, ${lastScan.summary.warning} warnings)\n- Checks: ${lastScan.checks.filter(c => c.status === 'drifted').map(c => c.check).join(', ') || 'none drifted'}` - : '\n\nNo recent scan found — run scan_drift first for a current drift report.' + : '\n\nNo recent scan found — run diff first for a current drift report.' return { messages: [ @@ -578,10 +604,10 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): scanContext, '', 'Steps:', - '1. Call scan_drift to get the latest drift report.', + '1. Call diff to get the latest drift report and the plan.', '2. Review critical issues (especially rls-coverage and rls checks).', '3. Confirm all SQL fixes look correct before applying.', - '4. If safe, run apply_fixes with dryRun=true first, then dryRun=false.', + '4. If safe, and the user agrees to the plan, run sync (prove=true to rehearse it first).', ].join('\n'), }, }, @@ -599,7 +625,7 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): }, () => { const lastScan = getLastScanResult() - let criticalSummary = 'No recent scan — run scan_drift first.' + let criticalSummary = 'No recent scan — run diff first.' if (lastScan) { const criticalIssues = lastScan.checks .flatMap(c => c.issues) @@ -626,11 +652,11 @@ export function createServer(cwd = process.cwd(), options: ServerOptions = {}): criticalSummary, '', 'Recommended steps:', - '1. Run scan_drift to confirm the current state.', + '1. Run diff to confirm the current state and see the plan.', '2. Review each critical issue and its sql.up fix.', - '3. Run apply_fixes with dryRun=true to preview the SQL.', - '4. Confirm the SQL is safe, then run apply_fixes with dryRun=false.', - '5. Run scan_drift again to confirm all critical issues are resolved.', + '3. Check the plan diff returned: the SQL sync will run, in order.', + '4. Confirm with the user, then run sync with the same arguments (prove=true to rehearse it first).', + '5. Run diff again to confirm all critical issues are resolved.', '', 'Pay special attention to rls-coverage issues — tables without RLS enabled', 'are the CVE-2025-48757 vulnerability pattern.', diff --git a/packages/cli/src/migration-file.ts b/packages/cli/src/migration-file.ts new file mode 100644 index 0000000..fbf3af5 --- /dev/null +++ b/packages/cli/src/migration-file.ts @@ -0,0 +1,106 @@ +/** + * Write a migration file from the schema drift between source and target. + * + * Shared by `migrate create` and the MCP `create_migration` tool, so the file + * an agent writes is the file the CLI would. + */ +import { writeFile, mkdir } from 'node:fs/promises' +import { join, resolve } from 'node:path' +import { runDbDiff, isDestructiveSql } from './dbdiff.js' +import { RolesCheck } from './checks/roles.js' +import type { CheckContext } from './checks/base.js' +import { DEFAULT_MIGRATIONS_DIR } from './checks/migrations.js' +import { pgQuery } from './db.js' +import { DEFAULT_IGNORE_SCHEMAS } from './defaults.js' +import { splitSqlStatements } from './utils/sql-split.js' +import { slugify } from './utils/strings.js' +import { targetFingerprint, writtenAgainstLine } from './written-against.js' +import type { SupaForgeConfig } from './types/config.js' + +export interface MigrationFile { + /** Where it was written; undefined when there was no drift to write. */ + filePath?: string + /** Statements in it that drop or delete data. */ + destructive: string[] +} + +/** The directory migrations are written to, as configured. */ +export function migrationsDir(config: SupaForgeConfig, cwd = process.cwd()): string { + return resolve(cwd, config.checks?.migrations?.dir ?? DEFAULT_MIGRATIONS_DIR) +} + +export async function writeMigrationFile(opts: { config: SupaForgeConfig; name: string; dir: string }): Promise { + const { config, name, dir } = opts + const sourceEnv = config.environments[config.source!] + const targetEnv = config.environments[config.target!] + const ignoreSchemas = config.ignoreSchemas ?? DEFAULT_IGNORE_SCHEMAS + + const result = await runDbDiff({ + sourceUrl: sourceEnv.dbUrl, + targetUrl: targetEnv.dbUrl, + type: 'schema', + include: 'both', + ignoreSchemas, + }) + + // The grants on what it makes, written out: the file is replayed where + // default privileges differ — a new project, a preview branch, a local + // reset — and Supabase no longer grants the Data API roles on new tables. + const grants = (await new RolesCheck(pgQuery, { explicit: true }).scan({ + source: sourceEnv, target: targetEnv, config, + } as CheckContext)).filter(i => i.id.startsWith('roles-grant-') && i.sql?.up) + + if (!result.up.trim() && grants.length === 0) return { destructive: [] } + + // Said when the file is written, not first when `migrate run` refuses it: + // a diff in which the target has a table the source lacks becomes a + // DROP TABLE here, and the file looks like any other. + const destructive = splitSqlStatements(result.up).filter(isDestructiveSql) + + // The target as the file was written against it, for migrate run to check. + const digest = await targetFingerprint(targetEnv.dbUrl, ignoreSchemas).catch(() => undefined) + const lines: string[] = [ + `-- Migration: ${name}`, + `-- Generated by supaforge migrate create`, + `-- Source: ${config.source!} → Target: ${config.target!}`, + ...(digest ? [writtenAgainstLine(config.target!, digest)] : []), + ...(destructive.length > 0 + ? [`-- WARNING: ${destructive.length} destructive statement(s) — drops or deletes data. Review before running.`] + : []), + '', + result.up, + ...(grants.length > 0 ? ['', '-- Grants', ...grants.map(i => i.sql!.up)] : []), + ] + + if (result.down.trim() || grants.length > 0) { + lines.push( + '', + '-- ==================== DOWN ====================', + '-- To revert this migration, run the SQL below manually:', + '--', + ...[...grants.map(i => i.sql?.down ?? '').filter(Boolean).reverse(), result.down].join('\n').split('\n').map(l => `-- ${l}`), + ) + } + + await mkdir(dir, { recursive: true }) + const filePath = join(dir, `${generateTimestamp()}_${slugify(name)}.sql`) + await writeFile(filePath, lines.join('\n') + '\n', 'utf8') + return { filePath, destructive } +} + +/** Generate a UTC timestamp in Supabase migration format: YYYYMMDDHHmmss */ +export function generateTimestamp(): string { + const now = new Date() + return [ + now.getUTCFullYear(), + pad(now.getUTCMonth() + 1), + pad(now.getUTCDate()), + pad(now.getUTCHours()), + pad(now.getUTCMinutes()), + pad(now.getUTCSeconds()), + ].join('') +} + +function pad(n: number): string { + return String(n).padStart(2, '0') +} diff --git a/packages/cli/src/prove-gate.ts b/packages/cli/src/prove-gate.ts new file mode 100644 index 0000000..674c685 --- /dev/null +++ b/packages/cli/src/prove-gate.ts @@ -0,0 +1,101 @@ +/** + * Prove a planned apply on a throwaway clone before it touches the target. + * + * A migration that executes without error can still leave the database in a + * state that is not the source: a flattened partition, or an index that never + * reached its partitions, both apply cleanly. Only replaying it and looking at + * the result catches that, so a proof that fails blocks the apply rather than + * warning after the fact. + * + * Shared by `diff --apply --prove` and the MCP `sync` tool, which render the + * same outcome their own way; the decision is made once, here. + */ +import { planWork, type PlanOptions, type PlannedSql } from './promote.js' +import { proveConvergence, residualHeldBack, proofScope, comparedSchemas, type ProofResult } from './prove.js' +import { DEFAULT_IGNORE_SCHEMAS, schemaDiffIgnores } from './defaults.js' +import type { ScanResult } from './types/drift.js' + +export interface ProofOutcome { + /** Whether the apply may go ahead. */ + proceed: boolean + /** + * - `converged`: the migration reproduces the source. + * - `converged-apart-from-held-back`: all that is left is what this run holds back on purpose. + * - `nothing-to-prove`: no planned fix is one the proof can replay; the apply goes ahead unproved. + * - `not-proven`: the proof could not run, so nothing is applied. + * - `diverged`: the migration does not reproduce the source, so nothing is applied. + */ + verdict: 'converged' | 'converged-apart-from-held-back' | 'nothing-to-prove' | 'not-proven' | 'diverged' + /** Why it could not prove, for `not-proven` and `nothing-to-prove`. */ + reason?: string + /** What still differs: held back, for the one verdict; unexplained, for `diverged`. */ + residual: string[] + /** Already different before, outside what this run compares. */ + outOfScope: string[] + /** Planned fixes the proof cannot speak for; they are applied all the same. */ + unproved: PlannedSql[] + /** Left out, as the apply will leave them: this server can't hold them. */ + notApplied: NonNullable + data?: ProofResult['data'] +} + +export async function proveBeforeApply(opts: { + scanResult: ScanResult + plan: PlanOptions + sourceUrl: string + targetUrl: string + ignoreSchemas?: readonly string[] +}): Promise { + const { scanResult, sourceUrl, targetUrl } = opts + const planned = planWork(scanResult, opts.plan) + const { replay, unproved } = proofScope(planned.sqlStatements) + + // Nothing left to replay means nothing to prove. Running it anyway compared + // the whole schema, and refused a cron-only apply over tables it was never + // going to touch. + const proof: ProofResult = replay.length === 0 + ? { converged: false, residual: [], cloneName: '', skipped: 'none of the planned fixes is one the proof can replay' } + : await proveConvergence({ + sourceUrl, + targetUrl, + migrationSql: replay.map(s => s.sql).join('\n'), + fixes: replay, + // Every schema the schema diff compares, not `public` alone: a migration + // dropping a schema the source lacks runs against it. + schemas: await comparedSchemas(sourceUrl, targetUrl, schemaDiffIgnores(opts.ignoreSchemas ?? DEFAULT_IGNORE_SCHEMAS)), + // The checks this run compares: without the schema check, what the + // target already lacks elsewhere is not this migration's to fix. + checks: scanResult.checks.map(c => c.check), + }) + + const base = { + outOfScope: proof.outOfScope ?? [], + unproved, + notApplied: proof.notApplied ?? [], + ...(proof.data && !proof.skipped ? { data: proof.data } : {}), + } + + if (proof.skipped && replay.length === 0) { + return { ...base, proceed: true, verdict: 'nothing-to-prove', reason: proof.skipped, residual: [] } + } + // Could not prove is not the same as failed to converge, but either way + // nothing is applied: a proof is asked for as a gate. + if (proof.skipped) return { ...base, proceed: false, verdict: 'not-proven', reason: proof.skipped, residual: [] } + if (proof.converged) return { ...base, proceed: true, verdict: 'converged', residual: [] } + + // What the replay left out because this server can't hold it, as the apply + // will: expected to stay different, so it counts as held back. + const heldBack = [...heldBackSql(scanResult, planned), ...base.notApplied.map(n => n.sql)] + const { unexplained } = residualHeldBack(proof.residual, heldBack) + return unexplained.length === 0 + ? { ...base, proceed: true, verdict: 'converged-apart-from-held-back', residual: proof.residual } + : { ...base, proceed: false, verdict: 'diverged', residual: unexplained } +} + +/** The SQL of the fixes this run plans to skip, which the proof did not replay. */ +function heldBackSql(scanResult: ScanResult, planned: { skipped: Array<{ issueId: string }> }): string[] { + const skipped = new Set(planned.skipped.map(s => s.issueId)) + return scanResult.checks.flatMap(c => c.issues) + .filter(i => skipped.has(i.id) && i.sql?.up) + .map(i => i.sql!.up) +} diff --git a/packages/cli/test/e2e/mcp-soak.test.ts b/packages/cli/test/e2e/mcp-soak.test.ts index 71915b6..e471921 100644 --- a/packages/cli/test/e2e/mcp-soak.test.ts +++ b/packages/cli/test/e2e/mcp-soak.test.ts @@ -91,7 +91,7 @@ describeE2E('e2e: the MCP server does not leak across many calls', () => { const peakConnections = { source: 0, target: 0 } for (let i = 1; i <= CALLS; i++) { - const res = await client.callTool({ name: 'scan_drift', arguments: {} }) + const res = await client.callTool({ name: 'diff', arguments: {} }) expect(res.isError, JSON.stringify(res.content).slice(0, 500)).toBeFalsy() if (i % 5 === 0) { await client.callTool({ name: 'get_check_result', arguments: { check: 'schema' } }) diff --git a/packages/cli/test/mcp/server-safety.test.ts b/packages/cli/test/mcp/server-safety.test.ts index 5709b22..00950cb 100644 --- a/packages/cli/test/mcp/server-safety.test.ts +++ b/packages/cli/test/mcp/server-safety.test.ts @@ -5,8 +5,8 @@ import { createServer } from '../../src/mcp/server.js' * What an MCP client can tell about these tools, and what it can reach (#96). * * Three things made it easy for an agent to write when it only meant to look: - * no tool carried annotations, so a client could not tell `scan_drift` from - * `apply_fixes` and could not confirm the right one; `apply_fixes` wrote unless + * no tool carried annotations, so a client could not tell the read-only scan from + * the tool that applies fixes and could not confirm the right one; that tool wrote unless * `dryRun: true` was passed, the opposite of everywhere else in supaforge; and * every tool accepted an arbitrary `configPath`, so a tool call — or a prompt * injected into one — could point the server at any config on disk, including @@ -22,7 +22,7 @@ import { createServer } from '../../src/mcp/server.js' */ function toolsOf(server: unknown): Record - inputSchema?: { parse: (value: unknown) => Record } + inputSchema?: { parse: (value: unknown) => Record; shape?: Record } handler: (args: unknown, extra: unknown) => Promise<{ isError?: boolean content: Array<{ text: string }> @@ -35,25 +35,25 @@ function toolsOf(server: unknown): Record { const tools = toolsOf(createServer('/tmp')) - it('registers the five documented tools', () => { + it('registers the documented tools', () => { expect(Object.keys(tools).sort()).toEqual([ - 'apply_fixes', 'create_migration', 'get_check_result', 'scan_drift', 'take_snapshot', + 'create_migration', 'diff', 'get_check_result', 'list_environments', 'sync', 'take_snapshot', ]) }) it('marks the read-only tools read-only', () => { - for (const name of ['scan_drift', 'get_check_result']) { + for (const name of ['diff', 'get_check_result', 'list_environments']) { expect(tools[name].annotations?.readOnlyHint, name).toBe(true) expect(tools[name].annotations?.destructiveHint, name).toBe(false) } }) - it('marks apply_fixes destructive, and nothing else', () => { + it('marks sync destructive, and nothing else', () => { // This is the one a client should ask about before running. - expect(tools.apply_fixes.annotations?.destructiveHint).toBe(true) - expect(tools.apply_fixes.annotations?.readOnlyHint).toBe(false) + expect(tools.sync.annotations?.destructiveHint).toBe(true) + expect(tools.sync.annotations?.readOnlyHint).toBe(false) - for (const name of ['scan_drift', 'get_check_result', 'take_snapshot', 'create_migration']) { + for (const name of ['diff', 'get_check_result', 'list_environments', 'take_snapshot', 'create_migration']) { expect(tools[name].annotations?.destructiveHint, name).toBe(false) } }) @@ -73,29 +73,33 @@ describe('MCP tool annotations', () => { }) }) -describe('MCP apply_fixes previews by default', () => { - it('defaults dryRun to true', () => { - // Writing is opt-in everywhere else in supaforge, where it needs --apply. - // Asserted by parsing an argument object with dryRun left out, which is - // exactly what the tool receives when a client does not pass it. - const tools = toolsOf(createServer('/tmp')) - const parsed = tools.apply_fixes.inputSchema?.parse({}) +describe('MCP planning and writing are separate tools', () => { + // A single tool with a "really write" argument has to be marked destructive, + // so a client asks before every plan too — or it is marked read-only and + // can write. Two tools let the annotations tell the truth about each. + const tools = toolsOf(createServer('/tmp')) - expect(parsed?.dryRun).toBe(true) + it('diff and sync take the same arguments, so the plan shown is the plan run', () => { + const args = (name: string) => Object.keys(tools[name].inputSchema?.shape ?? {}).sort() + expect(args('sync').filter(a => a !== 'prove')).toEqual(args('diff')) }) - it('still writes when a client asks for it', () => { - const tools = toolsOf(createServer('/tmp')) - const parsed = tools.apply_fixes.inputSchema?.parse({ dryRun: false }) + it('holds back drops, and does not prove, unless asked', () => { + // Parsed with the arguments left out, which is what the tool receives + // when a client does not pass them. + expect(tools.sync.inputSchema?.parse({})).toMatchObject({ allowDestructive: false, prove: false }) + expect(tools.diff.inputSchema?.parse({})).toMatchObject({ allowDestructive: false }) + }) - expect(parsed?.dryRun).toBe(false) + it('has no dry-run switch on sync: previewing is diff\'s job', () => { + expect(Object.keys(tools.sync.inputSchema?.shape ?? {})).not.toContain('dryRun') }) }) describe('MCP configPath', () => { /** Call a tool the way the transport would, and return its text. */ async function callScan(server: unknown, args: Record) { - return toolsOf(server).scan_drift.handler(args, {}) + return toolsOf(server).diff.handler(args, {}) } it('is refused by default, and says why', async () => { diff --git a/packages/cli/test/mcp/server.test.ts b/packages/cli/test/mcp/server.test.ts index e1207dd..6ffc801 100644 --- a/packages/cli/test/mcp/server.test.ts +++ b/packages/cli/test/mcp/server.test.ts @@ -139,8 +139,9 @@ describe('MCP tool registration', () => { it('lists all expected tools', async () => { const { tools } = await client.listTools() const names = tools.map(t => t.name) - expect(names).toContain('scan_drift') - expect(names).toContain('apply_fixes') + expect(names).toContain('diff') + expect(names).toContain('sync') + expect(names).toContain('list_environments') expect(names).toContain('take_snapshot') expect(names).toContain('create_migration') expect(names).toContain('get_check_result') @@ -165,10 +166,29 @@ describe('MCP tools', () => { await rm(tmpDir, { recursive: true, force: true }) }) - describe('scan_drift', () => { + describe('list_environments', () => { + it('names each environment and the defaults, and never the credentials', async () => { + await createTestConfig(tmpDir) + ;({ client, cleanup } = await makeClient(tmpDir)) + const result = await client.callTool({ name: 'list_environments', arguments: {} }) + expect(result.isError).toBeFalsy() + const text = (result.content as Array<{ text: string }>)[0].text + expect(JSON.parse(text)).toMatchObject({ + source: 'dev', + target: 'prod', + environments: [ + { name: 'dev', database: true, supabaseProject: false, accessToken: false }, + { name: 'prod', database: true, supabaseProject: false, accessToken: false }, + ], + }) + expect(text).not.toContain('postgresql://') + }) + }) + + describe('diff', () => { it('errors when config file is missing', async () => { ;({ client, cleanup } = await makeClient(tmpDir)) - const result = await client.callTool({ name: 'scan_drift', arguments: {} }) + const result = await client.callTool({ name: 'diff', arguments: {} }) expect(result.isError).toBe(true) const text = (result.content as Array<{ text: string }>)[0].text expect(text).toContain('Error') @@ -177,7 +197,7 @@ describe('MCP tools', () => { it('errors when config has no environments', async () => { await writeFile(join(tmpDir, 'supaforge.config.json'), JSON.stringify({ environments: {}, source: 'x', target: 'y' })) ;({ client, cleanup } = await makeClient(tmpDir)) - const result = await client.callTool({ name: 'scan_drift', arguments: {} }) + const result = await client.callTool({ name: 'diff', arguments: {} }) expect(result.isError).toBe(true) }) }) @@ -188,7 +208,7 @@ describe('MCP tools', () => { const result = await client.callTool({ name: 'get_check_result', arguments: { check: 'rls-coverage' } }) expect(result.isError).toBe(true) const text = (result.content as Array<{ text: string }>)[0].text - expect(text).toContain('scan_drift first') + expect(text).toContain('Run diff first') }) it('returns the check result when a scan exists', async () => { @@ -211,10 +231,10 @@ describe('MCP tools', () => { }) }) - describe('apply_fixes', () => { + describe('sync', () => { it('errors when config is missing', async () => { ;({ client, cleanup } = await makeClient(tmpDir)) - const result = await client.callTool({ name: 'apply_fixes', arguments: { dryRun: true } }) + const result = await client.callTool({ name: 'sync', arguments: {} }) expect(result.isError).toBe(true) }) @@ -230,10 +250,10 @@ describe('MCP tools', () => { target: 'prod', })) ;({ client, cleanup } = await makeClient(tmpDir)) - const result = await client.callTool({ name: 'apply_fixes', arguments: { dryRun: false, checks: ['webhooks'] } }) + const result = await client.callTool({ name: 'sync', arguments: { checks: ['webhooks'] } }) expect(result.isError).toBe(true) const text = (result.content as Array<{ text: string }>)[0].text - expect(text).toMatch(/Nothing compared or applied/) + expect(text).toMatch(/Nothing compared/) expect(text).toMatch(/Source \(dev\) not reachable/) expect(text).not.toContain('wrong') }, 60_000) @@ -259,7 +279,7 @@ describe('MCP tools', () => { describe('create_migration', () => { it('errors when config is missing', async () => { ;({ client, cleanup } = await makeClient(tmpDir)) - const result = await client.callTool({ name: 'create_migration', arguments: {} }) + const result = await client.callTool({ name: 'create_migration', arguments: { name: 'x' } }) expect(result.isError).toBe(true) }) }) @@ -315,7 +335,7 @@ describe('MCP resources', () => { const result = await client.readResource({ uri: 'supaforge://last-scan' }) const text = resourceText(result) const parsed = JSON.parse(text) - expect(parsed.message).toContain('scan_drift first') + expect(parsed.message).toContain('Run diff first') }) it('last-scan resource returns scan result after setLastScanResult', async () => { @@ -376,11 +396,11 @@ describe('MCP prompts', () => { expect(names).toContain('fix_critical_issues') }) - it('review_drift_before_deploy prompt mentions scan_drift and apply_fixes', async () => { + it('review_drift_before_deploy prompt mentions diff and sync', async () => { const result = await client.getPrompt({ name: 'review_drift_before_deploy', arguments: {} }) const text = result.messages[0].content.type === 'text' ? result.messages[0].content.text : '' - expect(text).toContain('scan_drift') - expect(text).toContain('apply_fixes') + expect(text).toContain('Call diff') + expect(text).toContain('run sync') }) it('review_drift_before_deploy prompt includes environment name when provided', async () => { diff --git a/packages/mcp/test/server.test.ts b/packages/mcp/test/server.test.ts index 8dd76a4..d0e1327 100644 --- a/packages/mcp/test/server.test.ts +++ b/packages/mcp/test/server.test.ts @@ -120,10 +120,10 @@ describe('MCP tools', () => { await rm(tmpDir, { recursive: true, force: true }) }) - describe('scan_drift', () => { + describe('diff', () => { it('errors when config file is missing', async () => { ;({ client, cleanup } = await makeClient(tmpDir)) - const result = await client.callTool({ name: 'scan_drift', arguments: {} }) + const result = await client.callTool({ name: 'diff', arguments: {} }) expect(result.isError).toBe(true) const text = (result.content as Array<{ text: string }>)[0].text expect(text).toContain('Error') @@ -132,7 +132,7 @@ describe('MCP tools', () => { it('errors when config has no environments', async () => { await writeFile(join(tmpDir, 'supaforge.config.json'), JSON.stringify({ environments: {}, source: 'x', target: 'y' })) ;({ client, cleanup } = await makeClient(tmpDir)) - const result = await client.callTool({ name: 'scan_drift', arguments: {} }) + const result = await client.callTool({ name: 'diff', arguments: {} }) expect(result.isError).toBe(true) }) }) @@ -143,7 +143,7 @@ describe('MCP tools', () => { const result = await client.callTool({ name: 'get_check_result', arguments: { check: 'rls-coverage' } }) expect(result.isError).toBe(true) const text = (result.content as Array<{ text: string }>)[0].text - expect(text).toContain('scan_drift first') + expect(text).toContain('Run diff first') }) it('returns the check result when a scan exists', async () => { @@ -167,10 +167,10 @@ describe('MCP tools', () => { }) }) - describe('apply_fixes (dry-run path)', () => { + describe('sync', () => { it('errors when config is missing', async () => { ;({ client, cleanup } = await makeClient(tmpDir)) - const result = await client.callTool({ name: 'apply_fixes', arguments: { dryRun: true } }) + const result = await client.callTool({ name: 'sync', arguments: {} }) expect(result.isError).toBe(true) }) }) @@ -252,7 +252,7 @@ describe('MCP resources', () => { const result = await client.readResource({ uri: 'supaforge://last-scan' }) const text = result.contents[0].text as string const parsed = JSON.parse(text) - expect(parsed.message).toContain('scan_drift first') + expect(parsed.message).toContain('Run diff first') }) it('last-scan resource returns scan result after setLastScanResult', async () => { @@ -313,11 +313,11 @@ describe('MCP prompts', () => { expect(names).toContain('fix_critical_issues') }) - it('review_drift_before_deploy prompt mentions scan_drift', async () => { + it('review_drift_before_deploy prompt mentions diff and sync', async () => { const result = await client.getPrompt({ name: 'review_drift_before_deploy', arguments: {} }) const text = result.messages[0].content.type === 'text' ? result.messages[0].content.text : '' - expect(text).toContain('scan_drift') - expect(text).toContain('apply_fixes') + expect(text).toContain('Call diff') + expect(text).toContain('run sync') }) it('review_drift_before_deploy prompt includes environment name when provided', async () => { @@ -374,8 +374,9 @@ describe('MCP tool registration', () => { it('lists all expected tools', async () => { const { tools } = await client.listTools() const names = tools.map(t => t.name) - expect(names).toContain('scan_drift') - expect(names).toContain('apply_fixes') + expect(names).toContain('diff') + expect(names).toContain('sync') + expect(names).toContain('list_environments') expect(names).toContain('take_snapshot') expect(names).toContain('create_migration') expect(names).toContain('get_check_result')