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 23 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. |
Codex review logSeventeen rounds on this branch, plus an invariant pass. 40 findings, 40 confirmed real, 39 fixed — 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
Round 6 — a719eb5
Correction: this round was first recorded here as returning no findings. That was wrong, and the error was mine rather than the reviewer's. The finding was posted as an inline comment at the same moment as an empty-bodied summary review; my query for inline comments was unpaginated, and with exactly 30 comments already present the new one fell past the first page, so the count appeared unchanged. Reading the review body alone showed boilerplate, and I concluded the round was clean. The lesson is that a count is not evidence when it sits on a page boundary — paginate, or compare identifiers rather than totals. Round 7 — 18ada3e (final round)
The first of these also exposed a test that had been passing for the wrong reason: it let the retry settle immediately, so the wait returned on settle whatever the flag said. It now hangs the retry, which is the only way the flag is under test. Round 8 — 1a5d85a
Also added in this round, at the request of the workspace-precedence work: a read-only Round 9 — 791a286
Shipped alongside, authorised separately rather than found by review: an engine that cannot be run is now described as such instead of as out of date. The version probe reads stdout only and returns nothing when the process fails, so "no version" means broken rather than old. Both previously reported as "too old", which sent more than one debugging session hunting a version mismatch that did not exist. Round 10 — d6f5b5b
Folded in as agreed rather than found by review: config reads are now fresh by construction. Three separate bugs came from reading a per-instance cache after someone else wrote — our own write, a disconnect writing to disk, and an IDE rewriting the entry, which never goes through the cache at all. Two defeated a fix from an earlier round. The writers cannot be enumerated, so freshness belongs at the point of read. Round 11 — fee2a0c
A note on the second: racing a promise does not cancel what it is racing, so a Round 12 — 4e3e28a
Two sibling functions in the same API client share the body-window shape. They are pre-existing and were left untouched rather than swept in silently. Round 13 — d2f924a
The rule this corrects is worth stating on its own: revalidate before answering, because an answer this flow gives is acted on. Guarding only the writes left the read path asserting something it had not rechecked. Rounds 14-15, and an invariant pass between themRound 14 — the version probe could not execute a Windows Between rounds, the attach contract was written as invariants rather than one test per past fix. One failed on its first run: the guard after the engine add did not cover the tool listing that follows it, so a re-link during that read left the previous workspace installed and reported as attached. Fifteen review rounds had not found it. The two guards became one, placed after every await that follows the install. Round 15 — the shared gateway key was reported as "already connected" for any datamate without checking its pin. Pinning that key is this branch's doing, so after one workspace attaches, asking for another reported success while the runtime served the first one's tools and credentials. Round 16 — fd5f2f8
The two sources disagree in both directions and each direction cost a round. That is the argument for reading intent from one authority rather than inferring it from whichever signal is nearest. Residual, named: a client already connected keeps serving until MCP drops it. This flow stops attaching and stops re-enabling; it does not tear down a live client on the strength of someone else's config edit. Round 17 — 42fb816
The invariant that should have caught both was itself too weak. It asserted only that the runtime client was removed, so it passed while a stale pin sat on disk and while the reuse path detached nothing. An invariant is only as good as its definition of "nothing": it now covers the config and the reuse path, and each half fails independently when its fix is reverted. Worth remembering when the consolidation leans on these. 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. Round 18 — 37dd23d
Undoing a write is only correct if it restores what that write replaced. The restore was reading from a different source than the write it was reversing. Round 19 — ce5331d
Two things worth keeping from this round. A residual is a claim, and claims expire. Round 16 named "a live client keeps serving until MCP drops it" as a bounded limit and accepted it. It was not bounded: the tools stayed in the model's hands for the rest of the session, which is the thing this module exists to control. Naming a limit honestly is not the same as being right that it is acceptable. The fix that closes a window can open one. Round 17 asked for a config restore; round 18 added it; the added disk read reopened the very gap round 14's invariant pass had closed. Every await after a guard belongs to the guard's problem. Where this ended45 findings across 19 rounds, 45 confirmed real, 44 fixed, 1 open product question — whether a pilot flag should retroactively disable an entry already persisted in a user's own project config. That one is a product call, not a defect, and is held rather than decided here. No round was closed on assertion. Each fix in rounds 16-19 was proven by reverting it alone and watching a named test fail, and the four end-to-end rows — fresh attach, reuse, replace-unpinned, disabled — were re-run against every pushed head, with the config compared byte-for-byte where the flow is required not to write. One test fails in the wider suite on this head: Known residualsNamed as they were found rather than discovered later. None is a defect this In the attach flow
Coverage that is honest rather than complete
Left to other work
|
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:d6f5b5be54
ℹ️ 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 { kind: "superseded" } | ||
| } | ||
| await persist(DATAMATE_KEY, cfg) | ||
| await client.add(DATAMATE_KEY, cfg) |
There was a problem hiding this comment.
When the initial 15-second wait expires, or when a repairable failure is retried non-blockingly, resolveTools snapshots the catalog before this add completes. Fresh evidence beyond the prior retry change is that MCP.add() only stores the client (src/mcp/index.ts:906-911) and does not publish MCP.ToolsChanged; therefore the newly attached tools are unavailable to the current model invocation, and no expected refresh is delivered when they land. Publish the event after a successful add so slow and repaired attachments become visible without requiring another user turn.
Useful? React with 👍 / 👎.
| // the hint it produced. | ||
| if (sameWorkspace && !isRepairable(previous!.outcome)) { | ||
| // Re-probe before trusting a cached success — see `engineStillConnected`. | ||
| if (!wasServing(previous!.outcome) || (await engineStillConnected())) return previous!.task |
There was a problem hiding this comment.
When a live project is linked A→B→A, with another session attaching B in between, this session's key again matches its original A memo while the instance-wide datamate client still serves B. Fresh evidence beyond the earlier binding-cache fix is that this check validates only status === "connected", so it returns the old successful task without checking the current entry's workspace pin; every subsequent turn can therefore expose B's tools under binding A. Revalidate the live entry's pin/version before accepting the cached success.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| const declaredKeys = await declared(workspaceId) |
There was a problem hiding this comment.
When the Altimate API accepts a connection but stalls, declared() waits on AltimateApi.getDatamate() and listIntegrations(), whose shared request() has no abort timeout. Because this optional reporting lookup runs before the local binary is launched, a usable cached binding and installed engine never attach; after the first turn's wait expires, later ensure() calls keep returning the same still-pending task because it has no settled repairable outcome. Bound or defer this lookup so an unavailable catalog cannot block the local engine.
Useful? React with 👍 / 👎.
…ses, close the stale-config class Codex round 10. Three findings, all verified; each fix confirmed to fail only its own test when reverted. The config-cache class fix is folded in as agreed. **A late attachment now announces itself.** MCP.add stores the client but publishes nothing, so an attach landing after the turn's bounded wait — or on a repair retry, which never waits — produced tools the session had no way to learn about until the user sent another message. This branch has been documenting a tools-changed fallback as the justification for the wait being safe; that fallback depended on an event nobody published. It is published now. **A cached success is re-attributed, not merely re-connected.** Link A to B and back to A, with another session attaching B in between, and this session's key matches its original memo while the instance-wide client is serving B. The round-8 re-probe only checked that something was connected, so every later turn would expose B's tools under binding A. The live entry's pin is now checked too, which is what makes it ours. **The optional catalog lookup can no longer block a local spawn.** declared() is reporting only, but it runs before the engine is launched and its HTTP layer has no abort timeout, so an API that accepts a connection and then stalls stopped a good cached binding and an installed engine from ever attaching — and later turns kept returning the same pending task, since a pending attach has no settled repairable outcome. It is bounded now; reporting degrades, attaching does not wait on it. **Config reads are fresh by construction.** Three separate bugs in this module came from reading a per-instance cache after someone else wrote: our own addMcpToConfig, MCP.disconnect writing enabled:false, and an IDE rewriting the entry — which never goes through Config at all. Two of them defeated a fix from an earlier round. The writers cannot be enumerated, so freshness belongs at the point of read: freshConfig() is now the module's only path to config, and the single reader goes through it, which makes all three call sites correct without touching them. The ad-hoc invalidation added last round is removed rather than left as a second answer to the same question. The cost is named rather than hidden: invalidating drops the shared per-instance cache, so other Config consumers re-read. Also fixed in the test harness: the mocked config now reflects an entry after it is persisted, as production does. Without that the pin re-check saw no entry and a legitimate memo looked like a workspace change.
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:fee2a0ce41
ℹ️ 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".
| // attach used to say so. Reuse is the COMMON path, so staying silent | ||
| // here is where the gap would actually go unnoticed. | ||
| const present = engineToolKeys(await client.tools()) | ||
| const declaredKeys = await declared(workspaceId) |
There was a problem hiding this comment.
Fresh evidence beyond the prior catalog-timeout finding is that the timeout was added only to the fresh-spawn path: a compatible pinned engine still awaits declared() here without a bound, while AltimateApi.request() supplies no abort signal. If that API accepts a connection and stalls, the first prompt waits 15 seconds and the attach task remains pending forever; a later re-link also queues behind this project attach indefinitely, allowing the old workspace engine to remain exposed. Additionally, the Promise.race used below does not cancel its underlying fetches, so repair retries can accumulate stalled requests. Put an actual abortable timeout around declared() itself and use it on both reuse and spawn paths.
Useful? React with 👍 / 👎.
| await AppRuntime.runPromise( | ||
| EventV2Bridge.Service.use((events) => events.publish(ToolsChanged, { server: DATAMATE_KEY })), | ||
| ) |
There was a problem hiding this comment.
Fresh evidence after the prior late-attachment finding is that publishing this event still does not deliver tools to the active model invocation: the session's ToolsChanged subscriber in src/session/prompt.ts lines 531-537 only logs that tools will refresh on the next turn, and the tools object already passed to the AI SDK is never rebuilt. Consequently, an attach completing after the bounded wait or during a non-blocking repair retry remains unusable until another user turn despite the new announcement; trigger a session/tool refresh that affects the active flow rather than only publishing the notification.
Useful? React with 👍 / 👎.
…stating the late-attach fallback Codex round 11. Two findings, both verified. **The allowlist bound covered only one of two call sites.** Last round bounded the fresh-spawn path and left a reused engine awaiting the same lookup with no limit — a partial fix that read as a complete one. Both paths now go through a single bounded helper, so there is one answer rather than two. The underlying request was genuinely unbounded, not merely slow: the generic API request performed a bare fetch with no abort signal, while two other functions in that same client already attach one. It does now, so a stalled server releases its socket instead of accumulating pending fetches across repair retries — which a Promise.race alone cannot do, since racing a promise does not cancel what it is racing. **Publishing a tool-change event does not refresh the running turn, and this module said otherwise.** The invocation's tool set is passed to the model before a late attach completes and cannot be rebuilt mid-call; the session's subscriber only logs, and the next resolveTools is what picks the tools up. So exceeding the bounded wait costs a turn, not a session. That is a correction to a claim this branch has repeated since the wait was introduced. Publishing the event remains right — nothing downstream could otherwise observe a late attach at all — but it is traceability and a hook for subscribers that act between turns, not a live refresh, and the comment now says so rather than promising delivery it cannot make. 1 test: a stalled catalog lookup no longer blocks the reuse path, and reuse still succeeds with only the optional reporting degraded. It fails with the reuse-path bound reverted.
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:4e3e28a380
ℹ️ 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".
| }, | ||
| ...(body ? { body: JSON.stringify(body) } : {}), | ||
| }) | ||
| }).finally(() => clearTimeout(timeout)) |
There was a problem hiding this comment.
When the server sends response headers within 15 seconds but stalls while transmitting the JSON body, fetch() resolves and this finally clears the abort timer before res.json() runs. The request can then remain pending indefinitely; declaredBounded() hides it from the attach path, but each lookup leaves a live socket and promise behind. Fresh evidence beyond the prior catalog-timeout finding is this response-body window; keep the timeout active until the body has been consumed.
Useful? React with 👍 / 👎.
| const before = await client.status() | ||
| const existing = before[DATAMATE_KEY] | ||
| if (existing) { |
There was a problem hiding this comment.
When a datamate entry is added directly by an IDE or user after the MCP/Config cache was initialized, client.status() reads the stale cached config and omits the entry, so this condition is false and the fresh existingEntry() read is never reached. The flow then persists its managed entry over the externally authored configuration. Fresh evidence beyond the prior stale-config fix is this status gate before the fresh read; refresh config before obtaining status or inspect the fresh entry independently of whether status contains the key.
Useful? React with 👍 / 👎.
| return Promise.race([ | ||
| declared(workspaceId), | ||
| new Promise<null>((resolve) => { | ||
| const timer = setTimeout(() => { |
There was a problem hiding this comment.
Whenever declared() resolves successfully in under four seconds, Promise.race() returns but leaves this timer running. It later emits a false timeout warning for a lookup that succeeded and retains the closure until the deadline, producing misleading logs on every normal attach or reuse; clear the timer when the declared lookup wins.
Useful? React with 👍 / 👎.
…e status gate, cancel the losing timer Codex round 12. Three findings, all verified, all follow-ups on last round's fixes. **The abort now stays armed until the response body is read.** fetch resolves on headers, so clearing the timer there left a server that sends headers and then stalls mid-body hanging indefinitely, holding its socket. The bound added last round covered the wrong half of the request. Two sibling functions in the same client share this shape. They are pre-existing and untouched here rather than swept in silently; noted for a follow-up. **Config is read before the MCP status gate, not after.** MCP.status() reads the same cached config as everything else, so an entry added directly by an IDE or a user after the cache was warmed is absent from status. The entry check then never ran and the managed entry was persisted straight over the externally authored one. Reading the entry first is what refreshes that cache, so the status gate becomes trustworthy rather than merely fresh-looking. That is the fourth route by which this cache has produced a wrong answer, and the first where the stale read was inside MCP rather than here — the freshConfig accessor fixed this module's own reads but could not fix a gate that consults the cache independently. **The losing allowlist timer is cancelled.** Racing does not cancel the loser, so a lookup that succeeded in well under the bound still fired its timeout later and warned that it had timed out — on every normal attach and reuse. A fix whose only symptom was misleading logs, which is the kind that survives longest. 1 test pinning the ordering: the config read must precede the status gate, and it fails with the two swapped. The timer cancellation is verified by inspection — its only observable effect is a log line, and manufacturing a seam to assert on a log would be worse than the bug.
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:d2f924ab08
ℹ️ 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 present = engineToolKeys(await client.tools()) | ||
| const declaredKeys = await declaredBounded(workspaceId) | ||
| const missing = declaredKeys ? declaredKeys.keys.filter((k) => !present.has(k)) : [] |
There was a problem hiding this comment.
When a compatible engine for workspace A is already connected, declaredBounded() can wait up to four seconds; if the project is re-linked to workspace B during that await, this branch still returns reused without the stillCurrent() check used by the spawn path. whenAttached() then completes and the current prompt snapshots A's tools under binding B, including A's workspace credentials, until a later user turn triggers replacement. Revalidate the binding after this awaited lookup before accepting the reused client.
Useful? React with 👍 / 👎.
… engine Codex round 13. One finding, verified, and it exposes a gap in how the earlier guards were reasoned about. Every mutation in this flow revalidates the binding before acting — the spawn, the teardown, the post-handshake check. Returning `reused` was treated as different because it changes nothing. It is not different: it ASSERTS that the connected engine serves the current binding, and the caller acts on that answer exactly as it acts on a spawn. The reuse branch also awaits the allowlist lookup first, so there is a real window in which a re-link can land. The consequence was the same one the attribution work exists to prevent: the turn snapshots the previous workspace's tools, and its credentials, under the new binding, and nothing corrects it until a later turn triggers replacement. The guard is not "revalidate before mutating". It is "revalidate before answering", because an answer this flow gives is acted on. 1 test: a re-link landing inside the reuse lookup yields `superseded` rather than the old workspace's tools, and it fails with the check removed.
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:686e3bb267
ℹ️ 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".
| function versionOf(bin: string): Promise<string | null> { | ||
| if (syncInternals.versionOf) return syncInternals.versionOf(bin) | ||
| return new Promise((resolve) => { | ||
| execFile(bin, ["--version"], { timeout: 5000 }, (err, stdout) => { |
There was a problem hiding this comment.
On Windows, an npm-installed datamate is normally resolved by which() to a .cmd shim, but Node's execFile() cannot execute .cmd or .bat files directly without a shell. The callback therefore reports an error, versionOf() returns null, and every bound Windows user with the normal global npm installation is told that the engine is not runnable even though MCP's cross-platform launcher could start it. Use the repository's cross-spawn path or explicitly invoke the command interpreter for Windows shims.
Useful? React with 👍 / 👎.
| // in between, and this session's key matches its original memo while the | ||
| // instance-wide client is serving B — so the cached success would expose B's | ||
| // tools under binding A. The pin is what makes it ours. | ||
| return pinnedWorkspace(await existingEntry(DATAMATE_KEY)) === workspaceId |
There was a problem hiding this comment.
When a previously successful session's datamate entry is reconnected or replaced with the same workspace pin but a pre-0.7 binary, this validation accepts it solely because it is connected and the argv pin still matches. The cached outcome then bypasses run() and its version floor indefinitely, even though those older engines do not lock the workspace pin and may drift to another workspace. Fresh evidence after the earlier cached-attribution finding is that the new engineStillOurs() implementation checks pinnedWorkspace() but never calls versionOf() or otherwise verifies the live engine generation/version.
Useful? React with 👍 / 👎.
…check the floor on cached successes Codex round 14. Two findings, both verified. **The version probe could not run a Windows shim.** `which` honours PATHEXT, so an npm-installed engine on Windows resolves to a `.cmd`, and Node cannot execute `.cmd` or `.bat` without a shell — the probe just errored. Every bound Windows user with an ordinary global install would have been told the engine was not runnable, while MCP's own launcher started that same engine without trouble. The probe now uses cross-spawn, which is what the rest of this repo already uses for exactly this reason. Worth noting the interaction: the previous round made an unreadable version report "not runnable rather than out of date", which was the right message and would have made this platform bug read as a confident, accurate diagnosis on every Windows machine. **A cached success now re-checks the floor, not just the pin.** The pin is only trustworthy because the floor is — engines below it do not lock the pin. An entry reconnected or replaced behind the same pin with a pre-floor binary rode the cached success indefinitely without passing through the attach flow again. Re-probed only when the entry's command changes, because probing spawns a process and this runs on every turn. The residual is narrow and stated rather than hidden: a binary swapped in place under an unchanged command is not noticed until the next session. That optimisation had a bug of its own, caught by its own test: the validated command was recorded on the outgoing entry while a fresh entry is built per call, so it was discarded and the probe ran every turn anyway. State that is not copied forward is state that is silently rebuilt. 3 tests: a cached success stops being trusted when the engine drops below the floor, an unchanged command is not re-probed per turn, and the existing suite pins the rest.
…e gap one found Phase 1 of the consolidation: the module's contract expressed as invariants rather than as one test per historical fix. A per-fix test says "this bug is gone"; an invariant says "this cannot happen", which is what catches the next instance of a class rather than the last one. Four fixes in this file's history created the following defect, and no per-fix test could have seen that. Six invariants, one per contract clause: one engine per project; no MCP mutation on a stale binding; every config read is fresh; an actionable failure is never silent; a superseded attach leaves nothing installed; a cached success is re-probed and re-attributed. The stale-binding one is a matrix that re-links the project at each await seam in turn, which is the shape that generalises. **One invariant was already violated, and that is the point.** The re-link matrix found a window fourteen review rounds had not: the guard after the engine add did not cover the tool listing that follows it, so a re-link during that read left the previous workspace installed and reported as attached. The two guards are now one, placed after every await that follows the install — late on purpose, since everything before the announcement is still revocable. Light dedupe, each subsumption proven by reverting the fix and confirming both tests fail: three per-fix tests dropped as covered by the invariants. Two candidates were kept because they are not covered — the pre-persist guard asserts the stronger "never installs at all" where the invariant only requires "never leaves it installed", and the floor re-check on a cached success is reached by no invariant. Coverage is not reduced. src 1,210 lines unchanged; tests 1,484 to 1,430 with six invariant suites added. The larger reduction belongs to Phase 2, where the guards themselves collapse into a transition function; deleting more tests before that would remove the net under it.
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:5a34902227
ℹ️ 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".
| await persist(DATAMATE_KEY, cfg) | ||
| await client.add(DATAMATE_KEY, cfg) |
There was a problem hiding this comment.
When an IDE transport is present, handleAdd treats any configured and connected DATAMATE_KEY as the generic single gateway and returns “already connected” for the requested datamate without checking its pin (packages/opencode/src/altimate/tools/datamate.ts:254-272). Persisting this workspace-specific --datamate <id> process under that same key means that, after workspace 42 attaches, asking datamate_manager to add datamate 99 falsely reports 99 as connected while the runtime continues exposing workspace 42's tools and credentials. Use a distinct key for the pinned process or make handleAdd validate and replace pinned entries.
Useful? React with 👍 / 👎.
…te you asked for Codex round 15. One finding, verified, and it is a consequence of this branch rather than something it inherited. Before this work the shared gateway key was generic: whatever was connected under it answered for any datamate, so "already configured and connected" was a true answer to "add datamate N". Pinning that same key to one workspace made the key workspace-specific while the manager tool kept treating it as generic. After workspace 42 attaches, asking to add datamate 99 reported 99 as connected while the runtime went on serving 42's tools — and 42's credentials. The gateway entry's pin is now checked. An unpinned entry is still the generic gateway and answers for any datamate, which keeps the extension-written shape working. A pin for a different workspace is replaced rather than reported as a success, which is what naming a datamate explicitly asks for. The two remedies suggested were a separate key for the pinned process, or validating the pin here. A separate key is not viable: reusing this key is what lets an IDE-started engine be adopted at all, and rule 1 is built on it. So the check belongs where the wrong assumption lives. The decision is extracted as `isPinnedToOtherWorkspace` and unit-tested directly. That is deliberate and worth stating: `handleAdd` has no test harness — it needs MCP, config and transport — and building one to reach a three-line predicate would have been a worse trade than testing the predicate. The wiring around it is covered by inspection, not by test, and the tests assert the decision, including that an unpinned entry keeps answering for any datamate.
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:fd5f2f8b67
ℹ️ 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".
| // the owning entry is global. Read the file before deciding. | ||
| // `existingEntry` is always fresh now, so `entry` already reflects disk. | ||
| const owning = entry | ||
| if (existing.status === "disabled" && owning?.enabled === false) { |
There was a problem hiding this comment.
When an IDE or direct config edit changes this entry to enabled: false while its existing client remains connected, MCP.status() still reports connected from runtime state (src/mcp/index.ts:875-878), so this disabled check is skipped and the engine continues serving tools; for an unpinned or foreign entry, the later replacement path can even persist it enabled again. Fresh evidence beyond the earlier disabled-status fix is this reverse mismatch—fresh config is disabled while runtime status remains connected—so gate on entry?.enabled === false before branching on runtime connectivity.
Useful? React with 👍 / 👎.
…ivity Codex round 16. One finding, verified, and it is the mirror of round 9's. Round 9 handled the runtime saying "disabled" while the config said enabled — a synthesized status that a teardown produces. This is the reverse: the config says `enabled: false` while MCP still reports "connected" from live client state, because an IDE or a direct edit can disable the entry without stopping the running client. The disable check was nested inside the not-connected branch, so that case was skipped entirely — and for an unpinned entry the replacement path below would then have persisted it enabled again, undoing the very edit the user had just made. The check is lifted above the connectivity branch. The config's `enabled` flag is the only place a user expresses "off", so it is consulted before anything else; runtime connectivity answers a different question and cannot stand in for intent. The two sources disagree in both directions, and each direction cost a round to find. That is the argument for reading intent from one authority rather than inferring it from whatever signal is nearest. Residual, named rather than hidden: a client that is already connected keeps serving until MCP drops it. This flow stops attaching and stops re-enabling, but it does not tear down a live client on the strength of someone else's config edit. 1 test: a config disable is honoured while the runtime still reports connected — no attach, no persist, no reconnect. It fails with the check re-nested.
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:42fb81633a
ℹ️ 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 (!(await stillCurrent())) { | ||
| log.info("binding changed while reusing; abandoning rather than answering for the old workspace", { | ||
| workspaceId, | ||
| }) | ||
| return { kind: "superseded" } |
There was a problem hiding this comment.
When the project is re-linked from workspace A to B while declaredBounded() is running, this new guard returns superseded but leaves A's client and tools registered in MCP. The caller proceeds directly to resolveTools regardless of the outcome (session/prompt.ts:1026-1049), so the current invocation still receives A's tools and credentials under binding B. Fresh evidence beyond the earlier reuse-race comment is that the added guard only returns an outcome and never calls client.remove; detach the old client before returning or otherwise exclude it from the ensuing tool snapshot.
Useful? React with 👍 / 👎.
| if (!(await stillCurrent())) { | ||
| log.info("binding changed before the attach could be reported; removing what we installed", { workspaceId }) | ||
| await client.remove(DATAMATE_KEY).catch((err) => { | ||
| log.warn("could not remove the superseded engine", { err: String(err) }) | ||
| }) |
There was a problem hiding this comment.
If the binding changes while client.add(), the status check, or the tool listing is awaited, persist() has already written the old workspace ID into the project config. This cleanup closes only the runtime client, leaving that stale entry enabled on disk; a restart before the next B attach can therefore bootstrap workspace A again, and an unbound or pilot-disabled run will leave it active. Remove or restore the persisted entry on this superseded path, or defer committing the pin until the attachment has been generation-validated.
Useful? React with 👍 / 👎.
Codex round 17. Two findings, both gaps in the supersede path this branch added, and both exposed by strengthening an invariant that had been passing while they existed. **A superseded reuse now detaches, rather than only declining.** Returning `superseded` left the previous workspace's client registered, and the caller runs resolveTools whatever the outcome is — so that turn was handed the old workspace's tools and credentials regardless. The outcome is advice; the registration is what the model actually sees. Declining to answer is not the same as not answering. **A superseded attach now restores the config, not just the runtime.** persist() commits the pin before the engine is known to be ours, so undoing only the runtime client left the abandoned workspace pinned on disk — and MCP bootstraps every enabled entry, so a restart before the next attach would start the workspace we had just walked away from. The previous entry is put back, or ours removed if there was none. The invariant that should have caught both asserted only that the runtime client was removed. It passed while a stale pin sat on disk and while the reuse path detached nothing. An invariant is only as good as its definition of "nothing": it now covers the config and the reuse path, and each half fails independently when its fix is reverted. That is the second time an invariant has been the thing that found the gap, and the first time one of them was itself too weak — worth remembering when Phase 2 leans on them.
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:3597444766
ℹ️ 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 (sameWorkspace && !isRepairable(previous!.outcome)) { | ||
| // Re-probe before trusting a cached success — see `engineStillConnected`. | ||
| const boundTo = await attachKeyWorkspace() | ||
| if (!wasServing(previous!.outcome) || !boundTo || (await engineStillOurs(boundTo, entry))) return previous!.task |
There was a problem hiding this comment.
When a session has a cached success for workspace A and the project is re-linked to B while engineStillOurs() awaits status, config, or version work, boundTo remains A and the probe can confirm that the still-running A engine is valid, causing this branch to return the old successful task without reading the binding again. The current prompt then snapshots A's tools and credentials under binding B until the next turn triggers replacement. Fresh evidence beyond the earlier reuse-path guard is that this memoized-success path performs its own awaited validation outside run() and has no corresponding final binding check.
Useful? React with 👍 / 👎.
| }) | ||
| // And the config: `persist()` committed the pin before the engine was known | ||
| // to be ours, and bootstrap starts every enabled entry. | ||
| await persistRestore(DATAMATE_KEY, entry) |
There was a problem hiding this comment.
When the previous datamate entry comes only from global config, existingEntry() returns that merged global value, while persist() writes the temporary workspace pin into the project config. If the attach is then superseded, passing the merged value here writes a copy of the global entry into the project file instead of removing the project override; the project permanently shadows later global updates, disables, or removal even though the abandoned attach was meant to leave configuration unchanged. Snapshot the raw project entry before persisting and restore that value instead.
Useful? React with 👍 / 👎.
…re the project entry Codex round 18. Two findings, both verified, both proven by revert. **The memoised-success path needed the final binding check too.** Validating a cached success is itself awaited work — status, config, and sometimes a version probe — so the binding can move underneath it. That path lives in `ensure()`, outside `run()`, and so never had run's closing check: a confirmed-valid engine for the workspace just left was returned as the answer for the one just joined, and the turn took its tools and credentials. This is the same rule that has now been applied in four places: revalidate before answering, because an answer this flow gives is acted on. The rule was right; it had not been carried to the one path that does its validating elsewhere. **A superseded attach now restores the project entry, not the merged one.** `existingEntry()` returns the merged view, which may come from global config, while `persist()` writes to the project file. Restoring the merged value wrote a copy of the global entry into the project — a permanent override shadowing every later global update, disable or removal, produced by an attach that was meant to leave configuration untouched. The project file's own entry is snapshotted before persisting and that is what goes back, which means removing the override when there was nothing there before. Undoing a write is only correct if it restores what that write replaced, and the thing replaced was never the merged view.
ralphstodomingo
commented
Aug 26, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
altimate-code/packages/opencode/src/altimate/tools/datamate.ts
Lines 321 to 322 in 37dd23d
Pin the replacement gateway to the requested datamateWhen an IDE transport exists and the saved datamate entry is pinned to another workspace, this branch replaces it with mcpConfig, but that value is merely the IDE's unchanged URL or argv and never incorporates args.datamate_id (lines 243-252). It may therefore remain pinned to the old workspace, or be unpinned and follow the IDE's active workspace as documented in engine-sync.ts:14-20, while the result at lines 353-356 claims the requested datamate was connected. Fresh evidence after the earlier foreign-pin fix is that its replacement path still does not encode the requested workspace.
ℹ️ 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".
| // enabled again, undoing the very edit the user made. | ||
| // | ||
| // `existingEntry` is always fresh, so `entry` already reflects disk. | ||
| if (entry?.enabled === false) { |
There was a problem hiding this comment.
When a user or IDE changes a live datamate entry to enabled: false, this branch reports entry-disabled but leaves the connected runtime client registered. MCP.tools() enumerates connected runtime clients without checking the config's enabled field (src/mcp/index.ts:1022-1041), and resolveTools runs immediately afterward, so the supposedly disabled workspace tools and credentials remain available to the model. Fresh evidence after the earlier live-disable fix is that the newly added branch never calls client.remove() before returning.
Useful? React with 👍 / 👎.
| const entry = await existingEntry(DATAMATE_KEY) | ||
| if (pinnedWorkspace(entry) !== workspaceId) return false |
There was a problem hiding this comment.
For a session with a memoized successful attach, changing the pinned entry to enabled: false is still ignored: engineStillOurs() reads the fresh entry but validates only connectivity, pin, and version, then ensure() returns the previous successful task without entering run() and its disable check. The next prompt therefore continues exposing the disabled client's tools. Fresh evidence after the live-disable fix is this separate cached-success shortcut, which bypasses that fix entirely.
Useful? React with 👍 / 👎.
| } | ||
| // Snapshot what persist() is about to overwrite — the project file's own | ||
| // entry, not the merged view — so a supersede can put back exactly that. | ||
| const projectBefore = await projectEntry() |
There was a problem hiding this comment.
If the project is re-linked while this asynchronous disk read is pending, the stillCurrent() check immediately above has already passed, so the stale workspace is persisted and handed to client.add(). The replacement attach is serialized behind this one and its 15-second prompt wait can expire while the stale handshake runs, recreating the checked-to-write race that the earlier mutation guard addressed. Fresh evidence is that the newly added project-entry snapshot introduced an awaited seam between that guard and both mutations; revalidate again after this await.
Useful? React with 👍 / 👎.
…estore fix opened Three defects, each proven by individual revert. A disabled entry stopped at the config. `MCP.status()` reports live client state and `MCP.tools()` gates on exactly that status — it reads the config only for a timeout — so an entry disabled AFTER it connected kept exporting its tools and credentials to `resolveTools`. The branch already documented this in a comment and still returned without touching the runtime. It now detaches through the existing rejection path, which is runtime-only and writes no config: respecting the edit, not re-applying it. The same check was unreachable from the memoised-success path. Validation covered connectivity, pin and version but never `enabled`, so a session that had already attached rode its memo past a disable for the rest of its life. The check goes in ahead of the command-unchanged shortcut, and returns false rather than detaching directly — routing the session back through `run()`, where the reporting and the teardown already live. The third was introduced by the previous commit: snapshotting the project entry for a restore put an awaited disk read between the final binding check and the install it guards. The late guard would undo the stale attach, but only after spawning an engine and taking the per-project lock — long enough for the replacement's first-turn wait to expire, which is the failure that guard exists to prevent. The snapshot moves above the check, so nothing awaits between the check and the mutations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y9X4vNjkwQ3vTiJ8w1kKR6
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