WIP: MCP event pipeline + passive status panel - #39
Merged
Conversation
Wire Codex mcpServer/* events through codexEventMapping → ProviderRuntimeIngestion into orchestration activities. Add mcp-session-logic view model derivation, shared mcp utilities, ThreadMcpStatusPanel component, and ChatView/composer integration. Also includes harness-side OpenCode session cleanup (model discovery clause fix, session manager adjustments, MCP channel handlers). Panel is passive-only — renders MCP server statuses from streamed events. Real management surface (WS API, fetch-on-open, action buttons) to follow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size:XL
vouch:trusted
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end MCP (managed control plane) support: providers emit MCP events, runtime mapping produces mcp.* activities, server exposes MCP RPCs and WS routing, harness/tests include MCP stubs, and the web app gains MCP view-model logic, UI components, and native API bindings. Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as Provider (opencode/claude)
participant Harness as Harness Session
participant Mapper as Event Mapper
participant Orch as Orchestration Server
participant Client as Web Client
Provider->>Harness: emit MCP update (startup/status/oauth)
Harness->>Mapper: normalize notification (mcpServer/startupStatus/updated)
Mapper->>Orch: map -> OrchestrationThreadActivity (kind: mcp.* , payload includes server+status)
Orch->>Client: push activity via WebSocket
Client->>Client: deriveMcpSessionViewModel(activities)
Client->>Client: render ThreadMcpStatusPanel (badges/messages/auth state)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/shared/src/mcp.ts (1)
14-18: Consider capitalizing each word for better display names.The fallback humanization only capitalizes the first character of the entire string, not each word. For a server like
"myCustomServer", this produces"My custom server"instead of"My Custom Server". This might be intentional for a more sentence-like appearance, but Title Case is more common for labels.♻️ Optional: Title-case all words
const spaced = server .replace(/([a-z0-9])([A-Z])/g, "$1 $2") .replace(/[_-]+/g, " ") .trim(); -return spaced.length > 0 ? spaced[0]!.toUpperCase() + spaced.slice(1) : server; +if (spaced.length === 0) return server; +return spaced.split(" ").map(w => w.charAt(0).toUpperCase() + w.slice(1).toLowerCase()).join(" ");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/mcp.ts` around lines 14 - 18, The current humanization builds a spaced string into the variable spaced and only uppercases the very first character; update the return to Title Case by transforming each word in spaced (split on spaces), capitalizing the first character of each word and lowercasing the rest, then join them back with spaces; modify the return expression that currently uses spaced[0]!.toUpperCase() + spaced.slice(1) to perform this per-word capitalization so inputs like "myCustomServer" yield "My Custom Server".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/harness/lib/harness_web/harness_channel.ex`:
- Around line 216-219: The fallback handle_in clauses currently match absent
keys but still allow JSON nulls through (e.g., the handler def
handle_in("mcp.status", _params, socket) lets %{"threadId" => nil} slip); change
the success clause signatures to explicitly bind and guard non-nil values (for
example: def handle_in("mcp.status", %{"threadId" => threadId} = params, socket)
when not is_nil(threadId) do ...) or convert those handlers to the same
with-style validation used elsewhere so that %{"threadId" => nil} / %{"name" =>
nil} / %{"config" => nil} are rejected and routed to the existing nil-match
error clauses instead; apply the same pattern to the other MCP handlers (the
prior success clauses around session creation/update/status) so null values are
rejected, not forwarded to SessionManager.
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 1455-1461: fetch_providers/1 never receives decoded JSON arrays
because http_get/1 only treats maps as already-decoded; when Req returns a list
body it falls through to Jason.decode(to_string(body)) which fails. Modify
http_get/1 so it treats lists like maps: change the branch that currently checks
is_map(body) to check is_map(body) or is_list(body) (i.e., if is_map(body) or
is_list(body), do: {:ok, body}, else: Jason.decode(to_string(body))) so that
http_get/1 returns {:ok, body} for already-decoded lists and fetch_providers/1
can handle the list-response branch.
In `@apps/web/src/components/ChatView.tsx`:
- Line 346: The mutual-exclusion logic is only applied in the toggle handlers
(togglePlanSidebar / toggleMcpPanel) but not where the panels are directly
opened (e.g. calls to setPlanSidebarOpen(true)) and mcpPanelOpen is never reset
on thread changes; fix this by centralizing open logic: create helper functions
openPlanSidebar() and openMcpPanel() (or replace direct setPlanSidebarOpen(true)
/ setMcpPanelOpen(true) calls) so each opener sets its own boolean true and
force-sets the other panel false, and call these helpers everywhere the panels
are opened (including places currently calling setPlanSidebarOpen(true) and on
thread-change handlers) so the two panels can never be true simultaneously and
mcpPanelOpen is cleared on thread switches.
In `@apps/web/src/components/ThreadMcpStatusPanel.tsx`:
- Around line 41-59: The Card layout doesn't create an internal scroll region
because the card isn't a constrained column; update ThreadMcpStatusPanel so the
Card element becomes a vertical flex container (e.g., add "flex flex-col") and
make CardContent a growing scrollable area (replace/augment its classes with
"flex-1 overflow-y-auto" or similar) so long lists scroll inside the card when
the parent provides a max-height (this touches the Card and CardContent elements
in ThreadMcpStatusPanel; ensure ChatView still supplies a max-h-* to constrain
the card).
In `@apps/web/src/mcp-session-logic.ts`:
- Around line 148-170: hasActiveWarning currently only compares warningAt to
oauthSuccessAt, so a later status update (statusAt) won't clear the warning;
change hasActiveWarning to treat a warning as active only if warningAt is newer
than the most recent non-null authentication/status timestamp (i.e., compare
warningAt against server.statusAt if present, otherwise server.oauthSuccessAt),
e.g. compute latestEvent = server.statusAt ?? server.oauthSuccessAt and return
!!server.warningAt && (!latestEvent || server.warningAt > latestEvent); update
any related logic in serverDisplayState and serverLastEventAt to rely on the
corrected hasActiveWarning so warnings are cleared when a newer status update
(statusAt) arrives.
---
Nitpick comments:
In `@packages/shared/src/mcp.ts`:
- Around line 14-18: The current humanization builds a spaced string into the
variable spaced and only uppercases the very first character; update the return
to Title Case by transforming each word in spaced (split on spaces),
capitalizing the first character of each word and lowercasing the rest, then
join them back with spaces; modify the return expression that currently uses
spaced[0]!.toUpperCase() + spaced.slice(1) to perform this per-word
capitalization so inputs like "myCustomServer" yield "My Custom Server".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b363094-de57-403b-bc7b-f05d19f6c8ef
📒 Files selected for processing (14)
apps/harness/lib/harness/model_discovery.exapps/harness/lib/harness/providers/opencode_session.exapps/harness/lib/harness/session_manager.exapps/harness/lib/harness_web/harness_channel.exapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/provider/Layers/codexEventMapping.tsapps/web/src/components/ChatView.tsxapps/web/src/components/ThreadMcpStatusPanel.tsxapps/web/src/components/chat/CompactComposerControlsMenu.tsxapps/web/src/mcp-session-logic.tsapps/web/src/session-logic.tspackages/contracts/src/providerRuntime.tspackages/shared/package.jsonpackages/shared/src/mcp.ts
Comment on lines
+216
to
+219
| @impl true | ||
| def handle_in("mcp.status", _params, socket) do | ||
| {:reply, {:error, %{message: "Missing required param: threadId"}}, socket} | ||
| end |
There was a problem hiding this comment.
These fallback clauses still let JSON null through.
The earlier heads on Line 206, Line 222, Line 245, and Line 262 already match %{"threadId" => nil} / %{"name" => nil} / %{"config" => nil}, so these new handlers only fire when the key is absent, not when the client sends null. That still forwards malformed MCP commands into SessionManager instead of returning the intended validation error. Guard the success clauses with when not is_nil(...) or switch them to the same with-style validation used in the session handlers.
Also applies to: 232-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness_web/harness_channel.ex` around lines 216 - 219, The
fallback handle_in clauses currently match absent keys but still allow JSON
nulls through (e.g., the handler def handle_in("mcp.status", _params, socket)
lets %{"threadId" => nil} slip); change the success clause signatures to
explicitly bind and guard non-nil values (for example: def
handle_in("mcp.status", %{"threadId" => threadId} = params, socket) when not
is_nil(threadId) do ...) or convert those handlers to the same with-style
validation used elsewhere so that %{"threadId" => nil} / %{"name" => nil} /
%{"config" => nil} are rejected and routed to the existing nil-match error
clauses instead; apply the same pattern to the other MCP handlers (the prior
success clauses around session creation/update/status) so null values are
rejected, not forwarded to SessionManager.
Comment on lines
1455
to
1461
| defp fetch_providers(state) do | ||
| case http_get("#{state.base_url}/provider") do | ||
| {:ok, %{"all" => providers}} when is_map(providers) -> | ||
| {:ok, %{"all" => providers}} when is_list(providers) -> | ||
| {:ok, providers} | ||
|
|
||
| {:ok, providers} when is_map(providers) -> | ||
| {:ok, providers} when is_list(providers) -> | ||
| {:ok, providers} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Req (Elixir), when a JSON response body is a top-level array, is response.body decoded into an Elixir list by default?
💡 Result:
Yes, in Req (Elixir), when a JSON response body is a top-level array and the Content-Type header indicates application/json, response.body is decoded into an Elixir list by default. Req's decode_body response step automatically detects the format from the content-type header and uses Jason.decode!/2 to parse JSON bodies. Jason decodes JSON arrays to Elixir lists [1,2,3] -> [1,2,3]. This is enabled by default (decode_body: true).
Citations:
- 1: https://hexdocs.pm/req/Req.Steps.html
- 2: https://hexdocs.pm/req/0.4.0/Req.Steps.html
- 3: https://hexdocs.pm/jason/Jason.html
🏁 Script executed:
cd apps/harness && sed -n '1330,1341p' lib/harness/providers/opencode_session.exRepository: Ranvier-Technologies/t3code-OTP
Length of output: 495
The new list-response branch is unreachable with the current http_get/1 implementation.
Line 1331-1334 checks only for maps; when Req decodes a JSON array from /provider, it returns an Elixir list. Since is_map(body) is false for lists, the code calls Jason.decode(to_string(body)). The to_string/1 call converts the list to its string representation (e.g., "[%{...}]"), which is not valid JSON. The subsequent Jason.decode/1 call will raise an error, preventing fetch_providers/1 from ever receiving the decoded list. This breaks provider discovery on any endpoint returning a JSON array.
Fix: Update the condition to if is_map(body) or is_list(body), do: {:ok, body}, else: Jason.decode(to_string(body)) to pass through already-decoded lists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 1455 -
1461, fetch_providers/1 never receives decoded JSON arrays because http_get/1
only treats maps as already-decoded; when Req returns a list body it falls
through to Jason.decode(to_string(body)) which fails. Modify http_get/1 so it
treats lists like maps: change the branch that currently checks is_map(body) to
check is_map(body) or is_list(body) (i.e., if is_map(body) or is_list(body), do:
{:ok, body}, else: Jason.decode(to_string(body))) so that http_get/1 returns
{:ok, body} for already-decoded lists and fetch_providers/1 can handle the
list-response branch.
| useState<Record<string, number>>({}); | ||
| const [expandedWorkGroups, setExpandedWorkGroups] = useState<Record<string, boolean>>({}); | ||
| const [planSidebarOpen, setPlanSidebarOpen] = useState(false); | ||
| const [mcpPanelOpen, setMcpPanelOpen] = useState(false); |
There was a problem hiding this comment.
Mutual exclusion is only enforced in the toggle handlers.
Because the sidebars render independently on Line 4190 and Line 4209, any path that leaves both booleans true will show both panels. Right now only togglePlanSidebar / toggleMcpPanel cross-close each other; direct opens later in this file still call setPlanSidebarOpen(true) (for example Line 1900 and Line 2989), and mcpPanelOpen is never cleared on thread changes. That breaks the PR’s stated mutual-exclusion behavior and can also reopen the MCP panel on a different thread.
Also applies to: 1623-1637, 4209-4218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/ChatView.tsx` at line 346, The mutual-exclusion logic
is only applied in the toggle handlers (togglePlanSidebar / toggleMcpPanel) but
not where the panels are directly opened (e.g. calls to
setPlanSidebarOpen(true)) and mcpPanelOpen is never reset on thread changes; fix
this by centralizing open logic: create helper functions openPlanSidebar() and
openMcpPanel() (or replace direct setPlanSidebarOpen(true) /
setMcpPanelOpen(true) calls) so each opener sets its own boolean true and
force-sets the other panel false, and call these helpers everywhere the panels
are opened (including places currently calling setPlanSidebarOpen(true) and on
thread-change handlers) so the two panels can never be true simultaneously and
mcpPanelOpen is cleared on thread switches.
Comment on lines
+41
to
+59
| <Card className={cn("w-full", className)}> | ||
| <CardHeader className="gap-1 p-4 pb-3"> | ||
| <CardTitle className="text-base">MCP status</CardTitle> | ||
| <CardDescription>Runtime MCP state for the active thread session.</CardDescription> | ||
| {onClose ? ( | ||
| <CardAction> | ||
| <Button | ||
| type="button" | ||
| aria-label="Hide MCP status" | ||
| size="icon-xs" | ||
| variant="ghost" | ||
| onClick={onClose} | ||
| > | ||
| <XIcon className="size-3.5" /> | ||
| </Button> | ||
| </CardAction> | ||
| ) : null} | ||
| </CardHeader> | ||
| <CardContent className="min-h-0 space-y-3 overflow-y-auto p-4 pt-0"> |
There was a problem hiding this comment.
The content area will not actually scroll with the current card layout.
CardContent has overflow-y-auto, but the card itself is not a constrained flex column. In the current integration (apps/web/src/components/ChatView.tsx, Line 4215), the panel only gets a max-h-* class, so a long server list will overflow past the card instead of creating an internal scroll region.
Suggested layout fix
- <Card className={cn("w-full", className)}>
+ <Card className={cn("flex min-h-0 w-full flex-col overflow-hidden", className)}>- <CardContent className="min-h-0 space-y-3 overflow-y-auto p-4 pt-0">
+ <CardContent className="min-h-0 flex-1 space-y-3 overflow-y-auto p-4 pt-0">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Card className={cn("w-full", className)}> | |
| <CardHeader className="gap-1 p-4 pb-3"> | |
| <CardTitle className="text-base">MCP status</CardTitle> | |
| <CardDescription>Runtime MCP state for the active thread session.</CardDescription> | |
| {onClose ? ( | |
| <CardAction> | |
| <Button | |
| type="button" | |
| aria-label="Hide MCP status" | |
| size="icon-xs" | |
| variant="ghost" | |
| onClick={onClose} | |
| > | |
| <XIcon className="size-3.5" /> | |
| </Button> | |
| </CardAction> | |
| ) : null} | |
| </CardHeader> | |
| <CardContent className="min-h-0 space-y-3 overflow-y-auto p-4 pt-0"> | |
| <Card className={cn("flex min-h-0 w-full flex-col overflow-hidden", className)}> | |
| <CardHeader className="gap-1 p-4 pb-3"> | |
| <CardTitle className="text-base">MCP status</CardTitle> | |
| <CardDescription>Runtime MCP state for the active thread session.</CardDescription> | |
| {onClose ? ( | |
| <CardAction> | |
| <Button | |
| type="button" | |
| aria-label="Hide MCP status" | |
| size="icon-xs" | |
| variant="ghost" | |
| onClick={onClose} | |
| > | |
| <XIcon className="size-3.5" /> | |
| </Button> | |
| </CardAction> | |
| ) : null} | |
| </CardHeader> | |
| <CardContent className="min-h-0 flex-1 space-y-3 overflow-y-auto p-4 pt-0"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/ThreadMcpStatusPanel.tsx` around lines 41 - 59, The
Card layout doesn't create an internal scroll region because the card isn't a
constrained column; update ThreadMcpStatusPanel so the Card element becomes a
vertical flex container (e.g., add "flex flex-col") and make CardContent a
growing scrollable area (replace/augment its classes with "flex-1
overflow-y-auto" or similar) so long lists scroll inside the card when the
parent provides a max-height (this touches the Card and CardContent elements in
ThreadMcpStatusPanel; ensure ChatView still supplies a max-h-* to constrain the
card).
Comment on lines
+148
to
+170
| function hasActiveWarning(server: MutableServerState): boolean { | ||
| if (!server.warningAt) { | ||
| return false; | ||
| } | ||
| if (!server.oauthSuccessAt) { | ||
| return true; | ||
| } | ||
| return server.warningAt > server.oauthSuccessAt; | ||
| } | ||
|
|
||
| function serverDisplayState(server: MutableServerState): McpSessionServerState { | ||
| if (hasActiveWarning(server)) { | ||
| return "warning"; | ||
| } | ||
| return server.statusState; | ||
| } | ||
|
|
||
| function serverLastEventAt(server: MutableServerState): string | null { | ||
| if (hasActiveWarning(server)) { | ||
| return server.warningAt; | ||
| } | ||
| return server.statusAt ?? server.oauthSuccessAt; | ||
| } |
There was a problem hiding this comment.
A newer status update never clears an older warning.
hasActiveWarning() only compares warningAt with oauthSuccessAt. If a server emits a startup warning and then later sends mcp.status.updated with ready or failed, serverDisplayState() still returns "warning" forever unless an OAuth completion event also arrives. That leaves stale warnings pinned in the panel even after a newer status event supersedes them.
Suggested fix
function hasActiveWarning(server: MutableServerState): boolean {
if (!server.warningAt) {
return false;
}
- if (!server.oauthSuccessAt) {
- return true;
- }
- return server.warningAt > server.oauthSuccessAt;
+ const clearedAt = [server.statusAt, server.oauthSuccessAt].reduce<string | null>(
+ (latest, value) => {
+ if (!value) return latest;
+ return !latest || value > latest ? value : latest;
+ },
+ null,
+ );
+ return !clearedAt || server.warningAt > clearedAt;
}Also applies to: 203-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/mcp-session-logic.ts` around lines 148 - 170, hasActiveWarning
currently only compares warningAt to oauthSuccessAt, so a later status update
(statusAt) won't clear the warning; change hasActiveWarning to treat a warning
as active only if warningAt is newer than the most recent non-null
authentication/status timestamp (i.e., compare warningAt against server.statusAt
if present, otherwise server.oauthSuccessAt), e.g. compute latestEvent =
server.statusAt ?? server.oauthSuccessAt and return !!server.warningAt &&
(!latestEvent || server.warningAt > latestEvent); update any related logic in
serverDisplayState and serverLastEventAt to rely on the corrected
hasActiveWarning so warnings are cleared when a newer status update (statusAt)
arrives.
- Add mcp.status/add/connect/disconnect to WS_METHODS, input schemas, and WebSocketRequestBody union in contracts - Add MCP methods to ProviderServiceShape with capability-gated implementation via optional HarnessClientAdapter (Effect.serviceOption) - Route 4 new WS methods through wsServer routeRequest switch - Add mcp namespace to NativeApi interface and wsNativeApi transport - ThreadMcpStatusPanel now fetches status on mount via NativeApi.mcp.status, merges fetched baseline with streamed event data - MCP toggle shows based on provider mcpConfig capability, not just events - Extract mcp_servers from Claude CLI system/init in claude_session.ex, normalized to mcpServer/startupStatus/updated for uniform pipeline Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/components/ChatView.tsx (1)
1629-1643:⚠️ Potential issue | 🟠 MajorMutual exclusion still leaks outside these toggles.
These handlers only cross-close on toggle clicks. Line 1906 and Line 2995 still open the plan sidebar with direct
setPlanSidebarOpen(true), and nothing clearsmcpPanelOpenon thread switches, so both panels can still end up open at once or the MCP panel can carry over to the next thread. Please funnel every open through shared helpers and reset the MCP panel in the thread-change path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 1629 - 1643, The mutual-exclusion logic is incomplete because opens are done directly with setPlanSidebarOpen(true) or setMcpPanelOpen(true) elsewhere and the MCP state isn't reset on thread changes; refactor so all places that open either panel call shared helpers (e.g., create openPlanSidebar() and openMcpPanel() that call setPlanSidebarOpen(true)/setMcpPanelOpen(true) and also close the opposite panel) and replace direct setPlanSidebarOpen(true)/setMcpPanelOpen(true) usages (including the thread-switch path and the locations referenced around activePlan?.turnId and sidebarProposedPlan?.turnId) to use those helpers; additionally, ensure the thread-change handler explicitly resets mcpPanelOpen (via setMcpPanelOpen(false) or the helper) when switching threads so the MCP panel cannot carry over.
🧹 Nitpick comments (1)
apps/server/src/wsServer.test.ts (1)
1298-1301: Add explicit WS MCP route tests (not just interface stubs).Great that the mock now satisfies
ProviderServiceShape, but it would be stronger to add directsendRequest(..., WS_METHODS.mcp*)assertions to verify payload mapping and invocation args on the provider mock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/wsServer.test.ts` around lines 1298 - 1301, Add explicit tests that call sendRequest with WS_METHODS.mcpStatus, WS_METHODS.mcpAdd, WS_METHODS.mcpConnect and WS_METHODS.mcpDisconnect and assert the provider mock's corresponding methods (mcpStatus, mcpAdd, mcpConnect, mcpDisconnect) are invoked with the expected payloads and arguments; update the existing provider mock used in the test to record received args (e.g., spy/stub behavior) and in each test assert that sendRequest(...) produced the correct payload mapping and invocation count on the mock to ensure the WS MCP routes are exercised beyond just satisfying ProviderServiceShape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/harness/lib/harness/providers/claude_session.ex`:
- Around line 611-616: The code iterates over Map.get(msg, "mcp_servers", [])
without guarding for a nil value, which can crash when the external JSON
contains "mcp_servers": null; change the logic around the for-comprehension to
first fetch Map.get(msg, "mcp_servers") and pattern-match or use a case to only
run the for when the value is a list (e.g., case Map.get(msg, "mcp_servers") do
servers when is_list(servers) -> for %{"name" => name, "status" => status} <-
servers do emit_event(state, :notification, "mcpServer/startupStatus/updated",
%{"name" => name, "status" => status}) end; _ -> :ok end), so only lists are
iterated and nil or other types are ignored; reference Map.get(msg,
"mcp_servers"), the for comprehension, and emit_event to locate the change.
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 1089-1121: The current requireMcpCapable only checks mcpConfig and
can still return inactive or direct-routed sessions; update requireMcpCapable
(used by withMcpHarness) to additionally reject sessions where routed.isActive
=== false and where routed.getAdapterPath?.() === "direct" by returning a
validation error (use toValidationError with the same operation message style),
so MCP calls are blocked for stopped threads or direct routes before delegating
to HarnessClientAdapter.
In `@apps/web/src/components/ChatView.tsx`:
- Around line 4215-4224: The fixed 340px wrapper is rendered even when
ThreadMcpStatusPanel returns null, producing an empty gutter; fix by only
rendering the wrapper when the panel has content—either add a predicate check
(e.g., if mcpViewModel.hasFetchedServers || mcpViewModel.hasActivity or a new
mcpViewModel.hasContent() accessor) alongside the existing mcpPanelOpen &&
activeThreadId condition before rendering the div, or move the wrapper (the div
with className "flex h-full w-[340px] ...") into ThreadMcpStatusPanel so the
component controls whether the shell is rendered when it would otherwise return
null.
---
Duplicate comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 1629-1643: The mutual-exclusion logic is incomplete because opens
are done directly with setPlanSidebarOpen(true) or setMcpPanelOpen(true)
elsewhere and the MCP state isn't reset on thread changes; refactor so all
places that open either panel call shared helpers (e.g., create
openPlanSidebar() and openMcpPanel() that call
setPlanSidebarOpen(true)/setMcpPanelOpen(true) and also close the opposite
panel) and replace direct setPlanSidebarOpen(true)/setMcpPanelOpen(true) usages
(including the thread-switch path and the locations referenced around
activePlan?.turnId and sidebarProposedPlan?.turnId) to use those helpers;
additionally, ensure the thread-change handler explicitly resets mcpPanelOpen
(via setMcpPanelOpen(false) or the helper) when switching threads so the MCP
panel cannot carry over.
---
Nitpick comments:
In `@apps/server/src/wsServer.test.ts`:
- Around line 1298-1301: Add explicit tests that call sendRequest with
WS_METHODS.mcpStatus, WS_METHODS.mcpAdd, WS_METHODS.mcpConnect and
WS_METHODS.mcpDisconnect and assert the provider mock's corresponding methods
(mcpStatus, mcpAdd, mcpConnect, mcpDisconnect) are invoked with the expected
payloads and arguments; update the existing provider mock used in the test to
record received args (e.g., spy/stub behavior) and in each test assert that
sendRequest(...) produced the correct payload mapping and invocation count on
the mock to ensure the WS MCP routes are exercised beyond just satisfying
ProviderServiceShape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 933a1e8f-7f79-4fbd-a675-503b8bf16ea6
📒 Files selected for processing (13)
apps/harness/lib/harness/providers/claude_session.exapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/Services/ProviderService.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/src/components/ChatView.tsxapps/web/src/components/ThreadMcpStatusPanel.tsxapps/web/src/wsNativeApi.tspackages/contracts/src/ipc.tspackages/contracts/src/ws.ts
✅ Files skipped from review due to trivial changes (1)
- apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/ThreadMcpStatusPanel.tsx
Comment on lines
+611
to
+616
| for %{"name" => name, "status" => status} <- Map.get(msg, "mcp_servers", []) do | ||
| emit_event(state, :notification, "mcpServer/startupStatus/updated", %{ | ||
| "name" => name, | ||
| "status" => status | ||
| }) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/harness/lib/harness/providers/claude_session.ex | sed -n '605,620p'Repository: Ranvier-Technologies/t3code-OTP
Length of output: 665
🏁 Script executed:
cd apps/harness && head -20 lib/harness/providers/claude_session.exRepository: Ranvier-Technologies/t3code-OTP
Length of output: 971
🏁 Script executed:
cat -n apps/harness/lib/harness/providers/claude_session.ex | sed -n '550,650p'Repository: Ranvier-Technologies/t3code-OTP
Length of output: 3842
🏁 Script executed:
cat > /tmp/test_map_get.exs << 'EOF'
# Test Map.get behavior with nil values
msg1 = %{"mcp_servers" => nil}
msg2 = %{}
result1 = Map.get(msg1, "mcp_servers", [])
result2 = Map.get(msg2, "mcp_servers", [])
IO.inspect(result1, label: "Map.get with nil value")
IO.inspect(result2, label: "Map.get with missing key")
# Test if for loop crashes with nil
IO.puts("\nTesting for loop with nil:")
try do
result = for item <- nil, do: item
IO.inspect(result)
rescue
error -> IO.inspect(error, label: "Exception")
end
EOF
elixir /tmp/test_map_get.exsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 122
🏁 Script executed:
cat -n apps/harness/lib/harness/providers/claude_session.ex | sed -n '540,600p'Repository: Ranvier-Technologies/t3code-OTP
Length of output: 2186
🏁 Script executed:
rg -n "handle_sdk_message\|jason\|JSON" apps/harness/lib/harness/providers/claude_session.ex | head -20Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
Guard mcp_servers before iterating to avoid GenServer crashes.
Line 611 iterates directly over Map.get(msg, "mcp_servers", []), which crashes if the key exists with a nil value. Since this is external JSON from the Claude CLI, it can contain "mcp_servers": null. Use a case guard to safely check the type before iteration.
Suggested fix
- for %{"name" => name, "status" => status} <- Map.get(msg, "mcp_servers", []) do
- emit_event(state, :notification, "mcpServer/startupStatus/updated", %{
- "name" => name,
- "status" => status
- })
- end
+ mcp_servers =
+ case Map.get(msg, "mcp_servers") do
+ servers when is_list(servers) -> servers
+ _ -> []
+ end
+
+ Enum.each(mcp_servers, fn
+ %{"name" => name, "status" => status} when is_binary(name) and is_binary(status) ->
+ emit_event(state, :notification, "mcpServer/startupStatus/updated", %{
+ "name" => name,
+ "status" => status
+ })
+
+ _ ->
+ :ok
+ end)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for %{"name" => name, "status" => status} <- Map.get(msg, "mcp_servers", []) do | |
| emit_event(state, :notification, "mcpServer/startupStatus/updated", %{ | |
| "name" => name, | |
| "status" => status | |
| }) | |
| end | |
| mcp_servers = | |
| case Map.get(msg, "mcp_servers") do | |
| servers when is_list(servers) -> servers | |
| _ -> [] | |
| end | |
| Enum.each(mcp_servers, fn | |
| %{"name" => name, "status" => status} when is_binary(name) and is_binary(status) -> | |
| emit_event(state, :notification, "mcpServer/startupStatus/updated", %{ | |
| "name" => name, | |
| "status" => status | |
| }) | |
| _ -> | |
| :ok | |
| end) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/providers/claude_session.ex` around lines 611 - 616,
The code iterates over Map.get(msg, "mcp_servers", []) without guarding for a
nil value, which can crash when the external JSON contains "mcp_servers": null;
change the logic around the for-comprehension to first fetch Map.get(msg,
"mcp_servers") and pattern-match or use a case to only run the for when the
value is a list (e.g., case Map.get(msg, "mcp_servers") do servers when
is_list(servers) -> for %{"name" => name, "status" => status} <- servers do
emit_event(state, :notification, "mcpServer/startupStatus/updated", %{"name" =>
name, "status" => status}) end; _ -> :ok end), so only lists are iterated and
nil or other types are ignored; reference Map.get(msg, "mcp_servers"), the for
comprehension, and emit_event to locate the change.
Comment on lines
+1089
to
+1121
| const requireMcpCapable = (threadId: ThreadId, operation: string) => | ||
| Effect.gen(function* () { | ||
| const routed = yield* resolveRoutableSession({ | ||
| threadId, | ||
| operation, | ||
| allowRecovery: false, | ||
| }); | ||
| if (routed.adapter.capabilities.mcpConfig === "none") { | ||
| return yield* toValidationError( | ||
| operation, | ||
| `Provider '${routed.adapter.provider}' does not support MCP management.`, | ||
| ); | ||
| } | ||
| return routed; | ||
| }); | ||
|
|
||
| const harnessOption = yield* Effect.serviceOption(HarnessClientAdapter); | ||
|
|
||
| const withMcpHarness = <A>( | ||
| operation: string, | ||
| threadId: ThreadId, | ||
| fn: (harness: HarnessClientAdapterShape) => Effect.Effect<A, ProviderAdapterError>, | ||
| ) => | ||
| Effect.gen(function* () { | ||
| yield* requireMcpCapable(threadId, operation); | ||
| const harness = Option.getOrUndefined(harnessOption); | ||
| if (!harness) { | ||
| return yield* Effect.fail( | ||
| toValidationError(operation, "MCP management requires the Elixir harness."), | ||
| ); | ||
| } | ||
| return yield* fn(harness); | ||
| }); |
There was a problem hiding this comment.
Reject inactive or non-harness routes before delegating to the harness.
resolveRoutableSession({ allowRecovery: false }) can still return isActive: false, and getAdapterPath() can still be "direct" for MCP-capable threads. Because this guard only checks mcpConfig !== "none", the new MCP methods can call HarnessClientAdapter for stopped threads or direct-routed providers, which turns what should be a validation failure into a transport failure.
🛠️ Suggested guard
- const requireMcpCapable = (threadId: ThreadId, operation: string) =>
+ const requireMcpHarnessSession = (threadId: ThreadId, operation: string) =>
Effect.gen(function* () {
const routed = yield* resolveRoutableSession({
threadId,
operation,
allowRecovery: false,
});
+ if (!routed.isActive) {
+ return yield* toValidationError(
+ operation,
+ `Thread '${threadId}' does not have an active MCP-manageable session.`,
+ );
+ }
if (routed.adapter.capabilities.mcpConfig === "none") {
return yield* toValidationError(
operation,
`Provider '${routed.adapter.provider}' does not support MCP management.`,
);
}
+ if (routed.adapterPath !== "harness") {
+ return yield* toValidationError(
+ operation,
+ `Provider '${routed.adapter.provider}' does not expose MCP management via the ${routed.adapterPath} adapter path.`,
+ );
+ }
return routed;
});
@@
- yield* requireMcpCapable(threadId, operation);
+ yield* requireMcpHarnessSession(threadId, operation);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const requireMcpCapable = (threadId: ThreadId, operation: string) => | |
| Effect.gen(function* () { | |
| const routed = yield* resolveRoutableSession({ | |
| threadId, | |
| operation, | |
| allowRecovery: false, | |
| }); | |
| if (routed.adapter.capabilities.mcpConfig === "none") { | |
| return yield* toValidationError( | |
| operation, | |
| `Provider '${routed.adapter.provider}' does not support MCP management.`, | |
| ); | |
| } | |
| return routed; | |
| }); | |
| const harnessOption = yield* Effect.serviceOption(HarnessClientAdapter); | |
| const withMcpHarness = <A>( | |
| operation: string, | |
| threadId: ThreadId, | |
| fn: (harness: HarnessClientAdapterShape) => Effect.Effect<A, ProviderAdapterError>, | |
| ) => | |
| Effect.gen(function* () { | |
| yield* requireMcpCapable(threadId, operation); | |
| const harness = Option.getOrUndefined(harnessOption); | |
| if (!harness) { | |
| return yield* Effect.fail( | |
| toValidationError(operation, "MCP management requires the Elixir harness."), | |
| ); | |
| } | |
| return yield* fn(harness); | |
| }); | |
| const requireMcpHarnessSession = (threadId: ThreadId, operation: string) => | |
| Effect.gen(function* () { | |
| const routed = yield* resolveRoutableSession({ | |
| threadId, | |
| operation, | |
| allowRecovery: false, | |
| }); | |
| if (!routed.isActive) { | |
| return yield* toValidationError( | |
| operation, | |
| `Thread '${threadId}' does not have an active MCP-manageable session.`, | |
| ); | |
| } | |
| if (routed.adapter.capabilities.mcpConfig === "none") { | |
| return yield* toValidationError( | |
| operation, | |
| `Provider '${routed.adapter.provider}' does not support MCP management.`, | |
| ); | |
| } | |
| if (routed.adapterPath !== "harness") { | |
| return yield* toValidationError( | |
| operation, | |
| `Provider '${routed.adapter.provider}' does not expose MCP management via the ${routed.adapterPath} adapter path.`, | |
| ); | |
| } | |
| return routed; | |
| }); | |
| const harnessOption = yield* Effect.serviceOption(HarnessClientAdapter); | |
| const withMcpHarness = <A>( | |
| operation: string, | |
| threadId: ThreadId, | |
| fn: (harness: HarnessClientAdapterShape) => Effect.Effect<A, ProviderAdapterError>, | |
| ) => | |
| Effect.gen(function* () { | |
| yield* requireMcpHarnessSession(threadId, operation); | |
| const harness = Option.getOrUndefined(harnessOption); | |
| if (!harness) { | |
| return yield* Effect.fail( | |
| toValidationError(operation, "MCP management requires the Elixir harness."), | |
| ); | |
| } | |
| return yield* fn(harness); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ProviderService.ts` around lines 1089 - 1121,
The current requireMcpCapable only checks mcpConfig and can still return
inactive or direct-routed sessions; update requireMcpCapable (used by
withMcpHarness) to additionally reject sessions where routed.isActive === false
and where routed.getAdapterPath?.() === "direct" by returning a validation error
(use toValidationError with the same operation message style), so MCP calls are
blocked for stopped threads or direct routes before delegating to
HarnessClientAdapter.
Comment on lines
+4215
to
+4224
| {mcpPanelOpen && activeThreadId ? ( | ||
| <div className="flex h-full w-[340px] shrink-0 flex-col border-l border-border/70"> | ||
| <ThreadMcpStatusPanel | ||
| key={`mcp:${activeThreadId}`} | ||
| threadId={activeThreadId} | ||
| mcp={mcpViewModel} | ||
| onClose={() => setMcpPanelOpen(false)} | ||
| className="max-h-[min(70vh,36rem)]" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Don't render the fixed sidebar shell when the MCP panel has no content.
ThreadMcpStatusPanel already returns null when there are no fetched servers and no MCP activity. With the 340px wrapper outside the component, opening the toggle on an MCP-capable-but-empty thread leaves a blank gutter instead of no panel or an explicit empty state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/ChatView.tsx` around lines 4215 - 4224, The fixed
340px wrapper is rendered even when ThreadMcpStatusPanel returns null, producing
an empty gutter; fix by only rendering the wrapper when the panel has
content—either add a predicate check (e.g., if mcpViewModel.hasFetchedServers ||
mcpViewModel.hasActivity or a new mcpViewModel.hasContent() accessor) alongside
the existing mcpPanelOpen && activeThreadId condition before rendering the div,
or move the wrapper (the div with className "flex h-full w-[340px] ...") into
ThreadMcpStatusPanel so the component controls whether the shell is rendered
when it would otherwise return null.
The real Codex event method is codex/event/mcp_startup_update (not
mcpServer/startupStatus/updated) with payload { server, status: { state } }
instead of { name, status }. Handle both forms for forward compatibility.
Also silence codex/event/mcp_startup_complete in QUIET_UNMAPPED_EVENTS.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…allback Bug 2: ClaudeAgent was showing the MCP button when mcpConfig: 'none' because the old logic 'mcpCapable || mcpViewModel.hasAnyMcpActivity' allowed MCP activities (from claude_session.ex system/init mcp_servers extraction) to override the provider's declared capability. Now showMcpToggle = mcpCapable, so providers with mcpConfig: 'none' never show the button regardless of activity. Bug 3: OpenCode MCP button was not shown on some threads because selectedProvider could resolve to a non-MCP provider (e.g. cursor or claudeAgent) when the session provider was stale or the composer draft had a different provider selected. Now mcpCapable also checks threadProvider (from modelSelection) as a fallback, so if the thread was created with OpenCode, the button shows even if the resolved selectedProvider differs. Co-Authored-By: Bastian Venegas Arevalo <r2d2@ranvier-technologies.com>
Set BUN_RUNTIME_TRANSPILER_CACHE=0 in both dev-harness.sh and dev-runner.ts so server-side TS changes are always picked up on restart without manual cache busting. Fixes stale event mapping code being served after edits (root cause of Bug 1 — empty MCP panel for Codex). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Replace fragile string pattern matching in list_models_from_session
with a {:provider_mismatch, provider} tuple. The old code matched on
the exact error message string from with_opencode_session, which would
silently break if the message wording ever changed.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codex wraps MCP startup data in a `msg` field: `{ msg: { server, status } }`.
The mapping code was reading `payload.server` directly, missing the data
entirely and causing all mcp_startup_update events to fall through as
unmapped. Now unwraps `payload.msg` when present.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The threadProvider fallback picked up the project's default modelSelection.provider (codex), making mcpCapable=true for all new threads even when the user selected Claude or Cursor (mcpConfig: none). The selectedProvider already resolves correctly for started threads via lockedProvider, so the fallback is unnecessary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. Change Claude mcpConfig from "none" to "basic" — unblocks the MCP
button for Claude threads. Claude CLI already discovers MCP servers
and the harness emits mcpServer/startupStatus/updated events.
2. Add normalizeMcpState() in codexEventMapping.ts — maps provider-
specific status strings ("connected"→"ready", "disabled"→"cancelled",
"needs_auth"→"failed") to canonical enum before reaching consumers.
3. Widen ParsedMcpStatus to accept "unknown" — parseMcpStatus now falls
back to { state: "unknown" } instead of returning null for
unrecognized states, preventing silent data loss.
4. Panel UX: add loading/empty states — the panel no longer disappears
when empty. Shows "Loading MCP status..." during fetch and
"No MCP servers detected" when no servers are found.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks
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.
Summary
mcpServer/*events throughcodexEventMapping→ProviderRuntimeIngestioninto orchestration activities (mcp.status.updated,mcp.oauth.completed)mcp-session-logic.tsview model derivation,@t3tools/shared/mcputilities,ThreadMcpStatusPanelcomponentChatView.tsxwith sidebar toggle and mutual exclusion against PlanSidebarserverfield toMcpStatusUpdatedPayloadin contractsStatus
Panel is passive-only — renders MCP server statuses from streamed events. Next: wire WS MCP API (fetch-on-open, connect/disconnect actions) and extract Claude CLI
mcp_serversfromsystem/init.Test plan
.t3/mcp.json, start Codex session, verify events flow to panelbun typecheck/bun lint/bun fmtall pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests