Uh oh!
There was an error while loading. Please reload this page.
feat(workspace): route warehouse tools through the bound workspace's engine - #1156
feat(workspace): route warehouse tools through the bound workspace's engine#1156ralphstodomingo wants to merge 10 commits into
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34260894 | Triggered | Generic CLI Secret | b53c00e | packages/opencode/test/cli/help/snapshots/help-snapshots.test.ts.snap | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ralphstodomingo
commented
Aug 26, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:7ecb94a92c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| async function derive(tools: Record<string, unknown>): Promise<Precedence> { | ||
| if (escapeHatchOn()) return EMPTY("escape-hatch") |
There was a problem hiding this comment.
When ALTIMATE_WORKSPACE is unset after a previously opted-in run, the persisted workspace binding and pinned datamate MCP entry can still materialize engine tools, and this derivation enables routing because it checks only the escape hatch. This bypasses the documented opt-in gate and can unexpectedly redirect local warehouse calls for users who disabled the workspace pilot; return disabled precedence when CoreFlag.ALTIMATE_WORKSPACE is false.
Useful? React with 👍 / 👎.
| // Adapter not initialised far enough to answer; leave the type undetermined. | ||
| } | ||
| if (!type) type = await adapterTypeFromManifest() | ||
| return { source: "dbt", type } |
There was a problem hiding this comment.
When a warehouse-less sql_execute has a usable dbt adapter, this always reports the dbt target, but the actual handler falls back to Registry.list()[0] whenever dbt execution throws or returns an unrecognized result. If the dbt type is unshadowed while that registry connection is a shadowed type, precedence allows the call and the fallback then executes locally against a connection that should have been routed to the workspace engine. The routing decision must account for this fallback or be applied again at the point where the native fallback is selected.
Useful? React with 👍 / 👎.
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
7ecb94a to
bd7edfaCompareralphstodomingo
commented
Aug 26, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:8a017dd81f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Ahead of the write-permission prompt on purpose: a shadowed write should be | ||
| // redirected, not approved and then redirected. | ||
| const precedence = await Precedence.check(ctx.sessionID, "sql_execute", args.warehouse) | ||
| if (precedence.redirect) return precedence.redirect |
There was a problem hiding this comment.
When workspace precedence shadows the selected connection, this return happens before classifyAndCheck, so DROP DATABASE, DROP SCHEMA, and TRUNCATE calls that previously threw the unconditional “cannot be overridden” error now instruct the model to invoke the engine execution tool instead. The MCP wrapper only checks permission for the engine tool name and does not apply this SQL hard-deny list, so following the redirect bypasses the native destructive-query safeguard; preserve the hard-deny check before deciding whether to redirect.
Useful? React with 👍 / 👎.
| async execute(args, ctx) { | ||
| // altimate_change start — workspace precedence | ||
| const precedence = await Precedence.check(ctx.sessionID, "sql_explain", args.warehouse) | ||
| if (precedence.redirect) return precedence.redirect |
There was a problem hiding this comment.
When the default or named warehouse is shadowed, this early return bypasses both preflight validators below it. Consequently, inputs such as an empty/placeholder SQL string or warehouse: "" return a successful-looking workspace redirect instead of the existing INVALID INPUT response, encouraging the model to pass malformed arguments to the engine tool; perform these input checks before consulting precedence.
Useful? React with 👍 / 👎.
5945afd to
5551619Compareralphstodomingo
commented
Aug 26, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:5551619a37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const precedence = await Precedence.check(ctx.sessionID, "sql_execute", args.warehouse) | ||
| if (precedence.redirect) return precedence.redirect |
There was a problem hiding this comment.
When the builder submits an INSERT, UPDATE, DELETE, or other non-hard-denied write against a shadowed connection, this returns before the sql_execute_write permission check, and the model's subsequent engine call is checked only under its datamate_* tool key (session/prompt.ts:1866-1872). The default builder rules allow that key through the wildcard rule while requiring sql_execute_write: "ask", so the redirected write can modify the workspace warehouse without the confirmation the same native call previously required; propagate or independently enforce the write permission before the engine executes.
Useful? React with 👍 / 👎.
808282a to
83d0636Compare
…engine When a bound workspace's integration engine attaches, the model sees two implementations of the same capability with different credential sources: the native warehouse tools over local keychain connections, and the engine's MCP tools over the SaaS connection. Nothing arbitrated between them, so the model picked by description — and that choice silently decided which credentials ran the query and whether the call was audited. Add `altimate/workspace/precedence.ts`, which decides per session which side serves a call. One principle runs through it: shadow only what is materialised AND attributed; anything undetermined runs locally with an explicit notice; nothing is ever silent. - Materialised, not declared: derived from the engine tool keys actually present in the model-facing MCP map, so a declared-but-broken integration shadows nothing. - Attributed: the engine must be pinned to the bound workspace. Attach guarantees this; precedence re-checks it and refuses to engage otherwise. - Capability-scoped, not type-scoped: the engine's warehouse integrations are not symmetric — snowflake serves execute/explain/inspect, bigquery and postgresql serve execute only, databricks serves execute only. Keying on the individual materialised tool key keeps `sql_explain` on a BigQuery connection local instead of redirecting it to a tool that does not exist. - Redirect, no fallback: a shadowed call returns a result naming the exact engine key and executes nothing. The result carries `metadata.redirected` and `metadata.redirect_to`, because `Tool.wrap` reports every returning body as a successful call and a redirect would otherwise be indistinguishable from a real execution. Add `resolveDefaultTarget(op)` so a call with no `warehouse` is judged against the target it would really reach. Only `sql.execute` consults dbt; explain and inspect stay registry-only, or the guard would drag adapter construction (Python bridge, manifest rebuild, file watchers) onto paths that never touch dbt today. Adapter creation is now single-flight — two concurrent `warehouse`-less calls used to construct it twice. Add `canonicalType()` to the connection registry, derived by inverting `DRIVER_MAP` so aliases cannot desync: `postgresql`/`postgres` are one driver, as are `mariadb`/`mysql` and `mssql`/`fabric`/`sqlserver`. `redshift` keeps its own identity and is not served by a postgresql integration. Precedence is re-derived every turn from the live tool map rather than cached at attach, so it stays correct when an engine's tool set changes underneath. Both tool resolvers call the same description helpers. `--integrations=local` turns shadowing off for the session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
Unit coverage per mechanism: what materialises confers precedence and what merely declares does not; an engine pinned elsewhere (or not pinned at all) confers none and fails open so the local call still runs; capability scoping, including the case the design turns on — `sql_explain` against a BigQuery connection stays local rather than redirecting to an engine tool that does not exist; driver-alias collapse end to end, with `redshift` proven not to be served by a postgresql integration; the redirect's telemetry marker; the escape hatch; and re-derivation as an engine's tool set changes. The registry is seeded with real connections in `beforeEach`. Without that, `check()` returns "run" simply because the connection is unknown, and every "stays local" assertion passes without proving anything. Also cover `resolveDefaultTarget`: insertion order decides the registry default, an empty registry resolves to nothing rather than guessing, explain and inspect never report a dbt source, and two concurrent execute resolutions agree. Replace this module's private `--datamate` parser with attach's exported `pinnedWorkspace`. The private one read only `command` as an argv array, only the space-separated spelling, and took the first match — so it would have reported "unattributed" for engines that are correctly pinned via a string `command` plus `args`, via `--datamate=<id>`, or via a repeated flag where last wins. Precedence fails open, so that would have quietly disabled the feature rather than breaking loudly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
Precedence is re-derived every turn, but what changed is a once-per-session statement: which capabilities the bound workspace's engine now serves for which warehouse types, how many local connections that shadows, and — when shadowing is off — why. Silence is the one outcome this design does not allow; repeating the line every turn would be noise. The escape hatch and an unattributable engine both announce themselves. An ordinary unbound session says nothing, because nothing changed for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
…-side only The new `--integrations` flag is longer than any existing root option, so yargs re-wraps the whole option table — the snapshot churn is realignment plus the one new row. Verified whitespace-normalised: the only new content is that flag's own description. Also record why this module must never be imported from a TUI plugin. Plugins load in a separate module realm in the same process, so such an import is a different instance sharing neither module state nor `globalThis`: it would typecheck, unit-test green, and return an empty precedence forever. Only the event bus crosses, which is why the inventory line is published as a TUI event rather than read directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
…back Two correctness holes found in review, both confirmed before fixing. The workspace pilot is opt-in, but precedence checked only its own escape hatch. A binding and a pinned `datamate` entry both persist in config, and the MCP client connects that entry on its own regardless of the pilot flag — so engine tools can materialise for someone who has switched the pilot off, and their local warehouse calls would start redirecting. Verified against a live workspace: with the flag unset, an engine still spawned and served its tools. Precedence now returns disabled unless the pilot is on. `sql_execute` with no `warehouse` falls back to the first registry connection whenever the dbt attempt yields nothing — not only when dbt is absent, but on an unrecognised result shape or any throw inside `tryExecuteViaDbt`. Reporting only the dbt target let a call through whose execution then landed locally on a connection that should have been routed to the workspace engine: silent, and precisely what this design exists to prevent. `resolveDefaultTarget` now reports that fallback alongside the dbt target, and a shadowed fallback redirects. That trades a possible false redirect — a dbt project on an unserved type whose first registry connection is served — for never executing silently against a served connection. The redirect is visible and recoverable (name a `warehouse`, or `--integrations=local`); the silent local execution is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
…nsist A call with no `warehouse` can be redirected because the *fallback* target is served, not the target it would have tried first — and the dbt attempt behind it might well have succeeded. The generic message gave no way to tell those apart. The fallback redirect now names the connection it would have landed on, says plainly that which path it takes is only known once it runs, and gives both exits: name a warehouse, or keep everything local for the session. Marked `metadata.via = "dbt-fallback"` so the two cases stay distinguishable downstream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
A redirect returns early, so anything the guard was placed above stopped running. Two checks were being jumped over. `sql_execute`'s hard deny on DROP DATABASE / DROP SCHEMA / TRUNCATE says it "cannot be overridden", and the engine's execution tools apply no equivalent list. With the guard above it, a blocked statement against a served connection came back as an instruction to call the engine tool — a way around the block. The deny now runs first. Precedence still comes before the write prompt, because approving a write and then redirecting it asks the user to authorise something that never runs. `sql_explain`'s pre-flight validators turn malformed input into an actionable message. A redirect reads as success, so returning one first sent the model to the engine tool carrying the same bad arguments. The validators now run first. `schema_inspect` has no such pre-flight checks, so its guard stays where it is. Tests assert the ordering from both sides — the deny and the validators survive, and an ordinary call on the same connection is still redirected, so the fix cannot quietly over-correct into never redirecting. Both new sql_execute cases fail against the previous ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
A write against a served connection was redirected before the `sql_execute_write` prompt, and the model's follow-up engine call is checked only under its `datamate_*` key — which the builder's `"*": "allow"` rule matches, while `sql_execute_write` is `"ask"`. So an INSERT, UPDATE or DELETE reached the warehouse without the confirmation the same statement needed a moment earlier. The guard now runs last, after every native safety check: hard deny, then write prompt, then precedence. An earlier draft put precedence first on the grounds that approving a write which then redirects asks the user to authorise something that never runs. That was wrong twice over — the write does run, through the engine, and what the user authorises is the write itself, not which connection carries it. Both checks guard something the other side has no equivalent for, which is why neither can sit behind the redirect. Tests assert the ordering from both ends: a write prompts and is then redirected, a denied write is never redirected, and a read is still redirected with no prompt at all. The two new cases fail against the previous ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
When precedence declines to route a call and lets it run locally, the result carries a one-line reason and a `precedence` marker so the decision is neither silent nor invisible to telemetry. Both were attached only to the successful return, so a call that then failed came back with neither. That is the wrong way round. An undetermined target usually means a misconfigured setup, so those calls are more likely than average to fail — the marker went missing in exactly the population it exists to measure, and the measured fail-open rate would read low in a way nothing would flag. Review named the three catch blocks. Auditing every exit after the guard found three more: `sql_explain`'s explicit failure return, and two early error returns in `schema_inspect`. `schema_inspect` now routes all of its failure exits through one local helper rather than three call sites, so overlooking one later is harder. The test reaches the undetermined verdict through the registry rather than dbt — a connection whose type no driver serves resolves as the default target and cannot be canonicalised — and asserts the marker on a real failure, not a mocked one. It fails against the unannotated version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
83d0636 to
e772cb7Compareralphstodomingo
commented
Aug 26, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:e772cb7da0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const type = canonicalType(target.type) | ||
| if (!type) { |
There was a problem hiding this comment.
When the dbt adapter exists but neither getAdapterType() nor the manifest yields a recognized type, target can still include a shadowed registry fallback, yet this branch returns before that fallback is checked. If the subsequent dbt execution throws or returns an unrecognized result shape, sql.execute then executes against the first local connection, bypassing workspace routing and its server-side audit path. Fresh evidence after the earlier fallback review is that the unknown-type early return still precedes the added target.fallback handling; evaluate the fallback before returning the undetermined verdict.
Useful? React with 👍 / 👎.
… type The undetermined verdict was returned before the fallback was examined, so a dbt target whose type cannot be identified failed open even when the registry connection behind it is served. `sql.execute` reaches that fallback whenever the dbt attempt yields nothing — an unrecognised result shape or a throw, not only an absent project — and an undetermined type is *more* likely to be the broken setup that yields nothing. The call then executed locally against exactly the connection precedence exists to route, which is the failure this design is for. This is the third defect in the order of these branches, so rather than reorder them again the decision is now a pure exported function with the order documented and its own suite. Reaching a dbt-sourced target through `check()` needs a real dbt project, which is why the order was never directly testable before and why each fix could reopen a sibling path. Branches, in order: the target's own type is served, then the fallback behind it, then an undetermined type runs locally and says so, then anything else runs. The new suite covers each, including an unserved dbt type with a served fallback, and the case that fails against the previous order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
ralphstodomingo
commented
Aug 26, 2026
Codex review logKept in one comment and edited in place, so the PR description stays short. Every finding below was confirmed against the code or the running system before anything was changed; each fix's test was checked against the pre-fix version and observed to fail. Round 1 — 2 findings, both real
Round 2 — 2 findings, both real
Round 3 — 1 finding, real
This corrected my own reasoning. I had put precedence ahead of the write prompt because approving a write that then redirects seemed to authorise something that never runs. The write does run — through the engine — and what the user authorises is the write, not which connection carries it. Routed in from the stacked PR — 1 finding, real
Round 4 — 1 finding, real
That was the third defect in the order of the same branches. The pattern was the real finding: each fix was correct and each reopened a sibling path, because reaching a dbt-sourced target through the live path needs a real dbt project, so the order was never directly testable. The decision is now a pure function with the order documented and its own suite. TallySix findings across four rounds plus one routed in. All six real, all six fixed, no false positives. |
ralphstodomingo
commented
Aug 26, 2026
@codex review |
Issue for this PR
Closes#1155
Type of change
What does this PR do?
Stacked on #1154 — review that first; this PR is the commits on top.
When a bound workspace's engine attaches, the model gets two ways to do the same thing: the native warehouse tools over local keychain connections, and the engine's MCP tools over the workspace's SaaS connection. Nothing chose between them, so the model picked whichever description read better — and that pick decided which credentials ran the query and whether it was audited. Engine calls are audited server-side; native ones aren't.
This adds a per-session decision: shadow only what materialised and is attributable to the bound workspace; anything undetermined runs locally and says why; nothing is silent. Why it works:
--integrations=localturns it off for a session.Two deliberate deviations: the guard needs a companion call to attach the fail-open notice, which a pre-execution check can't do; and I left an adjacent warehouse-type reporting bug alone, since fixing it changes a shipped telemetry field.
How did you verify your code works?
bun run typecheckand the workspace test suites pass; lint adds no new errors. 250 unit tests, including each decision rather than just the happy path.End-to-end against a live workspace, engine serving 12 tools:
--integrations=local→ plain listing, local executionNot exercised: the asymmetry case needs a workspace serving only some capabilities (unit-tested instead); the dbt default path needs a bound dbt project; de-materialisation is held until other testing here finishes.
Automated review: four rounds plus one routed in, six findings, all real, all fixed — details in the Codex review log.
GitGuardian flags a masked placeholder in a help-text snapshot that only moved columns; it is present unchanged on the base commit and is not a credential.
Screenshots / recordings
n/a — CLI change, no UI.
Checklist