Uh oh!
There was an error while loading. Please reload this page.
feat(workspace): attach the bound workspace's integration engine - #1154
feat(workspace): attach the bound workspace's integration engine#1154ralphstodomingo wants to merge 7 commits into
Conversation
Integrations are served by the local datamate engine — the same process the VS Code extension spawns as `datamate start-stdio`. Altimate Code could reuse an entry an IDE had already written, but could not acquire an engine on its own: with no entry present it fell through to the hosted SSE endpoint, which runs in multi-user mode and serves a DIFFERENT tool set (no connection validation, no extension-bridge tools, server-side cwd). A terminal session in a bound project therefore had either the IDE's tools or the wrong ones. `workspace/engine-sync.ts` closes that gap with `ensure(sessionID)`, idempotent per session and gated on the workspace pilot flag. Its rules, in order: **Reuse.** A connected `datamate` MCP entry wins — that is an IDE-written or previously persisted entry, and attaching to it is free. If it is down, what it is decides what happens next. A URL entry is an IDE's in-process engine or the hosted endpoint; neither can be revived from here, so with a binding and a usable engine on PATH we spawn locally and report what was replaced. The IDE's own config is never touched. A command entry that failed is retried once, then reported — spawning a second engine beside a failing one is the duplicate-process problem the single-gateway design exists to avoid. **Opportunistic use, never an install.** A `datamate` on PATH whose `--version` clears the floor is spawned as `datamate start-stdio --datamate <id>`, pinned to the bound workspace and persisted to the project config so later sessions start it at boot. With no engine present the user is told which workspace tools are unavailable and how to install one; the CLI ships as a self-contained binary with no Node runtime, so it must not pull one in. **Never fall back to hosted on failure.** The local and hosted tool sets diverge in both directions, so a silent fallback would change the workspace's declared contract. A failed engine is reported, not routed around. **Report what was declared but not delivered.** The engine intersects the workspace allowlist with what it managed to build and says nothing about the difference; this diffs declared keys against the tools that actually arrived and surfaces the gap. **First-turn readiness.** A turn resolves its tool list before the per-turn work that starts the attach, so a session that spawned its own engine listed the engine's tools one turn late — the model saw `datamate_manager` alone on the first turn and the integration tools only from the second. The attach now starts ahead of tool resolution and `whenAttached` gives it a bounded window, so those tools make the first tool list. A cold attach measures ~6.5s (≈1s to probe `--version`, ≈1s for the declared allowlist, ≈4.5s for the engine to boot, handshake and build its tools), against a 15s cap set well clear of that and far below MCP's own 30s connect timeout. Past the cap the turn proceeds and `tools/list_changed` delivers the tools when they land. Unbound and disabled sessions settle without I/O and wait for nothing. `datamate_manager list-integrations` now hides extension-type integrations, which are RPC into a live VS Code host and have no meaning on the CLI surface, and reports how many it hid rather than pretending they do not exist. Inert without a local binding. 21 unit tests cover the decision logic through the `syncInternals` seams.
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 |
Rule 1 reused any CONNECTED `datamate` entry without inspecting it. That is not enough to know whose engine it is. `--datamate <id>` is the whole of an engine's workspace identity, and the extension writes its entry WITHOUT one (`datamate start-stdio`), so that engine serves whichever teammate the IDE has active — and that changes at runtime, from a UI this client does not control. The consequence was a silent cross-workspace path: a session bound to workspace A could reuse an engine serving B, then report "workspace A: N tools" about it. Attach alone would merely hand over the wrong tools, but workspace precedence acts on that inventory — it would shadow local connections by B's types and route the model into B's credentials, under a no-hosted-fallback rule, with nothing naming the discrepancy. An entry is now reused only when it is live AND pinned to this workspace AND its binary clears the version floor. Anything else that is live — unpinned, pinned elsewhere, below the floor, or a URL — is replaced by a pinned local spawn and what it was is reported. That costs the other client nothing: a stdio entry is a per-client child process, so an IDE keeps its own engine and only our registration changes. A connected URL entry is replaced for the same reason rule 4 exists: the hosted endpoint serves a different tool set. A retry that brings a dropped entry back is gated identically, which it was not before. Two things fall out of the same mechanism: **Replacing a live entry closes it first.** `MCP.add` does not close the client it overwrites, so adding over a running stdio server starts a second engine and abandons the first with its pipes open — the duplicate-engine hazard this module already refuses for a failing entry. Left in, it wedged the session; observed as a hang, and reproduced against the previous commit as a clean reuse. **The floor is enforced on reuse, not only on spawn.** A stale persisted entry could otherwise keep an engine old enough that its `--datamate` pin is not locked — exactly the drift the attribution check exists to exclude. Below the floor, a newer engine on PATH is preferred; if PATH cannot do better, it is reported rather than reused. `MIN_ENGINE_VERSION` moves to 0.7.0, the first engine that locks the pin. SEQUENCING: this must not merge before `@altimateai/datamate` 0.7.0 is on npm, or every bound user gets `engine-too-old` for a version they cannot install. 15 further tests: the pin parser over both config shapes, both flag spellings and last-wins; each of the three connected-entry states; the recovered-entry gate; the disconnect-before-spawn contract; and the floor on the reuse path.
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:dfb183807d
ℹ️ 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".
| // Close the live registration first: `MCP.add` does not close the client it | ||
| // overwrites, so adding over a running stdio server starts a second engine | ||
| // and abandons the first with its pipes still open. | ||
| await client.disconnect(DATAMATE_KEY).catch((err) => { |
There was a problem hiding this comment.
When the connected datamate entry comes from global configuration and is unpinned or pinned to another workspace, this calls MCP.disconnect, whose implementation persists enabled: false to the source config. The subsequent persist only writes a project-local replacement, so after leaving this project the user's global Datamate remains disabled for every other project. Close or replace the runtime client without persisting the global entry's disabled state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ec21cc03d.
Verified the mechanism before changing anything: persistMcpEnabledUnlocked walks findAllConfigPaths(directory, globalConfig) and writes enabled: false into the first config that contains the entry. For an IDE-written or user-global datamate that is the global config, while our replacement is written project-local via resolveConfigPath(projectRoot()) — so the entry stayed disabled for every other project, exactly as described.
Fix is MCP.remove rather than MCP.disconnect. remove is runtime-only teardown — closeClient, drop from clients/status, publish ToolsChanged — and touches no file, which is precisely the "close or replace the runtime client without persisting" you asked for. The reason for closing at all is unchanged: MCP.add does not close the client it overwrites, so replacing a live stdio server without it orphans a second engine.
Test pins the contract (h.removes), plus assertions that reuse and the dead-URL path tear down nothing.
| const existing = sessions.get(sessionID) | ||
| if (existing) return existing.task |
There was a problem hiding this comment.
After the first turn, every later ensure for this session returns the original outcome without reading the binding again. If the user links an initially unbound project or uses the TUI's supported re-link flow while keeping the session open, subsequent turns either never attach an engine or continue exposing tools from the old workspace. Invalidate this state when recordApprovedBinding changes the binding, or scope it to the current project and binding rather than only the session ID.
AGENTS.md reference: packages/opencode/AGENTS.md:L102-L105
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ec21cc03d.
Checked reachability rather than assuming it: recordApprovedBinding is called from src/plugin/tui/altimate/workspace.tsx (lines 398, 546, 719) as well as src/cli/cmd/link.ts, so a live session's binding really can change under it. Both failure modes you named were real — a session that started unbound never attached, and a re-linked session kept serving the previous workspace's tools, silently, for the rest of the session.
The memo is now keyed on the bound workspace (workspace:<datamateId> / unbound / disabled) rather than the session id alone, so a re-link produces a fresh attach on the next turn with its own wait budget, and an unchanged binding stays memoised.
One thing worth recording, since it nearly turned this into a regression: the obvious implementation — await the binding read, then register — breaks whenAttached. prompt.ts calls it on the line after ensure, and it looks the session up by id, so a registration that happens after an await is missed and the turn skips the wait entirely, reintroducing the first-turn gap this module exists to close. ensure is therefore deliberately notasync and registers synchronously, with all async work inside the task. The existing whenAttached tests caught it, and there is now a test asserting the registration is visible immediately.
Four tests added: re-link mid-session, unbound-then-linked, unchanged binding still memoised, and synchronous registration.
| v | ||
| .trim() | ||
| .replace(/^v/, "") | ||
| .split("-")[0] |
There was a problem hiding this comment.
Stripping the prerelease suffix makes 0.7.0-beta.1 compare equal to the required stable 0.7.0, so that beta passes all three compatibility gates. This defeats the stated version floor and can reuse or launch an engine predating the stable pin-lock behavior the attribution checks rely on; preserve SemVer prerelease ordering so a prerelease of the minimum version remains below the floor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ec21cc03d.
Reproduced it directly before changing anything — compareVersions("0.7.0-beta.1", "0.7.0") returned 0, so a beta cleared the floor and was trusted for reuse and for launch. Your reasoning about why that matters is the part that decided it: the floor exists to require the locked --datamate pin that shipped in the 0.7.0 release, and a pre-release of that version predates the behaviour the attribution checks depend on. The stripping was deliberate on my part and it was wrong.
Precedence now follows SemVer §11.3: a release outranks any pre-release of it; identifiers compare numerically where numeric; numeric ranks below alphanumeric; a shorter identifier set ranks lower. Build metadata is ignored. Non-numeric cores still compare as older, so unreadable --version output can never clear a floor.
Tests cover 0.7.0-beta.1 < 0.7.0, alpha < beta, beta.2 < beta.10 (numeric, not lexical), alpha < alpha.1, alpha.1 < alpha.beta, build metadata ignored, and an ensure case where an engine reporting 0.7.0-beta.1 is refused as engine-too-old.
Three findings from the codex review of dfb1838, all verified against the branch before fixing. **Replacing a live entry must not disable it in the config that owns it.** Teardown used `MCP.disconnect`, which persists `enabled: false` to whichever config file actually holds the entry — for an IDE-written or user-global `datamate`, that is the GLOBAL config. Our replacement is written project-local, so the user's engine stayed disabled in every OTHER project. `MCP.remove` is the right call: runtime-only teardown that closes the client, drops it from state and publishes ToolsChanged, touching no file. The reason for closing at all is unchanged — `MCP.add` does not close the client it overwrites. **The memo now follows the binding, not just the session id.** `ensure` was memoised per session, but `recordApprovedBinding` is reachable mid-session from the TUI workspace panel as well as `altimate-code link`. A session that started unbound would therefore never attach, and one re-linked to another workspace kept serving the old workspace's tools — both silently, for the rest of the session. The memo is keyed on the bound workspace, so a re-link produces a fresh attach on the next turn with its own wait budget, and an unchanged binding stays memoised. `ensure` is deliberately NOT async and registers its entry SYNCHRONOUSLY. `whenAttached` is called on the following line and looks the session up by id; an await before registration made that lookup miss, so the turn skipped the wait entirely — reintroducing the first-turn gap this module exists to close. Caught by the existing `whenAttached` tests, and now pinned by one that asserts the registration is visible immediately. **Pre-release versions no longer clear the floor.** `compareVersions` stripped the pre-release suffix, so `0.7.0-beta.1` compared equal to `0.7.0` and passed every compatibility gate. The floor exists to require behaviour that shipped in a release — the locked `--datamate` pin the attribution checks depend on — and a pre-release of that version predates it. Precedence now follows SemVer §11.3: a release outranks any pre-release of it, identifiers compare numerically where numeric, numeric ranks below alphanumeric, and a shorter identifier set ranks lower. Build metadata is ignored. Non-numeric cores still compare as older, so unreadable `--version` output can never clear a floor. 9 further tests: pre-release precedence and build metadata, an engine reporting a pre-release of the floor, re-link mid-session, unbound-then-linked, unchanged binding still memoised, and synchronous registration.
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:ec21cc03d9
ℹ️ 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 (replacedLive) { | ||
| // Close the live registration first: `MCP.add` does not close the client it | ||
| // overwrites, so adding over a running stdio server starts a second engine | ||
| // and abandons the first with its pipes still open. | ||
| // |
There was a problem hiding this comment.
When a connected datamate entry is unpinned, pinned to another workspace, or below the version floor, replacedLive is set but teardown does not occur until this block. If the PATH/version checks return engine-missing or engine-too-old first, the rejected MCP client remains connected and the immediately following resolveTools call still exposes its tools, allowing the model to invoke the wrong or untrusted workspace engine. Remove the rejected client before those failure returns, not only after a viable replacement is found.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 248043922.
Verified the ordering before changing anything: replacedLive was set at lines 439 and 473, but teardown ran at 529 — after the engine-missing return (497) and the engine-too-old return (508). So exactly as you describe, a rejected engine stayed connected whenever no replacement could be produced, and the immediately following resolveTools served its tools.
It was one case worse than reported. The engine-too-old return inside rule 1 — a pinned engine below the floor with nothing better on PATH — has the same shape: we conclude the engine is untrustworthy and then return while it is still serving. Fixed all three exits, not just the two on the PATH/version checks.
This mattered more than a normal ordering slip because the tools being served are the ones attribution had just decided the model must not have: an unpinned engine following whichever workspace its owner has active, or one whose --datamate pin the engine does not lock. That's worse than the pre-attribution behaviour, where at least nothing had judged it.
detachRejected now runs at each rejection site, so "we will not use this engine" and "this engine stops serving this session" are the same event. Still runtime-only (MCP.remove) — no config file is touched, per the round-1 finding.
Three tests, one per irreplaceable-rejection exit, each asserting the client is removed and nothing is spawned.
| log.warn("could not close the engine entry being replaced", { err: String(err) }) | ||
| }) | ||
| } | ||
| await persist(DATAMATE_KEY, cfg) |
There was a problem hiding this comment.
Persisting this auto-managed entry as enabled makes the feature flag and binding checks ineffective on later launches: MCP bootstrap automatically starts every enabled config entry (src/mcp/index.ts:762-781) before the prompt, while run() simply returns for a disabled or unbound project without removing that entry. After one successful attach, launching with ALTIMATE_WORKSPACE off or with a binding that no longer resolves therefore still exposes the previously pinned workspace tools; keep this registration runtime-only or remove/disable the managed entry when the gate no longer passes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed in part, fixed in part, and the remainder is deliberately held rather than guessed at.
Verified the premise: MCP bootstrap does start every entry whose enabled is not false (src/mcp/index.ts:762-781), independently of the pilot flag, while run() returned early for a disabled or unbound project without touching it.
Fixed — the unbound half. Unlinking a project left the previously pinned workspace's tools attached on every later launch. run() now detaches a stale entry when the binding is gone. Two constraints on that, both tested: only an entry matching the exact command we persist is torn down, so an IDE-written or hand-edited entry is left alone as the user's; and the teardown is runtime-only, so the config file is not modified.
Held — the flag half. Acting on "launched with ALTIMATE_WORKSPACE off" means doing MCP work while the gate is closed, which is the opposite of what the gate is for; the flag exists so the pilot is invisible to users who have not opted in. And whether a pilot flag should retroactively disable an entry the user now has in their own project config is a product decision, not a correctness one — the entry is a normal, user-visible MCP entry once written, indistinguishable from one they added by hand.
Your first suggestion, keeping the registration runtime-only, would resolve both halves cleanly, but it costs a ~6.5s attach on every session instead of once (measured: ~1s --version probe, ~1s allowlist, ~4.5s engine boot/handshake/tool build) and reverses the design's explicit choice to persist so later sessions start at boot. That trade is also a product call.
Both are recorded in the PR body under "Held for Ralph" with this reasoning, unfixed.
| if (found && compareVersions(found, MIN_ENGINE_VERSION) >= 0) { | ||
| const available = engineToolKeys(await client.tools()).size | ||
| log.info("reusing existing engine entry", { workspaceId, available, version: found }) | ||
| return { kind: "reused", available } |
There was a problem hiding this comment.
When a compatible pinned engine is already connected but one of its declared integrations failed to initialize, this branch returns after merely counting the delivered tools and never calls declared() or emits the declared-versus-delivered warning. Reused engines, particularly after a restart, can therefore silently omit workspace tools even though the fresh-attach path reports that gap; compute the missing allowlist entries before returning reused as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 248043922.
You're right that this is an inconsistency in the module's own stated rules — rule 5 says report what was declared but not delivered, and the reuse branch counted tools and returned without ever calling declared().
Worth adding that reuse is the branch where it actually matters. Fresh attach happens once per project; reuse is the common path, so silence there is precisely where a gap goes unnoticed. It was visible in this branch's own testing: the fixture workspace declares 52 keys and delivered 12, then 7 after a connection was removed — a change the reuse path would have reported as an unqualified success.
reused now carries declared and missing, and warns when the gap is non-empty. An unreadable allowlist returns the bare { kind: "reused", available } rather than inventing an empty gap.
One honest cost: this adds the allowlist fetch (~900ms, two API calls) to the common path, inside the turn's bounded wait. Re-measured end-to-end after the change, the reuse row went from 19s to 26s wall-clock, though run-to-run variance on these is several seconds so most of that is noise. I judged a silent missing-tools gap worse than sub-second latency on a 15s budget; happy to revisit if a reviewer disagrees.
Three tests: the gap plus its toast, no gap meaning no toast, and the unreadable-allowlist degradation.
Codex round 2 on ec21cc0. Three findings, all verified against the branch. **A rejected engine is now detached at the moment of rejection.** Teardown ran just before the replacement spawn, so every exit that failed to produce a replacement — `engine-missing`, and both `engine-too-old` returns — came back with the rejected engine still connected. The turn's `resolveTools` then handed the model exactly the tools the attribution check had just decided it must not have: an unpinned engine serving whichever workspace its owner had active, or one below the floor whose `--datamate` pin the engine does not lock. Worse than the pre-attribution behaviour, because the client had explicitly judged it untrustworthy and served it anyway. `detachRejected` now runs at each rejection site, so "we will not use this engine" and "this engine is no longer serving this session" are the same event. It stays runtime-only (`MCP.remove`): the config file is never touched. **Reuse reports declared-versus-delivered, like the fresh attach.** A running engine that lost an integration — a deleted connection, a restart that dropped one — serves fewer tools than the workspace declares, and only the fresh-attach path said so. Reuse is the common path, so silence there is exactly where the gap goes unnoticed; it was visible in this branch's own testing, where a workspace declaring 52 keys delivered 12 and then 7 across a connection change. `reused` now carries `declared` and `missing` and warns when the gap is non-empty. An unreadable allowlist degrades quietly rather than inventing a gap. **An unbound project no longer keeps a stale managed entry.** MCP bootstrap starts every enabled config entry before the prompt (`src/mcp/index.ts:762-781`), while `run()` returned for an unbound project without touching it — so unlinking a project left the previously pinned workspace's tools still attached. Only an entry matching the exact command we persist is torn down; an IDE-written or hand-edited entry is the user's and is left alone, and the config is not modified either way. The related half of that finding — a project attached under the pilot flag keeps its tools when the flag is later off — is NOT fixed here. Acting on it means doing MCP work while the gate is closed, which is the opposite of what the gate is for, and deciding whether a pilot flag should retroactively disable an entry the user now has in their project config is a product call. Recorded in the PR body under "Held for Ralph" instead of guessed at. 9 further tests: detach on each irreplaceable-rejection exit, the reuse gap and its toast, an unreadable allowlist, and unbound detaching our managed entry while leaving an IDE entry alone.
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:248043922c
ℹ️ 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".
| // First attach for this session, or the binding changed under it. A changed | ||
| // binding gets a fresh attach AND a fresh wait budget: the previous budget | ||
| // was spent on a different workspace's engine. | ||
| entry.key = key | ||
| entry.waitTimedOut = false | ||
| return attachOnce(sessionID) |
There was a problem hiding this comment.
When an attach for workspace A is still running after the bounded wait and the user re-links the live session to workspace B, this starts a second attachOnce without cancelling or invalidating A. Both tasks can subsequently call MCP.add("datamate", ...), whose last completion replaces the runtime client, so the slower A task can overwrite B and restore the old workspace's tools; the B memo then remains settled and will not repair the runtime state on later turns. Add a generation/current-entry check before a task mutates MCP state, or serialize replacement attaches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 37714b4bc.
The race is real as described: ensure starts a fresh attachOnce when the binding key changes without ordering it against the in-flight one, both end in MCP.add, and the last completion owns the runtime client. The part that makes it stick is the one you identified — the new memo is already settled, so no later turn repairs the runtime state.
I took your second suggestion (serialize) over the generation check, because it gives the same guarantee with less machinery: the superseded task is awaited before the replacement starts, so the final MCP.add is the current workspace's by construction rather than by a check that could itself be raced. The superseded attach still completes and still tears down/spawns, but it does so first and is then overwritten in the correct order.
It costs the replacement attach the tail of the one it supersedes, which is bounded by MCP's own connect timeout and does not block the turn — whenAttached caps that independently.
Test drives it directly: a slow first MCP.add for workspace 42, a re-link to 99 mid-flight, and an assertion that the last add is 99's.
| const present = (await client.status())[DATAMATE_KEY] | ||
| if (present) { | ||
| const stale = await existingEntry(DATAMATE_KEY) | ||
| if (isManagedEntry(stale)) { | ||
| log.info("detaching a managed engine entry in an unbound project", { entry: describeEntry(stale) }) |
There was a problem hiding this comment.
After a fresh attach, persist() writes the project MCP entry directly with addMcpToConfig, but it never invalidates the per-instance Config.get() cache that was already initialized by the preceding client.status(). If the binding later stops resolving in the same server process, status() still sees the runtime entry while existingEntry() can return the old global entry or null; isManagedEntry then fails and the old workspace engine remains connected in the unbound project. Fresh evidence beyond the prior unbound-entry fix is this raw-write/cache mismatch; invalidate Config after persisting or read the owning entry directly from disk.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 37714b4bc. This one was the most useful finding of the three rounds, because it silently disarmed a fix from the previous round.
Verified the mechanism: Config.get() resolves to InstanceState.use(state, s => s.config) — cached per instance — and persist() writes with addMcpToConfig, a raw file write that never touches that cache. So every later existingEntry() in the same process sees pre-write config, isManagedEntry fails to recognise our own entry, and the unbound teardown added in 248043922 does nothing. The fix and its defeat shipped in the same commit.
persist() now calls Config.invalidate() after the write. That is the codebase's own established pattern rather than something new — the local-config write path in config.ts carries an explicit invalidateCurrent() with the comment "make subsequent Config.get() reload after local config writes", for exactly this reason. The invalidation is failure-tolerant: a cache that cannot be invalidated is logged, not fatal, since the write itself already succeeded.
I did not take the "read the owning entry directly from disk" alternative — it would leave every otherConfig.get() consumer in the process holding the same stale view, so invalidating is the fix that addresses the cause rather than one symptom.
| const key = await attachKey() | ||
| // Same workspace as the attach we already did for this session: reuse it. | ||
| if (previous && previous.key === key) return previous.task |
There was a problem hiding this comment.
For an unchanged binding, every settled result is cached, including engine-missing, engine-too-old, and connect-failed. Thus, if the user follows the install/update hint or repairs a failed engine while keeping the session open, subsequent turns return the original failure without checking PATH or MCP status and never expose the tools until a new session is created. Only reuse successful outcomes, or invalidate failed outcomes before the next turn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 37714b4bc.
The user story makes it sharper than it looks: engine-missing prints "install it with npm i -g @altimateai/datamate", the user does exactly that, and nothing happens for the rest of the session. We produced the hint and then cached the state that ignores it.
engine-missing, engine-too-old and connect-failed are now re-probed on the next turn. Success is still memoised, so this does not become an attach every turn.
One deliberate constraint, since re-probing is what makes this safe or unsafe: the retry does not re-arm the bounded turn wait. That block runs on every user turn, and a connect-failed retry can sit inside MCP's 30s connect budget — re-arming would charge every turn the full 15s cap, turning a P2 into a much worse latency bug. The retry runs unwaited and the repaired engine's tools arrive over tools/list_changed, which is the same fallback the first-turn wait already documents. There is a test asserting whenAttached returns immediately on a retry turn.
Four tests: retry after the engine appears, retry after an update, success still memoised across three turns, and the retry not re-arming the wait.
| const entryBin = commandArgv(entry)[0] | ||
| const found = entryBin ? await versionOf(entryBin) : null | ||
| if (found && compareVersions(found, MIN_ENGINE_VERSION) >= 0) { |
There was a problem hiding this comment.
When a pinned MCP entry uses a valid wrapper command such as npx @altimateai/datamate@0.6.3 start-stdio --datamate 42, entryBin is npx, so this checks npx --version rather than the running datamate version. A modern wrapper therefore lets the pre-0.7 engine clear the floor even though that engine does not provide the locked workspace pin on which attribution relies. Restrict reuse to a directly identifiable datamate executable or obtain the engine version through the connected server.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 37714b4bc.
commandArgv(entry)[0] is the wrapper for any wrapped command, so npx @altimateai/datamate@0.6.3 start-stdio --datamate 42 had us probing npx --version and accepting a pre-floor engine on the wrapper's version. Since the floor is a proxy for the locked --datamate pin, that is precisely the case where a wrong answer defeats attribution rather than merely being untidy.
Only a directly identifiable datamate executable is probed now (bare name or any path ending in it). Anything else yields no version and falls through to the existing below-floor handling — replaced by a pinned spawn from PATH, or reported as too old. Conservative in the right direction: an entry we cannot vouch for is not reused.
I did not take the second option, obtaining the version from the connected server. serverInfo.version is a hard-coded placeholder on engines at or below 0.6.4 — exactly the ones this floor exists to exclude — so it would report a passing version for the failing case. That is also why the probe uses the CLI flag in the first place.
Two tests: an npx-wrapped entry is never probed as npx and gets replaced by a pinned spawn; an absolute /opt/bin/datamate is probed and reused.
All four verified against the branch before fixing. **A superseded attach can no longer overwrite the current one.** Re-linking a live session started a second attach without ordering it against the first. Both end in `MCP.add`, and whichever completes last owns the runtime client, so a slower attach for the workspace just left could land after the new one and restore its tools — with the new memo already settled, so no later turn would repair it. Replacement attaches are now serialized per session: the superseded task is awaited before the next one starts, which makes the final `MCP.add` the current workspace's by construction. **Persisting the entry now invalidates the config cache.** `Config.get()` is cached per instance and `addMcpToConfig` is a raw file write that does not touch that cache, so every later `existingEntry()` in the process still saw the pre-write config. A managed entry then became unrecognisable to `isManagedEntry`, which is what leaves a stale engine attached in a project whose binding stopped resolving — the failure mode the previous commit's unbound teardown was supposed to prevent. The local-config write path in `config.ts` already invalidates for this exact reason; the engine entry now does too. **A repairable failure is re-probed on the next turn.** `engine-missing`, `engine-too-old` and `connect-failed` were memoised for the life of the session, so a user who followed the install hint we had just printed saw nothing happen until they started a new session. Those three outcomes are now retried; success stays memoised, so this does not mean re-attaching every turn. The retry deliberately does NOT re-arm the bounded wait: it runs on every turn, and a `connect-failed` retry sitting in MCP's 30s connect budget would otherwise charge each turn the full cap. Repaired tools arrive over `tools/list_changed`. **The version probe targets the engine, not its wrapper.** For a pinned entry like `npx @altimateai/datamate@0.6.3 start-stdio --datamate 42`, the probe ran `npx --version`, so a modern wrapper vouched for a pre-floor engine that does not provide the locked workspace pin attribution relies on. Only a directly identifiable `datamate` executable is probed now; anything else yields no version and falls through to the below-floor handling — replaced from PATH, or reported. Asking the running server instead is not an option: `serverInfo.version` is a hard-coded placeholder on precisely the engines this floor excludes. 7 further tests: retry after install and after update, success still memoised, the retry not re-arming the wait, npx not probed and replaced, an absolute datamate path probed and reused, and a slow superseded attach losing to the re-linked workspace.
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:37714b4bcd
ℹ️ 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".
| } else { | ||
| // A command entry that failed: one retry, then report — never a second | ||
| // spawn beside a failing one. | ||
| await client.connect(DATAMATE_KEY).catch(() => undefined) |
There was a problem hiding this comment.
When a configured command entry has enabled: false, MCP.status() reports disabled, so this branch treats it as a failed connection and calls MCP.connect(). That function persists enabled: true (src/mcp/index.ts:913-918); for a global datamate entry, the first prompt in a bound project therefore silently re-enables it for every other project, even if this flow subsequently removes or replaces the runtime client. Only retry entries whose status is actually failed, rather than overriding an explicit disable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a84e7c393.
Verified the whole chain rather than just the symptom: bootstrap sets status: "disabled" for an entry with enabled: false (mcp/index.ts:772), this flow read anything not connected as a failed connection, and MCP.connect persists enabled: true via persistMcpEnabled (mcp/index.ts:913-918) into whichever config owns the entry. So for a global datamate, the first prompt in any bound project silently re-enabled it everywhere.
That is the same class as the round-1 teardown finding — writing global config state as a side effect of a local decision — in the opposite direction. Worth noting the pair: teardown was persisting enabled: false globally, and retry was persisting enabled: true globally. Both are now runtime-only or not taken at all.
A disabled entry is now left alone and reported as entry-disabled, with a toast saying which tools are unavailable and why. Only a genuinely failed entry is retried, exactly as you suggested. entry-disabled also joins the repairable outcomes, so enabling the entry mid-session is picked up on the next turn rather than needing a new session.
Verified end-to-end, not just in unit tests: with a disabled entry in a bound project, the session attaches no engine tools (1 datamate_* tool — the manager — instead of 13), and the config file is byte-identical afterwards with "enabled":false intact.
Two tests, both confirmed to fail against the unfixed code first: a disabled entry is neither connected nor persisted, and a failed entry is still retried exactly once.
| // slower attach for the workspace we just left could otherwise land after | ||
| // this one and restore its tools — with this session's memo already | ||
| // settled, so no later turn would repair it. | ||
| if (previous) await previous.task.catch(() => {}) |
There was a problem hiding this comment.
When two sessions in the same project overlap a re-link, this await only orders work recorded under the same session ID. Session A can still be attaching workspace A while session B attaches the newly linked workspace B; because both mutate the instance-wide datamate client and MCP.add() is last-writer-wins, A can land last while B's successful memo prevents subsequent repair. Fresh evidence beyond the earlier per-session race fix is that SessionRunState maintains independent runners per session ID (src/session/run-state.ts:35-68), so these prompts can overlap; serialize replacement attaches at the project/instance scope instead.
AGENTS.md reference: packages/opencode/AGENTS.md:L102-L104
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a84e7c393.
You're right that the previous fix was scoped too narrowly, and the evidence you cite is the reason: the MCP client lives in instance state, not session state, so MCP.add being last-writer-wins is an instance-wide hazard, while SessionRunState keeps independent runners per session id, so two prompts in the same project genuinely overlap. Ordering within a session cannot see the other session's attach at all.
Attaches now run through a per-project chain, which subsumes the per-session ordering rather than sitting beside it. Keyed on the project root rather than globally, so a slow attach in one project cannot delay another in the same process.
A note on how this was tested, because my first two attempts were worthless. I wrote a test asserting the re-linked workspace's MCP.add lands last — it passed against the unfixed code, so it proved nothing. The second attempt failed the same way. The reason is that both attachKey() and run() resolve the binding independently, so a test that switches a shared binding between two ensure calls ends up with both attaches targeting the same workspace and no race to observe.
The test that actually bites asserts the invariant directly: instrument MCP.add to track concurrent entries and require a peak of 1 across two overlapping sessions. That fails against the unfixed code (peak 2) and passes after. Both round-4 tests were confirmed red before the fix was written.
…project Codex round 4 on 37714b4. Two findings, both verified, and both new tests were confirmed to fail against the unfixed code before the fix was written. **An explicitly disabled entry is no longer silently re-enabled.** A configured entry with `enabled: false` is reported by `MCP.status()` as `disabled`, which this flow read as "not connected" and retried with `MCP.connect`. That call persists `enabled: true` into whichever config file owns the entry, so for a global `datamate` the first prompt in any bound project quietly re-enabled it for every other project — the same class as the round-1 teardown finding, in the opposite direction. A disabled entry is now left alone and reported as such; only a genuinely `failed` entry is retried. **Attaches are serialized per project, not merely per session.** The previous commit ordered replacement attaches within a session, but the MCP client is instance-wide, `MCP.add` is last-writer-wins, and `SessionRunState` keeps independent runners per session id — so two prompts in the same project overlap for real. A slower attach from one session could land after another's and leave the runtime serving a workspace nobody is bound to, with both memos settled so no later turn would repair it. Attaches now run through a per-project chain, which also subsumes the per-session ordering. `entry-disabled` joins the repairable outcomes, so enabling the entry mid-session is picked up on the next turn rather than requiring a new session. 3 further tests: a disabled entry is neither connected nor persisted, a failed entry is still retried exactly once, and two overlapping sessions in one project never hold the mutating phase at the same time. The last of these asserts the invariant directly (peak concurrency of 1) after two attempts that passed against the unfixed code and so proved nothing.
👋 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. |
1 similar 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. |
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:a84e7c3930
ℹ️ 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".
| return !!outcome && REPAIRABLE.has(outcome.kind) | ||
| } | ||
| const sessions = new Map<string, SessionAttach>() |
There was a problem hiding this comment.
In a long-running server that creates sessions or opens multiple projects, this module-level map—and the later attachChains map—retains every session ID, project path, and settled promise indefinitely; only the test-only resetForTests() clears them. Store this state in InstanceState and clear session entries when their lifecycle ends so disposed projects cannot accumulate unbounded process-global state.
AGENTS.md reference: packages/opencode/AGENTS.md:L102-L105
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a719eb51b, though with a smaller fix than suggested.
The leak is real: both maps are module-level, a long-running serve process creates sessions indefinitely, and only the test-only reset clears them.
I bounded the session map with oldest-first eviction rather than moving the state into InstanceState. Reasoning, since I'd rather say it than have it look like a shortcut: eviction is safe here because the map is a memo, not a source of truth — an evicted session simply re-attaches on its next turn, which is correct if not free. That makes the bound a complete fix for the unbounded-growth problem, whereas the InstanceState move is a structural change to how this module holds state, landing at the end of a review cycle on a branch two other branches are stacked on.
attachChains is keyed by project root rather than session, so it is bounded by the number of distinct projects a process opens — a much slower-growing set, and one that would be addressed by the same InstanceState move.
I have recorded the InstanceState scoping in the PR body as follow-up work rather than treating the bound as the end of it. Test asserts the map stays within its cap after exceeding it, and fails with the eviction loop removed.
| let connected = existing.status === "connected" | ||
| if (!connected) { | ||
| if (existing.status === "disabled") { |
There was a problem hiding this comment.
After a connected entry is rejected and MCP.remove() runs, a repairable retry is misclassified here as a user disable: MCP.remove deletes s.status[name] but leaves the enabled file/runtime config, while MCP.status() reports any configured entry with no status as disabled (src/mcp/index.ts:875-882,949-954). Thus, if the first attempt returns engine-missing or engine-too-old, installing/updating the engine and trying another turn always returns entry-disabled until restart; check the config's actual enabled: false value rather than interpreting this synthesized status as user intent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a719eb51b. This is the most valuable finding of the five rounds — it silently undid the previous round's fix, and my own tests could not see it.
Verified the mechanism exactly as you describe: MCP.remove deletes s.status[name], and MCP.status() reports any configured entry with no runtime status as disabled (mcp/index.ts:875-882). So after any rejection teardown, the next turn reads a synthesized disabled and the round-4 check treated it as user intent.
The consequence is worse than a misclassification: it defeats the repairable-retry fix from round 3 on that fix's single most likely path — reject an unattributable engine, fail to replace it, print the install hint, the user installs the engine, and every later turn returns entry-disabled until they start a new session. Two fixes from consecutive rounds, each correct alone, combining into a regression neither test suite could see because both were exercised in isolation.
Intent is now read from the config's actual enabled: false, which is the only place a user expresses it. The synthesized status is treated as what it is — absence of information, not a decision.
The test is the full repair scenario rather than the misclassification alone: reject and tear down, engine-missing, then the engine appears and a later turn attaches with the pinned command. It fails with the enabled: false check reverted.
| function isManagedEntry(entry: ExistingEntry | null): boolean { | ||
| const argv = commandArgv(entry) | ||
| return argv.length === 4 && argv[0] === ENGINE_BINARY && argv[1] === "start-stdio" && argv[2] === PIN_FLAG && !!argv[3] |
There was a problem hiding this comment.
When an unbound project has a hand-authored MCP entry using the natural datamate start-stdio --datamate <id> command, this predicate labels it as written by this feature even though argv contains no provenance. The unbound path consequently calls MCP.remove() and silently takes the user's server and tools offline on every first prompt, contradicting the stated requirement to leave hand-edited entries alone; persist and verify an explicit ownership marker instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a719eb51b — by removing the teardown rather than by adding a marker.
You are right that argv is not provenance, and the sharpest part of the finding is that the code's own comment claimed to leave hand-edited entries alone while the predicate could not tell them apart. A hand-authored datamate start-stdio --datamate <id> is byte-identical to what this feature writes, so an unbound project with a hand-authored entry had its server taken offline on every first prompt.
This module's thesis is that you do not act on something you cannot attribute — that is the whole argument for rule 1 — and it has to apply to the module itself. Since ownership cannot be established, the unbound path now reports the situation and leaves the entry alone.
I did not implement the ownership marker in this change. Writing a provenance field into the user's MCP config raises its own questions — whether the config schema tolerates unknown keys, and whether an IDE's config sync would strip it — and answering those properly is a separate change rather than a fifth-round addition to a branch two others are stacked on. It is recorded in the PR body as follow-up.
Worth noting what this costs: a genuinely stale entry we did write now survives in an unlinked project, which is the case the round-2 teardown was added for. I judged silently disabling a user's own server the worse of the two, since one is a missing cleanup and the other is destroying working configuration. The round-2 test that asserted the teardown is reversed deliberately, with a comment saying why.
Codex round 5 on a84e7c3. Three findings, all verified; each new test was confirmed to fail with its fix reverted. **A removed entry is no longer mistaken for a user disable.** `MCP.remove` deletes the runtime status, and `MCP.status()` reports any *configured* entry with no status as `disabled`. So every rejection teardown made the following turn look like an explicit user disable, and the session returned `entry-disabled` for good — silently undoing the repairable-retry fix from the previous round for its most likely path: reject an unattributable engine, fail to replace it, install the engine, and never recover. Intent is now read from the config's actual `enabled: false`, which is the only place a user expresses it; the synthesized status is treated as the absence of information it is. **An unbound project no longer tears down an entry it cannot prove it owns.** Ownership was inferred from argv shape, but argv carries no provenance: a hand-authored `datamate start-stdio --datamate <id>` is byte-identical to what this feature writes, so the teardown took the user's own server offline on every first prompt — the opposite of the guarantee its comment claimed. This module's thesis is that you do not act on what you cannot attribute, and that has to apply to the module itself, so it now reports and leaves the entry alone. Doing better needs an explicit ownership marker written at persist time; that is a separate change and is recorded in the PR body rather than guessed at here. **The session and attach-chain maps are bounded.** They are module-level and a long-running `serve` process creates sessions indefinitely, so they grew for the life of the process with only a test-only reset to clear them. Sessions are now capped with oldest-first eviction; an evicted session simply re-attaches on its next turn, which is correct if not free. Storing this in `InstanceState` would be the thorough fix and is noted for later. 4 further tests: a removed entry recovering through install on a later turn, a genuinely disabled entry still respected, a pinned entry left alone in an unbound project, and the session map staying within its cap. The round-2 test that asserted the unbound teardown is reversed deliberately, and the round-4 disabled test now sets `enabled: false` rather than relying on the synthesized status.
👋 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. |
2 similar comments
👋 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. |
👋 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. |
ralphstodomingo
commented
Aug 26, 2026
Codex review logFive rounds on this branch. 15 findings, 15 confirmed real, 14 fixed, 1 open question — none dismissed. Every finding was verified against the branch before any fix; from round 4 on, each new test was confirmed to fail with its fix reverted. Round 1 — dfb1838
Round 2 — 2480439
Round 3 — 37714b4
Round 4 — a84e7c3
Round 5 — a719eb5
The pattern worth naming: fixes that interactTwice, a fix reported as landed was defeated by a later one, and no test suite could see it because each was exercised alone:
The individual fixes were each correct. The risk on this module is not bad fixes; it is fixes that combine. A second, related pair: rounds 1 and 4 are the same defect in opposite directions — global config state written as a side effect of a local decision, once persisting disabled and once persisting enabled. The rule the branch now follows is that attach may change runtime state freely but never persists outside the project it is attaching. One reversalRound 5 removed a fix from round 2. The unbound teardown inferred ownership from argv, which carries none, so it took hand-authored servers offline. Removing it means a genuinely stale entry can survive in an unlinked project — a missing cleanup, judged the lesser harm against destroying working configuration. |
Issue for this PR
Closes#1153
Type of change
What does this PR do?
Lets a terminal session acquire the local datamate engine for the workspace its project is bound to, instead of falling through to the hosted endpoint, which serves a different tool set.
New module
workspace/engine-sync.ts, idempotent per session and gated on the workspace pilot flag. Its rules:Attach state is never persisted outside the project being attached, and an entry that cannot be attributed is never torn down.
First-turn readiness. A turn resolves its tool list before the per-turn work that starts the attach, so a session that spawned its own engine listed the engine's tools one turn late. The attach now starts ahead of tool resolution with a bounded wait, so those tools make the first tool list. Past the cap the turn proceeds and a tools-changed notification delivers them. Unbound and disabled sessions wait for nothing.
Also: the integrations listing hides extension-type integrations, which need a live VS Code bridge, and says how many it hid.
Engine gate cleared: the required engine version is published, verified against a clean install from the registry.
How did you verify your code works?
Unit — 68 tests over the module's seams;
test/altimate/workspace/177 pass / 0 fail; typecheck clean. Three failures elsewhere in the wider suite reproduce identically on a clean worktree atorigin/main.End-to-end, against a real bound workspace, each row re-run on the current commit:
Five rounds of automated review were triaged on this branch: 15 findings, 15 confirmed real, 14 fixed. Full log, including two cases where one fix silently disarmed another, is in the Codex review log comment.
Screenshots / recordings
Not a UI change; toasts are TUI-only and terminal evidence is above.
Checklist