fix: scope reverts to the ticked objects, and emit DDL that parses - #263
Merged
Conversation
Writing an e2e test for the revert flow proved it had never once landed. The
plan looked correct and the driver rejected it:
500 POST /lokee/databases/…/revert {"error":"near \".\": syntax error"}
CREATE INDEX IDX_CUSTOMERS_EMAIL ON main.customers (email);
Two defects in one statement.
**SQLite cannot take a qualified table in CREATE INDEX.** The schema belongs on
the index name — `CREATE INDEX main.idx ON customers(email)` — while every other
dialect qualifies the table, which is why the shared generator does. SQLite gets
a `createIndexStatement` hook that moves the qualifier across.
**The generator used compare's match key as an identifier.** CLAUDE.md opens with
"The compare key is not an identifier", and this spread the source IndexInfo then
overwrote `name` with the uppercased key, emitting IDX_CUSTOMERS_EMAIL for
idx_customers_email. SQLite folds case so it survived there; on a case-sensitive
target it creates a differently named index and the next compare reads a rename
that never happened. The cause was a type: `IndexDiff.source` omitted `name`, so
the generator could not reach the identifier compare had been passing all along.
Widened it — optional, not required, because this package is published and a
required field would break consumers constructing these objects.
Both are pinned by unit tests, since the e2e suite is not in the CI gate.
Alongside the fix, the History toolbar work this proved out:
- The version pickers and the capture credential were two connection-shaped
controls on two rows, reading as "which of these databases am I looking at?".
They are one database — recorded and live — so capture moved onto the pickers'
row and defaults to the saved connection matching the history database.
- The graph no longer follows the pickers. Choosing Version 1 as Original used to
hide every version between the sides, so the history overview changed as a side
effect of choosing what to compare. The checkboxes are the only filter now.
- Execute was dead on the Blueprint tab because the data-loss acknowledgement
lives on Migration SQL. A risk chip now rides beside the button, and a blocked
button carries the reader to the decision instead of greying out.
HistoryCompareBar renders in TopToolbar while the fetching lives in
LokeeWeaveView, so it asks for work by bumping a counter in the store. Both
watchers compare against the value seen at mount: the store outlives the
component, and replaying the last request would re-snapshot the database on every
visit.
Also repairs the existing e2e suite, which CI does not run and which the shared
SchemaBlueprint change had broken: the summary is `N versions` rather than
`Total Versions: N`, and the inspector's sections carry `blueprint-*` ids — with
indexes now rendering where the old panel stored but never showed them.
Verified: revert e2e 3/3 (reads the SQLite file, not the UI), history e2e 6/6,
1613 unit tests, tsc clean, eslint 0 errors.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Left behind when the capture test moved from clicking a button to bumping the store counter — the button it used to click now lives in HistoryCompareBar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…port Browse was a mode hiding inside Compare, reachable only from a button on one of Compare's two connection cards. It is its own pane now — Compare | Browse | History — with its own one-connection bar, type filters beside the tree they filter, and a card naming the database being read. Compare keeps its own bar untouched. A revert recorded `source: 'revert'` and nothing else, so history could say an undo happened but never which version was restored. Migration 15 adds `revert_from_version_id` / `revert_to_version_id`; the graph node now reads "↩ reverted to v1". The compare dialog gained a Markdown change report — deliberately no SQL, for the reviewer or the ticket rather than the person running the migration — and its Execute button now names its own blocker instead of showing a dead "(0)". Also: the History Compare button moved into the Target card (the pair is finished being chosen there), the minimap is themed so it stops rendering as a grey slab over a light canvas, and the deploy row's chips no longer wrap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…revert-report # Conflicts: # apps/e2e/src/pages/LokeeHistoryPage.ts # apps/e2e/src/tests/schema-revert.test.ts # apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx # apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx # apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx # apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx # apps/web/src/frontend/store/lokeeHistoryStore.ts
Two ways a revert could rewrite a database nobody asked it to touch. Executing with **zero** objects ticked sent `objectKeys: undefined`, which the backend reads as "the whole schema" — one click reverted an entire database from a dialog where nothing was selected. An empty tick set is now refused, and `objectKeys` is always sent explicitly. Since that made a whole-schema revert unreachable by accident, Select all / Clear plus an `N of M ticked` counter keep the destructive path available but deliberate. Worse, `planRevert` filtered the *risk verdicts* by the ticked keys while handing the **unfiltered** state maps to the compare that generates the SQL. The dialog said "1 object" and the migration rewrote every table. Both maps are narrowed now. Ticking a lone child also carries its `table:` container along as context — `hydrateTableSchemas` drops any group without one, so the plan came back empty — but only when that container exists on both sides, so it can never turn a one-column tick into a DROP TABLE. The existing tests asserted only on `reversal.verdicts`, which is exactly how the statement generator drifted unnoticed; the new ones assert on `statements`. Also fixes a stale `blocked` memo (missing `selectedKeys`/`changed` deps left the button saying "Tick objects to revert" after you had ticked one), and hides the Compare button rather than disabling it when both sides resolve to one version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ntains Found by two new harnesses rather than by hand: a seeded generator that builds adversarial schemas and checks properties across all 14 dialects at once, and a test that hands the generated DDL to a real `node:sqlite` engine and lets it judge. String assertions only ever prove the output matches what somebody expected it to be. * **Identifiers were never quoted.** A table called `Order Details` — Northwind ships one — produced unparseable SQL on every dialect, as did a column called `order id` or `select`. Adds an optional `quoteIdentifier` dialect hook (ANSI default; backticks for MySQL/MariaDB/TiDB, brackets for SQL Server/Azure), applied only when a name cannot be written bare, so every ordinary name is byte-identical to what this generator emitted before. `ident` is idempotent, which lets ADD COLUMN and CREATE INDEX be fixed for all 14 at one call site instead of in fourteen hooks. * **`ColumnDiff.name` is the uppercased compare key**, the same trap as `tableName`, and the ALTER paths emitted it — renaming a user's `new col` to `NEW COL`. Now uses `source?.name ?? target?.name`; the field is optional because this package is published. * **`ALTER TABLE … ADD CONSTRAINT` was emitted for SQLite and ClickHouse**, which reject it outright. `dialectSupportsFk` already knew this and only the blueprint UI was reading it. FKs now inline into CREATE TABLE where the dialect allows it, and otherwise emit `-- review:` — never DDL that cannot run. * **Decimal precision was dropped.** `NUMBER(10)` / `DECIMAL(10)` tokenize their single argument as a *length*, which `shapeCanonical` ignored for decimals, so an Oracle `NUMBER(10)` column silently widened to full 38-digit precision on every migration. Fixed in the shared shaper, not per dialect. * **Redshift rendered `varchar(max)`** for TEXT and XML. That is T-SQL syntax Redshift rejects; its documented maximum is `varchar(65535)`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`referencedTableNames` promises the *physical* tables a statement touches, and the multi-table write warning is built on it. It got both directions wrong. `FROM orders JOIN customers` reported only `orders`: the optional-alias group matched `JOIN` as the alias of `orders` and moved `lastIndex` past it, so `customers` was never scanned. A write across two tables looked like a write across one — the warning under-reported, which is the fail-open direction. The opposite error too: `WITH recent AS (…) SELECT * FROM recent` counted `recent`, a name that exists only inside the query, so the warning counted objects that do not exist. CTE names are now excluded — at this caller only, since autocomplete legitimately wants them. Adds 44 adversarial CTE/subquery cases against the safety gates: data-modifying CTEs (`WITH x AS (DELETE …) SELECT 1` leads with the word WITH), nested CTEs, `EXPLAIN ANALYZE`, and write verbs hidden inside string literals and comments. They encode the rule that a misread must fail closed — calling a read a write costs one dialog; calling a write a read runs unreviewed DDL. The existing gates passed all of them unchanged. The first version of the CTE-name scan was a regex with adjacent optional whitespace groups, which eslint's security plugin correctly flagged as ReDoS-prone — reachable from the editor, where the input is whatever the user typed. Replaced with a single-pass scanner, pinned by a timing test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The minimap rendered as an empty white box over a canvas full of nodes. React Flow measures the DOM for the canvas itself, but it only writes those measurements back to the caller's node objects through `onNodesChange` — and this graph is fully controlled without one. So from the minimap's side every node reported undefined dimensions and `MiniMap` skipped all of them (`nodeHasDimensions(userNode)`), while the canvas rendered perfectly from internal state. Measured before the fix: 14 canvas nodes, 0 minimap nodes, and a minimap SVG holding nothing but its mask path. Declaring `initialWidth`/`initialHeight` satisfies the check without pinning the rendered size, so nodes still grow to fit their content — and unlike adding `onNodesChange`, it introduces no state that could re-render in a loop. Verified in the browser: 14 of 14. Separately, this file held four literal NUL bytes as composite-key separators, which made git treat it as binary and every diff of it opaque. Written as `\x00` escapes they are the same string and the file is text again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ec66fe8a-aa31-4384-8e47-8c1cb307ccae) |
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
1 participant
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.
What this is
A Browse pane and a no-SQL change report, plus the bugs that turned up once
generated schemas and a real SQL engine were pointed at the migration path.
Several are the kind that quietly write to the wrong place.
The two that matter most
A selective revert rewrote the whole schema.
planRevertfiltered the riskverdicts by the ticked objects but handed the unfiltered state maps to the
compare that generates the SQL. The dialog said "1 object" and the migration
touched every table. Separately, executing with nothing ticked sent
objectKeys: undefined, which the backend reads as "the whole schema" — oneclick reverted an entire database from a dialog where nothing was selected.
The existing tests asserted only on
reversal.verdicts, which is precisely howthe statement generator drifted unnoticed. The new ones assert on
statements.Identifiers were never quoted. A table called
Order Details— Northwindships one — emitted unparseable SQL on all 14 dialects, as did a column called
order idorselect.Everything else fixed here
ColumnDiff.nameis the uppercased compare key, and the ALTER paths emitted it — renaming a user'snew coltoNEW COLALTER TABLE … ADD CONSTRAINTemitted for engines that reject it, whiledialectSupportsFkalready knew betterNUMBER(10)/DECIMAL(10)lost their precision, silently widening an Oracle column to 38 digits on every migrationvarchar(max)— T-SQL syntax Redshift rejectsFROM orders JOIN customerscounted one table, so the multi-table write warning under-reported (fail-open)onNodesChange, and this graph is controlled without oneHow they were found
Three harnesses, all in the normal
unitproject:schema-fuzz.test.ts— a seeded generator (deterministic; a fuzzer thatfinds a different bug each run is a flaky test) building adversarial schemas,
then checking properties across all 14 dialects: self-compare is empty,
swapping sides mirrors added/removed, hostile names are quoted, type rendering
is a fixed point, and the dialects are cross-checked against each other.
generated-ddl-runs.test.ts— hands the generated DDL to a realnode:sqliteengine. String assertions only ever prove the output matcheswhat somebody expected it to be.
cte-syntax.test.ts— 44 adversarial CTE/subquery cases against thesafety gates, encoding the rule that a misread must fail closed.
Notes for review
quoteIdentifieris an optional dialect hook and quoting applies only whena name cannot be written bare, so every ordinary name is byte-identical to
before — which is why 1,555 pre-existing tests passed untouched through it.
ColumnDiff.source.nameis optional for the same reason: this package ispublished.
sql-editore2e tests fail on this branch. I stashed the work andconfirmed they fail identically on a clean tree — pre-existing, not from
these changes.
never revert, revert lossy executes without ticking. I kept the safety
property and made the lossy tests tick explicitly. That is a product decision
made on the maintainer's behalf and worth a second opinion.
package.jsonpins
dompurify: 3.4.13andadm-zip: 0.6.0inoverrides, but the lockfileresolves 3.2.7 and 0.5.18 — the vulnerable versions. The overrides are inert in
this tree and
npm ciinstalls the vulnerable ones. npm's suggested fix(monaco 0.56.0) is insufficient: it depends on dompurify 3.4.8 and the advisory
range is
<=3.4.12. Not touched here.Verification
npx vitest run— 1788 passed, 31 skippedcd apps/web && npx tsc --noEmit— cleannpx eslinton the changed areas — 0 errors🤖 Generated with Claude Code
Note
High Risk
Changes affect live-database revert execution, migration DDL generation across dialects, and SQL write-detection gates—any regression can apply wrong schema changes or under-report dangerous writes.
Overview
This PR tightens history revert so ticked objects drive both risk verdicts and generated SQL (
inSelectioninplanRevert), and the compare modal refuses Execute with zero ticks while always sending an explicitobjectKeysarray instead of widening to the full schema. Revert captures now store from/to version ids (migration 15) and the graph shows ↩ reverted to vN labels.Schema Sync Browse becomes its own pane (
BrowseBar, type filters in the tree, connection card in the detail panel) instead of a button buried on Compare’s connection cards.The SQL generator quotes hostile identifiers (dialect
quoteIdentifier, reserved words, real column names viaColumnDiff.source?.name), skips invalid FK ALTER on SQLite/ClickHouse, and fixes NUMBER(10) precision and Redshiftvarchar(max).generated-ddl-runs.test.tsexecutes generated DDL innode:sqlite;schema-fuzz.test.tssweeps all dialects deterministically.SQL editor safety (
sql-splitter): data-modifying CTEs classify as writes; CTE aliases are excluded from multi-table table counts;FROM orders JOIN customersalias parsing is fixed.Also adds a no-SQL Markdown change report export from the version compare modal, history compare button on the Target card, minimap theming fixes, and expanded e2e coverage for browse and forward/back revert provenance.
Reviewed by Cursor Bugbot for commit 65555b4. Bugbot is set up for automated code reviews on this repo. Configure here.