fix: capability honesty and comment-tolerant statement classification (#275, #272, #269, #273) by cevheri · Pull Request #289 · libredb/libredb-studio

cevheri · 2026-08-04T21:53:14Z

Four capability-honesty and classification defects, each its own commit, produced by the maintainer
loop (loop/) and reviewed commit by commit before publishing.

Fixes #275. Fixes #272. Fixes #269. Fixes #273.

What changed

Commit	Issue	Change
`refactor(sql)`	#275	New `src/lib/sql/leading-keyword.ts`: one comment-tolerant leading-keyword primitive with offsets, in a neutral module both `lib/db` and `lib/explain` can depend on. `classifySelectPrefix` rewired onto it; no behaviour change (its test file passes with a zero-line diff).
`fix(db)`	#275	Every site that reads a statement's leading keyword flips atomically: the limiter's type and CTE detection, its already-bounded probe, MSSQL's `TOP` splice, and `sql-base`'s read-only / schema-modifying predicates.
`fix(monitoring)`	#272	The Tables tab receives provider capabilities and offers a maintenance control only where the provider declares that operation.
`fix(db)`	#269	Inline row editing gates on a new declared capability, and a multi-row apply sends one request per row instead of one newline-joined payload.
`fix(schema-diff)`	#269	A modified column is answered per dialect: ClickHouse DDL for ClickHouse, an explicit "cannot express this" comment for the type ids that have no such statement, instead of PostgreSQL DDL for everything unlisted.
`feat(db)`	#273	`QueryResult` gains two additive optional channels — engine warnings and declared per-column types — filled by the providers that already computed and discarded them (Couchbase warnings, ClickHouse and Druid column types).
`feat(results)`	#273	Warnings render as a badge beside AUTO-LIMITED; declared types render in the result grid's column header.
`fix(loop)`	—	Harness fix: `loop.sh` no longer leaks its stage env-file pointer into the agent process. The gate's own fixture tests spawn these scripts, so the leak pointed them at the live config and started a real agent — one case took 367s instead of 450ms and failed the gate of a clean iteration.

User-visible behaviour changes, all deliberate

#275 — a comment-led SELECT that returns every row today starts returning the default 500 rows.
MSSQL now reports wasLimited: false for a CTE and for an already-TOP-bounded statement instead of
claiming a limit it never applied. # counts as a leading comment on every dialect, so a
#-annotated statement gains both a LIMIT and an Explain button. A comment-led SELECT on SQLite
returns its rows instead of an empty write result.

#272 — maintenance buttons disappear wherever the provider does not declare that operation: Druid
and the embedded libredb lose all three; MySQL, MSSQL, ClickHouse, Oracle and Redis lose Vacuum and
Reindex; MongoDB loses Reindex; Couchbase loses Vacuum. Every one was a guaranteed HTTP 400 before. No
maintenance control renders until provider metadata resolves, and none at all if that request fails
(fail-closed by design).

#269 — the EDIT toggle and editable cells disappear on ClickHouse, Druid, Couchbase, MongoDB, Redis
and libredb. A multi-row apply now runs one statement per row: it skips the dangerous-query
confirmation (the Apply action is the confirmation, and the statements are generated and primary-key
scoped), shows the last row's result, and can apply partially — the toast says "submitted" accordingly.
Double-clicking a cell with editing off no longer opens a throwaway input. Couchbase declares the
capability false: SQL++ has UPDATE, but the collection-open query projects the key as an alias the
key heuristic then matches against nothing, which was a silent no-op.

#273 — results carry engine warnings and declared column types. Standalone single-statement runs
only: neither channel travels /api/db/multi-query or the embedded workspace adapter yet (#285).

Verification

./loop/scripts/gate.sh (format, lint, typecheck, knip, test, build) green on every commit, and
again on the final tree
merged coverage 28695/28695 lines (100.00%)
./loop/scripts/functional-smoke.sh green — the real app booted, a PostgreSQL connection created
through the UI, a query run, rows rendered. It is the last gate for a reason: each commit passed its
own gate in isolation, so an interaction failure could only show up here, and this milestone changed
the statement classifier on the hot query path plus the results grid's cell and header renderers.
every task carried a fresh-context adversarial review; one round returned BLOCK (the per-row apply
would have executed nothing in production because the dangerous-query gate answered each call) and
was fixed before the commit landed
the Druid wire-header row order the column-type channel depends on was confirmed against a live
Druid: row 0 names, row 1 native types, row 2 SQL types. The probe also shows why the SQL type is the
one carried — a timestamp column's native type reads LONG, its SQL type TIMESTAMP

Follow-ups filed, not fixed here

Adjacent defects found while doing this work, each verified in the code before filing: #279 (per-dialect
row-update statements, the deferred half of #269), #280 (a trailing line comment swallows the injected
LIMIT while still reporting wasLimited), #281 (the multi-statement route keeps its own comment-blind
SELECT test), #282 (the admin Operations tab is #272's untouched twin), #284 (the schema-diff
transaction wrapper and ADD/DROP COLUMN paths are still single-dialect), #285 (neither result channel
reaches the multi-statement or embedded paths), #287 (a data-modifying CTE is classified as SELECT, so
the limiter appends a bound to a write and it silently commits 500 rows), #288 (a published workspace
flag nothing reads).

#287 is the one worth reading first: it is pre-existing, needs no comment to trigger, and its
consequence is a partial write rather than a truncated read.

…rocess pipeline.sh hands each stage its own env file through LOOP_ENV_FILE. loop.sh consumed it and then left it in the environment, which the agent inherits - and so does everything the agent spawns, including this repo's own gate. The gate's loop-script unit tests run loop.sh/pipeline.sh against throwaway fixtures, so an inherited pointer sent those fixtures at the LIVE loop config and started a real agent: one case hung for minutes instead of milliseconds, failing the gate of an otherwise clean iteration. loop.sh now unsets the variable after sourcing it, and the fixtures' spawn helper strips it as well so the suite stays hermetic when run by hand from a stage shell. A new pipeline test pins the invariant by recording what the agent process sees.

…#275) Move the comment-skipping classifier out of `lib/explain/select-prefix.ts` into `lib/sql/leading-keyword.ts`, so the query limiter can reuse it without importing upward from `lib/db` into `lib/explain`. No observable behaviour change anywhere: this is the seam for the fix, not the fix. `readLeadingKeyword` reports whichever word leads a statement, upper-cased, with the offsets it occupies in the input. Vocabulary-agnostic on purpose - the query limiter and `isReadOnlyQuery` need different keyword lists - and offsets rather than a bare string because the MSSQL path has to splice `TOP n` in after the real keyword instead of re-scanning for it. The ReDoS reasoning travels with the pattern: three separately measured backtracking traps, one of them found by CodeQL, are documented on the pattern and pinned by a bounded-time guard. Every adversarial input in that guard now ends in a character that cannot open a word, because the timing only pins the shape when the match FAILS - with a letter tail the anchorless mutation matches in 0.0ms and searches nothing. `tests/unit/lib/explain/select-prefix.test.ts` passes unchanged, which is the evidence this changed no behaviour.

…ied (#275) A leading SQL comment is not whitespace, so the shared statement classifier's `^\s*KEYWORD\b` tests all missed behind one and an annotated SELECT fell through to `OTHER`. No LIMIT was injected, the whole result set came back, and the badge reported the query as unlimited. Every reading site flips at once, because a classifier-only fix would make MSSQL claim a limit it never applied. - query-limiter: statement type, `hasCTE` and the MSSQL `TOP` probe all come from `readLeadingKeyword`. Searches that read the statement's text rather than just its leading keyword start at the keyword, so a word in the comment cannot answer for the statement. - sql-base: `isReadOnlyQuery` / `isSchemaModifyingQuery` move off `startsWith`, which also makes them exact-word (`selected_rows_view` is no longer read-only). This is the SQLite symptom: a commented SELECT took the write branch and returned no rows. - mssql: `TOP` is spliced after the real keyword using the primitive's offsets, after `DISTINCT` where present. The path declines with `wasLimited: false` rather than reporting a limit it did not apply, and refuses to splice past a `TOP` that is already there (which also fixes the pre-existing double-`TOP` on `SELECT DISTINCT TOP n` and `SELECT TOP(n)`). - MySQL's `#` joins `--` and `/* */` as leading trivia; without it the reported bug stayed live on that dialect. No statement can open with `#` elsewhere, so skipping it changes which syntax error the server reports and never a result. The bounded-time guard covers the new alternative. - oracle is untouched: it appends `FETCH FIRST` at the tail, so it corrects itself once classification does. User-visible: a comment-led SELECT that returns every row now returns the default 500.

…ability (#272) The monitoring Tables tab rendered per-row Analyze / Vacuum / Reindex for every connection and received no capability information, so on a provider that declares maintenance unsupported the controls could only ever answer HTTP 400. TablesTab now takes the connected provider's capabilities and renders only the operations declared in maintenanceOperations, and nothing at all when supportsMaintenance is false. The controls are keyed to the MaintenanceType vocabulary rather than loose strings, so the client gate mirrors the checks /api/db/maintenance already performs. MonitoringDashboard supplies the data through the existing useProviderMetadata hook - no new API surface. Unknown capabilities hide the controls rather than showing them: provider metadata is also absent when the request fails, and failing open would restore the dead buttons on exactly the connections this gate exists for. This matches how Studio gates Explain on supportsExplain. Scope is the monitoring Tables tab only. The admin Operations tab still renders maintenance controls without reading capabilities; druid.md, libredb.md and the Druid provider comment now say so explicitly instead of describing the intent.

) Adds supportsInlineRowEdit to ProviderCapabilities and hides the results-grid editing affordance where the engine has no single-table row update of the form the shared hook builds. Every provider declares the flag, each value derived from that engine's behaviour: true for PostgreSQL, MySQL, SQLite, Oracle and SQL Server; false for ClickHouse (a bare UPDATE ... SET is code 48 NOT_IMPLEMENTED, a row mutation is ALTER TABLE ... UPDATE), Druid (no row-level DML), MongoDB, Redis and LibreDB (JSON command languages), and Couchbase (SQL++ has UPDATE, but the collection-open query projects the document key as an alias the generated WHERE cannot address - deferred to #279). With the flag false Studio withholds the toggle callback, following the onExplain precedent, and the grid renders non-interactive cells; unresolved provider metadata hides the control too rather than falling open. The grid previously opened an editor on double-click for every provider and discarded the typed value, which this removes. Applying edits now sends one awaited request per edited row instead of a newline-joined payload, with the safety dialog skipped: that gate matches every UPDATE ... SET and returns without executing while remembering only the last statement, so an unflagged loop would have applied nothing but the row the user confirmed. The trailing semicolon is dropped, since splitStatements used to strip it on the multi-statement route and oracledb rejects one. Provider triad: all 11 providers' code, capability docs and integration tests move together, plus the capability table in docs/ADDING_A_PROVIDER.md and the inline-editing limitations recorded in clickhouse.md, couchbase.md and druid.md.

…aulting to PostgreSQL DDL (#269) The migration generator's modified-column path branches on the connection's type id with PostgreSQL as the trailing else, so every canonical id without a branch of its own was handed `ALTER TABLE ... ALTER COLUMN ... TYPE / SET NOT NULL` regardless of what that engine can run. ClickHouse now gets its own branch: `MODIFY COLUMN <col> <declared type>` plus, when a default disappears, `MODIFY COLUMN <col> REMOVE <kind>`. Nullability needs no separate statement because it lives inside the declared type here (`Nullable(T)`), which is what this provider's introspection reports. The five ids with no column-modification statement at all -- Couchbase, Druid, MongoDB, Redis and the embedded LibreDB -- emit a comment naming the limitation, the same answer the SQLite branch has always given for a change it cannot express. Output for postgres, mysql, sqlite, oracle and mssql is unchanged. The ClickHouse forms were live-probed against the pinned clickhouse-server 26.7.1.1315 build rather than assumed, and three probes changed the code: a computed column's default arrives kind-first (`MATERIALIZED toYear(d)`), so prefixing DEFAULT onto it is a syntax error (code 62); a bare MODIFY COLUMN does not clear an existing default, which is why the explicit REMOVE is emitted; and REMOVE rejects EPHEMERAL (code 62) while REMOVE DEFAULT against a column that has none is an error too (code 36), so both cases are guarded. The test's dialect table is a `Record<DatabaseType, ...>`, so a new provider fails typecheck until it is classified rather than silently inheriting the PostgreSQL branch -- the defect class this fix closes. ADDING_A_PROVIDER.md lists it among the exhaustive maps, and the six provider docs whose claims this changes move with it.

#273) QueryResult gains two optional channels, both additive: `warnings?: QueryWarning[]` for notices an engine attached to a statement it completed, and `columnTypes?: Record<string, string>` for the declared type per column, keyed by its name in `fields`. Nothing became required and `fields` keeps its shape, so every existing consumer compiles unchanged; `QueryWarning` is re-exported from src/exports/types.ts because the type ships in the npm package. Absence is the signal throughout: a run that produced no warnings, and a response that declared no column types, omit the field rather than sending an empty array or `{}`, so the UI can decide what to render from presence alone. Three providers stop dropping what they already know: - Couchbase carries through the warnings its transport collects. `CouchbaseWarning.code` became optional in the process: the transport substituted `code: 0` for an entry that reported none, which was harmless as plumbing and becomes a fabricated code on screen once the field is published, and 0 is itself a legal code. - ClickHouse carries the envelope's declared types verbatim, wrappers included - `Nullable(String)` is what tells the user the column accepts nulls, and for a computed column it is the only source of a type at all. - Druid carries the SQL type, and the native type deliberately stops at the provider: it reports LONG for an ISO timestamp string and for a boolean, so it is the half a column may not be labelled with. Druid also gains the fact the channels exist for. The cluster answers a query it could only partly serve with an ordinary 200, so a short row set is indistinguishable from a correct one. The seam now requires `unavailableSegments: number | null`, the transport counts the response context's missing-segment list (its length only, so respelling a descriptor changes nothing), and the provider turns a positive count into one warning. Null and zero stay distinct: only "the source confirmed a whole answer" licenses trusting the rows, and every unreadable shape answers null rather than claiming completeness. This closes the limitation druid.md had recorded as blocked on exactly this channel. One pre-existing assertion changed because it was wrong, not inconvenient: the Couchbase transport test asserted `{ code: 0, message: "" }`, i.e. it pinned the fabrication above. Three Druid fixtures and one exact-key-set assertion changed because the neutral seam type genuinely gained a required field; the key set stays exact, so it still fails on a seventh field. The seam guard was tightened, not loosened - the response-context header and its list join the wire vocabulary that may appear only in http-transport.ts. Docs move with the code: the three providers' result-shaping tables, API_DOCS, ADDING_A_PROVIDER's rule for a future provider, and the hand-mirrored QueryResult in docs/editor/query-optimization.md that had gone stale. Not carried yet, deliberately: /api/db/multi-query and the embedded workspace adapter both pick result fields explicitly, so both channels travel on the single-statement path only.

The result type carries both channels since the provider half of #273, but nothing rendered them. Surface each where its neighbours already live. Warnings become an amber badge beside the AUTO-LIMITED badge in the stats bar, rendered only when the engine reported one - the messages are in its tooltip and in an sr-only twin, so they are reachable without hover. A result with no rows has no stats bar, and that is exactly the case the channel exists for: an engine can answer 200 with every segment unavailable. So the empty state lists the notices itself, above the "operation was successful" hint rather than below it, and both surfaces word a warning through one shared formatter. Declared column types render beside the column name in the desktop table's header and are part of that header's accessible name. The compact table below the md breakpoint takes them as a tooltip only: it sizes header and body cells from their own content, so visible text in the header alone would push the columns out of step with the rows. A column the engine declared no type for, and a run that reported no warnings, render exactly as before.

Copilot

Pull request overview

Improves SQL classification, provider capability honesty, result metadata, and loop-test isolation.

Changes:

Adds comment-tolerant SQL classification and safer provider-specific limiting/editing behavior.
Surfaces engine warnings and declared column types in query results.
Gates unsupported maintenance/editing controls and expands regression coverage and documentation.

Reviewed changes

Copilot reviewed 91 out of 91 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
`tests/unit/sql/leading-keyword.test.ts`	Tests SQL keyword parsing and performance.
`tests/unit/schema-diff/migration-generator.test.ts`	Tests dialect-specific column migrations.
`tests/unit/loop-scripts.test.ts`	Tests environment-variable isolation.
`tests/unit/lib/query-generators.test.ts`	Updates capability fixtures.
`tests/unit/lib/explain/select-prefix.test.ts`	Tests hash-comment classification.
`tests/unit/db/sql-base.test.ts`	Tests comment-tolerant predicates.
`tests/unit/db/query-limiter.test.ts`	Tests commented-statement limiting.
`tests/unit/db/druid/transport.test.ts`	Covers segment availability metadata.
`tests/unit/db/druid/seam-guard.test.ts`	Extends Druid transport boundaries.
`tests/unit/db/druid/introspect.test.ts`	Updates Druid result fixture.
`tests/unit/db/druid/http-transport.test.ts`	Tests response-context parsing.
`tests/unit/db/couchbase/http-transport.test.ts`	Tests optional warning codes.
`tests/unit/db/base-provider.test.ts`	Verifies default edit capability.
`tests/unit/components/results-grid-utils.test.ts`	Tests warning descriptions.
`tests/integration/db/sqlite-provider.test.ts`	Covers commented reads and editing.
`tests/integration/db/redis-provider.test.ts`	Verifies editing is unsupported.
`tests/integration/db/postgres-provider.test.ts`	Verifies editing capability.
`tests/integration/db/oracle-provider.test.ts`	Verifies editing capability.
`tests/integration/db/mysql-provider.test.ts`	Covers comments and editing.
`tests/integration/db/mssql-provider.test.ts`	Tests comment-aware TOP insertion.
`tests/integration/db/mongodb-provider.test.ts`	Verifies editing is unsupported.
`tests/integration/db/libredb-provider.test.ts`	Verifies editing is unsupported.
`tests/integration/db/druid-provider.test.ts`	Tests warnings, types, and capabilities.
`tests/integration/db/couchbase-provider.test.ts`	Tests warnings and editing gate.
`tests/integration/db/clickhouse-provider.test.ts`	Tests column types and editing gate.
`tests/hooks/use-tab-manager.test.ts`	Updates metadata fixture.
`tests/hooks/use-query-execution.test.ts`	Updates metadata fixture.
`tests/hooks/use-provider-metadata.test.ts`	Updates metadata fixture.
`tests/hooks/use-inline-editing.test.ts`	Tests sequential per-row updates.
`tests/helpers/mock-provider.ts`	Updates default capabilities.
`tests/components/studio/StudioMobileHeader.test.tsx`	Tests hidden editing action.
`tests/components/studio/QueryToolbar.test.tsx`	Tests hidden EDIT control.
`tests/components/Studio.test.tsx`	Tests editing capability gating.
`tests/components/schema-explorer/SchemaExplorer.test.tsx`	Updates capability fixtures.
`tests/components/ResultsGrid.test.tsx`	Tests types, warnings, and editing.
`tests/components/results-grid/StatsBar.test.tsx`	Tests warning badges.
`tests/components/QueryEditor.test.tsx`	Updates capability fixture.
`tests/components/monitoring/TablesTab.test.tsx`	Tests maintenance gating.
`tests/components/monitoring/MonitoringDashboard.test.tsx`	Tests capability propagation.
`src/lib/types.ts`	Adds warning and column-type channels.
`src/lib/sql/leading-keyword.ts`	Adds shared keyword parser.
`src/lib/schema-diff/migration-generator.ts`	Adds dialect-aware column handling.
`src/lib/explain/select-prefix.ts`	Reuses shared keyword parsing.
`src/lib/db/utils/query-limiter.ts`	Classifies commented statements.
`src/lib/db/types.ts`	Adds inline-edit capability.
`src/lib/db/providers/sql/sqlite.ts`	Declares editing support.
`src/lib/db/providers/sql/sql-base.ts`	Uses shared keyword predicates.
`src/lib/db/providers/sql/postgres.ts`	Declares editing support.
`src/lib/db/providers/sql/oracle.ts`	Declares editing support.
`src/lib/db/providers/sql/mysql.ts`	Declares editing support.
`src/lib/db/providers/sql/mssql.ts`	Makes TOP insertion comment-aware.
`src/lib/db/providers/sql/druid/transport.ts`	Carries segment availability.
`src/lib/db/providers/sql/druid/index.ts`	Exposes warnings and column types.
`src/lib/db/providers/sql/druid/http-transport.ts`	Parses availability headers.
`src/lib/db/providers/sql/clickhouse/introspect.ts`	Documents EPHEMERAL defaults.
`src/lib/db/providers/sql/clickhouse/index.ts`	Exposes declared column types.
`src/lib/db/providers/keyvalue/redis.ts`	Disables inline editing.
`src/lib/db/providers/embedded/libredb.ts`	Disables inline editing.
`src/lib/db/providers/document/mongodb.ts`	Disables inline editing.
`src/lib/db/providers/document/couchbase/transport.ts`	Makes warning codes optional.
`src/lib/db/providers/document/couchbase/index.ts`	Exposes engine warnings.
`src/lib/db/providers/document/couchbase/http-transport.ts`	Preserves absent warning codes.
`src/lib/db/base-provider.ts`	Adds default editing capability.
`src/hooks/use-inline-editing.ts`	Submits updates per row.
`src/exports/types.ts`	Exports `QueryWarning`.
`src/components/studio/StudioMobileHeader.tsx`	Conditionally renders editing.
`src/components/studio/QueryToolbar.tsx`	Conditionally renders EDIT.
`src/components/Studio.tsx`	Gates editing by capabilities.
`src/components/ResultsGrid.tsx`	Renders types and warnings.
`src/components/results-grid/utils.ts`	Formats engine warnings.
`src/components/results-grid/StatsBar.tsx`	Adds warning badge.
`src/components/monitoring/tabs/TablesTab.tsx`	Gates maintenance actions.
`src/components/monitoring/MonitoringDashboard.tsx`	Fetches provider capabilities.
`README.md`	Updates feature claims.
`loop/scripts/loop.sh`	Stops environment-pointer leakage.
`docs/providers/sqlite.md`	Documents classification and editing.
`docs/providers/redis.md`	Documents capability limitations.
`docs/providers/postgres.md`	Documents comment-aware limiting.
`docs/providers/oracle.md`	Documents editing support.
`docs/providers/mysql.md`	Documents hash-comment limiting.
`docs/providers/mssql.md`	Documents TOP behavior.
`docs/providers/mongodb.md`	Documents editing limitations.
`docs/providers/libredb.md`	Documents maintenance and editing.
`docs/providers/druid.md`	Documents warnings, types, and gates.
`docs/providers/couchbase.md`	Documents warnings and editing.
`docs/providers/clickhouse.md`	Documents types and migrations.
`docs/FEATURES.md`	Updates inline-editing behavior.
`docs/editor/README.md`	Adds result-level signals.
`docs/editor/query-optimization.md`	Documents warnings and types.
`docs/API_DOCS.md`	Documents result metadata fields.
`docs/ADDING_A_PROVIDER.md`	Extends provider guidance.

Suppressed comments (2)

src/components/ResultsGrid.tsx:529

The compact header repeats the direct lookup problem from the desktop header: if a partial type map omits a column named constructor or toString, this resolves to an inherited function instead of no type and is passed to the DOM as title. Restrict the lookup to own properties.

                  title={result.columnTypes?.[field]}

README.md:751

Marking ClickHouse migration generation complete overstates the implementation: this PR only makes modified-column statements dialect-aware, while transaction wrapping and ADD/DROP paths remain invalid and are explicitly deferred to #284. Do not mark ClickHouse complete until the generated script is runnable, or qualify this milestone as partial modified-column support.

- [x] **Phase 11**: Schema Diff & Migration (Snapshot Timeline, Cross-Connection Diff, Migration SQL Generation for PostgreSQL, MySQL, SQLite, Oracle, SQL Server, and ClickHouse).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+  // The statement from its own first keyword onward. Anything that searches the
+  // statement's TEXT rather than just its leading keyword has to start here, or a
+  // word written in the leading comment answers for the statement itself.
+  const fromKeyword = leading === null ? trimmed : trimmed.slice(leading.start);


+  // statement it needs (issue #269). Unknown hides it, like Explain below: metadata
+  // is also null when /api/db/provider-meta fails, and offering a control that can
+  // only error is the defect this gate exists to fix.
+  const canEditRows = metadata?.capabilities.supportsInlineRowEdit === true;



+  // Declared provider capabilities, so tabs can hide controls the provider cannot
+  // perform (issue #272). Same hook Studio uses — no new API surface.
+  const { metadata } = useProviderMetadata(selectedConnection);


+                  // Tooltip only: this table sizes header and body cells from their own
+                  // content, so visible type text here would push the header out of step
+                  // with the rows below it. The desktop table shares one measured width.
+                  title={result.columnTypes?.[field]}


+                              <Button
+                                key={type}
+                                variant="ghost"
+                                size="icon"
+                                className={className}


        const isSensitive = effectiveMaskingEnabled && sensitiveColumns.has(field);
+        // The type the wire format declared for THIS result - the only source for a
+        // computed column, which has no catalog entry the schema tree could answer with.
+        const declaredType = result.columnTypes?.[field];


 - **Visual EXPLAIN**: Graphical execution plans to identify performance bottlenecks.
 - **Interactive ER Diagrams**: Visual schema graph with real foreign key edges, cardinality labels, MiniMap navigation, table search/filter, compact mode, and PNG/SVG export. Automatic hierarchical layout powered by ELK.js.
- **Schema Diff & Migration**: Compare schema snapshots or cross-connection schemas side-by-side. Color-coded diff view (added/removed/modified) with automatic migration SQL generation for PostgreSQL, MySQL, SQLite, Oracle, and SQL Server.
+- **Schema Diff & Migration**: Compare schema snapshots or cross-connection schemas side-by-side. Color-coded diff view (added/removed/modified) with automatic migration SQL generation for PostgreSQL, MySQL, SQLite, Oracle, SQL Server, and ClickHouse.


+**The admin Operations tab still has the same gap.** `src/components/admin/tabs/OperationsTab.tsx`
+renders its global `Run Analyze` / `Run Vacuum` / `Run Reindex` controls and its per-table
+Analyze/Vacuum buttons without reading `getCapabilities()`, so those still answer 400 here. #272's
+bar covers the monitoring Tables tab only; the Operations tab is a known gap, not yet filed. Stated


+`maintenanceOperations = []`. The monitoring **Tables** tab hides the maintenance actions for this
+provider: it renders no per-row maintenance control when a provider declares maintenance unsupported
+(issue #272). The admin **Operations** tab does not read capabilities yet, so its maintenance buttons
+still appear here and answer HTTP 400 — a known gap, not yet filed, outside #272's bar.


Nine of ten review comments on PR #289 were valid; each fix carries its own regression test. - query-limiter: three probes still searched the whole statement text, so a leading comment answered for the body. A comment reading "switch to ROWNUM <= 10" marked a SELECT already bounded and it ran unbounded - the symptom #275 removed, through a different door. UNION detection and the nested-SELECT count had the same blindness. All three now read from the statement's own first keyword. - use-provider-metadata: the hook kept the previous connection's capabilities while fetching, and a late response could overwrite a newer one - so the inline-edit and maintenance gates could offer a control the selected engine rejects. Metadata now clears on a connection change and every settle path checks the request is still the selected connection's. - ProviderCapabilities.supportsInlineRowEdit is optional: the interface is published, and a required field added after the fact stops external implementers from compiling. A compile-time pin covers it; runtime behaviour is unchanged because the UI already gated on === true. - ResultsGrid: a declared type is read with an own-key check. A column named 'constructor' otherwise answered with Object.prototype.constructor, handing React a function as header content. The compact header's type also ships as screen-reader text, since a title on a non-focusable element is unreachable by touch. - Docs: the README's ClickHouse migration claim is narrowed to column modifications (the wrapper and ADD/DROP paths remain #284), and four stale 'not yet filed' notes now point at #282. Declined: converting TablesTab's shadcn icon Button to a plain button. The rule is real, but the pattern appears 15 times across 10 files and this change added a condition rather than a restyle - the repo-wide audit belongs on its own.

cevheri · 2026-08-04T22:33:11Z

Reviewed all ten comments against the code. Nine are valid and fixed in one follow-up commit; one is
declined with a reason. Verification for each claim is below, since two of them changed my read of the
diff.

Fixed

1. Leading trivia could still answer whole-statement probes (query-limiter.ts) — confirmed, and
worse than the comment says: three probes read the full text, not just the ROWNUM one. normalized is
now built from the statement's own first keyword, so a leading comment cannot answer for the body.

-- switch to ROWNUM <= 10 on oracle before a SELECT no longer marks it bounded (it ran unbounded —
the symptom A leading SQL comment defeats the query limiter, so a commented SELECT runs unbounded #275 removed, through a different door)
-- consider UNION ALL here no longer makes the statement a union
-- SELECT was here no longer counts as a nested SELECT (hasSubquery)

Real bounds are still detected, including behind a comment. Five regression cases added.

2 and 3. Stale capabilities from useProviderMetadata — confirmed, both halves, and the second is
the sharper one: the hook did not clear metadata on a connection change, AND a late response from the
previous connection overwrote the newer answer. A test proved the ClickHouse case reported
supportsInlineRowEdit: true from PostgreSQL's response. Fixed once in the hook, so both call sites
benefit: metadata clears when a new connection is taken, and every settle path checks the request is
still the selected connection's before answering.

The check is against the connection-id ref rather than an effect-cleanup flag deliberately: a cleanup
firing for the same connection would discard the only response, and the hook's id guard means no
refetch would follow.

4. The new capability was required on a published type — confirmed and the most consequential of
the ten. ProviderCapabilities is re-exported from src/exports/types.ts, so a required field added
after the fact stops every external implementer from compiling. Now optional, with a compile-time pin
in base-provider.test.ts: a capability object that omits it must satisfy the type. Before the fix that
pin failed with TS2741: Property 'supportsInlineRowEdit' is missing. Runtime behaviour is unchanged —
the UI already gated on === true, so an absent flag reads as unsupported. Doc table updated.

5. Compact header exposed the type through title on a non-focusable element — confirmed. Visible
text there is not free (that table sizes header and body cells independently, so it would desync the
columns — the reason the tooltip-only choice was made), so the type now also ships as screen-reader
text, the same treatment the warnings badge uses. The tooltip stays for pointer users.

7. Prototype-chain lookup for a declared type — confirmed, and SELECT 1 AS constructor is legal
SQL. Both sites go through one helper with an own-key check. Test covers columns named constructor
and toString against a partial type map.

8. README overstated ClickHouse migration support — confirmed, and this PR is what introduced it
(the T5 commit added ClickHouse to that list). Both lines now read "plus ClickHouse column
modifications", which is what actually landed; the wrapper and ADD/DROP paths are #284.

9 and 10. Stale "not yet filed" notes — confirmed. Both now point at #282. Two more instances of
the same staleness in clickhouse.md and couchbase.md ("the UI never offers them") were fixed in the
same pass: true of the monitoring Tables tab after this PR, still false of the admin Operations tab.

Declined

6. Button size="icon" in TablesTab — the rule is real and correctly cited, but this is not the
place to act on it. size="icon" appears 15 times across 10 component files, the Platform Integration
Rules check passes on this diff, and this PR added a capability condition rather than restyling a
control that already looked this way. Converting one of fifteen call sites would leave the codebase
less consistent, not more, and carries visual risk on a monitoring tab. The repo-wide audit belongs in
its own change.

Not from the review, found while verifying it

analyzeQuery's end-anchored bound probes read the trailing text, so a statement ending in
-- LIMIT 10 is treated as already bounded and the injection is skipped. That is the same
trailing-trivia family as #280 and is noted there rather than widened into this PR.

Copilot

Pull request overview

Copilot reviewed 92 out of 92 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/components/monitoring/tabs/TablesTab.tsx:215

These icon controls still use <Button size="icon">, which violates the package integration rule in .claude/rules/platform-integration.md:34: platform Button CSS can override these dimensions when Studio is embedded. Since this refactor recreates the controls, use plain <button> elements; also give each icon-only button an accessible name.

                              <Button
                                key={type}
                                variant="ghost"
                                size="icon"
                                className={className}
                                onClick={() => handleMaintenance(type, table.tableName)}
                                disabled={!!actionLoading}
                                title={label}

src/hooks/use-provider-metadata.ts:69

The new settle guard can leave isLoading permanently true: if a request is in flight and connection becomes null, the effect sets lastConnectionId.current to null, so this finally refuses to clear the loading state, and the null branch does not clear it either. Reset loading when the connection is cleared (while retaining the stale-response guard for replacement connections).

      .finally(() => {
        clearTimeout(timeoutId);
        if (lastConnectionId.current === requestedId) setIsLoading(false);

+    for (const statement of statements) {
+      await executeQuery(statement, undefined, false, { skipSafety: true });


…y SQL (#269) A result field is named by whatever the query aliased it to, so the inline editor's generated UPDATE interpolated arbitrary text as an identifier. A column aliased 'x = 1; DELETE FROM users; --' produced two statements plus a comment that swallowed the WHERE, and node-postgres runs a multi-statement string through the simple query protocol. Applying edits skips the dangerous-query dialog (#269), so nothing showed that SQL to the user first - the statement has to be inert by construction. - New src/lib/sql/identifier.ts: quoteIdentifier() per dialect, escaping the closing quote character, plus isBareIdentifier() for names that cannot be quoted safely. It lives beside the other dialect-agnostic SQL-text utilities, so lib/db and lib/schema-diff both reach it without inverting a dependency. - Column and primary-key identifiers are quoted. They come from the result's own field list, so they are exactly what the engine reports and quoting keeps case semantics intact. Side effect worth having: a column named with a space or a reserved word now produces legal SQL instead of a syntax error. - The table name is validated instead. It is a GUESS - a tab title or the first word after FROM - so quoting it would break a hand-typed lowercase name on Oracle, where the real table is upper-cased. A guess that is not a bare identifier is refused with a toast, the same way a missing primary key already is. - schema-diff's private escapeIdentifier is replaced by the shared one; its copy did not escape an embedded closing quote either. Test expectations that assert the emitted SQL are re-baselined: identifiers are quoted now, deliberately. The value half of the same statement is NOT fixed here - on MySQL and ClickHouse a backslash can still close the literal early - and is filed as #290, because the honest fix is a parameter channel rather than more escaping.

cevheri · 2026-08-04T22:59:45Z

Valid, and the mechanism checks out: a result field is named by whatever the query aliased it to, so
SELECT id AS "x = 1; DELETE FROM users; --" made the generated statement

UPDATE t SET x = 1; DELETE FROM users; -- = 'new' WHERE id = 1

which node-postgres runs through the simple query protocol as two statements. The confirmation dialog
used to expose that SQL before execution and this PR's skipSafety removed that exposure, so the point
stands as written.

Fixed by the first of your two remedies rather than the second: 90aec97 quotes the identifiers. The
alternative — restoring a confirmation — would put back the defect an earlier review round caught, since
isDangerousQuery matches every UPDATE ... SET and the gate returns without executing while
remembering only the last query it was handed, so a per-row loop would apply nothing but the row the
user happens to confirm.

What landed

New src/lib/sql/identifier.ts with quoteIdentifier(name, dialect), escaping the closing quote
character as well — the existing private helper in schema-diff/migration-generator.ts did not, so a
name holding one ended the quoted span there too. That copy is now gone and both callers share this
one.
Column and primary-key identifiers are quoted. They come from the result's own field list, so they
are exactly what the engine reports and quoting keeps case semantics intact. It also fixes an
everyday break: a column named first name, or one named order on MySQL, previously produced
invalid SQL.
The table name is validated, not quoted. It is a guess — a tab title, or the first word after
FROM — and quoting a guess changes its case semantics: a hand-typed lowercase name would stop
resolving on Oracle, where the real table is upper-cased. A guess that is not a bare (optionally
dotted) identifier is refused with a toast, the same way a missing primary key already is.

Tests: the hostile-alias shape, a space-bearing column on MySQL, a rejected table guess, and a
schema-qualified name that still works. Existing expectations that assert the emitted SQL are
re-baselined, since identifiers are quoted now on purpose.

Deliberately not fixed here: the value half. #290. The same statement interpolates the edited
value with single-quote doubling, which is enough for PostgreSQL, SQLite, Oracle and SQL Server but not
for MySQL/MariaDB or ClickHouse, where a backslash escapes inside a string literal. A value of
\' WHERE 1=1 -- closes the literal early and the trailing predicate becomes the statement's real
WHERE, so every row is updated — and no second statement is needed, which means mysql2's
multipleStatements: false default does not prevent it. The honest fix is a parameter channel through
executeQuery (provider.query(sql, params?) already exists), not more escaping, so it belongs in its
own change rather than riding along here.

Copilot

Pull request overview

Copilot reviewed 94 out of 94 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/components/monitoring/tabs/TablesTab.tsx:212

These are small icon buttons in a platform-facing component, but .claude/rules/platform-integration.md:34 requires plain <button> elements because platform Button CSS can override Studio's size classes. Keeping shadcn Button size="icon" here can therefore silently change the controls' dimensions when embedded; render the mapped actions as styled native buttons instead.

                              <Button
                                key={type}
                                variant="ghost"
                                size="icon"
                                className={className}

src/hooks/use-provider-metadata.ts:38

Clearing metadata in a passive effect is not immediate: the connection-changing render still returns the previous connection's metadata, so Studio/TablesTab are rendered once with the wrong capability set before this effect runs. This contradicts the fail-closed contract and can briefly expose controls for the newly selected engine. Track which connection produced the stored metadata and return it only when that id matches the current connection.id during render.

    setMetadata(null);

src/hooks/use-inline-editing.ts:110

handleApplyChanges is now a long-running async loop, but there is no re-entry guard and pending changes remain visible until every request completes. A second click starts another loop and submits the same UPDATEs concurrently; even idempotent values can fire triggers twice. Add an isApplying/ref guard and disable Apply until the loop finishes.

      // statement that carries one (ORA-00933).
      statements.push(`UPDATE ${tableName} SET ${setClauses.join(", ")} WHERE ${quote(pkColumn)} = ${pkVal}`);
    }

src/components/ResultsGrid.tsx:371

This gate runs after the isEditing branch. If editing is revoked while an input is already open (for example, during a connection/capability change), that branch keeps rendering the editable input even though editingEnabled is now false. Close/reset editingCell when editing becomes disabled, or include editingEnabled in the earlier branch condition.

        // No editor is opened unless editing is on: the commit paths above already
        // required it, so without this a cell offered an input whose edit was
        // silently discarded — including where the provider declares no inline row
        // editing at all (issue #269).
        if (!editingEnabled) {

src/components/results-grid/StatsBar.tsx:83

The warning details are available only through title on a non-focusable span. The hidden text helps screen-reader users, but sighted keyboard users cannot focus the badge to discover why a result may be incomplete. Use the existing focusable Tooltip pattern (or render the details inline) so the warning is reachable without a pointer.

        {warnings.length > 0 && (
          <span className="text-amber-400 text-xs bg-amber-500/10 px-2 py-0.5 rounded" title={warningDetail}>
            {warnings.length} WARNING{warnings.length > 1 ? "S" : ""}
            <span className="sr-only">: {warningDetail}</span>
          </span>

sonarqubecloud · 2026-08-04T23:04:20Z

Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.1% Duplication on New Code

See analysis details on SonarQube Cloud

cevheri added 8 commits August 4, 2026 14:25

cevheri requested a balanced review from Copilot August 4, 2026 21:58

Copilot started reviewing on behalf of cevheri August 4, 2026 21:59 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

cevheri mentioned this pull request Aug 4, 2026

A trailing line comment swallows the injected LIMIT while the query still reports wasLimited #280

Closed

cevheri requested a balanced review from Copilot August 4, 2026 22:37

Copilot started reviewing on behalf of cevheri August 4, 2026 22:38 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

Comment thread src/hooks/use-inline-editing.ts

Comment on lines +108 to +109

for (const statement of statements) {

await executeQuery(statement, undefined, false, { skipSafety: true });

cevheri mentioned this pull request Aug 4, 2026

Inline row edit interpolates the cell value, so a backslash-escaping dialect can turn it into SQL #290

Closed

cevheri requested a balanced review from Copilot August 4, 2026 22:57

Copilot started reviewing on behalf of cevheri August 4, 2026 22:58 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

cevheri merged commit 5e9a06a into main Aug 4, 2026
18 checks passed

cevheri deleted the fix/maintainer-sweep-3 branch August 4, 2026 23:16

Navigation Menu

Uh oh!

Uh oh!

fix: capability honesty and comment-tolerant statement classification (#275, #272, #269, #273) - #289

fix: capability honesty and comment-tolerant statement classification (#275, #272, #269, #273)#289
cevheri merged 10 commits into
mainfrom
fix/maintainer-sweep-3

cevheri commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

cevheri commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

cevheri commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

sonarqubecloud Bot commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		for (const statement of statements) {
		await executeQuery(statement, undefined, false, { skipSafety: true });

Uh oh!

Uh oh!

Conversation

cevheri commented Aug 4, 2026

What changed

User-visible behaviour changes, all deliberate

Verification

Follow-ups filed, not fixed here

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

cevheri commented Aug 4, 2026

Fixed

Declined

Not from the review, found while verifying it

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

cevheri commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

sonarqubecloud Bot commented Aug 4, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants