Fix stalled resumes: wake after send + fail-closed MCP bridge#141
Open
leeroybrun wants to merge 1 commit intodevfrom
Open
Fix stalled resumes: wake after send + fail-closed MCP bridge#141leeroybrun wants to merge 1 commit intodevfrom
leeroybrun wants to merge 1 commit intodevfrom
Conversation
WalkthroughChanges enhance error handling for missing MCP server entrypoints and implement pending-queue wake/resume functionality when submitting messages in active sessions. Updates include new test cases, mocking improvements, and runtime logic adjustments across test and implementation files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR fixes two independent reliability bugs: (1) active sessions that stall after the runner dies mid-session are now recovered by issuing a best-effort
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| apps/ui/sources/components/sessions/shell/SessionView.tsx | Adds a best-effort "wake" call after a successful submitMessage in active sessions. Logic is sound and failure is properly non-fatal, but the new block introduces mixed tab/space indentation. |
| apps/cli/src/mcp/runtime/resolveNodeBackedMcpServerCommand.ts | Replaces the silent fallback (returning a non-existent packaged path) with a descriptive thrown error, making missing entrypoints fail closed. Clean and correct change. |
| apps/ui/sources/components/sessions/shell/SessionView.sendMessage.resumeInactive.pendingQueue.test.tsx | Adds a new test case for the active-session wake flow; introduces submitMessageSpy and a mutable sessionState to properly verify the two-step fire-and-forget behavior. Test design is solid. |
| apps/cli/src/agent/runtime/createHappierMcpBridge.test.ts | Adds existsSync mocks to three tests that previously relied on the old permissive fallback; necessary follow-up to the fail-closed change in resolveNodeBackedMcpServerCommand. |
| apps/cli/src/mcp/runtime/resolveNodeBackedMcpServerCommand.test.ts | Adds a test asserting the new throw path when both the packaged and source entrypoints are absent. Matches the production change exactly. |
| apps/cli/src/workspaces/replication/orchestration/executeWorkspaceReplicationJobWithLocalRuntime.test.ts | One-line fix: adds missing readFile to the node:fs/promises import. Trivially correct. |
Sequence Diagram
sequenceDiagram
participant User
participant SessionView
participant sync
participant fireAndForget
participant resumeSession
User->>SessionView: onSend() [active session]
SessionView->>fireAndForget: fireAndForget(submitMessage flow)
fireAndForget->>sync: submitMessage(sessionId, text, ...)
sync-->>fireAndForget: ok
fireAndForget->>SessionView: getPendingQueueWakeResumeOptions(...)
alt wakeOpts returned (session not thinking / no active requests)
SessionView->>fireAndForget: fireAndForget(wake flow)
fireAndForget->>resumeSession: resumeSession({ ...wakeOpts, serverId })
resumeSession-->>fireAndForget: result (success or error, both non-fatal)
else wakeOpts is null (session busy or no target)
SessionView-->>SessionView: skip wake silently
end
Reviews (1): Last reviewed commit: "fix: wake active sends + fail-closed MCP..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1746
to
+1783
| try { | ||
| await sync.submitMessage(sessionId, outbound.text, outbound.displayText, outbound.metaOverrides); | ||
| if (shouldSendReviewComments) { | ||
| storage.getState().clearSessionReviewCommentDrafts(sessionId); | ||
| } | ||
|
|
||
| const wakeOpts = getPendingQueueWakeResumeOptions({ | ||
| sessionId, | ||
| session, | ||
| resumeCapabilityOptions, | ||
| resumeTargetOverride: reachableMachineTarget | ||
| ? { | ||
| machineId: reachableMachineTarget.machineId, | ||
| directory: reachableMachineTarget.basePath, | ||
| } | ||
| : null, | ||
| permissionOverride: getPermissionModeOverrideForSpawn(session), | ||
| }); | ||
| if (wakeOpts) { | ||
| fireAndForget((async () => { | ||
| try { | ||
| const result = await resumeSession({ | ||
| ...wakeOpts, | ||
| serverId: capabilityServerId, | ||
| }); | ||
| if (result.type === 'error') { | ||
| // Non-fatal: message is already persisted in the transcript. | ||
| } | ||
| } catch { | ||
| // Non-fatal: message is already persisted in the transcript. | ||
| } | ||
| })(), { tag: 'SessionView.sendMessage.wakeActive' }); | ||
| } | ||
| } catch (e) { | ||
| setMessage(previousMessage); | ||
| Modal.alert(t('common.error'), e instanceof Error ? e.message : t('errors.failedToSendMessage')); | ||
| } | ||
| })(), { tag: 'SessionView.sendMessage.submitMessage' }); |
Contributor
There was a problem hiding this comment.
The new block (lines 1746–1783) is indented with a leading tab character (\t) followed by spaces, while every surrounding line in the file uses spaces exclusively. This creates inconsistent indentation that can cause visual diffs to appear garbled in editors and violates the file's existing style.
The affected lines should be re-indented to match the 28-space indent level used by the surrounding code:
try {
await sync.submitMessage(sessionId, outbound.text, outbound.displayText, outbound.metaOverrides);
if (shouldSendReviewComments) {
storage.getState().clearSessionReviewCommentDrafts(sessionId);
}
const wakeOpts = getPendingQueueWakeResumeOptions({
sessionId,
session,
resumeCapabilityOptions,
resumeTargetOverride: reachableMachineTarget
? {
machineId: reachableMachineTarget.machineId,
directory: reachableMachineTarget.basePath,
}
: null,
permissionOverride: getPermissionModeOverrideForSpawn(session),
});
if (wakeOpts) {
fireAndForget((async () => {
try {
const result = await resumeSession({
...wakeOpts,
serverId: capabilityServerId,
});
if (result.type === 'error') {
// Non-fatal: message is already persisted in the transcript.
}
} catch {
// Non-fatal: message is already persisted in the transcript.
}
})(), { tag: 'SessionView.sendMessage.wakeActive' });
}
} catch (e) {
setMessage(previousMessage);
Modal.alert(t('common.error'), e instanceof Error ? e.message : t('errors.failedToSendMessage'));
}
})(), { tag: 'SessionView.sendMessage.submitMessage' });
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/cli/src/agent/runtime/createHappierMcpBridge.test.ts (1)
43-46: Consider extracting the repeatedexistsSyncstub to a helper.The same mock implementation appears in three tests. A small helper would reduce duplication:
const stubDistBridgeExists = () => { vi.mocked(existsSync).mockImplementation((pathLike) => { const path = String(pathLike) return path.endsWith('/package-dist/backends/codex/happyMcpStdioBridge.mjs') }) }Then each test simply calls
stubDistBridgeExists().Also applies to: 64-67, 148-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/agent/runtime/createHappierMcpBridge.test.ts` around lines 43 - 46, The tests repeat the same existsSync mock implementation; extract that stub into a helper function (e.g., stubDistBridgeExists) and replace the repeated vi.mocked(existsSync).mockImplementation(...) occurrences with a single call to that helper in each test; locate the mocks by searching for usages of existsSync in createHappierMcpBridge.test.ts and replace the three duplicate blocks (the ones checking path.endsWith('/package-dist/backends/codex/happyMcpStdioBridge.mjs')) with calls to the new helper to remove duplication.apps/cli/src/mcp/runtime/resolveNodeBackedMcpServerCommand.ts (1)
58-62: Delay JS runtime resolution until after an entrypoint is chosen.Because Line 22 resolves
requireJavaScriptRuntimeExecutable()before anyexistsSyncchecks, this new error is still unreachable whenever runtime discovery fails first. Lines 10-17 inapps/cli/src/agent/runtime/createHappierMcpBridge.tslet that exception bubble straight up, so a broken install can still surfaceHAPPIER_JS_RUNTIME_PATHinstead of the clearer missing-entrypoint failure. Pick the usable entrypoint first, then resolve the runtime for that path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/mcp/runtime/resolveNodeBackedMcpServerCommand.ts` around lines 58 - 62, The code currently calls requireJavaScriptRuntimeExecutable() before checking which entrypoint exists, so runtime-resolution errors can mask a missing-entrypoint error; update resolveNodeBackedMcpServerCommand to first determine the usable entrypoint (check packagedEntrypoint and sourceEntrypoint with existsSync and tsxHookPath availability) and only after selecting the chosen entrypoint call requireJavaScriptRuntimeExecutable() for that path; ensure any thrown errors from requireJavaScriptRuntimeExecutable() occur after the entrypoint-not-found Error (the existing throw referencing packagedEntrypoint/sourceEntrypoint/tsxHookPath) so missing-entrypoint remains the surfaced failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/cli/src/agent/runtime/createHappierMcpBridge.test.ts`:
- Around line 43-46: The tests repeat the same existsSync mock implementation;
extract that stub into a helper function (e.g., stubDistBridgeExists) and
replace the repeated vi.mocked(existsSync).mockImplementation(...) occurrences
with a single call to that helper in each test; locate the mocks by searching
for usages of existsSync in createHappierMcpBridge.test.ts and replace the three
duplicate blocks (the ones checking
path.endsWith('/package-dist/backends/codex/happyMcpStdioBridge.mjs')) with
calls to the new helper to remove duplication.
In `@apps/cli/src/mcp/runtime/resolveNodeBackedMcpServerCommand.ts`:
- Around line 58-62: The code currently calls
requireJavaScriptRuntimeExecutable() before checking which entrypoint exists, so
runtime-resolution errors can mask a missing-entrypoint error; update
resolveNodeBackedMcpServerCommand to first determine the usable entrypoint
(check packagedEntrypoint and sourceEntrypoint with existsSync and tsxHookPath
availability) and only after selecting the chosen entrypoint call
requireJavaScriptRuntimeExecutable() for that path; ensure any thrown errors
from requireJavaScriptRuntimeExecutable() occur after the entrypoint-not-found
Error (the existing throw referencing
packagedEntrypoint/sourceEntrypoint/tsxHookPath) so missing-entrypoint remains
the surfaced failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97190716-d3c2-4c7f-9b91-f6d07b1ac44d
📒 Files selected for processing (6)
apps/cli/src/agent/runtime/createHappierMcpBridge.test.tsapps/cli/src/mcp/runtime/resolveNodeBackedMcpServerCommand.test.tsapps/cli/src/mcp/runtime/resolveNodeBackedMcpServerCommand.tsapps/cli/src/workspaces/replication/orchestration/executeWorkspaceReplicationJobWithLocalRuntime.test.tsapps/ui/sources/components/sessions/shell/SessionView.sendMessage.resumeInactive.pendingQueue.test.tsxapps/ui/sources/components/sessions/shell/SessionView.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Repro evidence:
Tests:
Summary by CodeRabbit
Bug Fixes
New Features
Tests