feat(workspace): attach the bound workspace's integration engine - #1154
feat(workspace): attach the bound workspace's integration engine#1154ralphstodomingo wants to merge 67 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.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 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 not async 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.
|
@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.
|
@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 other Config.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.
|
@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. |
|
@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: After the rewrite: two adversarial gate roundsThe rewrite was not reviewed by another codex round. It was reviewed by five Every lens found something, and the two that mattered most were not code
Attribution judged the config while the running client is whatever MCP started. Guards, and the rules that turned out to be localA guard on half the world is a guard on none of it. The pre-write check re-read A guard that fails open is worse than no guard, because its presence is what And that fix was itself unreachable: the reader beneath it swallowed and returned The sentence that explains this branchEvery fix here was correct exactly as far as its author's attention reached. The check order — intent, then connectivity, then attribution — was an artifact The corollary came from the branch this one integrates with: a confirmation is What the reviews could not have foundTwo defects existed only in the join between this branch and the install-offer The same exchange produced the defect class that no invariant here had asked The invariants the suite now holdsNot a numbered list — only the last one carries a number, because it arrived What may run, and for whom
When a mutation is allowed
What must be given back
What the user is told
What the union means
#13 — a failed read is never an answer, in three places, because one was And one suite named for what produced it rather than what it asserts: the Known residualsNamed as they were found rather than discovered later. Each is a bounded limit Not atomic, and cannot be made so here
In the attach flow
Coverage that is honest rather than complete
Left to other work
|
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c7ef580ee
ℹ️ 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".
| // Read as unreadable, it is detached and refused once, and the memo holds. | ||
| let found: string | null | ||
| try { | ||
| found = await engineVersionOf(entry) |
There was a problem hiding this comment.
When an IDE or direct config edit changes the command while the existing client remains connected, inspection.runtime and entry can have the same workspace pin but refer to different binaries. This probes the newly configured binary rather than the process recorded in inspection.runtime, so a new >=0.7 command can authorize reuse of a still-running pre-0.7 engine; that engine does not lock its workspace pin and can drift to another workspace. Probe the runtime entry whenever one is present.
Useful? React with 👍 / 👎.
| if (now?.enabled === false) { | ||
| log.info("the entry was disabled while we held it; keeping the disable rather than undoing it", { | ||
| workspaceId, | ||
| }) | ||
| const keep = projectBefore ? ({ ...projectBefore, enabled: false } as ExistingEntry) : now | ||
| return await persistRestore(DATAMATE_KEY, keep, configPath) | ||
| } | ||
| return await persistRestore(DATAMATE_KEY, projectBefore, configPath) |
There was a problem hiding this comment.
When the IDE or /altimate/mcp/reload-datamate writes a new enabled command or URL after this attach persists its pin but before the handshake or tool lookup finishes, now.enabled remains true and this rollback restores the stale projectBefore value over that newer edit. A failed or superseded attach can therefore silently discard a user-authored transport change; restore only if the current entry still matches the exact config installed by this attach.
Useful? React with 👍 / 👎.
| 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.
When another caller replaces datamate through MCP.add() before this attach rolls back, this unconditional removal closes the replacement rather than necessarily closing the client created here. Both the MCP route and the IDE reload handler call MCP.add() outside serializeAttach, so the race leaves the newly requested engine disconnected and its tools unavailable; generation-check the spawned entry/client before removing it.
Useful? React with 👍 / 👎.
Codex round 2: one P1 and two P2s, and they are two rules rather than three patches. The floor was asked of the CONFIGURED command while attribution was asked of both the config and the running engine. A config edit can change the command while the existing client stays connected, so the two can carry the same pin and be different binaries — and a freshly configured 0.7 command would then authorise reuse of a running pre-0.7 engine, which does not lock its pin and can drift to another workspace while we report this one. The pin and the floor are one mechanism; they are now asked of the same thing. The undo assumed that whatever it found was its own. It is not: the MCP route and the IDE's reload both call `MCP.add` outside this flow's serialization, and an IDE or the user can rewrite the file. So an unconditional removal could close the engine another caller had just installed, leaving it disconnected with its tools gone, and an unconditional restore could roll back a new command or URL written after our pin. Both halves now check that what is there is still what this attach put there — a disable was already honoured, and any other edit is equally not ours to undo. The undo harness modelled a project file that never received the write, so its stub returned the pre-install entry at undo time. It mirrors production now; a fixture that under-models the write cannot see whether the undo checks what it is undoing. Each fix is proven by reverting it alone, including the runtime-version case, which nothing had staged: a runtime binary differing from the configured one under the same pin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y9X4vNjkwQ3vTiJ8w1kKR6
The previous commit applied "anything asking about the engine that is RUNNING asks the runtime record" to the fresh-attach path and not to the memo path beside it — which is the path every turn after the first takes. So the same defect survived where it matters most: a memo attached to a running pre-floor engine, then a config edit to a floor-clearing command under the same pin. The plan agrees (the pins match), the command has changed so the floor is re-probed — and the probe reads the CONFIGURED binary, clears the floor, records that as validated, and the running pre-floor engine keeps serving for the rest of the session behind a memo that looks valid. A pre-floor engine does not lock its pin, which is the drift this check exists to exclude. The floor is now asked of the running engine here too, and the re-probe key carries both commands rather than the config's alone: a divergence between them is precisely the case that needs re-probing, so it cannot be the case that gets skipped. Two comments in the same block: the docblock still opened "Fails OPEN", which the code below it contradicts, and it narrated what an earlier version of the function did. Both replaced by what the function guarantees now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y9X4vNjkwQ3vTiJ8w1kKR6
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69b782499d
ℹ️ 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".
| log.info("skipping teardown; the binding changed while this attach was deciding", { workspaceId, ...why }) | ||
| return | ||
| } | ||
| await client.remove(DATAMATE_KEY).catch((err) => { |
There was a problem hiding this comment.
When an IDE reload or MCP route replaces the rejected client after inspection but while stillCurrent() is awaited, this unconditional removal closes the newly installed client rather than the one that was judged unattributable. If the local PATH engine is then missing or unusable, the flow returns a refusal with the valid replacement gone, so its tools and credentials disappear from the current session. Fresh evidence beyond the prior rollback-removal thread is that undoInstall() now generation-checks spawned(), but this separate rejection teardown still does not.
Useful? React with 👍 / 👎.
| const keep = projectBefore ? ({ ...projectBefore, enabled: false } as ExistingEntry) : now | ||
| return await persistRestore(DATAMATE_KEY, keep, configPath) | ||
| } | ||
| if (now && !sameCommand(now, installed as unknown as ExistingEntry)) { |
There was a problem hiding this comment.
When an IDE or user updates a local entry's environment, cwd, or timeout while this attach is in flight but leaves its argv unchanged, sameCommand() treats the newer entry as the temporary one and this branch restores projectBefore, silently discarding those edits. The same argv-only check can also remove a runtime replacement carrying different environment settings. Fresh evidence beyond the prior enabled-edit thread is that the new comparison covers only commandArgv, although these other fields affect the spawned process; compare the complete relevant transport configuration before undoing it.
Useful? React with 👍 / 👎.
…place Six findings in a row have been the same shape: a rule applied at one site and not at the site beside it. This commit answers the shape rather than the sites, and folds in round 3's two findings, which are both instances of it. Two named accessors replace field reads. `runningEngine(inspection)` answers "the engine that is running, else the one configured"; `configuredEntry` answers "the entry the user configured, whatever may be running". No site reads `inspection.entry` or `inspection.runtime` bare. Naming one and not the other would leave the other implicit, and implicitness at the point of use is the condition this class grows in — the field access whose meaning has to be inferred from what happens to surround it. One place verifies identity before destroying. Round 2 added that check to the install's undo; round 3 found the rejection teardown beside it without one, so a replacement installed between judging a client and closing it was closed instead — leaving the engine someone else had just asked for disconnected, with its tools and credentials gone from the turn. Every teardown now goes through `removeIfOurs`, which also owns the binding check, because ordering two guards across two functions is how one of them ends up on the wrong side of the other. Identity first, binding last, so the binding read stays the last await before the mutation — the adjacency invariant caught that ordering when I got it wrong here. Identity now means the whole transport. Comparing argv alone read an edit to `environment`, `cwd` or `timeout` as "still the entry I wrote", so the undo reverted a deliberate change while believing it was reverting its own write. The shape test asserts what no behavioural test can: the fallback expression appears exactly once, both accessors exist, and no bare field read survives. It is weaker than the rest of this suite on purpose — the defect is not a wrong answer at a site, it is a second site existing at all, and a test cannot exercise a site that does not exist yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y9X4vNjkwQ3vTiJ8w1kKR6
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4c8be46c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void WorkspaceEngine.ensure(sessionID).catch(() => {}) | ||
| await WorkspaceEngine.whenAttached(sessionID) |
There was a problem hiding this comment.
When a project already has a connected standalone entry such as datamate-foo—created by datamate_manager with the hosted URL—this attach adds the local datamate client but never disables or filters the hosted one. The following resolveTools call enumerates every connected MCP client, so the model receives both the bound workspace's local tools and the hosted tools, potentially under another datamate's credentials, despite the stated no-hosted-fallback contract. Filter those clients from bound turns or otherwise reconcile them before resolving tools.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Decision on this, from the product owner, with the reasoning so it is not re-raised.
The finding is real and the premise is confirmed. datamate_manager creates standalone datamate-<name> entries pointing at the hosted endpoint, and this flow owns only the datamate key — so after a successful attach the model can hold both tool sets, potentially under another datamate's credentials.
We surface it, we do not filter it. Filtering a server the user added deliberately, out of their own turns, is a product decision and it lives outside this module. Silently dropping their configured server would trade one surprise for another.
Implemented in cde3f61d8: on a successful attach or reuse, if any standalone datamate-<name> entry is also connected, one signal per session per SET names them and says plainly that their tools serve a different datamate under its own credentials. A stable set says it once; a changed set says it again. It is a second signal beside the attach toast rather than folded into it, because two different things happened.
It costs no extra read — the inspection already reads every server's status and keeps that record.
Named residual: the note rides the flow's decisions, so a set that changes while a memo stays valid is surfaced at the next re-decision rather than the moment it changes. The alternative is a status read on every turn for a warning, which is the wrong trade. Asserted as a test rather than left implicit.
| if (entryNow?.enabled === false) { | ||
| log.info("intent changed while deciding; not writing over a disable", { workspaceId }) | ||
| return "disabled" | ||
| } | ||
| if (!(await stillCurrent())) return "moved" | ||
| return "ok" |
There was a problem hiding this comment.
If an IDE or user replaces the inspected MCP entry with a different enabled command or URL while the version/catalog probes are running, this guard still returns ok because it checks only enabled and the workspace binding. The subsequent persist() then overwrites that newer entry, and MCP.add() can replace its newly started client. Compare entryNow with the entry used to derive the plan, or re-inspect and replan, before mutating.
Useful? React with 👍 / 👎.
| export async function engineVersionOf(entry: ExistingEntry | null): Promise<string | null> { | ||
| const bin = commandArgv(entry)[0] | ||
| const direct = bin && /(^|[\\/])datamate(\.[a-z]+)?$/i.test(bin) ? bin : null | ||
| return direct ? await versionOf(direct) : null |
There was a problem hiding this comment.
For a local entry whose bare datamate command uses a custom environment.PATH, this executes datamate --version with the parent process environment rather than the environment used by MCP to spawn the running client. A modern binary on the parent PATH can therefore approve a pre-0.7 engine actually selected by the entry's PATH, allowing reuse of an engine that does not lock its workspace pin; relative commands with a configured cwd are similarly probed from the wrong directory. Pass the entry's environment and resolved cwd to the version probe.
Useful? React with 👍 / 👎.
| export const MIN_ENGINE_VERSION = "0.7.0" | ||
| export const INSTALL_HINT = "npm i -g @altimateai/datamate" | ||
| export const ENGINE_BINARY = "datamate" |
There was a problem hiding this comment.
The newly added workspace sibling modules expose flat symbols but omit the required self-reexport, so consumers cannot use the package's prescribed namespace projection. Add an export * as ... from "./..." projection to each new sibling and import that projection from consumers.
AGENTS.md reference: packages/opencode/AGENTS.md:L17-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one, with the evidence, so it is not re-raised.
The self-reexport rule in AGENTS.md is for Effect service modules — the example it gives is Interface / Service / layer / defaultLayer, and src/mcp/index.ts follows it because it is one.
The five new modules are plain function modules, and they match their directory exactly: no module in src/altimate/workspace/ has a self-reexport — all fourteen pre-existing ones included (state.ts, detect.ts, memory-sync.ts, launch-resolve.ts, browser-handoff.ts, api-client.ts, and the rest).
Adding projections to only the five new siblings would make them the odd ones out against the convention the directory actually follows. If that directory should adopt the pattern, it is one change across nineteen files with its own reasoning — not five files added by this branch.
…e the engine runs Codex round 4, plus the identity-field exhaustiveness prepared earlier. The pre-write guard checked intent and the binding but not WHICH entry the plan was derived from. An IDE replacing that entry with a different enabled command while the probes run therefore left the plan describing something that was no longer there — and acting on it overwrote a newer entry and could displace the client it had just started. A disable is one way the entry can change; it is not the only one. The guard now compares against the entry the plan came from, before the write only: after the write what is on disk is our own, so there is no plan to compare, and a third-party rewrite landing later is answered by the undo, which already refuses to roll back an entry that is no longer ours. The version probe ran in this process's environment rather than the one the entry would be spawned in. A bare `datamate` under a custom `environment.PATH` resolves to a different binary than our PATH does, so a modern binary we happen to have could approve the pre-floor engine the entry actually selects — and a pre-floor engine does not lock its pin, which is the drift the floor exists to exclude. A relative command with a configured `cwd` was resolved from the wrong directory for the same reason. The probe takes both. Identity is now exhaustive at compile time. `IDENTITY_FIELDS` is keyed by `ExistingEntry`, so adding a field there fails the build until someone lists it or excludes it deliberately — verified by adding one and watching it fail by name. That closes the limit named in the previous commit, where a field the TYPE gained could pass unnoticed; `enabled` stays excluded, because a disabled entry is still the same entry. The entry type gains `environment`, `cwd` and `timeout`, which identity was already reading through casts. One test changed contract rather than expectation: an entry appearing between the config read and the write is no longer spawned over. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y9X4vNjkwQ3vTiJ8w1kKR6
`datamate_manager` can add standalone `datamate-<name>` entries pointing at the hosted endpoint, and those keep their own clients. This flow owns one key and never touches theirs, so after a successful attach the model can hold two tool sets at once — ours for the bound workspace, and another datamate's under its own credentials. That is the misattribution this branch exists to prevent, arriving from a direction the flow does not control. Surfaced, not filtered. The user added those servers deliberately, and removing a server from their own turns is a product decision rather than this module's to make; the ambiguity is made visible instead of silently resolved either way. One signal per session per SET, so a stable configuration says it once and a change says it again. It is a second signal beside the attach toast rather than folded into it, because two different things happened — the rule is one signal per event, not one element per screen. It costs no extra read: the inspection already reads every server's status, so it keeps that record and the neighbours are answered from the same moment as everything else the inspection decided from. Named residual, and asserted as such: the note rides the flow's decisions, so a set that changes while a memo stays valid is surfaced at the next re-decision rather than the moment it changes. The alternative is a read on every turn for a warning, which is the wrong trade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y9X4vNjkwQ3vTiJ8w1kKR6
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cde3f61d89
ℹ️ 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".
| // Late rather than early on purpose: the check is only meaningful at the | ||
| // last moment before we announce and answer, because everything before that | ||
| // is still revocable. The undo itself now belongs to the region. | ||
| const afterInstall = await worldUnchanged() |
There was a problem hiding this comment.
When the IDE reload endpoint or MCP route replaces datamate while the post-add status/tool awaits are running, this guard calls worldUnchanged() without an expected entry and therefore accepts any enabled replacement as ok. The attach is then committed and reported as serving the bound workspace even if the replacement is unpinned or pinned elsewhere, so the ensuing tool snapshot can expose another workspace's tools and credentials; verify that the runtime client and persisted entry still match cfg before setting committed.
Useful? React with 👍 / 👎.
| // `add` is none of those: it writes no config and starts exactly what it is | ||
| // handed. Reviving becomes the same operation as spawning, which is the | ||
| // real win — the retry stops being a special path with special rules. | ||
| const revive: LocalMcpConfig = { type: "local", command: commandArgv(configuredEntry(inspection)), enabled: true } |
There was a problem hiding this comment.
When a failed local entry relies on environment, cwd, or a custom timeout, this reconstruction keeps only its flattened argv. MCP.add() consequently restarts the child with the parent environment, project root, and default timeout rather than the settings under which the configured engine was intended to run; custom-PATH binaries may become unavailable or a relative command may resolve incorrectly. Carry the complete local transport configuration into the retry.
Useful? React with 👍 / 👎.
…ransport Codex round 5: one P1, one P2, both mine from the round before. After the install, the guard asked whether the world had moved but not whether what is SERVING is still ours. The status and tool reads are two awaits, and both the MCP route and the IDE's reload call `MCP.add` outside this flow's serialization — so a replacement landing there was committed and reported as the bound workspace's engine, and its tools and credentials reached the model. The runtime is now compared against what we installed before committing. Scoped to the runtime half deliberately. What is on disk after our write is our own, and an edit landing on it afterwards belongs to the undo, which already refuses to roll back an entry that is no longer ours; comparing it here would ask the same question twice and answer it in two places. The revive rebuilt the entry as bare argv, dropping `environment`, `cwd` and `timeout`. Those are what the configured engine was meant to run under — a custom PATH may be the only place its binary exists, and a relative command resolves from `cwd` — so the retry restarted a different process than the one that failed. It carries the whole transport now, which is the same lesson identity comparison learned two commits ago, applied to reconstruction. Three test harnesses were preferring their starting entry forever, modelling a config file that never received the write. That is invisible until something asks whether what is installed is still its own, and then it answers "no" for every successful attach — which is why I scoped this check away in the previous round instead of fixing the fixtures. Fixed here. The fourth harness is left alone: it models the file live and inferring from what was persisted would shadow the rewrites it exists to exercise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y9X4vNjkwQ3vTiJ8w1kKR6
…uestions first `reused` is an answer, like `attached`: it names the engine this turn is served by. The install region already re-asked, after its awaits, whether the world was unchanged (binding and intent) and whether what is serving is still the client it installed. The reuse answer re-asked only the binding, after the same kind of awaits (the tool listing, the allowlist lookup) that the MCP route and the IDE's reload can land an `MCP.add` inside — so a replacement was answered `reused` for the bound workspace, and a disable landing there served the turn from an engine the user had just switched off. One helper, `confirmServing`, now asks both questions for both answers. It returns a verdict rather than an outcome so that each caller runs its own teardown first — the install region undoes what it installed, the reuse answer detaches what it judged — and only then announces; a replacement is never ours to detach. The missing-declared-tools warning on the reuse path moves below those questions: nothing that names an engine, answer or toast, is emitted before they are asked. Three existing tests asserted the older contract (a disable landing between the inspection's two reads was served for a turn and repaired on the next; the retry path made three intent reads); they now assert the property. Reverting the source fails five named tests; disabling only the serving check fails the replacement test at both callers.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37cabe5ed8
ℹ️ 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 announceToolsChanged() | ||
| await notify({ |
There was a problem hiding this comment.
When the project is re-linked while announceToolsChanged(), the success notify(), or noteHostedNeighbours() is awaited, committed is already true and there is no subsequent binding/client check. run() therefore resolves attached for the old workspace and finally preserves its client; SessionPrompt waits for this task and then resolves tools, so the current turn under the new binding can receive the old workspace's tools and credentials. Return the outcome before these best-effort announcements or revalidate and undo after the final await.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5de0acf. The answer was fixed before the announcements and given after them; the world (binding, intent, and the runtime's launch record) is now asked once more after the last announcement, and a change there undoes the install and answers superseded — the toast was true when shown, the answer is true when given, no second toast. Revert-proven by three named tests.
| const servingNow = client.spawned ? await client.spawned(DATAMATE_KEY).catch(() => undefined) : undefined | ||
| if (servingNow && judged && !sameEntry(servingNow, judged)) return "replaced" | ||
| return "ok" |
There was a problem hiding this comment.
When another caller disconnects or removes datamate after the preceding status/tool reads but before this lookup completes, spawned() returns undefined. This condition treats that as success because it rejects only a different non-null record, so both the attach and reuse paths can report a serving engine even though the client has disappeared. Require servingNow to exist and match judged before returning ok.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5de0acf. No launch record now yields the verdict gone, handled at both callers like a replacement: the install is undone, the reuse answer is superseded, nothing is detached because nothing is there. Revert-proven at both callers.
…when it is given Two holes in the questions every named answer asks. A client that vanished is not a client that was replaced. `confirmServing` compared the runtime's launch record against the engine that was judged, but treated NO record as agreement — so a client removed or disconnected during the tool listing or the allowlist lookup was still answered `attached` or `reused`. The record is the only witness to "still serving"; its absence is now the verdict `gone`, handled at both callers like a replacement: the install is undone, the reuse answer is `superseded`, and nothing is detached because nothing is there. A harness that does not model the record is not asked. The attached answer was fixed before the success announcements and given after them, and the announcements are three awaits. A re-link, a disable or a replacement landing inside them left this turn holding `attached` for an engine that no longer served the bound workspace — the one residual the install region still named. Every await after a guard belongs to the guard: the world is asked once more after the last announcement, and the install is undone and answered `superseded` if it moved. The toast was true when it was shown; the answer is true when it is given; no second toast. The test that asserted the residual now asserts its closure; a read-count fixture gains the fourth intent read of the first attach. Removing the `gone` verdict fails two named tests; removing the post-announcement check fails three.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5de0acf143
ℹ️ 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".
| export async function engineVersionOf(entry: ExistingEntry | null): Promise<string | null> { | ||
| const bin = commandArgv(entry)[0] | ||
| const direct = bin && /(^|[\\/])datamate(\.[a-z]+)?$/i.test(bin) ? bin : null | ||
| return direct ? await versionOf(direct, { environment: entry?.environment, cwd: entry?.cwd }) : null |
There was a problem hiding this comment.
Fresh evidence after the earlier probe-environment fix is that this forwards entry.cwd unchanged, whereas MCP.connectLocal() resolves the same value against the active instance directory. With a relative cwd in a multi-project server, the probe runs under the process startup directory while the live engine runs under the project directory; it can therefore reject a valid engine or let a modern binary in one directory authorize a pre-floor engine in another. Resolve the probe cwd against the same instance directory used by MCP.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf30354. The probe now resolves a relative cwd against the instance directory exactly as MCP.connectLocal does, so it asks the binary the engine actually launches; an absolute cwd and no cwd are passed through unchanged. Revert-proven.
| const command = `${commandArgv(running).join(" ")}|${commandArgv(configuredEntry(inspection)).join(" ")}` | ||
| if (record && record.validated === command) return true |
There was a problem hiding this comment.
Fresh evidence beyond the cached-version fixes is that the validation key contains only argv. If the MCP route or IDE reload replaces a running bare datamate command with the same argv but a different environment.PATH or cwd, record.validated still matches and the replacement is accepted without probing; a previously validated modern engine can thus authorize a newly running pre-0.7 engine whose workspace pin may drift. Key the cache on all launch settings that affect executable resolution, or invalidate it whenever the spawned entry changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf30354. The memo's re-probe key now carries the whole launch identity of both halves (transport, argv, environment, cwd, timeout) via entryIdentity, the same single definition sameEntry uses — a replacement with the same argv under a different PATH or working directory is probed again. Revert-proven.
| await noteHostedNeighbours(reused) | ||
| return reused |
There was a problem hiding this comment.
Fresh evidence beyond the attached-path announcement fix is that the reuse path still returns immediately after awaited announcements. If the project is re-linked, disabled, or the client is replaced while the missing-tools toast or noteHostedNeighbours() is awaiting, the earlier confirmServing() result is stale and this returns reused; the prompt then snapshots the old workspace's tools and credentials under the new binding. Run the same final serving check used by the attached path after the last reuse announcement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf30354. The reuse answer asks the same two questions again after its announcements (the missing-tools warning and the hosted-neighbours note) and settles a change the same way — the verdict handling is one closure asked twice, so the two askings cannot drift. Revert-proven.
| const keep = projectBefore ? ({ ...projectBefore, enabled: false } as ExistingEntry) : now | ||
| return await persistRestore(DATAMATE_KEY, keep, configPath) |
There was a problem hiding this comment.
Fresh evidence beyond the full-entry rollback comparison is that this enabled === false branch bypasses that comparison. If a user or IDE replaces the temporary pin with a different command, URL, environment, or cwd and disables it before rollback, projectBefore is truthy and this overwrites the newly authored transport with the old transport plus enabled: false, silently discarding the edit. Preserve now when its full identity differs from installed; only project the disable onto projectBefore when the disabled node is otherwise still the one installed here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf30354. The enabled === false branch now checks identity first: an entry that was rewritten AND disabled while held is left untouched (neither the transport nor the disable is ours); the disable is projected onto projectBefore only when the disabled node is otherwise still the one this attach installed. Revert-proven.
…p asking until the answer is given Four sites where a rule stated in one place had not reached its neighbour. The reuse answer, like the attached one, is given after announcements that are awaits; it now asks the same two questions again after the last of them and settles a change the same way it would have before them. The verdict handling is one closure asked twice, so the two askings cannot drift. The version probe resolves a relative `cwd` against the instance directory, as the engine's own launch does; probed against wherever this process started, a relative command or PATH entry could name a different binary than the one the engine runs. The memo's re-probe key carries the whole launch identity of both halves, not their argv: a replacement with the same argv under a different PATH or working directory runs a different binary and is probed again. `entryIdentity` is the one definition, shared with `sameEntry`. The undo keeps an entry that was rewritten AND disabled while it was held. Projecting the disable onto what the undo replaced would have overwritten the newer transport with the old one; neither the transport nor the disable is ours. A read-count fixture gains the reuse answer's second intent read. Reverting each of the four fails a named test.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf30354f17
ℹ️ 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".
| // path lives outside `run()` and therefore never had its final check; | ||
| // without one, a confirmed-valid engine for the workspace we just left is | ||
| // returned as the answer for the one we just joined. | ||
| if (reusable && (await attachKeyWorkspace()) === boundTo) return previous!.task |
There was a problem hiding this comment.
When a same-workspace cached success is being validated and the IDE reload endpoint or MCP route replaces datamate after memoStillValid() performs its final spawned() read but while this final attachKeyWorkspace() call is awaiting, the unchanged binding makes this condition return the old successful task. resolveTools then immediately snapshots the replacement, which may be unpinned or pinned to another workspace, under the cached workspace attribution. Recheck the runtime launch record after the binding read before returning the memo.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc941b1. The memo validation now records the launch identity of the runtime it judged, and after the final binding read the record is read once more; the memo is returned only if it still names that engine — otherwise the turn re-attaches. Revert-proven.
| return await refuseDisabled() | ||
| } | ||
| installed = true | ||
| await client.add(DATAMATE_KEY, cfg) |
There was a problem hiding this comment.
When /altimate/mcp/reload-datamate or the MCP route calls MCP.add(DATAMATE_KEY, ...) while this add is still creating its client, serializeAttach provides no ordering because those callers bypass it. If the external add succeeds first and this creation then fails, MCP.createAndStore calls closeClient(s, name) on the currently registered client (src/mcp/index.ts:933-948), thereby closing the successful external replacement rather than the client associated with this attempt; successful completions can likewise overwrite each other according to completion order. Serialize all adds for this key or make the store/failure path generation-aware.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc941b1 (MCP side, marked). createAndStore captures the client it is replacing before creation; on failure it closes only that client, and if another caller registered under the key meanwhile, theirs is left alone — client, status and launch record. Completion-order overwrite between two successful adds remains last-writer-wins by design; the attach flow detects it afterwards (verdict replaced/gone) and never detaches a client it did not install. Revert-proven in the MCP lifecycle suite.
…ed replacement closes only what it replaced Two more places where the second question was asked one read too early. The memo path validated the runtime record, then read the binding one last time, then returned the memo. A client registered under the key between those two reads — the MCP route or the IDE's reload — was what `resolveTools` handed the model, under the cached attribution. The validation now records the launch identity it judged, and the memo is returned only after the record is read once more and still names it. `MCP.createAndStore` closed whatever was registered under the key when its own creation failed. Creation awaits a handshake, and nothing serializes adds to one key across callers, so a failure could close another caller's successful client and drop the record of what is actually running. It now captures the client it is replacing before creating, and on failure closes only that — if someone else registered meanwhile, theirs is left alone, status and record included. The workspace harness copies the runtime record instead of aliasing it to the config entry, as production does. Reverting either fix fails a named test.
The line itself is upstream; its new position is ours, and the marker guard reads a moved line as added code in an upstream-shared file.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4eeea09482
ℹ️ 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".
| s.spawned[name] = mcp | ||
| // altimate_change end | ||
| return yield* storeClient(s, name, result.mcpClient, result.defs!, mcp.timeout) |
There was a problem hiding this comment.
When two MCP.add calls for the same key overlap, an older slow creation can complete after a newer call has already installed its client; this success branch unconditionally replaces s.spawned and calls storeClient, which closes the newer client and restores the older workspace's tools and credentials. Fresh evidence after the failure-race fix is that only the !result.mcpClient branch checks s.clients[name] !== replacing; the successful branch still has no generation or identity guard.
Useful? React with 👍 / 👎.
| // one write to one file is not atomic, and a disable landing between the read | ||
| // and the `write` syscall is still lost. That residual is named on the PR | ||
| // rather than papered over; closing it needs write-then-verify. | ||
| if ((await addMcpToConfig(name, cfg, configPath, { refuseIfDisabled: true })) === null) { |
There was a problem hiding this comment.
When datamate is inherited from global config and the user disables that global entry after worldUnchanged() reads the merged configuration but before this call, refuseIfDisabled examines only the project file at configPath. It therefore sees no disabled node, writes an enabled project-level pin that shadows the new global disable, and the subsequent MCP.add starts the engine anyway; the final validation also sees the enabled override rather than the user's global intent. Recheck the merged configuration after the write or otherwise prevent a project write from overriding a concurrently applied global disable.
Useful? React with 👍 / 👎.
…lobal disable is honoured before the project write Two more of the same shape, on the last review round. `MCP.createAndStore` guarded its failure path against a client another caller registered meanwhile, but not its success path: an older creation completing after a newer add stored its client, closing the newer one and handing the runtime back to what the older call was asked to start. The newer call now wins whichever completes first — a late result is closed, not stored, and the late call answers with what is serving. `persist` checked the node it was about to replace, which is the PROJECT file's; intent can also live in the global config the project inherits from, and a project pin written over a global disable shadows it for good, since project wins the merge. The merged view is asked once more, immediately before the write. Same window as the write's own read; named, not closed. The lifecycle mock gains a one-shot connect delay so an older add can complete after a newer one; the real-file staging that asserts the W3 residual keys to the write's own read, now the second after the guard. Reverting either fix fails a named test.
1 participant
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
Consolidation
The attach flow has been restructured and hardened. The decision is a pure
function over one snapshot; every mutation sits behind a check of both halves of
the world it depends on; every install undoes itself on any non-success exit;
every refusal reaches the user exactly once. MCP now records what it spawned, so
attribution compares the running engine against the config rather than trusting
the config alone.
Reviewed by nineteen automated rounds, then three rounds of five parallel
adversarial passes. The round-by-round log, the invariants the suite holds, and
the residuals at the width the code actually has are in the pinned comment below.
The two most serious defects were not wrong code — they were this module being
internally consistent and wrong about the world. Behind most of the rest: a rule
that stopped being applied somewhere past where it was written down.