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 30 commits into
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34260894 | Triggered | Generic CLI Secret | 04e5180 | 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 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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
bd7edfa
Compare
|
@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
5551619
Compare
|
@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 👍 / 👎.
83d0636 to
e772cb7
Compare
|
@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 👍 / 👎.
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. Round 5 — 1 finding, real
This is the counterpart to round 3. There, an agent that allows everything let a redirected write through without the confirmation the native path required. Here, an agent that denies everything unnamed made the redirect impossible to follow. The same redirect crossing the same permission boundary fails in opposite directions depending on the agent, which is the general lesson: a redirect is only sound if the destination is reachable under the caller's own rules. Round 6 — 1 finding, real
Round 5 fixed the routing and left the reporting behind it. Worth naming as its own class: when a decision gains a new condition, everything that describes that decision inherits the condition too, and the describing code is easy to forget because nothing it does is wrong on its own. Round 7 — 2 findings, both real
The first is the same class as the pilot-flag and reachability findings: something outside this module can change under it, and reading the convenient signal rather than the authoritative one leaves a window. Verified against the running system before and after, because a wrong reading would have disabled routing everywhere silently rather than failing loudly. Round 8 — 1 finding, real
Triage of this one took two false starts, both environmental rather than code: a shared fixture whose entry another session had disabled, and the shared engine binary crashing before it could print a version — which attach reports as "engine too old", a plausible product outcome rather than a broken toolchain. Two smokes were read as evidence against the fix before the environment was checked. The corrective is now a standing rule: before attributing a behaviour change to code, verify the fixture's entry and that the engine actually runs. Round 9 — 2 findings, both real, both P2
Both are the same class as round 6 — when a decision gains a condition, the code describing it inherits the condition — this time at capability granularity rather than warehouse type. Three findings across the run have now been that class, which is why every user-facing report resolves through the same predicate the routing uses. Round 10 — cleanNo findings on Round 11 — 2 findings, both real
The P1 is the within-turn drift recorded as a residual early in the design — it had been named but not closed, and review found the concrete consequence. Worth noting that a documented residual is not a handled one. Round 12 — 1 finding, real
Third finding in the same class — the code that describes a decision keeps asserting what used to be true. Twice that was a stale claim; here it was silence, which is the same failure wearing different clothes. Round 13 — 2 findings, 1 real, 1 refuted as stated
First finding in this run that is wrong as written. Worth recording as such: the fix it prompted is real, but it is not the fix that was asked for, and taking the suggestion at face value would have added a recheck to a path that cannot race while leaving the path that can untouched. The round-12 P2 is also the first regression introduced by a review fix rather than found in the original design. Round 14 — 1 finding, real
Fifth finding in the class that keeps recurring — the code that describes a decision keeps asserting what used to be true. Here the false assertion was internal: the map claimed the session had been told something it had not. Both tests were checked against a wrong implementation as well as the real one: the first fails under record-before-publish-and-never-retry, the second under clear-on-failure, which loses the routing state and silently suppresses a later stop. A fix this small has two plausible wrong shapes, and only one of them is the reported defect. Round 15 — 1 finding, real, and it is a regression in round 14's fix
Two rounds in a row on the same eight lines, and the second was caused by the first. Worth being plain about why: round 14 fixed the symptom it was handed — an undelivered line remembered as delivered — by restoring the previous value, without asking whether that value was itself trustworthy. It was not. The bug was never the rollback; it was that one map was being asked to hold two different facts, what the session has been told and what is currently being sent. Splitting them is what actually closes it, and it makes the out-of-order case fall out for free rather than needing its own guard. Both tests were checked against a wrong implementation as well as the real one: the overlapping-failure test fails under round 14's rollback, and the out-of-order test fails when the identity guard is dropped. Round 16 — 1 finding, real, and the half of round 13's P1 that was left open
This one was known: the scope limit was recorded when round 13's fix landed, as the part needing the verdict bound to the connector rather than the name. Review found it independently, which is the useful outcome — it means the gap was real rather than theoretical, and it arrived with the evidence needed to fix it narrowly. The fix stays inside the handler: no coupling to precedence, no session identity, no enforcement at the connector layer that Decision 9 rejected. Both tests were checked against a wrong implementation as well as the real one, in both directions: the replacement test fails with no guard at all, and the still-runs test fails when the guard compares config identity instead of canonical type — too strict turns every harmless rewrite into a spurious failure, which is the more likely mistake of the two. Round 17 — 1 finding, real, and it corrects the shape of rounds 14–16
This one is worth reading as a correction to the three rounds before it rather than a fourth bug. The announcement path had accumulated a guard per race: a rollback (14), an identity check (15), a scope pin (16). Each was a correct answer to the finding it was handed. But ordering is a property worth having outright, not a race worth guarding, and once publications are chained the overtaken-publication case cannot occur at all — so the two tests written to guard that race now assert the ordering instead. Fewer moving parts than before the round. The premise check mattered and I nearly skipped it: the previous round's overlapping-publication finding was accepted and fixed without first establishing that two refreshes could overlap. They cannot. The scenario is reachable anyway — through the un-awaited publish rather than through concurrent refreshes — so the fix stands, but it was reached by luck rather than by evidence, and the mechanism recorded in that round's entry was wrong. Round 18 — 1 finding, real
This is the first finding in four rounds on this path that is an invariant violation rather than a race: the suite already asserts these caches are bounded, and this was a way through that assertion. That is the distinction now being applied — a stated invariant that can be broken gets fixed; a marginal race on a path that already has an invariant gets declined with the evidence. The test asserts the behaviour rather than the size. A size bound alone passed for an incidental reason on the first attempt — the count it produced (283) did not correspond to the defect being described, which is the same trap as a test named for one case that exercises another. Re-derived, a resurrected record suppresses the announcement an evicted session should get when it comes back, and that assertion fails for exactly the right reason. Round 19 — 1 finding, real, and fixing it removed a condition
Reachability is thin: it needs a toast publication to outlive a full turn, and the state self-corrects on the next refresh, so the exposure is one turn of stale guidance rather than an indefinite one. Under the standing rule that would be a candidate to decline. It was fixed instead because the fix removes a condition rather than adding a guard — the two separate comparisons, one against the delivered line and one against the pending line, collapse into a single question with the same answer whenever nothing is pending. A finding that makes the code smaller is worth taking regardless of how marginal the trigger is. Round 20 — 1 finding, real; last round
Taken rather than declined on the same test as round 19: the fix removes the second source of truth instead of adding a guard for the case. It also finishes a change that had been applied to one of the two questions and not the other, which is its own kind of defect — a half-applied invariant reads as deliberate and is harder to spot than an absent one. Test consolidationSeven tests retired into three that state the property directly, after the last round. The attestation block asserted three refused attach outcomes individually alongside a loop over the whole union, so the samples were already subsumed. The loop now carries what they were adding — the refusal reason, which is what the inventory line and the tool descriptions render, so a refusal with the wrong reason is a wrong explanation shown to the user — and covers Every consolidated invariant was checked by breaking what the deleted tests used to catch: making Line count barely moves — 1,003 to 1,001 — because an invariant carries more scaffolding and more explanation than the sample it replaces. That is the honest number, and it is not the point: the gain is that a new outcome variant or a new reporting surface is covered by construction rather than by remembering to add a case. Source grew 695 to 748 across the nine rounds of fixes. Why the later rounds keep landing in the same two placesThirteen findings reduce to a handful of shapes, and two of them account for six:
Both are structural rather than careless, which is why they recurred: the code made them easy. That is also the argument for having kept going — the later rounds were not finding new instances of the first bug, they were finding new classes. TallyTwenty-six findings across twenty rounds plus one routed in — twenty-five real and fixed, one refuted as stated. One false positive in twenty-six, and it still surfaced a real defect next door. Two of the twenty were regressions introduced by review fixes rather than defects in the original design, both in the announcement path and both in consecutive rounds. That is the honest cost of fixing at the point a finding lands: round 12's fix created round 13's P2, and round 14's created round 15's. Each was caught by the next round, which is the argument for running rounds until one is clean rather than stopping when the findings look small. The yield never decayed. Rounds 7, 8 and 9 were each run to see whether the well had run dry, and each returned something real; round 10 came back clean and rounds 11–20 returned eleven more. Two patterns account for most of it — a guard placed where an early return skips a check that matters (3), and describing code that keeps asserting what used to be true (5, once by staying silent and once by lying to itself). Both are structural rather than careless, which is why they recurred: the code made them easy. That is the argument for having kept going — the later rounds were not finding new instances of the first bug, they were finding new classes. Two findings were a pair worth naming: a redirect crosses a permission boundary, and the same crossing fails in opposite directions for an agent that allows everything versus one that denies everything unnamed. And one lesson arrives late — round 13's P1 is a reminder that a confident finding still has to be traced to a suspension point before it is believed. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55b64a366f
ℹ️ 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".
| if (warehouse) { | ||
| const type = canonicalType(Registry.getConfig(warehouse)?.type) | ||
| if (!type) return RUN | ||
| const entry = precedence.shadowed.get(type)?.get(capability) | ||
| return entry ? redirectFor(capability, entry, precedence.workspaceName, warehouse) : RUN |
There was a problem hiding this comment.
In the default analyst agent, sql_execute, sql_explain, and schema_inspect are explicitly allowed, but unspecified tools are denied (agent.ts:249-268). This branch nevertheless redirects those allowed native calls to datamate_* keys, whose wrapper performs a separate permission check (session/prompt.ts:1866-1872) that therefore denies the call. Consequently, an analyst using any connection shadowed by a workspace can no longer execute the read-only warehouse operations it is intended to use; precedence must account for the destination permission or enforce the native read/write policy on the engine call itself.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53516d51ce
ℹ️ 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 served = CAPABILITIES.filter((c) => byCapability.has(c)).map((c) => c.replace(/^(sql|schema)_/, "")) | ||
| const local = CAPABILITIES.filter((c) => !byCapability.has(c)).map((c) => c.replace(/^(sql|schema)_/, "")) |
There was a problem hiding this comment.
When the active ruleset denies an engine key—as the default analyst agent does—check() deliberately runs that capability locally via reachable(), but this function still classifies every materialized capability as served by the workspace. Consequently, warehouse_list reports rows such as “execute via workspace” (and increments shadowed) even though the same agent's execution will stay local; filter each capability through the captured ruleset, matching the routing decision.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed319f1e42
ℹ️ 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 cfg = (await Config.get()) as { mcp?: Record<string, ExistingEntry | undefined> } | ||
| const entry = cfg.mcp?.[DATAMATE_KEY] | ||
| if (!entry) return null |
There was a problem hiding this comment.
When a running server's datamate command is edited from an unpinned or wrong-workspace entry to one pinned to the current binding, Config.get() returns the new argv while MCP can continue serving the already-connected process. This then attributes that old client to the new workspace and redirects warehouse queries to an engine that may still serve another teammate; derive attribution from the runtime client/attach outcome or reconnect it before enabling precedence.
Useful? React with 👍 / 👎.
|
|
||
| /** Per-session precedence, refreshed once per turn by the tool resolver and read | ||
| * (never recomputed) by tool bodies mid-turn. */ | ||
| const bySession = new Map<string, Precedence>() |
There was a problem hiding this comment.
In a long-running serve process that handles many distinct session IDs, every refresh permanently adds an entry to bySession, and sessions that emit an inventory line are also retained in announced; neither cache is evicted or cleared when a session is deleted. This makes memory grow with the lifetime session count and retains each session's merged permission ruleset, so cap or evict both collections as the neighboring workspace session caches do.
Useful? React with 👍 / 👎.
7fef85c to
19cb5e5
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19cb5e5f17
ℹ️ 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 outcome = precedenceInternals.attachOutcome | ||
| ? await precedenceInternals.attachOutcome() | ||
| : await ensure(sessionID) |
There was a problem hiding this comment.
When the initial attach exceeds the 15-second whenAttached budget, or a repairable attach retry is still in flight, awaiting ensure(sessionID) here blocks resolveTools on the uncapped attach task. This defeats the timeout in session/prompt.ts:1027-1030 and the non-blocking retry contract in engine-sync.ts:880-918, so a broken MCP connection can delay every affected prompt for its full connection timeout rather than letting the turn proceed without workspace tools; attest only an already-settled outcome or use the same bounded wait.
Useful? React with 👍 / 👎.
Two ways a stale or absent decision could route a call wrongly. Re-linking mid-session is supported, so the turn's snapshot can name a workspace the project has already left. A redirect naming it would send the query to that workspace's engine, with its credentials — the mis-routing this design exists to prevent, arriving through the passage of time rather than a bad read. The binding is now re-checked before any redirect; it is a local cache read, and only the path about to redirect pays for it. A session with no snapshot returned a bare "run". That is indistinguishable from a considered "this connection is not served", so an evicted entry — possible between tool resolution and the call — would let a shadowed connection execute locally with no indication. Unknown now runs locally and says so, like every other undetermined case here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5de539e645
ℹ️ 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 line = inventoryLine(result) | ||
| if (line && announced.get(sessionID) !== line) { | ||
| announced.set(sessionID, line) | ||
| void announce(line).catch(() => {}) |
There was a problem hiding this comment.
When an engine previously materialized routing tools and later removes all of them, inventoryLine() changes from the announced routing summary to an empty string for nothing-materialised, but this truthiness guard suppresses the update and leaves the user believing calls are still routed while they now run locally. Fresh evidence after the earlier inventory review is that comparing the last line fixes partial capability changes but still cannot announce the all-capabilities-removed transition; emit an explicit shadowing-off correction for this state.
Useful? React with 👍 / 👎.
Comparing the last announced inventory catches a capability appearing or disappearing, but not routing ending altogether: with nothing left to enumerate the line is empty, and an empty line was treated as nothing to say. So a session told its calls were routed through the workspace kept believing that after the engine stopped serving anything, while the calls ran locally. That transition is the one a user most needs. It is now stated explicitly, and only to a session that had previously been told otherwise — a session that never had routing is still told nothing, because for it nothing changed. Third finding in the same class: the code that describes a decision keeps asserting what used to be true. Twice that was a stale claim; this time it was silence, which is the same failure wearing different clothes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9xasTNbk2k1eqhpF1Hp5
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a017abac2
ℹ️ 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".
| if (warehouse) { | ||
| const type = canonicalType(Registry.getConfig(warehouse)?.type) |
There was a problem hiding this comment.
When a warehouse call overlaps a parallel warehouse_add that replaces the same name, this lookup can classify the old connection as unserved and return RUN, while the handler later calls Registry.get(warehouseName) and executes the newly replaced, workspace-shadowed connection locally. The analogous race affects warehouse-less calls when concurrent add/remove operations change the first registry entry after default-target resolution. Because tool calls can execute concurrently, this can bypass workspace routing and its audit path; bind the verdict to the connector/config actually selected for execution or repeat precedence at that selection point.
Useful? React with 👍 / 👎.
| const previous = announced.get(sessionID) | ||
| const line = inventoryLine(result) || (previous ? STOPPED_ROUTING : "") |
There was a problem hiding this comment.
When the first refresh is unattributed, inventoryLine records a nonempty “shadowing off” announcement; if a later refresh becomes attributed but materializes no mapped or permitted capability, its inventory is empty and this truthiness check emits “nothing is served ... any more.” No routing ever occurred in that sequence, which is common when the initial attach exceeds its bounded wait and eventually exposes only non-warehouse tools. Track whether the previous state actually had served capabilities instead of treating every prior announcement as active routing.
Useful? React with 👍 / 👎.
`refresh()` remembered the last announcement as a bare string, so any non-empty prior line counted as "was routing". An announcement can be non-routing — "shadowing off, the engine could not be attributed" is a statement about a failed attribution, not about served capabilities. A session that got that line first and later refreshed to an attributed engine exposing no warehouse tools was told routing had "stopped", when it had never started. Track the announcement alongside whether it described actual routing, and gate the stopped-routing line on that. Found by automated review.
`sql.execute` resolved its default connection twice: once when the caller's routing decision was made, and again after `tryExecuteViaDbt` had awaited. The connection registry is a process-wide mutable singleton, so a `warehouse.add`/`remove` landing during that await changed which connection the call fell back to — after the decision had been made against the old one. Read the registry once, before the await. The decided and executed connections are now the same by construction, and a call whose connection disappears mid-flight reports that connection by name instead of quietly running somewhere else. The dbt-first ordering is unchanged. Found by automated review.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73034ac942
ℹ️ 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".
| if (line && previous?.line !== line) { | ||
| announced.set(sessionID, { line, routed }) | ||
| void announce(line).catch(() => {}) |
There was a problem hiding this comment.
When publishing the toast fails—for example, while the event bridge is temporarily unavailable—announce() catches the error, but this code has already recorded the line as delivered. Every later refresh with the same inventory then skips it because previous.line === line, so the session never receives the routing disclosure despite the failure being recoverable; update announced only after a successful publish, or retain a pending state that allows retries.
Useful? React with 👍 / 👎.
The inventory line was recorded as said before it was published, and `announce` swallowed its own failure, so the call site could not tell a delivered line from a dropped one. A toast lost to a briefly unavailable event bridge was therefore remembered as delivered — and every later turn with the same inventory skipped it as unchanged, so the session was never told what its calls were doing. `announce` now reports whether the line reached the session, and a failed delivery restores the previous record so the next turn retries. The record is still written before publishing, so a second refresh in the same window does not send the line twice; restoring the previous value rather than clearing it keeps whether the session had been routing, which a later "routing stopped" depends on. Found by automated review.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3468a36da1
ℹ️ 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".
| if (announced.get(sessionID)?.line !== line) return | ||
| if (previous) announced.set(sessionID, previous) | ||
| else announced.delete(sessionID) |
There was a problem hiding this comment.
When two consecutive refresh() calls publish different inventories before either publication settles, and both publications fail, the first failure leaves the newer optimistic record untouched while the second failure restores previous, even though that earlier line was never delivered either. A later refresh returning to that inventory then skips it as already announced, permanently losing the routing disclosure. Fresh evidence beyond the earlier delivery-failure review is this overlapping-failure rollback path; track confirmed delivery separately rather than restoring an optimistic predecessor.
Useful? React with 👍 / 👎.
The previous fix rolled a failed publication back to the record it had replaced — but that record was itself optimistic. Precedence is re-derived every turn, so two refreshes can publish different lines before either settles; when both failed, the second's rollback reinstated the first's undelivered line as delivered, and a later turn returning to that inventory skipped it as already said. The disclosure was lost for good. Confirmed deliveries and in-flight attempts are now separate. Nothing reaches the delivered map until the line actually arrives, so a failure leaves the session's known state untouched and the next turn retries. The in-flight record exists only to stop the same line being sent twice in one window, and each attempt is its own object, so identity settles which publication is current — one that finishes after a newer one started finds its attempt gone and records nothing. Found by automated review.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90e4877933
ℹ️ 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".
| "No warehouse configured. Use warehouse.add, set ALTIMATE_CODE_CONN_* env vars, or configure a dbt profile.", | ||
| ) | ||
| } | ||
| const connector = await Registry.get(fallbackName) |
There was a problem hiding this comment.
When a concurrent warehouse_add replaces the same connection name after precedence checks its old, unshadowed config, this lookup resolves the replacement config and can execute a now-workspace-shadowed connection locally, bypassing workspace routing and auditing. Fresh evidence after the prior registry-race finding is that the attempted fix pins only the fallbackName string; Registry.get(fallbackName) still consults the mutable registry after the dbt await. Bind the actual selected config/connector or repeat precedence against the connector selected here.
Useful? React with 👍 / 👎.
Pinning the name closed the case where the identity of the default connection changed mid-flight, but not a same-name replacement: the name can be re-added against a different warehouse while the call is suspended in the dbt attempt, and `Registry.get(name)` still consults the mutable registry afterwards. The call would then execute under a routing decision computed for the connection that name used to mean. The decision is a function of the connection's canonical type, so pinning the type pins the decision. A replacement whose type differs is refused with a message naming the connection, rather than run under a verdict that never covered it; a rewrite that keeps the type — including one that only changes an alias, `postgres` to `postgresql` — cannot change where the call is routed and still runs. Found by automated review.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebfc668499
ℹ️ 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".
| void announce(line).then((delivered) => { | ||
| if (publishing.get(sessionID) !== attempt) return | ||
| publishing.delete(sessionID) | ||
| if (delivered) announced.set(sessionID, attempt) |
There was a problem hiding this comment.
When two refresh() calls publish different inventories concurrently and both succeed out of order, the newer line can be delivered first and recorded in announced, then the older publication can arrive last but be ignored by this identity check. The user is left seeing the stale inventory, while a later refresh for the newer state is suppressed because it is already recorded as delivered; serialize publications per session or re-publish the current inventory after an overtaken delivery settles.
Useful? React with 👍 / 👎.
Refreshes are serialized by the prompt loop, but publishing is deliberately not awaited — a toast must never be able to stall a turn — so two lines could be in flight at once. Landing out of order left the stale line on screen while the newer one was recorded as the session's state, and the identity guard that decided which publication was current only made the older one silent rather than correct. Publications are now chained per session, so arrival order matches decision order and the last line a session sees is the newest one. Ordering is a property worth having outright rather than a race worth guarding: with the chain in place an overtaken publication cannot occur, so the two tests that guarded that race now assert the ordering itself. Found by automated review.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf9f35bb02
ℹ️ 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 delivered = await announce(line) | ||
| if (publishing.get(sessionID) === attempt) publishing.delete(sessionID) | ||
| if (delivered) announced.set(sessionID, attempt) |
There was a problem hiding this comment.
When a toast publication is still pending while more than 256 sessions cause this session to be evicted, remember() removes its announced entry, but the pending callback later executes this unconditional announced.set() and resurrects it. Repeating this with slow event delivery leaves entries whose session IDs are no longer present in bySession, so subsequent cap enforcement can never select them and announced again grows with lifetime session count. Fresh evidence beyond the earlier cache-bound review is this post-eviction asynchronous write; record delivery only if the session/attempt is still tracked, or apply eviction again after completion.
Useful? React with 👍 / 👎.
Publishing is not awaited, so a line can still be in flight when its session falls out of the cache. Writing the delivery back afterwards recreated an entry for a session eviction had already removed — and eviction only ever walks `bySession`, so nothing could reclaim it. The announcement cache would then grow with the lifetime session count, which is the bound the eviction exists to hold. The write-back now happens only while the session is still tracked. The test asserts the behaviour rather than the size alone: a resurrected record would suppress the announcement an evicted session gets when it is derived again, which is the visible consequence and a sharper signal than the count. Found by automated review.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: babb2c5c95
ℹ️ 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 routed = result.enabled && current !== "" | ||
| // Only a session that was actually routing can be told routing has stopped. | ||
| const line = current || (previous?.routed ? STOPPED_ROUTING : "") | ||
| if (line && previous?.line !== line && publishing.get(sessionID)?.line !== line) { |
There was a problem hiding this comment.
When consecutive turns change a session from an already-delivered inventory A to B and back to A before B's asynchronous publication completes, this condition suppresses the corrective A because previous.line is still A. The serialized queue then delivers B last and records it as announced even though the current routing state is A, leaving the user with stale routing guidance until another turn happens to refresh it. Fresh evidence beyond the earlier publication-order finding is that the new queue orders delivery but the deduplication still compares against the last delivered line rather than the last queued target; compare against the pending target/current state or enqueue a correction after B settles.
Useful? React with 👍 / 👎.
Inventory can return to what was already announced while a different line is still being published. Comparing only against the delivered line suppressed that correction, and the queue then delivered the stale line last — leaving the session looking at routing guidance that no longer matched where its calls went. The comparison is now against the newest line the session is committed to saying: the one still in flight if there is one, otherwise the one it has actually been told. That replaces the two separate guards — one against the delivered line, one against the pending line — with a single question, and it is the same answer as before whenever nothing is pending. Found by automated review.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d82419a56
ℹ️ 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 current = inventoryLine(result) | ||
| const routed = result.enabled && current !== "" | ||
| // Only a session that was actually routing can be told routing has stopped. | ||
| const line = current || (previous?.routed ? STOPPED_ROUTING : "") |
There was a problem hiding this comment.
When the first routing announcement is still pending and a subsequent refresh removes all materialized capabilities, previous is still unset, so this produces an empty line and queues no correction. The original routing announcement can then arrive after routing has already stopped, leaving the user with stale guidance until another turn refreshes the session. Fresh evidence beyond the earlier empty-inventory and queue findings is that the stop decision consults only the last delivered record rather than publishing.get(sessionID)?.routed; pending routed announcements must also trigger a queued stop notice.
Useful? React with 👍 / 👎.
The previous change made the dedupe compare against the newest line a session is committed to saying, but left the stop decision reading the delivered record alone. With a first routing announcement still in flight, a refresh that served nothing therefore queued no correction — and the routing line arrived after routing had already stopped. Both questions now consult the same record: the announcement still being published if there is one, otherwise the one the session has been told. That removes the second source of truth rather than adding a guard for the case, and it finishes a change that was only half applied. Found by automated review.
Seven tests retired into three that state the property directly. The attestation block asserted three refused attach outcomes individually alongside a loop over the whole union; the samples were subsumed. The loop now carries what they were adding — the refusal reason, which is what the inventory line and tool descriptions render, so a refusal with the wrong reason is a wrong explanation shown to the user — and covers `undefined`, which it could not reach before. The two qualifying outcomes merge into one loop over the other half of the same allowlist. The four reporting surfaces were asserted one test each against the same unreachable-caller ruleset. The failure they guard is precisely that these drift apart, so they now assert together that every surface agrees with the routing decision. Line count barely moves: an invariant carries more scaffolding than the sample it replaces. The gain is that a new outcome variant or a new reporting surface is covered by construction rather than by remembering to add a case.
1 participant
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