Skip to content

Commit d6f5b5b

Browse files
ralphstodomingoralphstodomingo
committed
fix(workspace): honour a live disconnect, and say when an engine is unrunnable
Codex round 9, the final review round. Two findings, both verified; each fix confirmed to fail only its own test when reverted. **A live disconnect is honoured even when the config cache is stale.** The runtime status is authoritative for "not running"; the config is authoritative for "the user turned it off" — and the two can disagree. MCP.disconnect writes enabled:false straight to disk without invalidating Config, so immediately after a user disconnects, the cached entry still reports enabled:true. Reading the cache made the disabled status look synthesized, so the entry was reconnected and persisted enabled again, undoing the disconnect — globally, when the owning entry is global. The owning config is now re-read before that decision. This is the third distinct route by which a raw config write without cache invalidation has produced a wrong answer in this module, and the second time it has defeated a fix from an earlier round. **The integrations listing no longer reports an empty catalog when entries were hidden.** A catalog of nothing but extension-type integrations filters down to empty, and the explanatory footer sits after the early return, so the tool said no integrations were available at all. The count and the VS Code requirement are now included in that branch too. Also, authorised separately rather than found by review: an engine that cannot be run is now described as such instead of as out of date. versionOf reads stdout only and returns null when the process fails, so a null means the binary produced no version — broken, not old. Both states were previously reported as "too old", which sent more than one debugging session hunting a version mismatch that did not exist. 3 tests: a stale-cache disconnect stays disconnected and is never reconnected or persisted, and the two refusal messages differ for an unreadable version versus a genuinely old one.
1 parent 791a286 commit d6f5b5b

3 files changed

Lines changed: 97 additions & 9 deletions

File tree

‎packages/opencode/src/altimate/tools/datamate.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,18 @@ async function handleListIntegrations() {
147147
const hidden = catalog.length - integrations.length
148148
// altimate_change end
149149
if (integrations.length === 0) {
150+
// A catalog of nothing but extension-type entries filters down to empty,
151+
// and the footer below never runs — so this branch used to report a
152+
// genuinely empty catalog. Say what was hidden here too, or the model
153+
// reports "no integrations" when the workspace in fact has several.
154+
const omitted =
155+
hidden > 0
156+
? ` ${hidden} extension-type integration${hidden === 1 ? " was" : "s were"} omitted — they require a live VS Code bridge and are not available from the CLI.`
157+
: ""
150158
return {
151-
title: "Integrations: none found",
159+
title: hidden > 0 ? `Integrations: none available on the CLI (${hidden} hidden)` : "Integrations: none found",
152160
metadata: { count: 0, hidden },
153-
output: "No integrations available.",
161+
output: `No integrations available.${omitted}`,
154162
}
155163
}
156164
const lines = ["ID | Name | Tools", "---|------|------"]

‎packages/opencode/src/altimate/workspace/engine-sync.ts‎

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ export const syncInternals: {
132132
persist?: (name: string, cfg: LocalMcpConfig) => Promise<void>
133133
/** The configured (merged) MCP entry under `name`, or null if none. */
134134
existingEntry?: (name: string) => Promise<ExistingEntry | null>
135+
refreshConfig?: () => Promise<void>
135136
declared?: (datamateId: string) => Promise<Declared | null>
136137
notify?: (toast: Toast) => Promise<void>
137138
} = {}
@@ -289,6 +290,18 @@ async function persist(name: string, cfg: LocalMcpConfig): Promise<void> {
289290
})
290291
}
291292

293+
/** Drop the per-instance config cache so the next read sees the file.
294+
*
295+
* `MCP.disconnect` writes `enabled: false` straight to disk and does not
296+
* invalidate `Config`, so a cached entry can still report `enabled: true` right
297+
* after a user has disconnected. */
298+
async function refreshConfig(): Promise<void> {
299+
if (syncInternals.refreshConfig) return syncInternals.refreshConfig()
300+
await Config.invalidate().catch((err) => {
301+
log.warn("could not refresh the config cache", { err: String(err) })
302+
})
303+
}
304+
292305
async function existingEntry(name: string): Promise<ExistingEntry | null> {
293306
if (syncInternals.existingEntry) return syncInternals.existingEntry(name)
294307
try {
@@ -380,6 +393,27 @@ async function notify(toast: Toast): Promise<void> {
380393
// The attach flow
381394
// ---------------------------------------------------------------------------
382395

396+
/** Why an engine was refused, in the user's terms.
397+
*
398+
* "Too old" and "could not be run at all" are the same code path but very
399+
* different problems, and conflating them sent more than one debugging session
400+
* hunting a version mismatch that did not exist. `versionOf` reads stdout only
401+
* and returns null when the process fails, so a null here means the binary did
402+
* not produce a version — broken, not merely out of date. */
403+
function describeRefusal(found: string | null, workspaceName: string): string {
404+
if (!found) {
405+
return (
406+
`The ${ENGINE_BINARY} on PATH did not report a usable version, so it cannot be used for workspace ` +
407+
`"${workspaceName}". It is more likely broken than out of date — try running \`${ENGINE_BINARY} --version\` ` +
408+
`directly. Reinstall with: ${INSTALL_HINT}`
409+
)
410+
}
411+
return (
412+
`Found ${ENGINE_BINARY} ${found}; workspace "${workspaceName}" needs ${MIN_ENGINE_VERSION} or newer. ` +
413+
`Update with: ${INSTALL_HINT}`
414+
)
415+
}
416+
383417
function describeMissing(missing: string[]): string {
384418
if (missing.length === 0) return ""
385419
const shown = missing.slice(0, 5).join(", ")
@@ -483,7 +517,16 @@ async function run(): Promise<Outcome> {
483517
// no runtime status, and `MCP.remove` deletes the status — so every
484518
// rejection teardown makes the next turn look like a user disable. Read the
485519
// config's actual flag instead; only that is user intent.
486-
if (existing.status === "disabled" && entry?.enabled === false) {
520+
// The runtime status is authoritative for "not running"; the CONFIG is
521+
// authoritative for "the user turned it off" — and the two can disagree.
522+
// `MCP.disconnect` writes `enabled: false` to disk WITHOUT invalidating
523+
// Config, so a cached entry still says `enabled: true` immediately after a
524+
// user disconnects. Treating that as a synthesized status would reconnect
525+
// and persist it enabled again, undoing their disconnect — globally, if
526+
// the owning entry is global. Read the file before deciding.
527+
if (existing.status === "disabled") await refreshConfig()
528+
const owning = existing.status === "disabled" ? await existingEntry(DATAMATE_KEY) : entry
529+
if (existing.status === "disabled" && owning?.enabled === false) {
487530
// The user turned this entry off deliberately. Do NOT call `MCP.connect`
488531
// to "retry" it: that persists `enabled: true` into whichever config
489532
// owns the entry, so for a global `datamate` the first prompt in any
@@ -601,10 +644,8 @@ async function run(): Promise<Outcome> {
601644
// return "too old" while still serving the too-old engine's tools.
602645
await detachRejected({ workspaceId, reason: "below-floor", found: label })
603646
await notify({
604-
title: "Workspace engine is too old",
605-
message:
606-
`The engine serving workspace "${binding.datamateName}" reports ${label}; this client needs ` +
607-
`${MIN_ENGINE_VERSION} or newer. Update with: ${INSTALL_HINT}`,
647+
title: found ? "Workspace engine is too old" : "Workspace engine is not runnable",
648+
message: describeRefusal(found, binding.datamateName),
608649
variant: "warning",
609650
})
610651
return { kind: "engine-too-old", found: label }
@@ -641,8 +682,8 @@ async function run(): Promise<Outcome> {
641682
if (!found || compareVersions(found, MIN_ENGINE_VERSION) < 0) {
642683
const label = found ?? "unknown"
643684
await notify({
644-
title: "Workspace engine is too old",
645-
message: `Found ${ENGINE_BINARY} ${label}; this client needs ${MIN_ENGINE_VERSION} or newer. Update with: ${INSTALL_HINT}`,
685+
title: found ? "Workspace engine is too old" : "Workspace engine is not runnable",
686+
message: describeRefusal(found, binding.datamateName),
646687
variant: "warning",
647688
})
648689
return { kind: "engine-too-old", found: label }

‎packages/opencode/test/altimate/workspace/engine-sync.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1074,3 +1074,42 @@ describe("settledOutcome — a read-only view for other modules", () => {
10741074
expect(h.added).toHaveLength(1) // no attach was triggered by reading
10751075
})
10761076
})
1077+
1078+
describe("ensure — round 9", () => {
1079+
test("a live disconnect is honoured even when the config cache is stale", async () => {
1080+
// MCP.disconnect writes enabled:false to disk without invalidating Config,
1081+
// so the cached entry still says enabled:true. Believing the cache would
1082+
// reconnect the entry and persist it enabled again — undoing the user's
1083+
// disconnect, globally if the owning entry is global.
1084+
let refreshed = false
1085+
const h = install({
1086+
statuses: [{ datamate: { status: "disabled" } }],
1087+
})
1088+
syncInternals.refreshConfig = async () => {
1089+
refreshed = true
1090+
}
1091+
syncInternals.existingEntry = async () =>
1092+
refreshed
1093+
? { type: "local", command: ["datamate", "start-stdio"], enabled: false } // on disk
1094+
: { type: "local", command: ["datamate", "start-stdio"], enabled: true } // stale cache
1095+
1096+
expect(await ensure("s1")).toEqual({ kind: "entry-disabled" })
1097+
expect(refreshed).toBe(true)
1098+
expect(h.connects).toHaveLength(0) // MCP.connect would persist enabled:true
1099+
expect(h.persisted).toHaveLength(0)
1100+
})
1101+
1102+
test("an unrunnable engine is described as broken, not as out of date", async () => {
1103+
const broken = install({ version: null })
1104+
expect(await ensure("s1")).toEqual({ kind: "engine-too-old", found: "unknown" })
1105+
expect(broken.toasts[0].title).toContain("not runnable")
1106+
expect(broken.toasts[0].message).toContain("did not report a usable version")
1107+
expect(broken.toasts[0].message).not.toContain("needs 0.7.0 or newer")
1108+
1109+
resetForTests()
1110+
const old = install({ version: "0.6.9" })
1111+
expect(await ensure("s2")).toEqual({ kind: "engine-too-old", found: "0.6.9" })
1112+
expect(old.toasts[0].title).toContain("too old")
1113+
expect(old.toasts[0].message).toContain("needs 0.7.0 or newer")
1114+
})
1115+
})

0 commit comments

Comments
 (0)