feat: MCP event pipeline + passive status panel - #40
Conversation
Wire Claude provider MCP config through mcpTranslation, add MCP config resolution in ClaudeAdapter, and harden the runtime ingestion pipeline for MCP status events across all providers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Claude-specific MCP support: name normalization, MCP config translation and file generation, Claude CLI flag propagation, runtime ingestion of configured MCP server states into normalized activities, UI refresh-state improvements, and related tests across server, harness, and web layers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Web as Web UI
participant Adapter as Server Adapter
participant McpSvc as MCP Config Service
participant FS as Filesystem
participant Claude as Claude CLI
participant Orch as Orchestration
User->>Web: Start Claude session (threadId)
Web->>Adapter: startSession(threadId)
Adapter->>McpSvc: getSnapshot(threadId)
McpSvc-->>Adapter: resolved MCP config
Adapter->>FS: mkdir generatedMcpDir(..., "claudeAgent", threadId)
Adapter->>FS: write .mcp.json (claudeConfigFromResolved)
FS-->>Adapter: file created
Adapter->>Claude: createQuery(..., mcpConfig: path)
Claude-->>Orch: emit session.configured (mcp_servers)
Orch->>Orch: normalize server names & map states
Orch-->>Web: emit mcp.status.updated activities
Web->>Web: deriveMcpSessionViewModel (normalize names)
Web-->>User: render ThreadMcpStatusPanel (refreshState/servers)
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 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 |
github-actions
Bot
added
size:L
vouch:trusted
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| export function claudeConfigFromResolved(config: ResolvedMcpConfig): string { | ||
| const payload = { | ||
| mcpServers: Object.fromEntries( | ||
| config.servers.map((server) => [ | ||
| server.name, | ||
| server.transport === "stdio" | ||
| ? { | ||
| command: server.command, | ||
| args: server.args ?? [], | ||
| ...(server.env ? { env: server.env } : {}), | ||
| } | ||
| : { | ||
| type: server.transport, | ||
| url: server.url, | ||
| }, | ||
| ]), | ||
| ), | ||
| }; | ||
| return `${JSON.stringify(payload, null, 2)}\n`; | ||
| } |
There was a problem hiding this comment.
🔴 claudeConfigFromResolved includes disabled MCP servers in Claude config
claudeConfigFromResolved maps all servers from the resolved config into the .mcp.json output, including servers with enabled: false. Unlike codexTomlFromResolved (which emits enabled = false) and openCodeConfigFromResolved (which emits enabled: false), the Claude format has no enabled field, so disabled servers silently become active when passed to the Claude CLI.
Example: disabled SSE server included without any disable indicator
The sseServer fixture has enabled: false, but claudeConfigFromResolved produces:
"event-stream": { "type": "sse", "url": "https://events.example.com/stream" }The Claude CLI will treat this as an active server and attempt to connect to it.
| export function claudeConfigFromResolved(config: ResolvedMcpConfig): string { | |
| const payload = { | |
| mcpServers: Object.fromEntries( | |
| config.servers.map((server) => [ | |
| server.name, | |
| server.transport === "stdio" | |
| ? { | |
| command: server.command, | |
| args: server.args ?? [], | |
| ...(server.env ? { env: server.env } : {}), | |
| } | |
| : { | |
| type: server.transport, | |
| url: server.url, | |
| }, | |
| ]), | |
| ), | |
| }; | |
| return `${JSON.stringify(payload, null, 2)}\n`; | |
| } | |
| export function claudeConfigFromResolved(config: ResolvedMcpConfig): string { | |
| const payload = { | |
| mcpServers: Object.fromEntries( | |
| config.servers | |
| .filter((server) => server.enabled) | |
| .map((server) => [ | |
| server.name, | |
| server.transport === "stdio" | |
| ? { | |
| command: server.command, | |
| args: server.args ?? [], | |
| ...(server.env ? { env: server.env } : {}), | |
| } | |
| : { | |
| type: server.transport, | |
| url: server.url, | |
| }, | |
| ]), | |
| ), | |
| }; | |
| return `${JSON.stringify(payload, null, 2)}\n`; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| {showEmptyState ? ( | ||
| <p className="py-4 text-center text-sm text-muted-foreground"> | ||
| No MCP servers detected for this session. | ||
| </p> |
There was a problem hiding this comment.
🟡 "No MCP servers detected" empty state shown while background refresh is still in progress
The refactored ThreadMcpStatusPanel renders the background refresh banner (showBackgroundRefresh) and the empty-state message (showEmptyState) as independent, non-exclusive conditions. When the fetch is still in-flight and no event-derived servers exist yet, both "Refreshing live MCP status" and "No MCP servers detected for this session" are shown simultaneously. The old code (apps/web/src/components/ThreadMcpStatusPanel.tsx:143-148 in the left diff) used a mutually exclusive ternary that showed "Loading MCP status..." exclusively during fetch.
| {showEmptyState ? ( | |
| <p className="py-4 text-center text-sm text-muted-foreground"> | |
| No MCP servers detected for this session. | |
| </p> | |
| {showEmptyState && !showBackgroundRefresh ? ( | |
| <p className="py-4 text-center text-sm text-muted-foreground"> | |
| No MCP servers detected for this session. | |
| </p> |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
apps/web/src/mcp-session-logic.ts (1)
218-218: Consider normalizing server name in runtime.warning handler.At line 218,
detail.serverfromparseMcpStartupWarningDetailis used directly without normalization. If the warning's server name differs from the normalized name (e.g.,"claude.ai OpenAI Documentation"vs"openaiDeveloperDocs"), the warning state will be stored under a different key than the status state, causing a mismatch.Consider normalizing for consistency:
♻️ Proposed fix
hasAnyMcpActivity = true; - const server = ensureServerState(byServer, detail.server); + const server = ensureServerState(byServer, normalizeMcpServerName(detail.server)); server.warningMessage = message;🤖 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` at line 218, The runtime.warning handler is passing detail.server from parseMcpStartupWarningDetail directly into ensureServerState, which can create a different key than the normalized server name used elsewhere; update the handler to normalize detail.server (using the same normalization routine used for status keys — e.g., the existing normalizeServerName/normalizeServerKey function or the same code path used when setting status) before calling ensureServerState so warnings and statuses share the same server key (apply normalization to detail.server immediately after parseMcpStartupWarningDetail and use that normalized value in ensureServerState and any subsequent lookups).
🤖 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/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts`:
- Around line 271-273: The new case in ProviderRuntimeIngestion that directly
calls sessionConfiguredMcpActivities for "session.configured" must be routed
through the centralized providerManager coordination point; replace the direct
call in the switch case (the "session.configured" branch in
ProviderRuntimeIngestion) with a call into the providerManager API (or a small
delegating function in providerManager) that accepts the same event and
maybeSequence and is responsible for invoking sessionConfiguredMcpActivities and
any thread-event logging. Update providerManager.ts to expose a handler (e.g.,
handleSessionConfigured or a generic dispatchThreadEvent) that performs the
sessionConfiguredMcpActivities call and logs the thread event, then call that
handler from ProviderRuntimeIngestion instead of calling
sessionConfiguredMcpActivities directly.
In `@apps/server/src/provider/Layers/ClaudeAdapter.ts`:
- Around line 173-175: Remove the custom ClaudeQueryOptionsWithMcp type and stop
using mcpConfig; instead construct and pass an mcpServers value (Record<string,
McpServerConfig>) directly into the query options you send to createQuery.
Update the code that builds the options for createQuery (the place currently
using ClaudeQueryOptionsWithMcp / mcpConfig and writing the .mcp.json) to
load/transform the existing MCP config into the correct mcpServers shape (keys =
server names, values = McpServerConfig) and include that mcpServers property on
the options object passed to createQuery; delete the ClaudeQueryOptionsWithMcp
type and any mcpConfig usage so the SDK receives the supported mcpServers field.
In `@apps/server/src/provider/mcpTranslation.test.ts`:
- Around line 198-210: The test shows a disabled MCP server ("event-stream" with
enabled: false) being serialized into claudeConfigFromResolved output; update
claudeConfigFromResolved to ignore servers where enabled is false (e.g., filter
the resolved servers map/array before building mcpServers) so only enabled
entries are included, and adjust the test expectation to assert that only the
enabled "remote-api" is present; reference the claudeConfigFromResolved function
and the makeConfig/httpServer/sseServer fixtures when locating the change.
- Around line 39-43: The Claude remote-MCP translation test currently strips out
the remote object's headers (and incorrectly expects timeout to be preserved) —
update the Claude translation assertion that currently expects just { type, url
} to include the headers field (e.g., { type, url, headers: { Authorization:
"...", "x-ref-api-key": "..." } }) and ensure timeout is not expected (timeout
is global-only), so the test asserts headers are preserved and timeout is
absent; change the expected value in the Claude translation assertion
accordingly.
In `@apps/server/src/provider/mcpTranslation.ts`:
- Around line 81-84: The current object spread emits server.timeout when typeof
server.timeout === "number", which can allow NaN/Infinity/zero/negative values;
update the guard to only emit timeout if Number.isFinite(server.timeout) &&
server.timeout > 0 (i.e., replace the typeof check with a finite positive-number
check for the server.timeout property used in the spread), so the constructed
config only contains valid positive finite timeout values.
In `@apps/web/src/components/ThreadMcpStatusPanel.tsx`:
- Around line 102-110: The fetchedServers state is not cleared when threadId
changes, causing stale server entries to persist; inside the useEffect that
depends on threadId (the one using ensureNativeApi().mcp.status(threadId)),
clear fetchedServers immediately when the effect runs (call setFetchedServers to
an empty array or initial value) before starting the async request and keep the
existing cancellation logic (cancelled flag and setting refreshState). This
ensures convertFetchedStatus(data) only populates results for the current
threadId and prevents showing stale entries on thread switches or refresh
failures.
---
Nitpick comments:
In `@apps/web/src/mcp-session-logic.ts`:
- Line 218: The runtime.warning handler is passing detail.server from
parseMcpStartupWarningDetail directly into ensureServerState, which can create a
different key than the normalized server name used elsewhere; update the handler
to normalize detail.server (using the same normalization routine used for status
keys — e.g., the existing normalizeServerName/normalizeServerKey function or the
same code path used when setting status) before calling ensureServerState so
warnings and statuses share the same server key (apply normalization to
detail.server immediately after parseMcpStartupWarningDetail and use that
normalized value in ensureServerState and any subsequent lookups).
🪄 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: 279db2f5-0035-48c9-ae71-3b0c74f03e88
📒 Files selected for processing (16)
AGENTS.mdapps/harness/lib/harness/providers/claude_session.exapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/HarnessClientAdapter.tsapps/server/src/provider/Layers/McpConfig.test.tsapps/server/src/provider/Layers/McpConfig.tsapps/server/src/provider/mcpTranslation.test.tsapps/server/src/provider/mcpTranslation.tsapps/web/src/components/ThreadMcpStatusPanel.tsxapps/web/src/mcp-session-logic.test.tsapps/web/src/mcp-session-logic.tspackages/contracts/src/provider.tspackages/shared/src/mcp.ts
| case "session.configured": { | ||
| return sessionConfiguredMcpActivities(event, maybeSequence); | ||
| } |
There was a problem hiding this comment.
Coordinate new session.configured thread-activity logging via providerManager.ts.
This adds another thread event logging path in ProviderRuntimeIngestion, which diverges from the required coordination point. Please broker this coordination through apps/server/src/providerManager.ts (or delegate from there) to keep event logging centralized.
As per coding guidelines: "Provider dispatch and thread event logging must be coordinated in apps/server/src/providerManager.ts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts` around
lines 271 - 273, The new case in ProviderRuntimeIngestion that directly calls
sessionConfiguredMcpActivities for "session.configured" must be routed through
the centralized providerManager coordination point; replace the direct call in
the switch case (the "session.configured" branch in ProviderRuntimeIngestion)
with a call into the providerManager API (or a small delegating function in
providerManager) that accepts the same event and maybeSequence and is
responsible for invoking sessionConfiguredMcpActivities and any thread-event
logging. Update providerManager.ts to expose a handler (e.g.,
handleSessionConfigured or a generic dispatchThreadEvent) that performs the
sessionConfiguredMcpActivities call and logs the thread event, then call that
handler from ProviderRuntimeIngestion instead of calling
sessionConfiguredMcpActivities directly.
| type ClaudeQueryOptionsWithMcp = ClaudeQueryOptions & { | ||
| readonly mcpConfig?: string; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the exact version of @anthropic-ai/claude-agent-sdkused by this repository, doesquery({ options })support anmcpConfig option? If yes, what type does it expect and how is it applied?
💡 Result:
No, the query({ options }) method in @anthropic-ai/claude-agent-sdk does not support an mcpConfig option. It supports mcpServers within the options object for configuring MCP servers. The mcpServers option expects a Record<string, McpServerConfig>, where keys are server names (e.g., "github") and values are configurations like: typescript mcpServers: { "github": { type: "stdio", command: "npx", args: ["-y", "@modelcontextprotocol/server-github"] } } This configuration is passed to the underlying Claude Code CLI subprocess spawned by query, which uses it to connect MCP servers, enabling tools from those servers (with allowedTools for permissions). mcpConfig appears in CLI contexts (e.g., --mcp-config flag or .mcp.json files auto-loaded by the SDK), but not as a direct query option. No evidence of mcpConfig in SDK docs, types, or code examples across versions up to 0.2.87 (latest as of 2026-03-30). The query cannot identify the "exact version used by this repository" without the specific repo, but the API is consistent across documented versions.
Citations:
- 1: https://docs.claude.com/en/docs/agent-sdk/mcp
- 2: https://docs.claude.com/en/docs/agent-sdk/typescript
- 3: https://platform.claude.com/docs/en/api/agent-sdk/typescript
- 4: https://docs.anthropic.com/en/docs/agent-sdk/typescript
- 5: https://github.com/anthropics/claude-agent-sdk-typescript/blob/main/CHANGELOG.md
Remove mcpConfig from query options; use mcpServers instead.
Lines 173-175 extend ClaudeQueryOptions with an unsupported field. The pinned @anthropic-ai/claude-agent-sdk does NOT accept mcpConfig as a query option—it only supports mcpServers (a Record<string, McpServerConfig> of server names to configurations). The generated .mcp.json file and type extension at lines 173-175 will have no effect on the SDK's behavior, and tests mocking createQuery will not catch this since they don't execute the real SDK. Replace the mcpConfig approach with direct mcpServers configuration in the query options passed at lines 2828-2845.
Also applies to: 2828-2845
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 173 - 175,
Remove the custom ClaudeQueryOptionsWithMcp type and stop using mcpConfig;
instead construct and pass an mcpServers value (Record<string, McpServerConfig>)
directly into the query options you send to createQuery. Update the code that
builds the options for createQuery (the place currently using
ClaudeQueryOptionsWithMcp / mcpConfig and writing the .mcp.json) to
load/transform the existing MCP config into the correct mcpServers shape (keys =
server names, values = McpServerConfig) and include that mcpServers property on
the options object passed to createQuery; delete the ClaudeQueryOptionsWithMcp
type and any mcpConfig usage so the SDK receives the supported mcpServers field.
| headers: { | ||
| Authorization: "Bearer secret-token", | ||
| "x-ref-api-key": "ref-123", | ||
| }, | ||
| timeout: 2500, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the current Claude Code / @anthropic-ai/claude-agent-sdkMCP config schema, what fields are supported for remote HTTP/SSE entries in.mcp.json? Specifically, are headersandtimeout allowed?
💡 Result:
In the current Claude Code / @anthropic-ai/claude-agent-sdk MCP config schema for .mcp.json, remote HTTP/SSE entries support the following fields: - type: "http" or "sse" (required) - url: string (required) - headers?: Record<string, string> (optional) The 'timeout' field is not supported directly in the per-server configuration for remote HTTP/SSE entries. Timeouts are handled via environment variables like MCP_TIMEOUT, which applies globally or to stdio servers, but not explicitly to remote connections in the schema.
Citations:
- 1: https://docs.anthropic.com/en/docs/claude-code/sdk/sdk-mcp
- 2: https://docs.anthropic.com/en/docs/claude-code/sdk/sdk-typescript
- 3: https://platform.claude.com/docs/en/agent-sdk/mcp
- 4: https://code.claude.com/docs/en/sdk/sdk-typescript
- 5: https://tessl.io/registry/tessl/pypi-claude-agent-sdk/0.1.0/docs/mcp-server-configuration.md
Preserve headers in Claude's remote MCP translation.
Lines 39-43 add headers and timeout to the remote fixture. Claude's MCP schema supports headers as an optional field for remote servers, but not timeout (which is only configurable globally via environment variables). The Claude translation at lines 203-206 currently drops both fields to { type, url }, but should preserve headers to maintain authentication for remote servers across both OpenCode and Claude implementations.
Code reference
headers: {
Authorization: "Bearer secret-token",
"x-ref-api-key": "ref-123",
},
timeout: 2500,
Lines 203-206 should include headers in the assertion.
Also applies to: 198-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/mcpTranslation.test.ts` around lines 39 - 43, The
Claude remote-MCP translation test currently strips out the remote object's
headers (and incorrectly expects timeout to be preserved) — update the Claude
translation assertion that currently expects just { type, url } to include the
headers field (e.g., { type, url, headers: { Authorization: "...",
"x-ref-api-key": "..." } }) and ensure timeout is not expected (timeout is
global-only), so the test asserts headers are preserved and timeout is absent;
change the expected value in the Claude translation assertion accordingly.
| it.effect("claudeConfigFromResolved maps remote server with explicit transport type", () => | ||
| Effect.sync(() => { | ||
| const result = claudeConfigFromResolved(makeConfig([httpServer, sseServer])); | ||
| const parsed = JSON.parse(result) as { mcpServers: Record<string, Record<string, unknown>> }; | ||
|
|
||
| assert.deepEqual(parsed.mcpServers["remote-api"], { | ||
| type: "http", | ||
| url: "https://api.example.com/mcp", | ||
| }); | ||
| assert.deepEqual(parsed.mcpServers["event-stream"], { | ||
| type: "sse", | ||
| url: "https://events.example.com/stream", | ||
| }); |
There was a problem hiding this comment.
Don’t serialize disabled MCP servers into Claude config.
This expectation still requires event-stream even though the fixture marks it enabled: false. The translation should filter disabled entries before building mcpServers; otherwise a server the user turned off gets handed back to Claude as active configuration.
Expected assertion once disabled entries are filtered out
assert.deepEqual(parsed.mcpServers["remote-api"], {
type: "http",
url: "https://api.example.com/mcp",
});
- assert.deepEqual(parsed.mcpServers["event-stream"], {
- type: "sse",
- url: "https://events.example.com/stream",
- });
+ assert.isUndefined(parsed.mcpServers["event-stream"]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/mcpTranslation.test.ts` around lines 198 - 210, The
test shows a disabled MCP server ("event-stream" with enabled: false) being
serialized into claudeConfigFromResolved output; update claudeConfigFromResolved
to ignore servers where enabled is false (e.g., filter the resolved servers
map/array before building mcpServers) so only enabled entries are included, and
adjust the test expectation to assert that only the enabled "remote-api" is
present; reference the claudeConfigFromResolved function and the
makeConfig/httpServer/sseServer fixtures when locating the change.
| ...("headers" in server && server.headers ? { headers: server.headers } : {}), | ||
| ...("timeout" in server && typeof server.timeout === "number" | ||
| ? { timeout: server.timeout } | ||
| : {}), |
There was a problem hiding this comment.
Tighten timeout emission guard to finite positive numbers only.
Current check allows any number value. Emitting NaN/Infinity/non-positive values can produce invalid downstream config payloads.
Suggested fix
- ...("timeout" in server && typeof server.timeout === "number"
- ? { timeout: server.timeout }
- : {}),
+ ...("timeout" in server &&
+ typeof server.timeout === "number" &&
+ Number.isFinite(server.timeout) &&
+ server.timeout > 0
+ ? { timeout: server.timeout }
+ : {}),📝 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.
| ...("headers" in server && server.headers ? { headers: server.headers } : {}), | |
| ...("timeout" in server && typeof server.timeout === "number" | |
| ? { timeout: server.timeout } | |
| : {}), | |
| ...("headers" in server && server.headers ? { headers: server.headers } : {}), | |
| ...("timeout" in server && | |
| typeof server.timeout === "number" && | |
| Number.isFinite(server.timeout) && | |
| server.timeout > 0 | |
| ? { timeout: server.timeout } | |
| : {}), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/mcpTranslation.ts` around lines 81 - 84, The current
object spread emits server.timeout when typeof server.timeout === "number",
which can allow NaN/Infinity/zero/negative values; update the guard to only emit
timeout if Number.isFinite(server.timeout) && server.timeout > 0 (i.e., replace
the typeof check with a finite positive-number check for the server.timeout
property used in the spread), so the constructed config only contains valid
positive finite timeout values.
| useEffect(() => { | ||
| let cancelled = false; | ||
| setIsFetching(true); | ||
| setRefreshState("refreshing"); | ||
| ensureNativeApi() | ||
| .mcp.status(threadId) | ||
| .then((data) => { | ||
| if (!cancelled) setFetchedServers(convertFetchedStatus(data)); | ||
| if (cancelled) return; | ||
| setFetchedServers(convertFetchedStatus(data)); | ||
| setRefreshState("ready"); |
There was a problem hiding this comment.
Clear fetched MCP server cache when threadId changes.
fetchedServers persists across thread switches, so stale entries can briefly appear (or remain on refresh failure) for the wrong thread.
Suggested fix
useEffect(() => {
let cancelled = false;
+ setFetchedServers([]);
setRefreshState("refreshing");
ensureNativeApi()
.mcp.status(threadId)📝 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.
| useEffect(() => { | |
| let cancelled = false; | |
| setIsFetching(true); | |
| setRefreshState("refreshing"); | |
| ensureNativeApi() | |
| .mcp.status(threadId) | |
| .then((data) => { | |
| if (!cancelled) setFetchedServers(convertFetchedStatus(data)); | |
| if (cancelled) return; | |
| setFetchedServers(convertFetchedStatus(data)); | |
| setRefreshState("ready"); | |
| useEffect(() => { | |
| let cancelled = false; | |
| setFetchedServers([]); | |
| setRefreshState("refreshing"); | |
| ensureNativeApi() | |
| .mcp.status(threadId) | |
| .then((data) => { | |
| if (cancelled) return; | |
| setFetchedServers(convertFetchedStatus(data)); | |
| setRefreshState("ready"); |
🤖 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 102 - 110, The
fetchedServers state is not cleared when threadId changes, causing stale server
entries to persist; inside the useEffect that depends on threadId (the one using
ensureNativeApi().mcp.status(threadId)), clear fetchedServers immediately when
the effect runs (call setFetchedServers to an empty array or initial value)
before starting the async request and keep the existing cancellation logic
(cancelled flag and setting refreshState). This ensures
convertFetchedStatus(data) only populates results for the current threadId and
prevents showing stale entries on thread switches or refresh failures.
… ID attribution Research-driven improvements to OpenCode harness resilience: 1. SSE reconnect MCP rehydration: After SSE reconnect (and initial setup), fetch GET /mcp and re-emit mcpServer/startupStatus/updated events so the MCP panel recovers from forward-only SSE gaps. Previously the panel went stale after any SSE connection drop. 2. MCP config version in resumeCursor: Include the config fingerprint in persist_binding so on resume the harness can detect config drift and decide if MCP servers need reconfiguration. 3. Session ID attribution: Tag all emitted events with opencode_session_id via Map.put_new in emit_event. Prepares for shared-runtime (multiple sessions per opencode serve process) where events need explicit scoping. Co-Authored-By: Bastian Venegas Arevalo <r2d2@ranvier-technologies.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/opencode_session.ex`:
- Around line 1645-1664: The persisted mcpConfigVersion is being written via
maybe_put_mcp_config_version into cursor_json and upserted with
Harness.Storage.upsert_binding, but the resume/reuse logic still only reads
sessionId; update the resume path that fetches the binding to also read and
compare the stored "mcpConfigVersion" against the current
state.mcp_config["version"] (or treat missing/unequal as a mismatch) and only
reuse the stored sessionId when the versions match; otherwise fall back to
creating a fresh OpenCode session so the MCP config change triggers
reconfiguration.
- Around line 228-231: Instead of calling rehydrate_mcp_status(state) directly
inside your setup/reconnect code paths (which races with older SSE messages),
change those direct calls to send/queue a :rehydrate_mcp_status message to the
process and implement handle_info(:rehydrate_mcp_status, state) to call
rehydrate_mcp_status(state) and return {:noreply, state}; update each place
mentioned (the setup/reconnect locations where rehydrate_mcp_status/1 is
currently invoked, including the spots around the current calls and any
reconnect handlers) so wait_for_ready/2 no longer depends on the immediate HTTP
call and the MCP snapshot is processed only after the mailbox’s live backlog is
drained.
🪄 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: dd87d811-36bb-46f0-84b0-f51951364c38
📒 Files selected for processing (1)
apps/harness/lib/harness/providers/opencode_session.ex
| # Rehydrate MCP server status from REST API so the panel | ||
| # populates immediately — SSE is forward-only and won't replay | ||
| # startup events that fired before we connected. | ||
| rehydrate_mcp_status(state) |
There was a problem hiding this comment.
Queue MCP rehydration after the live backlog, not inside setup/reconnect.
Calling rehydrate_mcp_status/1 inline from Line 231 and Line 329 emits the /mcp snapshot before older SSE updates already sitting in the mailbox are drained. The MCP view is last-write-wins per server, so those older queued events can overwrite the fresher rehydrated state and leave the panel stale after connect/reconnect. It also makes wait_for_ready/2 depend on an extra best-effort HTTP call.
💡 Suggested change
- rehydrate_mcp_status(state)
+ send(self(), :rehydrate_mcp_status)- rehydrate_mcp_status(state)
+ send(self(), :rehydrate_mcp_status)`@impl` true
def handle_info(:rehydrate_mcp_status, state) do
rehydrate_mcp_status(state)
{:noreply, state}
endAlso applies to: 327-329, 1666-1711
🤖 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 228 -
231, Instead of calling rehydrate_mcp_status(state) directly inside your
setup/reconnect code paths (which races with older SSE messages), change those
direct calls to send/queue a :rehydrate_mcp_status message to the process and
implement handle_info(:rehydrate_mcp_status, state) to call
rehydrate_mcp_status(state) and return {:noreply, state}; update each place
mentioned (the setup/reconnect locations where rehydrate_mcp_status/1 is
currently invoked, including the spots around the current calls and any
reconnect handlers) so wait_for_ready/2 no longer depends on the immediate HTTP
call and the MCP snapshot is processed only after the mailbox’s live backlog is
drained.
| # Only persist durable identifiers — port is ephemeral and stale after restart. | ||
| # Include mcpConfigVersion so on resume we can detect config drift. | ||
| cursor_json = | ||
| Jason.encode!(%{ | ||
| "threadId" => state.thread_id, | ||
| "sessionId" => state.opencode_session_id | ||
| }) | ||
| Jason.encode!( | ||
| %{ | ||
| "threadId" => state.thread_id, | ||
| "sessionId" => state.opencode_session_id | ||
| } | ||
| |> maybe_put_mcp_config_version(state) | ||
| ) | ||
|
|
||
| Harness.Storage.upsert_binding(state.thread_id, state.provider, cursor_json) | ||
| end | ||
|
|
||
| defp maybe_put_mcp_config_version(cursor, state) do | ||
| case state.mcp_config do | ||
| %{"version" => v} when is_binary(v) -> Map.put(cursor, "mcpConfigVersion", v) | ||
| _ -> cursor | ||
| end | ||
| end |
There was a problem hiding this comment.
Persisting mcpConfigVersion here does not change resume behavior yet.
These lines write the fingerprint, but the resume path still extracts only sessionId and reuses the session whenever it exists. A changed MCP config will therefore reopen the stale OpenCode session and skip reconfiguration. Please treat the persisted version as part of the reuse key, and fall back to a fresh session when it is missing or different.
🤖 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 1645 -
1664, The persisted mcpConfigVersion is being written via
maybe_put_mcp_config_version into cursor_json and upserted with
Harness.Storage.upsert_binding, but the resume/reuse logic still only reads
sessionId; update the resume path that fetches the binding to also read and
compare the stored "mcpConfigVersion" against the current
state.mcp_config["version"] (or treat missing/unequal as a mismatch) and only
reuse the stored sessionId when the versions match; otherwise fall back to
creating a fresh OpenCode session so the MCP config change triggers
reconfiguration.
1 participant
Summary
ThreadMcpStatusPanelUI showing real-time MCP server status per thread (Ready/Failed/Starting), gated on provider capabilitymcpTranslation, extractmcp_serversfromsystem/initpayload.msgenvelope formcp_startup_updateeventsnormalizeMcpState()maps provider-specific strings (connected→ready, disabled→cancelled) to canonical enumTest plan
system/init(Ready/Failed)bun typecheck/bun lint/bun fmtpass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests