-
Notifications
You must be signed in to change notification settings - Fork 134
feat(workspace): route warehouse tools through the bound workspace's engine #1168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/workspace-engine-overlay
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,9 @@ import { PostConnectSuggestions } from "./post-connect-suggestions" | |
| import { getCache } from "../native/schema/cache" | ||
| import * as Registry from "../native/connections/registry" | ||
| // altimate_change end | ||
| // altimate_change start — workspace precedence | ||
| import * as Precedence from "../workspace/precedence" | ||
| // altimate_change end | ||
|
|
||
| export const SqlExecuteTool = Tool.define("sql_execute", { | ||
| description: "Execute SQL against a connected data warehouse. Returns results as a formatted table.", | ||
|
|
@@ -38,6 +41,19 @@ export const SqlExecuteTool = Tool.define("sql_execute", { | |
| } | ||
| // altimate_change end | ||
|
|
||
| // altimate_change start — workspace precedence. | ||
| // Last, after BOTH native safety checks. A redirect returns early, so anything | ||
| // above it stops running — and neither check has an equivalent on the other side: | ||
| // the engine's execution tools apply no hard-deny list, and an engine tool key is | ||
| // matched by the builder's `"*": "allow"` rule while `sql_execute_write` is "ask". | ||
| // Redirecting first would let a write reach the warehouse without the confirmation | ||
| // the same statement needed a moment ago. Approving and then redirecting is not a | ||
| // wasted prompt: the write still happens, through the engine, and what the user | ||
| // authorised is the write — not which connection carries it. | ||
| const precedence = await Precedence.check(ctx.sessionID, "sql_execute", args.warehouse) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MAJOR — the approval is not bound to the call that eventually executes The ordering here is correct, and What is not enforced is the invariant the ordering is for. The redirect is model-facing text. The model then composes a new engine call with new arguments. The engine wrapper checks only the generic engine-tool permission with pattern UPDATE orders SET ...does not guarantee the engine call carries that statement — it could carry a different write, or one of the supposedly un-overridable The model could already call the engine tool before this PR — what changes is that for served connections the unguarded path becomes the recommended one, and E2E row 7 shows the model does follow redirects unprompted. Fix: forward the already-validated arguments server-side to the selected engine tool, or wrap each mapped engine execute tool with the same classifier, hard deny and confirmation applied to the engine call's own SQL. At minimum this belongs in the review log as a disclosed residual — a reader of Claim 3 today would reasonably conclude the gates still cover the query. |
||
| if (precedence.redirect) return precedence.redirect | ||
| // altimate_change end | ||
|
|
||
| // altimate_change start — shadow-mode pre-execution SQL validation | ||
| // Runs validation against cached schema and emits sql_pre_validation telemetry, | ||
| // but does NOT block execution. Used to measure catch rate before deciding | ||
|
|
@@ -87,18 +103,24 @@ export const SqlExecuteTool = Tool.define("sql_execute", { | |
| }) | ||
| } | ||
| // altimate_change end | ||
| return { | ||
| // altimate_change — carries the fail-open notice when the target could not be | ||
| // attributed to the workspace; a no-op otherwise. | ||
| return Precedence.annotate(precedence, { | ||
| title: `SQL: ${args.query.slice(0, 60)}${args.query.length > 60 ? "..." : ""}`, | ||
| metadata: { rowCount: result.row_count, truncated: result.truncated }, | ||
| output, | ||
| } | ||
| }) | ||
| } catch (e) { | ||
| const msg = e instanceof Error ? e.message : String(e) | ||
| return { | ||
| // altimate_change — annotate the failure too. A fail-open notice that only rides | ||
| // on success is worse than none: the reason vanishes exactly when the call went | ||
| // wrong, and the `precedence` marker under-counts fail-open in precisely the | ||
| // cases most likely to fail. | ||
| return Precedence.annotate(precedence, { | ||
| title: "SQL: ERROR", | ||
| metadata: { rowCount: 0, truncated: false, error: msg }, | ||
| output: `Failed to execute SQL: ${msg}\n\nEnsure the dispatcher is running and a warehouse connection is configured.`, | ||
| } | ||
| }) | ||
| } | ||
| }, | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAJOR — time-of-check/time-of-use between the routing decision and the executed target
This pin, and the check at
:497-501, close the window across the dbt await. But the routing decision was made earlier and elsewhere:Precedence.check()→resolveDefaultTarget(register.ts:139-160) does its ownRegistry.list().warehouses[0]read from inside the tool body, and the handler then resolves the target again, independently. Theawait Dispatcher.call(...)boundary and the handler's own awaits are enough for a queued concurrent mutation to land in between, so the comment's claim that this makes the decided and executed connection "the same by construction" is stronger than what the pin actually does.Concretely:
warehouse.removedrops it;sql.explainorschema.inspectthen picks the newly-first Snowflake connection and executes it locally, despite Snowflake being shadowed — unaudited execution on a served connection, the exact outcome this design exists to prevent;warehouse.addcan replace that name with a served type aftercheck()read it. The handler pins the already-replaced type and sees no subsequent change, so this check cannot detect that window.Note also that this pin exists only in
register("sql.execute")—sql.explain(:552-570) andschema.inspect(:678-691) have no equivalent guard at all.Fix: make the decision and the target acquisition atomic — move the precedence check into the handler after it pins the target (passing
sessionIDthrough), or return a lease{name, canonicalType, generation}that handlers must revalidate. Apply it to all three ops, explicit names included.Related, same seam:
Precedence.check()'sawait import("../native/connections/register")(precedence.ts:586-588) has no try/catch, andcheck()is called outside the surroundingtryin all three tool bodies — so a throw there takes outsql_execute,sql_explainandschema_inspecttogether instead of failing open.default-target.test.ts:123-151does not prove its stated invariant: it calls the dispatcher directly, omitting the preceding precedence decision, which is where the race actually is.