feat: preflight migration review / DDL risk analyzer (#100)

Deterministic DDL risk analyzer (MCP + API) with fail-closed parsing and measured Postgres lock rules.

Author: notSumit25

CLAUDE.md

@@ -412,6 +412,84 @@ than an `assertCan*` call — the case `Endpoint Authorization Rules` describes
 - Usage belongs to `QueryActorContextHolder` first and the security principal second, so
   under **View as** the spend is attributed to the target user, not the admin.
 
+## Pre-flight Migration Review
+
+`POST /migrations/analyze` (`MigrationRiskController` → `MigrationRiskService` →
+`DatabaseDialect.migrationRisk()`) classifies a DDL statement's blast radius —
+verdict, locks (per table), whether it rewrites the table, a coarse duration bucket,
+and a safer alternative — before anyone runs it. It is **deterministic by
+convention**, same as `IndexAdvisorService` and `ExplainPlanService`: the rule table
+and the target table's real size (`pg_class.reltuples` / `pg_total_relation_size`)
+decide the verdict; nothing here asks an LLM to judge risk. An LLM may narrate the
+report in chat, but it never computes it.
+
+- **The Postgres rule table is engine-verified, not hand-derived.** A Testcontainers
+  suite runs each rule's DDL against a real `postgres:18`, reading
+  `pg_relation_filenode` before/after to detect an actual rewrite and `pg_locks` to
+  read the actual lock mode taken — not PostgreSQL documentation summarized from
+  memory. If a rule disagrees with what the engine measured, the rule is wrong,
+  full stop.
+- **`DEFAULT now()` does NOT force a table rewrite — this is measured, not a bug.**
+  `now()` is STABLE (it returns the same value for the whole transaction), and
+  Postgres 11+ adds a column with a STABLE or constant default as a metadata-only
+  operation. Only a VOLATILE default (`random()`, `gen_random_uuid()`,
+  `clock_timestamp()`, `uuid_generate_v4()`) forces a full rewrite, because a
+  volatile function must be evaluated per row. Wrote this down after getting it
+  wrong from memory twice before the Testcontainers run corrected it.
+- **`ADD FOREIGN KEY` takes `ShareRowExclusiveLock` on the referenced table too**,
+  not just the table being altered — confirmed live: `ALTER TABLE child ADD
+  CONSTRAINT fk FOREIGN KEY (t_id) REFERENCES t(id)` returns a `locks` array with
+  two entries, `child` and `t`. This is why `MigrationRiskReport.locks` is a
+  per-table array rather than a single lock on the altered table — a statement can
+  block writes on a table it never names, and that is the finding an operator is
+  least likely to expect from reading the SQL alone.
+- **The JSqlParser 5.2 shim exists because the library cannot parse the forms this
+  tool exists to recommend.** JSqlParser has no grammar for `NOT VALID` or `CREATE
+  INDEX CONCURRENTLY` — both fail to parse outright, which would make the analyzer
+  unable to evaluate the very migration pattern (`ADD CONSTRAINT ... NOT VALID` +
+  `VALIDATE CONSTRAINT`, `CREATE INDEX CONCURRENTLY`) it recommends as the safer
+  alternative. `DdlStatementParser` pre-strips both into flags (`notValid`,
+  `concurrently`) before handing the rest to JSqlParser. Do not remove this step —
+  removing it silently turns every NOT VALID / CONCURRENTLY statement into a parse
+  failure, which fails closed (UNKNOWN) but defeats the point of the feature for
+  exactly the statements it is meant to bless as safe.
+- **Fail-closed, not best-effort.** Unparseable SQL, an ALTER with more than one
+  clause (a single `DdlFacts` cannot honestly represent two clauses' worth of risk),
+  an unrecognised default function DeepSQL cannot confirm the volatility of, and
+  MySQL (no verified rule table exists yet — `MySQLMigrationRiskProvider` always
+  returns `UNKNOWN`) all report `UNKNOWN` or `CAUTION` rather than guessing `SAFE`.
+  Verified live: `{"sql":"not sql"}` returns `verdict: UNKNOWN`, `safeToRun: false`.
+- **Known limitation: `ALTER COLUMN TYPE` over-warns.** `DdlFacts` carries no old
+  column type, only the new one, so the provider cannot tell a same-family widening
+  (`varchar(50)` → `varchar(100)`, which Postgres does NOT rewrite) from a genuine
+  type change (which does). It reports the conservative rewrite verdict for both
+  rather than risk a false SAFE.
+- **The verification test ranks lock strength by an explicit ordering list, not
+  `max(mode)` — the provider itself does not rank at all.**
+  `PostgresMigrationRiskProvider` never computes a lock mode; each rule hardcodes
+  the one it asserts (e.g. `addForeignKey` always reports `ShareRowExclusiveLock`).
+  The ranking lives only in `PostgresMigrationRiskVerificationTest`
+  (`LOCK_STRENGTH` + `strongestLock()`), which has to pick the strongest lock out
+  of however many rows a query against the real `pg_locks` returns. It needs the
+  ordering because Postgres lock mode names sort alphabetically in a way that has
+  nothing to do with strength — `"ShareLock".compareTo("AccessExclusiveLock") > 0`,
+  so a naive string-max would call `ShareLock` the stronger of the two, when
+  `AccessExclusiveLock` is in fact the most exclusive mode Postgres has. Any future
+  code that needs "the worst lock a query is holding" from `pg_locks` must rank
+  against Postgres's real lock hierarchy, not compare mode names — but that need
+  has not yet reached production code, only this test.
+- **Authorization is asserted in the service, before parsing, credential
+  decryption, or session opening** (`MigrationRiskService.analyze` calls
+  `accessControlService.assertCanReadConnectionContent` first). The controller is
+  exempted from the static `ConnectionScopedAuthorizationSafetyTest` sweep
+  (`AUTHORIZED_ELSEWHERE`) on that basis, same reasoning as
+  `DashboardWorkspaceController`. Verified live, not just by the mocked unit test
+  and the static scanner: a second user with zero grants on the target connection
+  (confirmed empty in `connection_access_grant`) received a genuine `403` from
+  `POST /migrations/analyze`, with no stack trace in the logs — proof the
+  controller's `catch (ResponseStatusException e) { throw e; }` before the
+  catch-all is live and not swallowing the 403 into a 500.
+
 ## Key Rules & Patterns
 
 ### Backend Rules

backend/pom.xml

@@ -356,6 +356,18 @@
             <version>4.2.2</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.testcontainers</groupId>
+            <artifactId>postgresql</artifactId>
+            <version>1.20.4</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.testcontainers</groupId>
+            <artifactId>junit-jupiter</artifactId>
+            <version>1.20.4</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

backend/src/main/java/com/dbaagent/controller/MigrationRiskController.java

@@ -0,0 +1,39 @@
+package com.dbaagent.controller;
+
+import com.dbaagent.dto.MigrationRiskReport;
+import com.dbaagent.service.migration.MigrationRiskService;
+import lombok.RequiredArgsConstructor;
+import org.springframework.http.ResponseEntity;
+import org.springframework.web.bind.annotation.*;
+import org.springframework.web.server.ResponseStatusException;
+
+import java.util.Map;
+
+@RestController
+@RequestMapping("/migrations")
+@RequiredArgsConstructor
+public class MigrationRiskController {
+
+    private final MigrationRiskService migrationRiskService;
+
+    @PostMapping("/analyze")
+    public ResponseEntity<?> analyze(@RequestBody Map<String, String> body) {
+        String connectionId = body.get("connectionId");
+        String sql = body.get("sql");
+        if (connectionId == null || connectionId.isBlank() || sql == null || sql.isBlank()) {
+            return ResponseEntity.badRequest().body(Map.of("message", "connectionId and sql are required"));
+        }
+        try {
+            // MigrationRiskService.analyze asserts assertCanReadConnectionContent before
+            // parsing, credential decryption or session opening — see AUTHORIZED_ELSEWHERE
+            // in ConnectionScopedAuthorizationSafetyTest.
+            MigrationRiskReport report = migrationRiskService.analyze(connectionId, sql);
+            return ResponseEntity.ok(report);
+        } catch (ResponseStatusException e) {
+            throw e;   // preserve 403/404 — a catch-all below would report it as a 500
+        } catch (Exception e) {
+            return ResponseEntity.internalServerError()
+                    .body(Map.of("message", "Migration analysis failed: " + e.getMessage()));
+        }
+    }
+}

backend/src/main/java/com/dbaagent/dto/MigrationRiskReport.java

@@ -0,0 +1,23 @@
+package com.dbaagent.dto;
+
+import java.util.List;
+
+public record MigrationRiskReport(
+        String dialect,
+        String verdict,            // SAFE | CAUTION | DANGER | FAILS | UNKNOWN
+        boolean safeToRun,
+        boolean dialectSupported,
+        String operation,
+        String table,
+        List<LockRef> locks,       // per table — a statement can lock tables it does not name
+        boolean rewritesTable,
+        long tableRows,
+        long tableSizeBytes,
+        String estimatedDuration,  // coarse bucket, never a number
+        String reason,
+        String saferAlternative,
+        String docsUrl,
+        String confidence) {
+
+    public record LockRef(String table, String mode, List<String> blocks) {}
+}

backend/src/main/java/com/dbaagent/dto/TableFacts.java

@@ -0,0 +1,3 @@
+package com.dbaagent.dto;
+
+public record TableFacts(long rowEstimate, long sizeBytes, boolean empty) {}

backend/src/main/java/com/dbaagent/provider/api/DatabaseDialect.java

@@ -93,4 +93,10 @@ public interface DatabaseDialect {
      * @return The sampling provider
      */
     SamplingProvider sampling();
+
+    /**
+     * Get the migration risk provider for this database.
+     * @return The migration risk provider
+     */
+    MigrationRiskProvider migrationRisk();
 }

backend/src/main/java/com/dbaagent/provider/api/MigrationRiskProvider.java

@@ -0,0 +1,9 @@
+package com.dbaagent.provider.api;
+
+import com.dbaagent.dto.MigrationRiskReport;
+import com.dbaagent.dto.TableFacts;
+import com.dbaagent.service.migration.DdlFacts;
+
+public interface MigrationRiskProvider {
+    MigrationRiskReport classify(DdlFacts facts, TableFacts table);
+}

backend/src/main/java/com/dbaagent/provider/mysql/MySQLDialect.java

@@ -28,6 +28,7 @@ public class MySQLDialect implements DatabaseDialect {
     private final MySQLQueryExecutionProvider queryExecutionProvider;
     private final MySQLPrivilegeCheckProvider privilegeCheckProvider;
     private final MySQLSamplingProvider samplingProvider;
+    private final MySQLMigrationRiskProvider migrationRiskProvider;
 
     @Override
     public String getCanonicalName() {
@@ -93,4 +94,9 @@ public PrivilegeCheckProvider privileges() {
     public SamplingProvider sampling() {
         return samplingProvider;
     }
+
+    @Override
+    public MigrationRiskProvider migrationRisk() {
+        return migrationRiskProvider;
+    }
 }

backend/src/main/java/com/dbaagent/provider/mysql/MySQLMigrationRiskProvider.java

@@ -0,0 +1,28 @@
+package com.dbaagent.provider.mysql;
+
+import com.dbaagent.dto.MigrationRiskReport;
+import com.dbaagent.dto.TableFacts;
+import com.dbaagent.provider.api.MigrationRiskProvider;
+import com.dbaagent.service.migration.DdlFacts;
+import org.springframework.stereotype.Component;
+
+import java.util.List;
+
+/**
+ * MySQL migration risk is not implemented. Returning UNKNOWN is deliberate: MySQL's
+ * online-DDL matrix varies by version, storage engine and ALGORITHM/LOCK clause, and a
+ * confident wrong answer about a lock is worse than no answer.
+ */
+@Component
+public class MySQLMigrationRiskProvider implements MigrationRiskProvider {
+
+    @Override
+    public MigrationRiskReport classify(DdlFacts facts, TableFacts table) {
+        return new MigrationRiskReport("mysql", "UNKNOWN", false, false,
+                facts.operation().name(), facts.table(), List.of(), false,
+                table.rowEstimate(), table.sizeBytes(), "unknown",
+                "DeepSQL does not yet have verified MySQL DDL rules. Review this by hand.",
+                null, "https://dev.mysql.com/doc/refman/8.0/en/innodb-online-ddl-operations.html",
+                "unverified");
+    }
+}

backend/src/main/java/com/dbaagent/provider/postgres/PostgresDialect.java

@@ -28,6 +28,7 @@ public class PostgresDialect implements DatabaseDialect {
     private final PostgresQueryExecutionProvider queryExecutionProvider;
     private final PostgresPrivilegeCheckProvider privilegeCheckProvider;
     private final PostgresSamplingProvider samplingProvider;
+    private final PostgresMigrationRiskProvider migrationRiskProvider;
 
     @Override
     public String getCanonicalName() {
@@ -93,4 +94,9 @@ public PrivilegeCheckProvider privileges() {
     public SamplingProvider sampling() {
         return samplingProvider;
     }
+
+    @Override
+    public MigrationRiskProvider migrationRisk() {
+        return migrationRiskProvider;
+    }
 }