Feat/preflight migration review - #100
Open
notSumit25 wants to merge 17 commits into
Open
Conversation
Deterministic DDL risk analyzer exposed as an MCP tool, following the codebase convention that the database decides verdicts and the LLM narrates. Rule table measured against a live Postgres 18.4 via pg_locks and pg_relation_filenode rather than recalled — which corrected the assumption that DEFAULT now() forces a rewrite (now() is STABLE, not VOLATILE). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ADD FOREIGN KEY takes ShareRowExclusiveLock on the referenced table as well as the altered one — a statement can block writes on a table it does not name. Output shape now reports locks per table rather than as a single field. NOT VALID surfaced as the safer path for both constraint types: same lock, skips the validating scan, with VALIDATE CONSTRAINT running under ShareUpdateExclusiveLock afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six TDD tasks. Records the JSqlParser 5.2 finding that NOT VALID and CREATE INDEX CONCURRENTLY fail to parse — the safe forms this tool recommends — and the verified normalization shim that fixes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…isclassification
- fromAlter refuses (Optional.empty()) when an ALTER TABLE has more than one
clause, since a single DdlFacts cannot honestly represent two clauses and
judging on the first alone let a table-rewriting clause hide behind a
harmless one.
- ADD_CHECK is now detected structurally via AlterExpression.getIndex() != null
instead of a rawSql.contains("CHECK") substring match, which misclassified
ordinary columns like check_flag as ADD_CHECK.
- Added tests locking in that referencedTable is never populated outside the
ADD_FOREIGN_KEY branch, plus DROP_COLUMN/RENAME_COLUMN coverage.
…fault functions - addColumn now reports CAUTION (not SAFE) when a default function is neither a known-volatile nor known-stable function, since volatility cannot be verified without a live pg_proc lookup - add STABLE_FALLBACK set and isStable helper - reword alterType's reason text to not claim a check the code can't perform - add tests for the new CAUTION branch, a constant-default SAFE case, and the five previously untested branches (RENAME_COLUMN, ALTER_COLUMN_TYPE, SET_NOT_NULL at both size thresholds, ADD_CHECK both notValid states, VALIDATE_CONSTRAINT)
Testcontainers-driven verification of PostgresMigrationRiskProvider against postgres:18: rewrite behavior via pg_relation_filenode(), FK lock strength via pg_locks, NOT NULL-without-default failure, and function volatility via pg_proc. jtest.sh gains a Docker socket mount + TESTCONTAINERS_HOST_OVERRIDE so the containerized Maven run can start a sibling Postgres container, and drops -o since the new testcontainers deps were not yet cached locally. Also documents (via a new test, not a rule-table change) a known limitation: DdlFacts carries only the new column type, never the old one, so alterType cannot tell a same-family widening ALTER (e.g. varchar(50)->varchar(100), which does not rewrite) apart from a genuine rewrite, and conservatively reports rewritesTable=true for both. Confirmed against real Postgres 18; left unfixed since a correct fix needs schema-aware facts, out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…legate via safety-test exemption MigrationRiskService.analyze already asserts assertCanReadConnectionContent before any work. Duplicating it in the controller was the same mechanism with the same failure mode, not defence-in-depth. Registers the controller in AUTHORIZED_ELSEWHERE and the service in DELEGATED_CHECKS so everyDelegatedCheckStillExists fails the build if the service-layer assert is ever removed.
Exposes Task 4's POST /migrations/analyze as an MCP tool (analyze_migration) and a `deepsql migration analyze` CLI command, per the MCP & CLI Release Rules: tool definition + handler + summarizer + dispatch case, CLI dispatcher + help text + drift-guard registration, SKILL_BODY.md/CLAUDE.md/README.md tool tables and counts, and a minor version bump (0.28.0 -> 0.29.0). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ring Fixes four review findings, all on failure paths: the Long.MAX_VALUE tableRows sentinel no longer renders as a huge fabricated number (both surfaces now say "table size unknown - treated as large"); the CLI's UNKNOWN path no longer prints "null" for an unparseable statement, now mirroring the MCP summarizer's existing handling; DANGER/FAILS verdicts get a plain-text "X " marker and CAUTION a "! " marker, following growth.js's existing severity-marker convention; and migration.js now wraps ApiError into friendly 403/404 messages like every sibling command. Also fixes a separate bug found while adding tests: buildOpts() in cli.js never wired --sql into opts at all, so the command was non-functional from the real CLI entry point regardless of the above. Adds unit tests for analyze_migration on both surfaces, covering the multi-lock FK case, the null-table UNKNOWN case, and the row-count sentinel (exact and JS-float-imprecise values). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stand Three classification bugs in DdlStatementParser shared one root cause: branches assumed rather than verified what the parse tree contained. - Multi-statement SQL: parse() kept only the first statement and silently discarded the rest, so a hidden DROP TABLE riding after a harmless ALTER reported SAFE while Postgres executed both. Switch to parseStatements() and require exactly one statement, matching the existing multi-clause guard. - DROP CONSTRAINT shared operation=DROP with DROP COLUMN and was reported as DROP_COLUMN (wrong operation, false "VACUUM reclaims it" reason, and locks missing the referenced table). Gate DROP_COLUMN on getColumnName() actually being set; anything else falls through to UNKNOWN. - ALTER COLUMN ... DROP NOT NULL fell into the same weak ALTER branch as a real type change and was reported as ALTER_COLUMN_TYPE with rewritesTable= true, though it is metadata-only. No existing rule models it correctly (SET_NOT_NULL's rule is specific to adding the constraint), so it now returns UNKNOWN rather than a confident wrong answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every ruling made during subagent-driven execution, the deferred minors, and the final whole-branch review verdict. Kept in-repo because the rulings explain why several rules deliberately fail closed rather than answer, which is otherwise invisible from the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
notSumit25
requested review from
a team,
geekypunk and
venkateshsakamuri-lab
as code owners
September 5, 2026 07:22
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged NOT_VALID's regex (\s+NOT\s+VALID\s*;?\s*$ via find()) as polynomial: the leading \s+ retries the match at every whitespace position in the input, so 80KB of padding stalled a request thread for 35s. sql arrives from POST /migrations/analyze, so it is attacker-controlled. The NOT VALID clause can only ever be a bounded trailing tail, so replace the regex with an index-based scan (notValidStart) that walks backward from the end of the string instead of re-scanning the attacker-controlled prefix. Verified equivalent to the old regex's behavior on all prior cases (case insensitivity, multiple internal spaces, trailing semicolons, and the existing negative cases including the trailing-comment non-match). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-flight Migration Review (DDL safety)
Deterministic Postgres DDL risk analyzer, exposed as one MCP tool
(
analyze_migration) and adeepsql migration analyzeCLI command.Answers "is this ALTER safe to run?" before it runs — no new UI.
Why deterministic
Follows the convention already in this codebase: the database (or arithmetic
over its catalog) decides the verdict, the LLM narrates. Same split as
IndexAdvisorService(hypopg + pg_class) andExplainPlanService(Postgresproduces the plan, the model interprets it).
Verified in practice: asked the same question, DeepSQL Agent answered from
recall that
DEFAULT gen_random_uuid()"does not rewrite the whole table."It does. That's the failure this replaces.
Verification
Rules are proven against a real PostgreSQL 18 via Testcontainers —
pg_relation_filenode()for rewrites,pg_locksfor lock modes. A rule thatdisagrees with the engine means the rule is wrong. Confirmed non-vacuous: a
constant-returning
classify()fails 4-5 of 9 cases.Live-verified over HTTP: SAFE/DANGER verdicts, FK locking both tables,
fail-closed on garbage, and a real 403 for an unauthorized user.
Findings worth knowing
now()is STABLE, soDEFAULT now()does not rewrite. Volatiledefaults (
random(),gen_random_uuid(),clock_timestamp()) do.ADD FOREIGN KEYtakesShareRowExclusiveLockon the referenced tabletoo — hence
locksis a per-table array.NOT VALIDorCREATE INDEX CONCURRENTLY—the very forms this tool recommends. A pre-parse shim strips them into flags.
Fail closed
Unparseable SQL, multi-statement SQL, multi-clause ALTERs, unrecognised default
functions,
DROP CONSTRAINT, and MySQL all yield UNKNOWN/CAUTION — never SAFE.Known limits
ALTER COLUMN TYPEover-warns: same-family widening(
varchar(50)→varchar(100)) is reported as a rewrite when it isn't.DdlFactscarries no old type. Documented and covered by a test that assertsboth engine truth and the rule's claim.
pg_proc.provolatilelookup isfollow-up work. Unrecognised functions yield CAUTION, so it fails safe.
Tests
55 Java (incl. Testcontainers engine verification), 291 JS,
ConnectionScopedAuthorizationSafetyTestgreen.Every ruling made during implementation is recorded in
docs/superpowers/specs/2026-09-04-preflight-migration-review-decisions.md.New