fix: [AI-7743] Altimate Code chat — MCP servers not getting added (question hang)#1007
fix: [AI-7743] Altimate Code chat — MCP servers not getting added (question hang)#1007saravmajestic wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
📝 WalkthroughWalkthroughQuestion pending state now uses a process-global, directory-keyed registry with disposal cleanup. Question ask, reply, and reject operations also publish corresponding wildcard Bus events for SSE subscribers. ChangesQuestion flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Question
participant Bus
participant EventSSE
Question->>Bus: publish question.asked
Bus->>EventSSE: deliver wildcard event
Question->>Bus: publish question.replied or question.rejected
Bus->>EventSSE: deliver wildcard event
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks 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 |
|
👋 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/question/index.ts (1)
228-240: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake lifecycle finalization independent of event publication.
A failed publication can leak an asked request or strand a replied/rejected deferred.
packages/opencode/src/question/index.ts#L228-L240: wrap publication and awaiting with cleanup that begins immediately afterpending.set.packages/opencode/src/question/index.ts#L254-L269: guaranteeDeferred.succeedwithEffect.ensuring.packages/opencode/src/question/index.ts#L279-L289: guaranteeDeferred.failwithEffect.ensuring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/question/index.ts` around lines 228 - 240, Make question lifecycle cleanup independent of publication and deferred completion: in packages/opencode/src/question/index.ts lines 228-240, wrap publication and Deferred.await immediately after pending.set with Effect.ensuring that always deletes the pending request; in lines 254-269, wrap Deferred.succeed with Effect.ensuring; and in lines 279-289, wrap Deferred.fail with Effect.ensuring so cleanup runs even when publication or completion fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/opencode/src/question/index.ts`:
- Around line 228-240: Make question lifecycle cleanup independent of
publication and deferred completion: in packages/opencode/src/question/index.ts
lines 228-240, wrap publication and Deferred.await immediately after pending.set
with Effect.ensuring that always deletes the pending request; in lines 254-269,
wrap Deferred.succeed with Effect.ensuring; and in lines 279-289, wrap
Deferred.fail with Effect.ensuring so cleanup runs even when publication or
completion fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 22566e2a-fd12-4a52-86af-c5cc09552b64
📒 Files selected for processing (4)
packages/opencode/src/question/index.tspackages/opencode/src/server/routes/question.tspackages/opencode/src/tool/plan.tspackages/opencode/src/tool/question.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Re-verified after the branch was force-pushed (previous review SHA no longer reachable). The change is now a single self-contained commit on
Files Reviewed (1 file)
Previous Review Summary (commit ab2bd9f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ab2bd9f)Status: No Issues Found | Recommendation: Merge The fix correctly addresses the AI-7743 hang from two angles, both verified against the actual shipped wiring:
Import normalizations ( Files Reviewed (4 files)
Reviewed by glm-5.2 · Input: 85K · Output: 22.3K · Cached: 1.4M Review guidance: REVIEW.md from base branch |
…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>
saravmajestic
force-pushed
the
fix/AI-7743-mcp-add-question-hang
branch
from
ab2bd9f to
cd96fea
Compare
|
👋 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. |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 4 finding(s)
- 4 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/question/index.ts (L266-L274)
[🔴 HIGH] If Bus.publish throws or rejects (e.g., due to bus errors), this Effect will terminate early, and Deferred.succeed will never be reached. This will cause the original ask operation that awaits this deferred to hang indefinitely.
Consider decoupling side-effects like event mirroring by using a non-blocking fork (e.g., Effect.fork(Effect.promise(...))), or placing it after Deferred.succeed, or applying explicit error handling (e.g., Effect.ignoreLogged / Effect.catchAllDefect) so it doesn't block the core business logic.
Suggested change:
// altimate_change start — mirror on the Bus wildcard for /event (webview) clients.
yield* Effect.fork(Effect.promise(() =>
Bus.publish(BusReplied, {
sessionID: existing.info.sessionID,
requestID: existing.info.id,
answers: input.answers.map((a) => [...a]),
}),
))
// altimate_change end
2. packages/opencode/src/question/index.ts (L293-L297)
[🔴 HIGH] Similarly, if Bus.publish throws or rejects here, the Effect will terminate early, preventing Deferred.fail from being executed and leaving the pending ask operation hanging indefinitely.
Consider placing this in a non-blocking fork or adding explicit error handling to avoid blocking the rejection resolution.
Suggested change:
// altimate_change start — mirror on the Bus wildcard for /event (webview) clients.
yield* Effect.fork(Effect.promise(() =>
Bus.publish(BusRejected, { sessionID: existing.info.sessionID, requestID: existing.info.id }),
))
// altimate_change end
3. packages/opencode/src/question/index.ts (L110-L118)
[🟠 MEDIUM] The use of any type (z.any()) is strictly prohibited by our review checklist unless explicitly justified. Using any bypasses Zod's validation and type inference, potentially allowing unexpected data structures to propagate.
Please use z.unknown() or define a stricter Zod schema that matches the expected structure of Question.Info to maintain type safety.
Suggested change:
const BusAsked = BusEvent.define(
"question.asked",
z.object({
id: QuestionID.zod,
sessionID: SessionID.zod,
questions: z.array(z.unknown()),
tool: z.object({ messageID: MessageID.zod, callID: z.string() }).optional(),
}),
)
4. packages/opencode/src/question/index.ts (L204-L206)
[🟠 MEDIUM] This promise resolution contains an empty catch block: .catch(() => {}). This silently swallows any unexpected errors that might occur when Effect.runPromise executes, which violates error-handling best practices and can obscure underlying bugs during instance cleanup.
Consider logging the error explicitly instead of silently ignoring it, e.g., .catch((e) => log.error(e)).
Suggested change:
for (const { deferred } of map.values()) {
await Effect.runPromise(Deferred.fail(deferred, new RejectedError())).catch(console.error)
}
| // altimate_change start — mirror on the Bus wildcard for /event (webview) clients. | ||
| yield* Effect.promise(() => | ||
| Bus.publish(BusReplied, { | ||
| sessionID: existing.info.sessionID, | ||
| requestID: existing.info.id, | ||
| answers: input.answers.map((a) => [...a]), | ||
| }), | ||
| ) | ||
| // altimate_change end |
There was a problem hiding this comment.
[🔴 HIGH] If Bus.publish throws or rejects (e.g., due to bus errors), this Effect will terminate early, and Deferred.succeed will never be reached. This will cause the original ask operation that awaits this deferred to hang indefinitely.
Consider decoupling side-effects like event mirroring by using a non-blocking fork (e.g., Effect.fork(Effect.promise(...))), or placing it after Deferred.succeed, or applying explicit error handling (e.g., Effect.ignoreLogged / Effect.catchAllDefect) so it doesn't block the core business logic.
Suggested change:
| // altimate_change start — mirror on the Bus wildcard for /event (webview) clients. | |
| yield* Effect.promise(() => | |
| Bus.publish(BusReplied, { | |
| sessionID: existing.info.sessionID, | |
| requestID: existing.info.id, | |
| answers: input.answers.map((a) => [...a]), | |
| }), | |
| ) | |
| // altimate_change end | |
| // altimate_change start — mirror on the Bus wildcard for /event (webview) clients. | |
| yield* Effect.fork(Effect.promise(() => | |
| Bus.publish(BusReplied, { | |
| sessionID: existing.info.sessionID, | |
| requestID: existing.info.id, | |
| answers: input.answers.map((a) => [...a]), | |
| }), | |
| )) | |
| // altimate_change end |
| // altimate_change start — mirror on the Bus wildcard for /event (webview) clients. | ||
| yield* Effect.promise(() => | ||
| Bus.publish(BusRejected, { sessionID: existing.info.sessionID, requestID: existing.info.id }), | ||
| ) | ||
| // altimate_change end |
There was a problem hiding this comment.
[🔴 HIGH] Similarly, if Bus.publish throws or rejects here, the Effect will terminate early, preventing Deferred.fail from being executed and leaving the pending ask operation hanging indefinitely.
Consider placing this in a non-blocking fork or adding explicit error handling to avoid blocking the rejection resolution.
Suggested change:
| // altimate_change start — mirror on the Bus wildcard for /event (webview) clients. | |
| yield* Effect.promise(() => | |
| Bus.publish(BusRejected, { sessionID: existing.info.sessionID, requestID: existing.info.id }), | |
| ) | |
| // altimate_change end | |
| // altimate_change start — mirror on the Bus wildcard for /event (webview) clients. | |
| yield* Effect.fork(Effect.promise(() => | |
| Bus.publish(BusRejected, { sessionID: existing.info.sessionID, requestID: existing.info.id }), | |
| )) | |
| // altimate_change end |
| const BusAsked = BusEvent.define( | ||
| "question.asked", | ||
| z.object({ | ||
| id: QuestionID.zod, | ||
| sessionID: SessionID.zod, | ||
| questions: z.array(z.any()), | ||
| tool: z.object({ messageID: MessageID.zod, callID: z.string() }).optional(), | ||
| }), | ||
| ) |
There was a problem hiding this comment.
[🟠 MEDIUM] The use of any type (z.any()) is strictly prohibited by our review checklist unless explicitly justified. Using any bypasses Zod's validation and type inference, potentially allowing unexpected data structures to propagate.
Please use z.unknown() or define a stricter Zod schema that matches the expected structure of Question.Info to maintain type safety.
Suggested change:
| const BusAsked = BusEvent.define( | |
| "question.asked", | |
| z.object({ | |
| id: QuestionID.zod, | |
| sessionID: SessionID.zod, | |
| questions: z.array(z.any()), | |
| tool: z.object({ messageID: MessageID.zod, callID: z.string() }).optional(), | |
| }), | |
| ) | |
| const BusAsked = BusEvent.define( | |
| "question.asked", | |
| z.object({ | |
| id: QuestionID.zod, | |
| sessionID: SessionID.zod, | |
| questions: z.array(z.unknown()), | |
| tool: z.object({ messageID: MessageID.zod, callID: z.string() }).optional(), | |
| }), | |
| ) |
| for (const { deferred } of map.values()) { | ||
| await Effect.runPromise(Deferred.fail(deferred, new RejectedError())).catch(() => {}) | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] This promise resolution contains an empty catch block: .catch(() => {}). This silently swallows any unexpected errors that might occur when Effect.runPromise executes, which violates error-handling best practices and can obscure underlying bugs during instance cleanup.
Consider logging the error explicitly instead of silently ignoring it, e.g., .catch((e) => log.error(e)).
Suggested change:
| for (const { deferred } of map.values()) { | |
| await Effect.runPromise(Deferred.fail(deferred, new RejectedError())).catch(() => {}) | |
| } | |
| for (const { deferred } of map.values()) { | |
| await Effect.runPromise(Deferred.fail(deferred, new RejectedError())).catch(console.error) | |
| } |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
2 participants
Summary
Fixes AI-7743 — in Altimate Code chat,
/discover-and-add-mcpsshowed the "which MCP / what scope" question, but after the user answered it the chat sat on "Thinking…" forever and the MCP server was never added.Root cause
After the v1.17.9 upstream merge (effect@4),
src/question/index.tsis bundled as two separate module instances. Proven: a module-scoped id logged from thequestiontool'sask()differs from the one logged in the HTTPreply()route; normalizing every importer to@/questiondid not make Bun dedupe it. Each copy ran its ownmakeRuntimeruntime, which broke the flow in two independent ways:questiontool registered the pendingDeferredin one copy'sInstanceStatepending-map, whilePOST /question/:id/replylooked it up in the other copy's empty map. The reply returnedNotFoundError(HTTP 500) and theDeferrednever resolved, so the agent loop blocked forever → "Thinking…". (GET /questionreturned[]for the same reason.)question.askedwas published only viaEventV2Bridge→GlobalBus, but the/eventSSE stream the webview subscribes to is fed by the Bus wildcard PubSub (Bus.publish) — a different channel. So the webview'spendingQuestionsstayed empty, the answer card's Submit resolvedrequestId: undefined, and submitting did nothing.Either half alone produces the reported "answer the question → nothing happens".
Fix (all in
question/index.ts+ import cleanup)globalThis(keyed by instance directory) so all module copies share one map. Restore per-instance cleanup viaregisterDisposerso entries don't leak across instances/tests.question.asked/replied/rejectedviaBus.publish(addedBusEventmirrors) so they reach/eventlike every other webview-visible event.../questionimports to@/question.Verification
End-to-end in Docker code-server (VS Code extension + real UI):
/discover-and-add-mcps→ question card renders → answer Yes / Project → ✅ datamate added to.altimate-code/altimate-code.json(enabled: true) and the chat shows the success summary.pendingQuestionspopulated, was[]) and the reply resolves (HTTP 200,message=replied).55question unit tests pass (test/question,test/tool/question,test/release-validation/question-937*,test/cli/run/question.shared).No changes were needed in the
vscode-altimate-mcp-serverextension.🤖 Generated with Claude Code
Summary by cubic
Fixes AI-7743: answering the MCP add question in
/discover-and-add-mcpsno longer hangs. We now share pending-question state across bundled module copies and mirror question events on the Bus so the webview receives them and the MCP server is added.globalThis(keyed by instance directory) and dispose per instance viaregisterDisposersoask()and HTTPreply()resolve the same request.question.asked/replied/rejectedviaBus.publish(newBusEventmirrors) so/eventdelivers them to the IDE webview.Written for commit cd96fea. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes