feat(workspace): tell the model what the bound workspace serves - #1182
feat(workspace): tell the model what the bound workspace serves#1182suryaiyer95 wants to merge 4 commits into
Conversation
|
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 |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2575d46 to
40c57a8
Compare
c5f0368 to
9e8e1fb
Compare
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
8f47bcd to
eb798cb
Compare
|
Restacked this branch onto the current #1169 head ( |
3aca807 to
1ac24db
Compare
ralphstodomingo
left a comment
ralphstodomingo
left a comment
There was a problem hiding this comment.
Review — 1ac24dbd9 (restacked on #1169 eb798cb57)
Scope, corrected: the 48-file compare was stale from the base-branch rewrites. The net change is six files, +540/−46: awareness.ts (new), precedence.ts (servedInventory added; inventoryLine / warehouseListNote routed through it — behaviour-preserving, same iteration order and servedFor filter as before), session/prompt.ts (+13, the per-step render into the system array), awareness.test.ts (new), a shared precedence-fixture.ts, and precedence.test.ts. Nothing else in the stack's files is touched here. No scope creep.
What holds up: the section is a projection of the same Precedence snapshot check() reads, filtered through the same servedFor → reachable gate — so the analyst case renders nothing and a served row is exactly a row check() would redirect; "agrees with check() on the same snapshot" is the right property test for that. Capability-scoped rows match the engine's real asymmetry (BigQuery execute-only). The exhaustive Record tables did their job during the restack (they caught two new disabledReasons at compile time). The escape hatch speaking, and only it, is correct. Ordering inside a step is right: tool resolution (which runs refresh) precedes system assembly in the same loop iteration, so section and guard read one snapshot. Smoke on the compiled UAT bundle from this head: warehouse_list + the section produced the served/local split with the exact engine keys on the first turn, no probe call.
No blockers. Two minors and some nits inline. One recorded residual, inherited from the stack: per-step re-derivation under a concurrent re-link (#1168's R-P1) — the section flips to the new engine's keys while the turn's catalog stays pinned; fails closed (pinned wrapper → closed client), lands with the lease work. Worth recording on this PR's log rather than fixing here.
PR body: no Jira keys or internal paths (clean). Two asks: (a) state the safety case as numbered claims + residuals in the stack's style — it is already four claims in prose (byte-identical prompt when not routing; section ⊆ check() redirects; converse never dropped; cap) plus R1 = the residual above — so the codex rounds have something to review against; (b) refresh the stale lines after the refactors and restack: "118 insertions, 0 deletions" (now +540/−46), "purely additive" (inventoryLine / warehouseListNote were rewritten), and "rebased onto 40c57a890" (now eb798cb57, head 1ac24dbd9).
Ralph is running the UAT bundle built from this head; any behavioural findings will follow as a separate comment.
| }) | ||
|
|
||
| describe("the size ceiling", () => { | ||
| test("stays under the cap and degrades by dropping whole types", async () => { |
There was a problem hiding this comment.
MINOR (test) — this cannot fail. The four-type fixture renders to about 1,180 characters against a 2,000 cap: four type lines, no "…further" line, so the truncation loop and the omitted-types line are never executed and both assertions pass trivially. INTEGRATION_TYPE has four ids, so synthetic keys cannot force more types either. Make the cap injectable (a parameter on assemble/systemSection, or a test seam) and assert the dropped-type count and the "…and N further" line directly.
| "", | ||
| ...lines, | ||
| ...(omitted > 0 | ||
| ? [`- …and ${omitted} further connection type${omitted === 1 ? "" : "s"} served by this workspace.`] |
There was a problem hiding this comment.
MINOR (wording) — a truncated section contradicts itself. This line says "N further connection types served by this workspace" while the converse paragraph below says every other type uses the local tools and forbids datamate_* for types not listed. Under truncation the model is told an unnamed type is served and forbidden from using its engine tool. Fail-safe (the local call meets the redirect backstop) and unreachable today (see the cap test), but cheap to make true: drop per-capability key detail, not whole types — types are ~10 characters, so every served type can stay named and the converse holds.
| * is a compile error here rather than silently rendering nothing. Only the escape | ||
| * hatch speaks: the others mean "no routing to describe", and the toast layer already | ||
| * tells the human why. */ | ||
| const DISABLED_COPY: Record<NonNullable<Precedence["disabledReason"]>, string> = { |
There was a problem hiding this comment.
Design question, not blocking. "" for unattributed / binding-unreadable / derive-failed is the right entry under the stated rule (only the hatch speaks) — and it is what the restack adaptation kept. But the hatch comment's own argument applies to these states too: engine tools can still be in the catalog (attach connected the entry; precedence merely refused attribution), and nothing steers the model away from calling datamate_* directly. An "unknown" copy ("workspace routing could not be established this turn; use the local tools") would close the same gap for the same reason. Your call.
| const short = (c: Capability) => c.replace(/^(sql|schema)_/, "") | ||
| const served = servedCaps.map(short) | ||
| const local = CAPABILITIES.filter((c) => !servedCaps.includes(c)).map(short) | ||
| // altimate_change - same projection as the toast and the prompt section. |
There was a problem hiding this comment.
NIT — warehouseListNote now computes the whole servedInventory per row, and warehouseListNotes calls it per warehouse. Trivial at four types; compute once in warehouseListNotes if this is revisited. Also cosmetic: the section names the canonical driver type (postgres) while the connection is configured as postgresql and the key reads datamate_postgresql_….
eb798cb to
3a873cf
Compare
|
Heads-up: this PR's base branch |
|
Restacked onto the current |
1ac24db to
12be6c8
Compare
9324653 to
d448ff3
Compare
returns a redirect naming the engine tool. It did not make the model *pick*
the engine first, so every session pays a wasted turn learning the rule.
The only model-visible steering today is a sentence `describeNativeTool`
appends to a description whose first line already matches user intent
("Execute SQL against a connected data warehouse."), and it never names the
engine key — so even an obedient model cannot comply without a probe call.
The routing table the model needs already exists as `inventoryLine`, and goes
only to a TUI toast. Nothing in the system prompt mentions the workspace.
`session/system.ts:129-142` records this repo's own benchmark finding: a
lazily-described capability fired in "<1% of tool calls", and guidance placed
at the END of a section was "treated as background reference rather than
binding directive" while the same content placed FIRST was applied. The
precedence suffix is exactly that shape.
So state it in the system prompt instead, per turn, naming the exact engine
keys, and say the converse explicitly so unserved types keep running locally.
Purely additive by construction — 118 insertions, 0 deletions:
- `awareness.ts` renders a string and nothing else. It does not touch
`check()`, `derive()`, `redirectFor()` or any tool body, so which calls are
shadowed and what a shadowed call returns are unchanged.
- It returns "" in every state except a bound, attributed workspace with
materialised engine tools. A session without a workspace assembles a
byte-identical system prompt to before this commit.
- `servedInventory()` is a projection over the snapshot the guard already
uses, filtered through the same `servedFor`/`reachable`, so the section can
never advertise a routing `check()` would not perform, nor one the caller's
agent is forbidden to follow.
- No tool descriptions change, so no existing description assertions move.
Deliberate details:
- Per capability, not per warehouse type. BigQuery serves execute only, so
its line says explain and inspect stay on the local tools — claiming the
type would steer the model off the only tools that work there.
- The converse paragraph is never dropped under the char cap; without it the
section reads as "prefer the workspace for everything", which is the
over-steering failure this most needs to avoid.
- The escape hatch speaks rather than falling silent: engine tools can still
materialise with `--integrations=local` on, so silence would leave the
model free to use tools it can see and should not.
- An agent denied the engine keys renders no section, matching what
precedence actually does for it.
Verification: `bun run typecheck` clean. 19 new tests (14 awareness, 5
precedence), all passing. Full `test/altimate/` sweep goes 4420 -> 4439 pass
with the same 3 pre-existing failures present on the untouched base commit
(cross-file pollution in `default-target.test.ts`, which passes 12/12 in
isolation on both).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cleanup pass over the tool-awareness change. No behaviour change: the full `test/altimate/` sweep is 4454 pass / 0 fail before and after. - `servedInventory()` returns a grouped `ServedType[]` instead of a flat list the caller immediately regrouped. `precedence.shadowed` is already grouped, so the flatten/regroup round trip was undoing work nothing asked for. This also drops two provably-dead branches (`shadowed.get(type)` after iterating `shadowed.keys()`; `byCapability.get(capability)` after `servedFor` already filtered on entry presence), and halves the per-type `PermissionNext.evaluate` work by deriving `local` from the same `servedFor` pass. - Deleted `localCapabilitiesFor()`. It was a third copy of `CAPABILITIES.filter((c) => !served.includes(c))`, already inline in `inventoryLine` and `warehouseListNote`; `local` now rides on the projection. - `ALL_LOCAL_TOOLS` is derived rather than hand-copied, and `CAPABILITY_COPY`'s `localTool` field is gone — every value equalled its key, because the `Capability` union IS the native tool id (`describeNativeTool` already relies on that identity). Output is byte-identical. - The disabled-state branch is a `Record` over `disabledReason`, not a ternary. The old test comment claimed adding a reason "fails to compile"; that was false — a `Union[]` annotation accepts a short list. A `Record` is genuinely exhaustiveness-checked: adding a sixth reason now raises TS2741 in both `awareness.ts` and its test, verified by doing it. - Corrected the module and call-site docs: `systemSection` runs once per STEP (inside the `while (true)` prompt loop), not once per turn. Noted why it is deliberately not memoised — a cached section outliving its snapshot would advertise routing that no longer holds. - Extracted `test/altimate/workspace/precedence-fixture.ts`. `bindTo`'s `attachOutcome` shape is coupled to the attach module's SERVING allowlist, so two hand-maintained copies break differently when it changes. Both suites now share the tool maps, warehouse configs and analyst ruleset; the awareness suite picks up the engine-less duckdb/redshift connections it had been omitting. - Dropped comments that restated an adjacent doc, and replaced rationale copied verbatim into tests with pointers to the source of truth. Not done, deliberately: `prompt.ts` and `precedence.test.ts` fail `prettier`, but they already fail on the untouched base, so reformatting them would add unrelated churn. The duplicated `short()` helper and the two `servedFor` sweeps in `describeNativeTool` are pre-existing and outside this diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… projection Closes the duplication the previous commit opened. `servedInventory` carried a third copy of `CAPABILITIES.filter((c) => !served.includes(c))`, alongside the two already inline in `inventoryLine` and `warehouseListNote`; both now consume the projection instead, leaving one copy. The twice-declared `short()` helper is hoisted to module scope beside `CAPABILITIES`. The payoff is not line count: the toast, the `warehouse_list` row note and the model-facing prompt section now derive "what is served" from one function, so they cannot disagree about which capabilities a workspace serves — previously three independent walks of the shadow table. Kept deliberately separate: `short()` (terse — `execute/explain/inspect` for a one-line toast) and the section's `CAPABILITY_LABEL` (prose — "table stats / schema inspection"). Two audiences, and the section's whole thesis is that vague phrasing is what failed to steer the model. Behaviour is unchanged and the tests prove it byte-for-byte: the suite asserts exact output including "snowflake: execute/explain/inspect via workspace analytics", "bigquery: execute via workspace analytics" and "explain/inspect stay local". Verification: typecheck clean. Full `test/altimate/` sweep 4434 -> 4453 pass with the same single pre-existing failure on both (`tracing-rename-race` M3-natural, which also fails in isolation on the untouched base 40c57a8). Not done: `test/altimate/precedence-guard-order.test.ts` keeps its own SNOWFLAKE_TOOLS. It is a genuinely different fixture — three keys rather than four, and a different binding — so folding it into the shared module would change what that suite covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… disabled reasons render nothing `binding-unreadable` and `derive-failed` arrived below this PR. Both mean the routing decision is unknown and the tool result already states why, so, like `unattributed`, the system section says nothing for them.
|
Restacked again — #1169 was rebased onto |
12be6c8 to
4a1c7d5
Compare
2 participants
Issue for this PR
Closes the remaining integrations gap called out after #1167/#1168/#1169 landed: the harness being unaware of when to reach for workspace tools.
Type of change
What does this PR do?
Stacked on #1169 — review that first; this PR is the commit on top.
#1168 made the engine win: a shadowed warehouse call executes nothing and returns a redirect naming the engine tool. It did not make the model pick the engine first, so every session pays a wasted turn learning the rule, and re-learns it next session.
The only model-visible steering today is a sentence
describeNativeToolappends to a description whose first line already matches user intent ("Execute SQL against a connected data warehouse.",altimate/tools/sql-execute.ts:21), and it never names the engine key — so even an obedient model cannot comply without a probe call. The routing table the model actually needs already exists asinventoryLine(precedence.ts:707-737) and goes only to a TUI toast. The system prompt says nothing about the workspace.This states it in the system prompt instead, per turn, naming the exact engine keys, with the converse said explicitly so unserved types keep running locally.
Rendered example (illustrative — every type, capability and key is derived from what materialised this turn):
Why the system prompt and not a better tool description:
session/system.ts:129-142records, from this repo's own benchmark trace analysis, that a lazily-described capability fired in "<1% of tool calls", and that guidance placed at the END of a section was "treated as background reference rather than binding directive" while the same content placed FIRST was applied. #1168's suffix is exactly that shape. This is the documented cure, not a guess.Why this is safe to land
Purely additive by construction — 118 insertions, 0 deletions.
awareness.tsrenders a string and nothing else. It does not touchcheck(),derive(),redirectFor()or any tool body, so which calls get shadowed and what a shadowed call returns are unchanged. If the section is wrong, the model reads a misleading sentence — it does not run a query against the wrong credentials.""in every state except a bound, attributed workspace with materialised engine tools.pilot-off,unbound,unattributed,nothing-materialisedand no-snapshot all render nothing, so a session without a workspace assembles a byte-identical system prompt to before this commit. That is asserted directly, not argued (the regression guarddescribe block).servedInventory()is a projection over the snapshot the guard already uses, filtered through the sameservedFor/reachable— so the section can never advertise a routingcheck()would not perform, nor one the caller's agent is forbidden to follow. There is a test that walks the inventory and assertscheck()redirects to each key it names.Deliberate details
--integrations=localon —deriverefuses before it looks at them, but MCP connects the configured entry regardless — so silence would leave the model free to use tools it can see and should not.UNIFIED_INJECTION_BUDGET— a routing directive must not compete with memory for space.How did you verify your code works?
bun run typecheckclean.awareness.test.ts, 5 inprecedence.test.ts.test/altimate/sweep on the current base (40c57a890): 4454 pass, 0 fail.089bb6223) the sweep went 4420 → 4439 pass with the same 3 failures present on the untouched base commit — pre-existing cross-file pollution indefault-target.test.ts, which passed 12/12 in isolation on both. Verified by running the sweep on a clean worktree of that base rather than inferred; those failures no longer occur after the restack.Not yet done: the before/after model-behaviour measurement (does the model call the engine tool first, and are redirects consumed → 0). Left out of this PR deliberately so the additive change can land on its own; the metrics come from trace JSON that already ships (
tracing.ts:907-930,metadata.redirected).Rebased onto the restacked #1169
Originally opened against
089bb6223. #1169 was force-pushed to40c57a890(the restack onto the newer #1167/#1168), so this has been rebased onto that. It applied with no conflicts — the changes are additive and touch none of the rewritten regions.Worth noting: the restacked base now carries
fix: undetermined outcomes always carry a stated reason in the result, theprecedence.tshardening (fail-opencheck/checkUnsafesplit, refusing attribution whenConfig.invalidate()fails) that the pre-restack #1169 was missing. Re-verified on the new base: typecheck clean, my two suites 111/111, and the fulltest/altimate/sweep is now 4454 pass / 0 fail — the 3 pre-existingdefault-target.test.tsfailures seen on the old base are gone.Follow-ups deliberately not in this PR
INTEGRATION_TYPE→ key-pattern discovery. Today that map is hardcoded to four ids (precedence.ts:90-97) andderive()iterates it, so a datamate that adds Redshift or Trino is silently ignored even when the engine advertisesredshift_execute_database_query— no shadowing, no warning. That fix rewrites feat(workspace): route warehouse tools through the bound workspace's engine #1168's enforcement core, so it belongs in its own PR with its own tests rather than sharing a diff (and a bisect) with a steering change. Note this section renders whateverPrecedencederives, so it becomes dynamic for new engines with no change to this PR's code.ask_knowledge_baseappears nowhere inpackages/opencode/src/, so the name is an assumption.Screenshots / recordings
n/a — system-prompt change, no UI.
Checklist