Repository navigation
fix(vscode-extension): ask before running a write, as the web editor does - #1990
Conversation
|
@cevheri this is ready for review. It makes Run in the VS Code extension ask before sending a statement The gate had to move out of To check it: Not run locally: a real VS Code window, the Docker-backed integration and e2e legs, One call you may want to weigh in on: the web dialog's AI risk analysis and the typed confirmation some engines use (etcd range delete, lease revoke) are not reproduced; the extension asks one question for those. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cevheri
left a comment
cevheri
left a comment
There was a problem hiding this comment.
Thanks Marc, the design is right and I want to merge it. One change first.
The required checks pass, but the VS Code Extension job is red here and green on main (run 40938036959). Two existing integration tests now run a statement your gate flags: the PostgreSQL UPDATE at results-view.test.ts:533 and the MongoDB non-JSON run at tree-editor.test.ts:907. The fake window (test/fakes/vscode.ts:47) answers any unscripted message with undefined, so your modal declines and nothing is sent. The failure at results-view.test.ts:603 follows from the first. The modal is right, the tests predate it.
Please script answerMessages(() => RUN_ANYWAY) in both tests and restore the default in a finally: the integration config does not load the unit leg's reset hook, and a leaked answer would change later tests. Also assert the modal itself, since these are the only tests where a real child's verdict reaches the host. That leg needs Docker, so CI can confirm it.
Reverting run-query.ts or handlers.ts turns your new tests red, good. Leaving out the AI analysis and etcd's typed confirmation is fine, the doc says so. If you want a follow-up for the etcd typed step, tell me and I will open the issue.
Done when:
- results-view.test.ts, PostgreSQL UPDATE case: RUN_ANYWAY scripted, default restored in
finally, exactly one modal warning whose detail isupdate it_update3 set n = n + 10. - tree-editor.test.ts, MongoDB non-JSON case: the same, one modal warning for
{ collection: orders }before the provider's error. - The VS Code Extension job is fully green on the new head (both integration legs, e2e, VSIX step, none skipped), required checks still green.
- No production file changes beyond the current diff.
|
hi @MarcBasas |
|
Thanks Marc. Since this has been quiet for a few days and #2204 builds on the gate move here, I'm taking this over: I'll rebase it on main, fix the two integration tests from my review, and push to this branch, keeping your commits. Your authorship stays on them. |
isDangerousQuery and writesUnderGrammar lived in a "use client" component file, which the VS Code extension's child cannot bundle (its boundary check refuses src/components). They move unchanged to src/lib/sql/dangerous-query.ts and the component re-exports both, so the hooks, the published package and the tests that mock the component import them as before.
…does Run sent the target straight to query/run, so DELETE FROM orders ran on the first click while the web editor asks first. query/target now also answers dangerous: true when isDangerousQuery flags the target under the connection's type, and Run shows a modal warning with the statement and a Run Anyway button before anything is sent. Cancel, Escape and a cancelled run send nothing. Select Rows (a generated SELECT) and Load More (pages of a statement already confirmed) ask nothing, as in the web editor. The AI risk analysis and the typed confirmation some engines require are not reproduced. Closes libredb#1935
… the gate now flags The PostgreSQL UPDATE in results-view.test.ts and the MongoDB text that is not JSON in tree-editor.test.ts are flagged by isDangerousQuery, so Run now asks before sending them, and the fake window declines any message it was not told how to answer. Both cases script Run Anyway, put the default answer back in a finally (the integration config loads no reset hook), and assert the one modal warning with the statement as its detail: these are the only tests where a real child's verdict reaches the host.
…on leaves out D409 now describes the gate in src/lib/sql/dangerous-query.ts with the dialog's re-export, and keeps open what the move did not do: the tests, the imports and the stub seams. The destructive-statement section names isDangerousQuery and its module. The extension README says the question is one modal dialog, without the web editor's AI risk analysis or etcd's typed confirmation, as docs/VSCODE_EXTENSION.md already does.
…t, never leave it out dangerous was optional and only ever present as true, so a child answer that left it out read as safe and Run sent the statement without asking. It is now a required boolean: the child answers isDangerousQuery's verdict on every target, false on an empty one, and the compiler refuses a child path or a test double that forgets it. The host still trusts its own bundled child's typed answers, as it does for every other method, so no runtime check is added and PROTOCOL_VERSION stays 1.
…s differs The extension README named only writes to data, objects and permissions, but the gate never asks on an INSERT or a CREATE and does ask on text it cannot read safely, such as MongoDB text that is not JSON. It now lists what asks and what does not. docs/VSCODE_EXTENSION.md said Select Rows runs a generated SELECT; it runs whatever read the engine's generator built, and unlike the web editor's table click it does not go through the gate, which the section now says with the two names that ask on the web. The destructive-statement paragraph in query-optimization.md puts each sentence on its own line.
…us is false The host asked only on a truthy dangerous, so a child answer that left the field out (a stale dist, a swapped backend) ran a write without the modal. The required type checks the child's handlers, not the wire. Run now skips the question only when dangerous is false exactly, and the protocol docblock says what the compiler does and does not hold.
…ows example The extension README said an INSERT or a CREATE never asks, but the gate asks on PostgreSQL INSERT ... ON CONFLICT DO UPDATE, Snowflake CREATE OR REPLACE TABLE and Spark INSERT OVERWRITE TABLE, each checked against isDangerousQuery. docs/VSCODE_EXTENSION.md named an S3 bucket with -- as a Select Rows read the web editor asks on; the generated aws s3 ls for such a bucket does not ask, so only the measured ClickHouse a$$b example stays.
dcbeb91 to
122edd3
Compare
Description
In the VS Code extension,
DELETE FROM ordersran on the first click, while the web editor asks first. Run now asks the same question before it sends: the statement goes through the web editor's ownisDangerousQuery, under the connection's type, and a flagged one opens a modalshowWarningMessagethat shows the statement and offers Run Anyway. Cancel, Escape and a cancelled run send nothing. A plainSELECTasks nothing.isDangerousQuerylived insrc/components/QuerySafetyDialog.tsx, and the extension's child bundle refuses anything undersrc/components(and the host bundle refusessrc/lib/sql). So the gate moves unchanged tosrc/lib/sql/dangerous-query.ts(first commit, a pure move) and the child reads it.query/targetalready hands the host the statement Run will send, so it now also carries the gate's verdictdangerousfor that statement, and the host asks unless the verdict is exactlyfalse. No second round trip and no second copy of the gate.Type of Change
Related Issue
Closes #1935
Changes Made
src/lib/sql/dangerous-query.ts:isDangerousQuery,writesUnderGrammarand their helpers, moved fromQuerySafetyDialog.tsxunchanged. The component re-exports both names, souse-query-execution,use-query-adapter, the package exports and every test that mocks the component are untouched.vscode-extension/src/protocol.ts,src/backend/handlers.ts:RunTargetResult.dangerous: boolean, required, set byrunTargetfromisDangerousQuery(statement, connection.type)on every answer (falsefor an empty target).PROTOCOL_VERSIONstays 1 (host and child ship in one VSIX).vscode-extension/src/host/editor/run-query.ts: after the target is read and before the request is built, a flagged statement asks in a modal. It fails closed: an answer without the verdict (a stale or swapped child bundle) asks too. The wait is raced against the run's cancellation token like the other waits inprepare.docs/VSCODE_EXTENSION.md(step 4, the sequence diagram, the component table, and the "what differs" entry that said there was no confirmation),vscode-extension/README.md, and twodocs/BACKLOG.mdlines that named the old file.Left out on purpose: the AI risk analysis in the web dialog, and the typed confirmation some engines require (an etcd range delete or lease revoke). The extension asks one question for those too.
docs/VSCODE_EXTENSION.mdsays so. Select Rows and Load More ask nothing, as in the web editor.Testing
New tests:
test/unit/editor/run-query.test.ts: a flagged target shows the modal with the statement and runs only after Run Anyway; dismissing it, and cancelling the run during the question, send nothing and settle the run; an unflagged target shows nothing.test/unit/backend-handlers.test.ts:query/targetflags aDELETE, a write behind a read in one selection, a MongoDBdeleteMany, leaves aSELECTand afindunflagged, and reads the same text differently under MySQL and PostgreSQL (#comment).test/integration/sqlite-child.test.ts: the builtdist/backend.jsanswersdangerousfor aDELETEand not for aSELECT.What I ran (Windows 11, Node 24.18.0, Bun 1.4.4):
bun run format,lint(0 errors),typecheck,knip,readme:check,security:check,channels:showcase:check,build,build:lib: pass.vscode-extension:npm run typecheck,npm run build(bundle boundary checks pass),npm run coverage:check(100% lines), and the SQLite child integration test on the node runtime: pass.npm run test:unitpasses exceptwebview-shims.test.ts, which compares a POSIX path and fails on Windows, and two webview scroll tests that time out when the machine is busy with the root suite (they passed in the first run).bun run test: 1719 of 1745 files pass, including every component, hook and SQL test around the gate. The 26 failing files are Windows-only (bun cannot pass a-escript throughbun.cmd,tarcannot take aC:path, TLS and transport probes); none of them mentions the gate or the dialog.Not run:
chart:check,test:coverage/coverage:checkat the repo root, the Docker-backed extension integration tests, the extension's e2e leg, and a manual run in a real VS Code window.Test Environment
Checklist
bun run test:coverageandbun run coverage:check)src/lib/db/providers/, I updated the matchingdocs/providers/documentation andtests/integration/db/tests in the same PR (provider triad)AGENT_STATEMENT_LANGUAGErow, I kept its agent leg in step: the engine overlay undersrc/lib/agent/languages/<statement-language>/engines/, theAgent language packsubsection of its provider document, andtests/unit/lib/agent/engine-docs.test.tsMaintainer update (2026-11-04)
I took this over to finish it; Marc's two commits are kept with his authorship, rebased on main without conflicts (the dialog had not changed on main, so the move is still verbatim).
Added on top:
results-view.test.tsPostgreSQL UPDATE,tree-editor.test.tsMongoDB non-JSON run) answer Run Anyway, restore the default in afinally, and assert the modal.dangerousis a required boolean, and the host fails closed when it is missing, with a unit test that fails if the check is reverted.INSERT ... ON CONFLICT DO UPDATE,CREATE OR REPLACE TABLEandINSERT OVERWRITEdo), where Select Rows differs from the web table click, and the gate's new module indocs/editor/query-optimization.md.Run on Linux at the rebased head: the full root gate set,
bun run test(2011 files),test:coveragepluscoverage:check(100% lines),build:lib,attw; invscode-extension, unit (1256 passing, 100% lines), integration on both the node and vscode runtimes against Docker (74 passing), e2e, andpackage.This PR is also the first step of #2204, which builds its shared statement analysis on
src/lib/sql/dangerous-query.ts.Additional Notes
The gate moved without edits, so the existing
tests/components/QuerySafetyDialog.test.tsxand the other gate tests keep covering it through the re-export. The root coverage gate is not checked locally; the moved file is exercised by the same tests as before.