Skip to content

Commit cd96fea

Browse files
saravmajesticclaude
andcommitted
fix: [AI-7743] mcp-add question hangs — share question state + event bus across bundled module copies
`/discover-and-add-mcps` rendered the "which MCP / what scope" question but, after answering, the chat sat on "Thinking…" and the server was never added. Root cause: after the v1.17.9 upstream merge, `src/question/index.ts` is bundled as TWO separate module instances (a module-scoped id differs between the tool's `ask()` and the HTTP route's `reply()`; Bun does not reliably dedupe it, and normalizing imports does NOT change that). Each copy ran its own `makeRuntime` runtime, which broke the flow two independent ways: 1. State split — the `question` tool registered the pending `Deferred` in one copy's `InstanceState` map, while `POST /question/:id/reply` looked it up in the OTHER copy's empty map. The reply 404'd and the `Deferred` never resolved, so the agent loop blocked forever ("Thinking…"). 2. Event never reached the IDE webview — `question.asked` was published only via `EventV2Bridge` -> `GlobalBus`, but the `/event` SSE stream the webview reads is fed by the Bus *wildcard PubSub* (`Bus.publish`). So `pendingQuestions` stayed empty and the answer card's Submit had no request id -> submitting did nothing. Fix (self-contained in `question/index.ts`): - Anchor the pending-question registry on `globalThis` (keyed by instance directory) so every bundled module copy shares one map. Restore per-instance cleanup via `registerDisposer` so entries don't leak across instances/tests. - Publish `question.asked`/`replied`/`rejected` via `Bus.publish` (added `BusEvent` mirrors) so they reach `/event` like every other webview-visible event. Every divergence from upstream is wrapped in `altimate_change start/end` markers (14/14 balanced) so future upstream merges are handled correctly; `--require-markers --strict` and the marker-integrity tests pass. Verified end-to-end in code-server: discover -> answer (Yes / Project) -> datamate written to `.altimate-code/altimate-code.json` (enabled) and the chat shows the success summary. Marker-integrity + question tests pass (134). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 0dc8850 commit cd96fea

1 file changed

Lines changed: 108 additions & 24 deletions

File tree

packages/opencode/src/question/index.ts

Lines changed: 108 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ import { EventV2Bridge } from "@/event-v2-bridge"
77
import { EventV2 } from "@opencode-ai/core/event"
88
// altimate_change start — makeRuntime for the restored Promise wrappers (see bottom of file)
99
import { makeRuntime } from "@/effect/run-service"
10+
import { registerDisposer } from "@/effect/instance-registry"
11+
import { Bus } from "@/bus"
12+
import { BusEvent } from "@/bus/bus-event"
13+
import z from "zod"
1014
// altimate_change end
1115

1216
// Schemas — these are pure data; nothing checks class identity (see PR
@@ -93,6 +97,39 @@ export const Event = {
9397
Rejected: EventV2.define({ type: "question.rejected", schema: Rejected.fields }),
9498
}
9599

100+
// altimate_change start — BusEvent mirrors of the question events.
101+
//
102+
// The EventV2 `Event` defs above publish to GlobalBus/EventV2 consumers only.
103+
// The IDE webview subscribes to the `/event` SSE route, which is fed by the Bus
104+
// *wildcard PubSub* (`Bus.publish`), a different channel. So `question.asked`
105+
// never reached the webview → `pendingQuestions` stayed empty → the mcp-add
106+
// question card had no request id to reply with → "submit does nothing".
107+
// Publish these via `Bus.publish` too so they reach /event like every other
108+
// webview-visible event. (Schemas are loose — Bus.publish does not re-validate;
109+
// they exist for the `type` string and typing.)
110+
const BusAsked = BusEvent.define(
111+
"question.asked",
112+
z.object({
113+
id: QuestionID.zod,
114+
sessionID: SessionID.zod,
115+
questions: z.array(z.any()),
116+
tool: z.object({ messageID: MessageID.zod, callID: z.string() }).optional(),
117+
}),
118+
)
119+
const BusReplied = BusEvent.define(
120+
"question.replied",
121+
z.object({
122+
sessionID: SessionID.zod,
123+
requestID: QuestionID.zod,
124+
answers: z.array(z.array(z.string())),
125+
}),
126+
)
127+
const BusRejected = BusEvent.define(
128+
"question.rejected",
129+
z.object({ sessionID: SessionID.zod, requestID: QuestionID.zod }),
130+
)
131+
// altimate_change end
132+
96133
export class RejectedError extends Schema.TaggedErrorClass<RejectedError>()("QuestionRejectedError", {}) {
97134
override get message() {
98135
return "The user dismissed this question"
@@ -108,9 +145,31 @@ interface PendingEntry {
108145
deferred: Deferred.Deferred<ReadonlyArray<Answer>, RejectedError>
109146
}
110147

111-
interface State {
112-
pending: Map<QuestionID, PendingEntry>
148+
// altimate_change start — process-global pending registry.
149+
//
150+
// The imperative `Question.ask()`/`reply()`/`list()` wrappers (bottom of file)
151+
// get bundled into MORE THAN ONE module instance (proven: a module-scoped id
152+
// differs between the tool's ask() and the HTTP route's reply()). Consistent
153+
// `@/question` imports did NOT dedupe them. Each copy ran its own module-scoped
154+
// `makeRuntime(...)` runtime with its own `InstanceState` cache, so the question
155+
// TOOL registered the pending Deferred in one copy's map while the HTTP reply
156+
// route looked it up in the OTHER copy's empty map — the Deferred never resolved
157+
// and the `/discover-and-add-mcps` question hung on "Thinking…" after answering.
158+
//
159+
// Anchor the registry on `globalThis` so every module copy shares one Map. Keyed
160+
// by instance directory so `list()` stays per-instance.
161+
type PendingByDir = Map<string, Map<QuestionID, PendingEntry>>
162+
const pendingByDir: PendingByDir = ((globalThis as Record<string, unknown>)["__altimateQuestionPending"] ??=
163+
new Map<string, Map<QuestionID, PendingEntry>>()) as PendingByDir
164+
function pendingFor(directory: string): Map<QuestionID, PendingEntry> {
165+
let map = pendingByDir.get(directory)
166+
if (!map) {
167+
map = new Map<QuestionID, PendingEntry>()
168+
pendingByDir.set(directory, map)
169+
}
170+
return map
113171
}
172+
// altimate_change end
114173

115174
// Service
116175

@@ -134,31 +193,30 @@ export const layer = Layer.effect(
134193
Service,
135194
Effect.gen(function* () {
136195
const events = yield* EventV2Bridge.Service
137-
const state = yield* InstanceState.make<State>(
138-
Effect.fn("Question.state")(function* () {
139-
const state = {
140-
pending: new Map<QuestionID, PendingEntry>(),
141-
}
142-
143-
yield* Effect.addFinalizer(() =>
144-
Effect.gen(function* () {
145-
for (const item of state.pending.values()) {
146-
yield* Deferred.fail(item.deferred, new RejectedError())
147-
}
148-
state.pending.clear()
149-
}),
150-
)
151-
152-
return state
153-
}),
154-
)
196+
197+
// altimate_change start — clear a directory's pending questions when its
198+
// instance is disposed/reloaded (mirrors the removed InstanceState finalizer)
199+
// so entries in the process-global registry don't leak across instances.
200+
const off = registerDisposer(async (directory) => {
201+
const map = pendingByDir.get(directory)
202+
if (!map) return
203+
pendingByDir.delete(directory)
204+
for (const { deferred } of map.values()) {
205+
await Effect.runPromise(Deferred.fail(deferred, new RejectedError())).catch(() => {})
206+
}
207+
})
208+
yield* Effect.addFinalizer(() => Effect.sync(off))
209+
// altimate_change end
155210

156211
const ask = Effect.fn("Question.ask")(function* (input: {
157212
sessionID: SessionID
158213
questions: ReadonlyArray<Info>
159214
tool?: Tool
160215
}) {
161-
const pending = (yield* InstanceState.get(state)).pending
216+
// altimate_change start — pending map lives in the globalThis registry (see top of file)
217+
const directory = yield* InstanceState.directory
218+
const pending = pendingFor(directory)
219+
// altimate_change end
162220
const id = QuestionID.ascending()
163221
yield* Effect.logInfo("asking", { id, questions: input.questions.length })
164222

@@ -171,6 +229,12 @@ export const layer = Layer.effect(
171229
}
172230
pending.set(id, { info, deferred })
173231
yield* events.publish(Event.Asked, info)
232+
// altimate_change start — also publish on the Bus wildcard so the IDE webview
233+
// (subscribed to /event) receives question.asked and can answer the card.
234+
yield* Effect.promise(() =>
235+
Bus.publish(BusAsked, { id, sessionID: input.sessionID, questions: [...input.questions], tool: input.tool }),
236+
)
237+
// altimate_change end
174238

175239
return yield* Effect.ensuring(
176240
Deferred.await(deferred),
@@ -184,7 +248,9 @@ export const layer = Layer.effect(
184248
requestID: QuestionID
185249
answers: ReadonlyArray<Answer>
186250
}) {
187-
const pending = (yield* InstanceState.get(state)).pending
251+
// altimate_change start — pending map lives in the globalThis registry (see top of file)
252+
const pending = pendingFor(yield* InstanceState.directory)
253+
// altimate_change end
188254
const existing = pending.get(input.requestID)
189255
if (!existing) {
190256
yield* Effect.logWarning("reply for unknown request", { requestID: input.requestID })
@@ -197,11 +263,22 @@ export const layer = Layer.effect(
197263
requestID: existing.info.id,
198264
answers: input.answers.map((a) => [...a]),
199265
})
266+
// altimate_change start — mirror on the Bus wildcard for /event (webview) clients.
267+
yield* Effect.promise(() =>
268+
Bus.publish(BusReplied, {
269+
sessionID: existing.info.sessionID,
270+
requestID: existing.info.id,
271+
answers: input.answers.map((a) => [...a]),
272+
}),
273+
)
274+
// altimate_change end
200275
yield* Deferred.succeed(existing.deferred, input.answers)
201276
})
202277

203278
const reject = Effect.fn("Question.reject")(function* (requestID: QuestionID) {
204-
const pending = (yield* InstanceState.get(state)).pending
279+
// altimate_change start — pending map lives in the globalThis registry (see top of file)
280+
const pending = pendingFor(yield* InstanceState.directory)
281+
// altimate_change end
205282
const existing = pending.get(requestID)
206283
if (!existing) {
207284
yield* Effect.logWarning("reject for unknown request", { requestID })
@@ -213,11 +290,18 @@ export const layer = Layer.effect(
213290
sessionID: existing.info.sessionID,
214291
requestID: existing.info.id,
215292
})
293+
// altimate_change start — mirror on the Bus wildcard for /event (webview) clients.
294+
yield* Effect.promise(() =>
295+
Bus.publish(BusRejected, { sessionID: existing.info.sessionID, requestID: existing.info.id }),
296+
)
297+
// altimate_change end
216298
yield* Deferred.fail(existing.deferred, new RejectedError())
217299
})
218300

219301
const list = Effect.fn("Question.list")(function* () {
220-
const pending = (yield* InstanceState.get(state)).pending
302+
// altimate_change start — pending map lives in the globalThis registry (see top of file)
303+
const pending = pendingFor(yield* InstanceState.directory)
304+
// altimate_change end
221305
return Array.from(pending.values(), (x) => x.info)
222306
})
223307

0 commit comments

Comments
 (0)