Skip to content

fix(sql): a commented-out DROP stopped becoming a statement, and two more - #496

Merged
cevheri merged 2 commits into
mainfrom
chore/backlog-round10
Aug 25, 2026
Merged

cevheri merged 2 commits into
mainfrom
chore/backlog-round10

Conversation

@cevheri

@cevheri cevheri commented Aug 25, 2026

Copy link
Copy Markdown
Member

Three BACKLOG entries close. Every claim was measured against a live engine and then verified in Chrome on a production build, and the browser found a defect three passes of gates did not.

S1 — a commented-out DROP became a statement the route ran

statement-splitter.ts walked spans itself instead of using spans.ts, so it disagreed with every other reader about where a statement ends — and unlike them, its disagreement is executed: /api/db/multi-query runs each fragment it returns.

postgres 18, the entry's own shape:
  /* a /* b */ ; DROP TABLE s1_victim; -- */ SELECT count(*) FROM s1_victim
  psql        -> 1 row, survived = 2        (block comments NEST: one statement)
  old reading -> 3 fragments, #2 = `DROP TABLE s1_victim`, and the route ran them
  the gate    -> silent, because it read the whole editor text, where the
                 operative keyword is nothing at all

The splitter now reads through the shared reader under the caller's dialect, and isDangerousQuery asks about the same fragments the runner will run. Every caller passes a dialect, including the agent's own draft reader.

The same defect survived on three shipped engines, because SqlGrammar could not carry CQL's third comment form. // is a line comment on Apache Cassandra 5.0.9, ScyllaDB 2026.2.4 and ClickHouse 26.7.1 — each probed with a control arm, so "the statement still ran" cannot be mistaken for "the ; was hidden" — and is refused or is an operator everywhere else (PostgreSQL 18: operator does not exist: integer // integer). SELECT ... // note; DROP ... was therefore cut into a read and a runnable bare DROP out of text the server reads as one statement.

doubleSlashComment is now a grammar fact, which the splitter got for free by reading through spans.ts — and it closed a silent ClickHouse bug in passing:

ClickHouse 26.7.1, prepareQuery(..., limit 5)
  before: SELECT number FROM numbers(1000) // note LIMIT 5   -> 1000 rows, wasLimited: true
  after:  SELECT number FROM numbers(1000) LIMIT 5 // note   ->    5 rows

A third reader, found in the browser and by no gate. QueryEditor.getEffectiveQuery picks the statement at the cursor with raw lastIndexOf(";") — no spans, no dialect, not even a string-literal check — and it decides the text that is sent. On the buffer above the request body carried a line comment plus the SELECT, and the grid read 0 rows where psql answers 2. It now reads through the shared splitter, so SplitStatement carries offsets. Re-verified in Chrome after the fix: survived = 2, 1 row, and the request still goes to /api/db/query.

One pinned policy changed deliberately: with the caret in an empty statement between two ;, the old reader fell through to the whole buffer — so "run the statement I am in" silently ran every statement, which with a DROP in the second one is a destructive surprise. It now runs the statement the caret is immediately after.

U9 — a control that named an operation the page behind it could not run

Four providers pointed vacuumAction at something that is not a vacuum (ClickHouse Optimize Table, SQL Server and Oracle Rebuild Indexes, Couchbase Compact), and MySQL rendered BaseDatabaseProvider's default "Vacuum Table" although its operations are analyze/optimize/check/kill.

Each provider now declares, next to the operation, which MaintenanceType its wording stands for and what kind of target it takes. #427's generic label-to-operation mapping is not repeated — it handed Oracle a table where ALTER INDEX wanted an index and answered ORA-01418 on every click, and was reverted. Oracle's per-table optimize was rebuilt to read USER_INDEXES WHERE TABLE_NAME = :t and proven against the live engine:

Oracle AI Database 26ai Free Release 23.26.2.0.0, through the provider
  old statement  ALTER INDEX "U9_PROBE" REBUILD    -> ORA-01418
  new            optimize U9_PROBE                 -> success, rebuilt 2 of 2
                 an UNUSABLE index                 -> VALID
                 LAST_DDL_TIME moved for the named table's indexes and no others

All three surfaces gate and title from the declaration — the monitoring Tables tab, the admin Operations tab and the schema explorer's row menu, which was still offering SQLite "Vacuum Table" for one table while the Tables tab correctly withheld it. Browser-verified: MySQL's Operations tab reads Run Optimize / Optimize Tables, with no vacuum card and no reindex card; PostgreSQL, which does vacuum, is unchanged.

Then the refusals those new controls exposed. MySQL discarded OPTIMIZE/CHECK TABLE's result rows, so a missing table answered "OPTIMIZE completed successfully" and CHECK's verdict — the entire point of the control — never reached anyone. Oracle reported success for a target the catalog does not know, and for a table where every index rebuild was refused (measured with a READ ONLY tablespace: success: true, "rebuilt 0 of 2", ORA-01647 discarded).

And none of it reached the user: use-monitoring-data toasted success on any HTTP 200 and the audit event hardcoded result: "success", so a refused operation showed a green tick reading "OPTIMIZE failed: ..." and was logged as completed. Verified end to end in the browser:

POST /api/db/maintenance {type:"optimize", target:"no_such_table_r10"}
  -> 200 {"success":false,"message":"OPTIMIZE failed: mysql.no_such_table_r10:
          Table 'mysql.no_such_table_r10' doesn't exist"}
  audit page: Total 1 ops, Success 0%      (before: 100%)

DOC2 — a copy-paste recipe the API server rejects

--set extraEnv[0].value="true" renders value: true, an unquoted YAML boolean, while core/v1.EnvVar.value is a string. Reproduced with helm v4.1.3 and the real chart, parsing the rendered value rather than eyeballing the quotes. Nothing catches it earlier: values.schema.json types extraEnv as plain objects, so helm lint --strict passes.

Both recipes use --set-string, plus two more coercion sites the entry did not name (ingress.annotations is a map[string]string; limit-rpm=120 rendered as an integer) and eleven bracket arguments that zsh globs away before helm is reached. A static lint over the README's bash fences pins the class, non-vacuous by mutation.

The chart is a released version, so a packaged-file change costs version: 0.1.50 and the operator mirror — the operator ships its own copy of this README and was still carrying the broken recipes.

Recorded rather than fixed

  • U20 — the maintenance API validates that an operation exists, not that it can take the target, so POST {type:"vacuum", target:"users"} still vacuums the whole SQLite file. Admin-gated; the invariant just lives in one layer only.
  • U21 — MSSQL and MongoDB declare check globally runnable and there is no card copy for it. Inventing copy for five providers without measuring what a whole-database DBCC CHECKDB costs is the mapping Redis schema-explorer actions generate MongoDB syntax: nothing the sidebar produces can be executed #427 reverted.
  • U22 — TableItem's items are deep links, so "the operation takes a table" and "the destination renders the control" are different questions.

Gates

Gate Result
bun run format clean (943 files)
bun run lint 0 errors
bun run typecheck clean
bun run knip clean
bun run chart:check chart 0.1.50 / appVersion 0.13.4 in sync
tests 0 fail; 33/33 component groups
coverage 100.00% (42786/42786 lines)
bun run build exit 0
build:lib + attw exit 0
gitleaks 1 commit scanned, no leaks

One note for review: the first implementation pass was killed mid-edit by a session limit and left the tree not compiling (Oracle called a method that was never written). It was finished from that point, and the three reviews then ran against the completed work — the CQL residual, the success-reporting defects and the chart blocker all come from those reviews rather than from the original passes.

…more

S1 - `statement-splitter.ts` walked spans itself, so it disagreed with every
other reader about where a statement ends, and unlike them its disagreement
is executed: /api/db/multi-query runs each fragment it returns. Measured on
postgres 18, `/* a /* b */ ; DROP TABLE users; -- */ SELECT 1` is ONE read
(block comments nest there), and the flat reading cut it into three whose
second was a bare DROP TABLE users. It now reads through spans.ts under the
caller's dialect, and the confirmation gate asks about the same fragments the
runner will run rather than about the whole editor text.

The same defect survived on three shipped engines because SqlGrammar could
not carry CQL's third comment form: `//` is a line comment on Cassandra
5.0.9, ScyllaDB 2026.2.4 and ClickHouse 26.7.1 (probed, each with a control
arm) and code everywhere else, so `SELECT ... // note; DROP ...` was cut into
a read and a runnable DROP out of text the server reads as one statement.
`doubleSlashComment` is now a grammar fact, which also fixed a silent
ClickHouse limiter bug: the bound landed inside the comment, so a query the
UI reported as LIMIT 5 returned all 1000 rows.

A third reader, found in Chrome and not by any gate: QueryEditor's
`getEffectiveQuery` picked the statement at the cursor with raw
`lastIndexOf(";")` - no spans, no dialect, no literal check - and it is what
decides the text that is SENT. The same buffer above reached postgres as a
line comment plus the SELECT and the grid read 0 rows where psql answers 2.
It now reads through the shared splitter, so SplitStatement carries offsets.

U9 - four providers pointed `vacuumAction` at an operation that is not vacuum
and MySQL rendered the base "Vacuum Table" for an engine with no vacuum, so a
per-row item named an operation the page behind it could not run. Each
provider now declares, next to the operation, which MaintenanceType its
wording stands for and what target it takes; all three surfaces gate and
title from that declaration. #427's generic mapping is not repeated: it sent
Oracle a table where ALTER INDEX wanted an index and answered ORA-01418 on
every click. Oracle's per-table optimize was rebuilt to `USER_INDEXES WHERE
TABLE_NAME = :t` and proven against Oracle AI Database 26ai Free - two of two
rebuilt, an UNUSABLE index back to VALID, and LAST_DDL_TIME moving only for
the named table.

Then the refusals the new controls exposed. MySQL discarded OPTIMIZE/CHECK
TABLE's result rows, so a missing table answered "completed successfully" and
CHECK's verdict never reached anyone; Oracle reported success for a target the
catalog does not know, and for a table where every rebuild failed. And none of
it reached the user: use-monitoring-data toasted `success` on any HTTP 200 and
the audit event hardcoded "success", so a refused operation showed a green
tick reading "OPTIMIZE failed: ..." and was logged as done. Verified in the
browser: the audit page now reads Success 0% for one refused optimize.

DOC2 - two chart README recipes rendered an unquoted YAML boolean where
core/v1.EnvVar.value is a string, so the API server rejects the manifest. Both
use --set-string now, plus two more coercion sites the entry did not name
(ingress annotations are map[string]string) and eleven bracket arguments a zsh
user's shell globs away. A static lint over the README's bash fences pins the
class. The chart is a released version, so this costs chart 0.1.50 and the
operator mirror.

Recorded rather than fixed: U20 (the maintenance API validates that an
operation exists, not that it can take the target), U21 (two providers declare
`check` globally runnable with no card copy) and U22 (TableItem's items are
deep links, so where the destination renders the control is a separate
question).
# Conflicts:
#	src/components/QueryEditor.tsx
@sonarqubecloud

Copy link
Copy Markdown

@cevheri
cevheri merged commit ed4128f into main Aug 25, 2026
19 checks passed
@cevheri
cevheri deleted the chore/backlog-round10 branch August 25, 2026 13:21
cevheri added a commit that referenced this pull request Aug 27, 2026
… no longer holds them (#519)

A `docs/BACKLOG.md` entry LEAVES the file when the work lands, and 208 citations across 101
files had outlived theirs - 30 dead ids. A reader who followed one found an empty grep and had
to decide whether the comment or the file was wrong. Nothing measured it: the structural guard
asks whether the index is derived from the bodies, and the one in `agent-documentation.test.ts`
asks the same of `docs/AGENT.md` alone, scoped to the M2 section's `B` ids.

Each dead id is replaced by the PR that closed it, verified one at a time by showing the entry
present at that commit's parent and absent at the commit. That is the rule this pins: a
citation of `docs/BACKLOG.md` says *this is open* and has to resolve, while a claim about work
already done cites the immutable record that carries the reasoning. A new guard in
`tests/unit/backlog-structure.test.ts` holds it, so closing an entry now means rewriting its
citations in the same PR - the lockstep the provider triad already runs on.

**The count arrived in three instalments, and both jumps were the pattern's fault rather than
the tree's.** Worth stating, because a guard that under-reports reads exactly like a clean one,
and the handover this round started from carried the first number as if it were the answer:

- Matching line by line found 77. Three citations wrap across a line break in a block comment,
  so the scan runs over whole files now and derives the line from the match offset.
- Those 80 were all of ONE form. An id parenthesised in the syntax of a PR reference - `#U9` -
  accounted for another 128, more than the form anyone was looking for, and 43 named one dead
  id. GitHub renders no link for it, so a reader takes it for a pull request.
- Four of the dead ids were removed by #518, the round before this one, which reported zero new
  dangling citations. True of the form it measured; false of the other.

**Three of the 208 were not pointer faults but stale CLAIMS**, and each argued for behaviour
the code no longer has:

- `context-snapshot.ts` told a reader the hold "does NOT carry the database that connection
  currently points at", four paragraphs above the comment describing the key that does. #509
  made the first half false; `connectionIdentity` hashes host, port, database and the role
  fields, and the map is keyed on it.
- `goal-verifier.ts` deferred the terminal-status vocabulary to the owner and said the
  recording "moves when B24 is ratified". It was ratified 2026-08-13 in the shape the module
  was built for, and `finalize` has been writing `goalVerdict` since.
- `AGENT_DEMO.md` told a demo driver not to show a Redis draft because "an open question the
  owner has not ruled on" hung over it. It was ruled on and declined on 2026-08-22, with the
  reasoning recorded at `planningDerivedGroupingsRule`.

Two documentation claims that a closure had falsified came out with them. `ADDING_A_PROVIDER.md`
said the Operations tab's per-table button title "is still the hardcoded Analyze"; the tab reads
`maintenanceOperationSpecs[...].label` through `maintenanceControl()` and falls back to the
generic verb, so it never read `analyzeAction` and the hardcoded title is only the fallback.
`docs/providers/couchbase.md` described a global Reindex card answering *"The reindex operation
requires a target"* - #496 withdrew that card (`global: false`) rather than rewording it, which
leaves the provider's three `reindexGlobal*` labels read by nobody. Recorded in the doc, not
changed in the code: whether they stay is the owner's call.

The guard's limit is stated rather than implied. A third form is not checked and cannot be: the
bare parenthesised id. 232 are in the tree and 47 distinct ids among them name no live entry,
but the shape is not the backlog's alone - `(S256)` is a PKCE code-challenge method and `(T6)` a task
number from issue #331. A bare id matching a live entry needs no help; one that does not is
either a dead citation or not a citation at all, and nothing in the text says which.

The guard's own fixtures were five of the first eighty-four sites it reported, and its docblock
example one more after that. They are interpolated from the document now, so the source text is
unscannable while the runtime string still exercises the shape, and no example is pinned to an
entry that will close. Non-vacuity is a positive control on each extractor plus a floor on the
file scan - never on the number of citations, which falls every time an entry closes. Proven by
mutation: one line carrying both forms turns it red on both, and nothing else moved.

No rendered string changed anywhere in this PR - `reviewedBy` is validated for non-emptiness and
never displayed - so there is nothing here a browser could verify, and no browser check is
claimed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant