Skip to content

feat(workspace): route warehouse tools through the bound workspace's engine - #1168

Draft
ralphstodomingo wants to merge 2 commits into
feat/workspace-engine-overlayfrom
feat/workspace-precedence-v2
Draft

feat(workspace): route warehouse tools through the bound workspace's engine#1168
ralphstodomingo wants to merge 2 commits into
feat/workspace-engine-overlayfrom
feat/workspace-precedence-v2

Conversation

@ralphstodomingo

@ralphstodomingo ralphstodomingo commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #1155

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Stacked on #1167 — review that first; this PR is the commit on top. It is the precedence change from #1156 restacked onto the overlay attach; the mechanism is unchanged, the attach seam it reads is now the overlay's.

When a bound workspace's engine is attached, 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 are not).

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.

  • Materialised, not declared — it reads the engine tool keys actually present, so a declared-but-absent integration shadows nothing.
  • Attributed — routing engages only when the attach outcome is attached (the overlay's own pinned engine, connected at this turn boundary) and the configured entry's pin names the bound workspace. Any other outcome, or none, fails open with a reason.
  • Per capability, not per warehouse type — Snowflake has execute, explain and table stats; BigQuery and Postgres have execute only. Keying on the type would send an explain on BigQuery to a tool that does not exist, and there is deliberately no fallback.
  • Calls naming no warehouse are judged against the target they would really reach, mirroring each handler's own resolution, including the registry connection dbt falls back to.
  • The redirect names the exact engine tool, executes nothing, is marked in metadata so telemetry can tell it from an execution, and is only offered to a caller whose agent may call that tool.
  • It runs after every native safety check — the hard deny on destructive statements and the write confirmation both guard things the engine side has no equivalent for.
  • --integrations=local turns 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 cannot do; and an adjacent warehouse-type reporting bug is left alone, since fixing it changes a shipped telemetry field.

How did you verify your code works?

bun run typecheck clean; precedence, default-target, guard-order and workspace suites pass (161 tests across the six directly affected files; the tool/native/prompt suites green). The union test now asserts the allowlist is exactly attached over the whole outcome union, so a future outcome kind refuses routing by default.

End-to-end rows from #1156 (shadow marking, redirect with a proven no-local-execution control, DuckDB control, default target, model following the redirect unprompted, write confirmation, escape hatch) are re-run on this stack and recorded in the review-log comment below before this leaves draft.

GitGuardian flags a masked placeholder (eight literal asterisks) 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

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Summary by cubic

Previously, attaching a bound workspace's engine left the model with two equally-visible ways to run each warehouse operation — native tools over the local keychain connection and the engine's MCP tools over the SaaS connection — and nothing chose between them, so the better-written description decided which credentials ran the query and whether it was audited server-side. A per-session precedence decision now routes each operation to the engine's tool only when that tool is materialised and attach proves the engine owns the bound workspace; anything undetermined runs locally with a stated reason, and --integrations=local disables routing for a session. Closes #1155.

Routing rules

  • Shadowing requires both that the engine tool is materialised (a declared-but-absent integration shadows nothing) and that the engine is attributed: attach outcome attached plus a configured pin naming the bound workspace; any other outcome fails open with a reason.
  • Matching is per capability, not per warehouse type: Snowflake gets execute, explain, and table stats; BigQuery and Postgres get execute only.
  • Calls naming no warehouse are judged against the target they would really reach, mirroring each handler's own resolution, including dbt's registry fallback.
  • The redirect names the exact engine tool, executes nothing, is marked in metadata for telemetry, and appears only to callers whose agent may call that tool.
  • The guard runs after the native safety checks, so the hard deny on destructive statements and the write confirmation are never bypassed.
  • An adjacent warehouse-type reporting bug is left alone because fixing it would change a shipped telemetry field.

Written for commit b8daef2. Summary will update on new commits.

Review in cubic

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gitguardian

gitguardian Bot commented Aug 27, 2026

Copy link
Copy Markdown

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
34260894 Triggered Generic CLI Secret 83c5075 packages/opencode/test/cli/help/snapshots/help-snapshots.test.ts.snap View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. 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


🦉 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.

@ralphstodomingo

ralphstodomingo commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Review log

Stacked on #1167 — its review-log comment carries the attach contract (claims, disclosed residuals, review policy). This PR is reviewed the same way: a finding is a reproducible trace that violates a claim; rounds are capped.

Claims (in addition to #1167's)

  1. Routing engages only when settledOutcome is attached and the configured datamate entry's pin names the bound workspace; any other outcome, or none, fails open with a stated reason.
  2. Redirects are offered per materialised capability, never per warehouse type; a capability the engine did not materialise runs locally.
  3. A redirect runs after the hard deny on destructive statements and after the write confirmation; it never executes anything itself; it is only offered to a caller whose agent may call the destination tool.
  4. --integrations=local turns routing off for the session.

Disclosed residuals (in addition to #1167's)

  • Routing is derived per turn; within a turn it does not re-derive.
  • Beyond 256 tracked sessions the oldest is dropped and re-derived on its next call.
  • A redirect can name a client another session removed; that is a tool-layer failure, not a query sent to the wrong warehouse.

End-to-end rows (from #1156, re-run on this stack 2026-08-28 against the demo workspace with a Snowflake connection; engine 0.7.0)

# Scenario Result
1 shadow marking per capability; local DuckDB rows unmarked warehouse_list: the Snowflake connection reads "execute/explain/inspect via workspace …", the DuckDB rows "local"
2 redirect, nothing executed — positive control in query history sql_execute on the served connection returned the redirect (redirected: true, redirect_to: datamate_snowflake_execute_database_query, precedence: shadowed). Two markers: a run that followed the redirect through the engine tool shows N=1 in information_schema.query_history (probe excluded); a run that stopped at the redirect shows no row for its marker
3 DuckDB control runs locally ✓ no redirect, executed locally (the "(0 rows)" rendering for a constant select is pre-existing: identical on the attach-only build)
4 asymmetry: an execute-only served type keeps explain local ✓ with a postgresql integration added to the workspace (engine tools: execute + list only) and a local Postgres connection: sql_explain on it ran locally (real EXPLAIN plan, no redirect), sql_execute on it redirected to datamate_postgresql_execute_database_query (precedence: shadowed); warehouse_list marked it "execute via workspace …; explain/inspect local" per capability. Scaffolding (server connection, workspace integration, container) removed afterwards
5 dbt default path blocked by a pre-existing fault on main, unit-covered: ensureDbtAdapter can never initialise under Bun — python-bridge@1.1.0 runs bluebird.promisifyAll(child_process) at module load and Bun throws TypeError: Cannot access invalid private field (evaluating 'this.#stdin') (oven-sh/bun#18693 class; reproduced on Bun 1.3.9/1.3.10/1.3.14/1.4.0, loads fine under Node 22). Verified on an untouched main checkout with a real dbt project (jaffle-shop-core), so the dbt-first sql_execute path has not engaged since it landed (#221); packages/dbt-tools/src/index.ts::diagnose() already names this incompatibility. Not introduced by this stack; the compiled binary embeds the same runtime. decideForTarget / resolveDefaultTarget remain covered by default-target.test.ts.
6 no warehouse named → default target resolved and redirected ✓ with the Snowflake connection first in the connection file: redirect naming the engine tool, "Not run locally"; file restored byte-identical afterwards
7 model follows the redirect unprompted ✓ "Query Snowflake …" in plain words → datamate_snowflake_list_database_connections then datamate_snowflake_execute_database_query, no native call
8 INSERT on the served connection stops at the write gate ✓ "rejected permission to use this specific tool call" (headless auto-rejects the ask); no redirect
9 --integrations=local → plain listing, local execution ✓ listing without the served-by note; sql_execute on the Snowflake connection executed locally, not redirected

Rounds

(none yet)

Codex rounds

round head findings outcome
1 f68a41a8c 0 — "Didn't find any major issues" no change
f68a41a8c893ff8f93 no review round: the three describeNativeTool/describeEngineTool hooks in prompt.ts/tools.ts used the single-line // altimate_change — marker form, which the strict marker guard that runs on pushes to main does not recognise (bun run script/upstream/analyze.ts --markers --base origin/main --strict flagged them; PR runs use the PR base and are non-strict) wrapped in start/end blocks; tools.ts re-formatted (it is prettier-clean on main). No behaviour change; strict guard now passes for the whole stack against main.

CI note — GitGuardian is red on this PR and that is a false positive. The "1 secret" is the literal placeholder github_pat_******** in the --token help text captured by packages/opencode/test/cli/help/__snapshots__/help-snapshots.test.ts.snap (a snapshot this PR regenerates). It was red on f68a41a8c before the marker-only commit too. Nothing to remediate; every other check is green.

@ralphstodomingo

Copy link
Copy Markdown
Contributor Author

@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: f68a41a8c1

ℹ️ 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".

ralphstodomingo added 2 commits August 28, 2026 08:47
…engine

Shadow a native warehouse capability only when the bound workspace's engine
materialised the matching tool and attach attests the engine is its own
(outcome `attached` plus the configured pin); redirect to the exact engine
tool after the native safety checks; fail open with a reason otherwise.
`--integrations=local` turns it off. Restacked onto the derived-overlay
attach; the allowlist is exactly `attached`.
…ormat tools.ts

The two `describeNativeTool` call sites used the single-line marker form, which
the strict marker guard that runs on pushes to main does not recognise. No
behaviour change.
@ralphstodomingo
ralphstodomingo force-pushed the feat/workspace-precedence-v2 branch from 893ff8f to b8daef2 Compare August 28, 2026 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant