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 2 commits 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: 1 Issue Found | Recommendation: Merge — 1 optional suggestion Overview
Incremental review of commit Verified against the shipped surface:
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit cd96fea)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit cd96fea)Status: 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 (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: 76.1K · Output: 29.6K · Cached: 806.8K 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>
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 |
There was a problem hiding this comment.
Fixed in 0b5ecd12f. Added a best-effort mirror() helper — Effect.promise(() => Bus.publish(...)) recovered with Effect.catchCause to a logged warning — so a publish failure can never abort the fiber. The Deferred is now settled first, then the mirror fires, so settlement can't be skipped. Applied to ask(), reply(), and reject().
| // 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 |
There was a problem hiding this comment.
Fixed in 0b5ecd12f. Added a best-effort mirror() helper — Effect.promise(() => Bus.publish(...)) recovered with Effect.catchCause to a logged warning — so a publish failure can never abort the fiber. The Deferred is now settled first, then the mirror fires, so settlement can't be skipped. Applied to ask(), reply(), and reject().
| 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(), | |
| }), | |
| ) |
There was a problem hiding this comment.
Fixed in 0b5ecd12f. Went further than z.unknown() per @sahrizvi's related note — the Bus schemas now reuse the structured Effect schemas via the zod adapter (zod(Request)), so the generated /event payload matches the SDK's typed shape.
| 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) | |
| } |
There was a problem hiding this comment.
Fixed in 0b5ecd12f. The dispose cleanup now recovers the cause with Effect.catchCause and logs Effect.logWarning("question cleanup failed on dispose", { cause }) instead of swallowing it — kept in Effect-land rather than console.error.
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
7-model consensus review — verdict: REQUEST CHANGES
Panel: Claude + GPT Codex, Kimi K2.5, Qwen 3.6, MiniMax M2.7, GLM-5.1, MiMo V2.5 (Gemini excluded — auth failure). Quorum 6/6 met.
Tests: bun test test/question/question.test.ts → 14/14 pass in the PR worktree.
Root-cause diagnosis and the two-part fix (globalThis pending registry + Bus.publish mirrors) are correct and unanimously praised. Architecture is sound.
One must-fix (MAJOR #1, unanimous across all 7 models) is inline. Other MINOR findings are inline where a diff line applies; the remainder are below.
MINOR #4 — No regression test for the actual Bus-mirror fix
waitForPending in test/question/question.test.ts listens on the EventV2 path, not Bus. Nothing asserts Bus.publish/Bus.subscribeAll delivery — deleting the three mirror lines would leave every test green. Add a test that subscribes via Bus.subscribeAll (or Bus.subscribe(BusAsked)) and asserts question.asked/replied/rejected arrive after ask/reply/reject. Ideally also: a test that reply/reject still settle the Deferred when Bus.publish fails (guards MAJOR #1).
MINOR #6 — Shutdown/disposal semantics + globalThis anchor asymmetry
The old InstanceState finalizer failed all pending Deferreds when the layer scope closed; the new layer finalizer only calls off() (unregister) and relies on registerDisposer firing per-directory. Closing a Question.layer scope without going through disposeInstance(directory) now leaves pending fibers unresolved and entries in the global map. Negligible on full-process shutdown, but a behaviour delta.
Related: state is anchored on globalThis to survive module duplication, but the disposer registers into the module-scoped disposers Set in instance-registry.ts — if that module is ever duplicated the same way, cleanup silently no-ops. This is an inherited InstanceState assumption, but the PR's own thesis is that the assumption isn't safe. Consider a per-layer ownership token that drains only its own entries, and/or anchoring disposers on globalThis too.
NITs
(globalThis as Record<string, unknown>)[...] as PendingByDir— a non-Map value at that key would crash; adeclare globalaugmentation or aninstanceof Mapguard is safer.Effect.runPromise(Deferred.fail(...)).catch(() => {})in the disposer spins a fresh runtime per deferred and swallows any future requirement;Effect.runFork(...)is lighter.Deferred.failis currentlyR = never, so the.catchguards a non-event.
Adjudicated / rejected claims (checked against source)
- Kimi — "
Deferred.failthrows if the Deferred is already completed": false. In Effect, completing an already-settled Deferred returnsfalse; it does not throw. The disposer loop is safe. - Qwen — "race condition in
pendingForon concurrent asks": false.pendingForis fully synchronous (noyield), so two fibers cannot interleave its get-or-create in single-threaded JS. - MiniMax — CRITICAL "
reply()/reject()miss the directory lookup": false and self-refuting — both callpendingFor(yield* InstanceState.directory), identical toask(). Not an issue.
Positive observations (consensus)
- Root cause precisely diagnosed and documented inline;
altimate_changemarkers balanced; change is minimal and confined to one file. - The globalThis registry keyed by instance directory is the correct, robust fix for the split-module pending map and preserves per-instance
list()semantics. - Both delivery channels (EventV2 and Bus wildcard) are genuinely needed; the PR correctly identifies they are separate.
- Existing dispose/reload tests pass, validating the
registerDisposercleanup path.
Convergence: all 7 reviewers independently surfaced finding #1 without prompting — no second round needed. Disagreements on severity (#2, #6) and three false-positive claims were adjudicated by reading the source, not by vote.
| yield* Effect.promise(() => | ||
| Bus.publish(BusAsked, { id, sessionID: input.sessionID, questions: [...input.questions], tool: input.tool }), | ||
| ) |
There was a problem hiding this comment.
[MAJOR] Bus mirror on the Deferred critical path — can reintroduce the exact hang this PR fixes.
Effect.promise converts a promise rejection into an unrecoverable defect that aborts the fiber. This mirror sits before the Effect.ensuring(Deferred.await, pending.delete) block at lines 239-244 that registers cleanup, so if Bus.publish ever rejects:
- the awaiting
Question.ask()fiber is aborted before it registers the cleanup finalizer; - the pending entry stays in the global registry until instance disposal (MINOR chore(deps): Bump @gitlab/gitlab-ai-provider from 3.6.0 to 4.1.0 #5 — same root cause, folded here);
- and any dependent Deferred is never settled → tool hangs on "Thinking…" — the exact failure class this PR was written to fix.
A fire-and-forget notification must never be able to abort core question settlement.
Required fix — make the mirror best-effort (also apply at reply()/reject()):
const mirror = <D extends BusEvent.Definition>(def: D, props: z.output<D["properties"]>) =>
Effect.promise(() => Bus.publish(def, props)).pipe(
Effect.catchAllCause((cause) =>
Effect.logWarning("question bus mirror failed", { type: def.type, cause }),
),
)
// ...
yield* mirror(BusAsked, { ... }).pipe(Effect.ignore) is the minimal form. Prefer settling the Deferred first, then mirroring.
Flagged by all 7 reviewers. Probability of a Bus.publish rejection is low, but the impact is a permanent hang and the fix is trivial — every model rated this a blocker.
There was a problem hiding this comment.
Fixed in 0b5ecd12f. Added a best-effort mirror() helper — Effect.promise(() => Bus.publish(...)) recovered with Effect.catchCause to a logged warning — so a publish failure can never abort the fiber. The Deferred is now settled first, then the mirror fires, so settlement can't be skipped. Applied to ask(), reply(), and reject().
| yield* Effect.promise(() => | ||
| Bus.publish(BusReplied, { | ||
| sessionID: existing.info.sessionID, | ||
| requestID: existing.info.id, | ||
| answers: input.answers.map((a) => [...a]), | ||
| }), | ||
| ) |
There was a problem hiding this comment.
[MAJOR] Same Bus-mirror-on-critical-path issue as ask() — this one is worse because the mirror runs after pending.delete(input.requestID) on line 259 but before Deferred.succeed on line 275.
If Bus.publish rejects:
Effect.promiseconverts the rejection to an unrecoverable fiber defect;- the Deferred is never succeeded → the awaiter of
Question.ask()hangs forever; - the pending entry is already deleted, so a retried reply returns
NotFoundError— unrecoverable.
Required fix — settle the Deferred first, then fire the mirror best-effort:
yield* Deferred.succeed(existing.deferred, input.answers)
yield* mirror(BusReplied, { ... }) // Effect.promise + Effect.catchAllCauseSee the MAJOR comment on ask() for the mirror helper.
There was a problem hiding this comment.
Fixed in 0b5ecd12f. Added a best-effort mirror() helper — Effect.promise(() => Bus.publish(...)) recovered with Effect.catchCause to a logged warning — so a publish failure can never abort the fiber. The Deferred is now settled first, then the mirror fires, so settlement can't be skipped. Applied to ask(), reply(), and reject().
| yield* Effect.promise(() => | ||
| Bus.publish(BusRejected, { sessionID: existing.info.sessionID, requestID: existing.info.id }), | ||
| ) |
There was a problem hiding this comment.
[MAJOR] Same critical-path Bus mirror as ask()/reply(). Mirror runs after pending.delete(requestID) on line 287 but before Deferred.fail on line 298.
On Bus.publish rejection: fiber aborts before Deferred.fail, awaiter hangs, pending entry already gone → unrecoverable.
Required fix — settle first, then fire the mirror best-effort:
yield* Deferred.fail(existing.deferred, new RejectedError())
yield* mirror(BusRejected, { ... })There was a problem hiding this comment.
Fixed in 0b5ecd12f. Added a best-effort mirror() helper — Effect.promise(() => Bus.publish(...)) recovered with Effect.catchCause to a logged warning — so a publish failure can never abort the fiber. The Deferred is now settled first, then the mirror fires, so settlement can't be skipped. Applied to ask(), reply(), and reject().
| z.object({ | ||
| id: QuestionID.zod, | ||
| sessionID: SessionID.zod, | ||
| questions: z.array(z.any()), |
There was a problem hiding this comment.
[MINOR] z.any() weakens the published contract.
/event's OpenAPI is generated from BusEvent.payloads(), so the documented question.asked payload becomes questions: any — diverging from the SDK's typed Array<QuestionInfo>. Apply the same to BusReplied.answers (z.array(z.array(z.string()))) which is looser than Array<Answer>.
Reuse the structured schema instead. The zod adapter from util/effect-zod is already used by the question route:
const BusAsked = BusEvent.define("question.asked", zod(Request))
const BusReplied = BusEvent.define("question.replied", zod(Replied))
const BusRejected = BusEvent.define("question.rejected", zod(Rejected))Flagged by Kimi, Qwen, GPT, MiniMax, GLM-5, MiMo.
There was a problem hiding this comment.
Fixed in 0b5ecd12f. Replaced the hand-written loose schemas with zod(Request) / zod(Replied) / zod(Rejected) from util/effect-zod, so the /event OpenAPI payloads now match Array<QuestionInfo> / Array<Answer> instead of any.
| // altimate_change start — BusEvent mirrors of the question events. | ||
| // | ||
| // The EventV2 `Event` defs above publish to GlobalBus/EventV2 consumers only. | ||
| // The IDE webview subscribes to the `/event` SSE route, which is fed by the Bus | ||
| // *wildcard PubSub* (`Bus.publish`), a different channel. So `question.asked` | ||
| // never reached the webview → `pendingQuestions` stayed empty → the mcp-add | ||
| // question card had no request id to reply with → "submit does nothing". | ||
| // Publish these via `Bus.publish` too so they reach /event like every other | ||
| // webview-visible event. (Schemas are loose — Bus.publish does not re-validate; | ||
| // they exist for the `type` string and typing.) | ||
| const BusAsked = BusEvent.define( | ||
| "question.asked", | ||
| z.object({ | ||
| id: QuestionID.zod, |
There was a problem hiding this comment.
[MINOR] Question events are now double-emitted on GlobalBus.
EventV2Bridge.listen already forwards every EventV2 event (incl. question.asked/replied/rejected) to GlobalBus, and Bus.publish here also emits to GlobalBus. So every GlobalBus//global/event consumer now receives each question event twice (with different top-level ids).
Verified impact — not the harm GLM-5 suspected: the TUI trace consumer (trace-consumer.ts handleEvent) only handles message.*/session.* and ignores question events entirely, so there is no double trace recording. TUI sync.tsx reconciles by request.id and notifications.ts dedupes via a Set<id> — both idempotent. No in-repo consumer breaks today.
Residual risk is only for external /global/event subscribers / side-effecting plugins that don't dedupe. Making the mirror best-effort (MAJOR #1) plus a one-line comment acknowledging the double is sufficient here; a cleaner long-term option is a local-only Bus publish that doesn't re-emit to GlobalBus.
Flagged by Claude, GPT, MiniMax, GLM-5, MiMo.
There was a problem hiding this comment.
Acknowledged in 0b5ecd12f. Kept the mirror (best-effort now) and added a comment documenting the intentional GlobalBus double-emit and your verification that no in-repo consumer is affected (TUI reconciles by request id, notifications dedupe by id, trace consumer ignores question events). Left the local-only-publish refactor as a future option.
|
👋 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. |
Address review feedback (flagged by all reviewers): the `Bus.publish` mirrors added for /event (webview) clients sat on the Deferred critical path. Since `Effect.promise` converts a promise rejection into an unrecoverable fiber defect, a `Bus.publish` rejection could abort the fiber before `Deferred.succeed`/`Deferred.fail` ran — leaving the awaiting `Question.ask()` hung on "Thinking…", the exact failure this PR fixes. - Add a best-effort `mirror()` helper: `Effect.promise(() => Bus.publish(...))` recovered with `Effect.catchCause` to a logged warning, so a publish failure can never abort core question settlement. - Settle the Deferred FIRST, then mirror, in `reply()` and `reject()`. - `ask()` uses `mirror()` too, so a publish failure can't abort it before the cleanup finalizer is registered. - Define the Bus mirror schemas from the structured Effect schemas via the `zod` adapter (`zod(Request)` / `zod(Replied)` / `zod(Rejected)`) so the generated /event OpenAPI payloads match the SDK types (no `any`/loose shapes). - Log (not silently swallow) failures in the instance-dispose cleanup. - Document the intentional GlobalBus double-emit (verified harmless in-repo). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
903c510 to
0b5ecd1
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. |
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. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| yield* Deferred.succeed(existing.deferred, input.answers) | ||
| // altimate_change start — mirror on the Bus wildcard for /event (webview) clients, | ||
| // AFTER settling the Deferred and best-effort so a publish failure can't re-hang ask(). | ||
| yield* mirror(BusReplied, { |
There was a problem hiding this comment.
SUGGESTION: The mirror(BusReplied, ...) object literal duplicates the events.publish(Event.Replied, ...) payload directly above it -- identical sessionID, requestID, and answers: input.answers.map((a) => [...a]). Hoisting a shared const replied = { ... } and passing it to both keeps the two channels (EventTable/GlobalBus vs /event SSE) from silently drifting. The same pattern repeats in reject() (lines 300-303 vs 307).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Approving — the question-hang fix is solid. The Bus mirror is now correctly kept off the Deferred settlement path (best-effort recovery via catchCause, and reply()/reject() settle before mirroring), the event schemas are structured via zod(Request/Replied/Rejected) instead of z.any(), and the duplicate emission on GlobalBus is documented and harmless for current consumers. Typecheck, the question suite, and the /event OpenAPI generation all pass.
Two non-blocking follow-ups for a separate PR:
1. Add a regression test for the Bus mirror. The fix's core invariant — a publish failure can't re-hang the tool — currently has no automated guard (deleting the three mirror lines leaves every test green). Suggested coverage:
- subscribe via
Bus.subscribeAlland assertquestion.asked/question.replied/question.rejectedare delivered afterask/reply/reject; and - inject a
Bus.publishfailure and assertreply()/reject()still settle theDeferred.
2. Same pattern exists (pre-existing) at session/session.ts:924. updatePartDelta publishes via the identical unguarded call:
yield* Effect.promise(() => Bus.publish(MessageV2.Event.PartDelta, input))Effect.promise turns a promise rejection into an unrecoverable fiber defect, so a Bus.publish rejection here would abort the streaming fiber rather than no-op — the same class this PR fixes for questions (lower impact: a streaming error rather than a stranded Deferred). Suggest routing it through the same best-effort recovery, ideally by extracting the mirror helper into a shared Bus utility so every Effect.promise(() => Bus.publish(...)) site shares one hardened path.
Minor: mirror uses Effect.catchCause, which also catches interrupts. Consider catchCauseIf(cause => !Cause.hasInterrupts(cause), …) so an interrupted fiber isn't logged as "mirror failed" and swallowed — matches the existing observe() pattern in event.ts.
3 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. Question state is shared across bundled module copies, and events reach the webview without blocking resolution so the MCP server is added.globalThis(keyed by instance directory) with per-instance cleanup viaregisterDisposer, soask()and HTTPreply()resolve the same request; cleanup failures are logged.question.asked/replied/rejectedto/eventviaBus.publishusingBusEventwithzodschemas; mirrors are best-effort and kept off the Deferred critical path—reply()/reject()settle first, then publish.Written for commit 0b5ecd1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes