fix(mcp-v2): proper relay path for workspaces, OAuth client_name in tracking#3918
Conversation
…Auth client_name
Two follow-up fixes from prod smoke testing.
1. workspaces_create/delete were calling cloud v2Workspace.* directly,
which only writes the DB row. The host-service is supposed to actually
materialize/remove the git worktree on disk. Switched to the same path
the CLI uses — POST to ${RELAY_URL}/hosts/{routingKey}/trpc — via a new
minimal hostServiceMutation transport helper. Now matches CLI behavior
end-to-end and surfaces relay 503 honestly when the host is not
connected (instead of silently returning success).
2. OAuth client_label in PostHog showed raw azp (random DCR client_id
like "yGixmCSVdmnmMulxajcjhDtrsxntdmtq") instead of human-readable
names. Fix is purely additive: oauthProvider's customAccessTokenClaims
passes through client_name from RFC 7591 client metadata; mcp-v2 reads
it on resolve, falls back to azp if absent. Zero per-request DB cost.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughResolveMcpContext now takes and propagates both apiUrl and relayUrl; a new host-service client posts SuperJSON TRPC-style requests to a relay with timeouts and errors; OAuth token claims may include client_name; in-memory MCP client and workspace create/delete tools were updated to use relay-enabled context and host service. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant API as API Route
participant Resolver as MCP Resolver
participant OAuth as OAuth Validator
participant Tool as Workspace Tool
participant HostSvc as Host Service (relay)
Client->>API: Request (Bearer JWT)
API->>Resolver: resolveMcpContext({ apiUrl, relayUrl })
Resolver->>OAuth: verify JWT, extract claims (organizationId, client_name)
OAuth-->>Resolver: claims
Resolver-->>API: McpContext (includes relayUrl)
API->>Tool: invoke workspace operation (uses McpContext)
Tool->>HostSvc: hostServiceMutation(relayUrl, procedure, input, jwt)
HostSvc->>HostSvc: serialize with SuperJSON and POST to relay
HostSvc-->>Tool: return deserialized result (result.data)
Tool-->>API: operation result
API-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 5/8 reviews remaining, refill in 21 minutes and 49 seconds.Comment |
Greptile SummaryThis PR fixes two production issues from v2 MCP smoke testing: workspace create/delete tools are rerouted from the cloud tRPC path (which only wrote a DB row) to the host-service relay path so the git worktree is actually materialized on disk, and OAuth
Confidence Score: 4/5Safe to merge after fixing the missing result.data guard in host-service-client.ts; the core routing fix and OAuth labeling are correct. One P1 bug: the new hostServiceMutation helper omits the result.data existence check that relay-client.ts includes, meaning unexpected relay responses produce a cryptic TypeError. Everything else — routing key construction, SuperJSON serialize/deserialize pattern, customAccessTokenClaims, relayUrl threading — is correct. packages/mcp-v2/src/host-service-client.ts — missing guard on result.data before SuperJSON.deserialize
|
| Filename | Overview |
|---|---|
| packages/mcp-v2/src/host-service-client.ts | New relay transport helper — mirrors relay-client.ts but is missing the result.data guard, causing an unhandled TypeError on unexpected relay responses. |
| packages/mcp-v2/src/tools/workspaces/create.ts | Switched from cloud tRPC to host-service relay path; removes the type field (not supported by the host-service procedure); input/output types match host-service workspace.create signature. |
| packages/mcp-v2/src/tools/workspaces/delete.ts | Adds DB lookup to resolve hostId before calling host-service relay; intentionally removes idempotency (not-found now throws instead of succeeds). |
| packages/mcp-v2/src/auth.ts | Adds relayUrl to McpContext and ResolveMcpContextOptions; removes the prefix-based mapOAuthClientId in favor of JWT client_name claim; clean refactor. |
| packages/auth/src/server.ts | Passes client_name from RFC 7591 DCR metadata into the OAuth access token claims; defensive type-checking is correct. |
| apps/api/src/app/api/v2/agent/[transport]/route.ts | Passes RELAY_URL into resolveMcpContext via the new options object; minimal change, correct. |
| packages/mcp-v2/src/in-memory.ts | Adds relayUrl to InMemoryClientOptions and threads it into the McpContext; straightforward. |
Sequence Diagram
sequenceDiagram
participant LLM as MCP Client (LLM)
participant API as apps/api (MCP v2)
participant Relay as Relay
participant Host as Host Service
Note over API: resolveMcpContext now receives relayUrl
LLM->>API: workspaces_create / workspaces_delete
API->>API: hostServiceMutation (host-service-client.ts)
API->>Relay: POST /hosts/{orgId:machineId}/trpc/workspace.create
Relay-->>Host: Proxies request
Host->>Host: git worktree add / remove (disk)
Host->>Host: Write cloud DB row
Host-->>Relay: result data
Relay-->>API: 200 OK (or 503 if host offline)
API->>API: SuperJSON.deserialize(result.data)
API-->>LLM: Workspace response
Note over LLM,API: OAuth path — client_name in PostHog
LLM->>API: Any MCP tool (OAuth token)
API->>API: resolveOAuth reads client_name claim
API->>API: PostHog mcp_tool_called with client_label
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/mcp-v2/src/host-service-client.ts:77-80
**Missing `result.data` guard before `SuperJSON.deserialize`**
If the relay returns a 2xx response with a body that lacks `result.data` (e.g., a tRPC error envelope or malformed payload), `parsed.result?.data` evaluates to `undefined`. Calling `SuperJSON.deserialize(undefined)` throws a cryptic `TypeError: Cannot destructure property 'json' of 'undefined'` instead of a descriptive `HostServiceCallError`.
The `relay-client.ts` file this mirrors includes an explicit guard for exactly this case (lines 86–92):
```ts
if (!parsed.result || parsed.result.data === undefined) {
throw new RelayDispatchError(
`missing result.data in relay response: ${rawBody.slice(0, 200)}`,
response.status,
rawBody,
);
}
```
```suggestion
const data = parsed.result?.data;
if (!parsed.result || data === undefined) {
throw new HostServiceCallError(
`missing result.data in relay response: ${rawBody.slice(0, 200)}`,
response.status,
rawBody,
);
}
return SuperJSON.deserialize(
data as Parameters<typeof SuperJSON.deserialize>[0],
) as TOutput;
```
### Issue 2 of 2
packages/mcp-v2/src/tools/workspaces/delete.ts:21-23
**Idempotency removed without a fallback**
The previous implementation was documented as "Idempotent — succeeds if the workspace is already gone." The new code throws a generic `Error` when the DB row is not found, which means a caller retrying a delete (e.g., after a transient failure) gets a hard error on the second attempt. The tool description was updated to remove the idempotency claim, but callers or agents that relied on that guarantee will now fail.
If intentional, consider returning a success/no-op response when the workspace is already absent (matching the old behavior and the host-service's own idempotent teardown semantics).
Reviews (1): Last reviewed commit: "fix(mcp-v2): proxy workspace ops to host..." | Re-trigger Greptile
| const data = parsed.result?.data; | ||
| return SuperJSON.deserialize( | ||
| data as Parameters<typeof SuperJSON.deserialize>[0], | ||
| ) as TOutput; |
There was a problem hiding this comment.
Missing
result.data guard before SuperJSON.deserialize
If the relay returns a 2xx response with a body that lacks result.data (e.g., a tRPC error envelope or malformed payload), parsed.result?.data evaluates to undefined. Calling SuperJSON.deserialize(undefined) throws a cryptic TypeError: Cannot destructure property 'json' of 'undefined' instead of a descriptive HostServiceCallError.
The relay-client.ts file this mirrors includes an explicit guard for exactly this case (lines 86–92):
if (!parsed.result || parsed.result.data === undefined) {
throw new RelayDispatchError(
`missing result.data in relay response: ${rawBody.slice(0, 200)}`,
response.status,
rawBody,
);
}| const data = parsed.result?.data; | |
| return SuperJSON.deserialize( | |
| data as Parameters<typeof SuperJSON.deserialize>[0], | |
| ) as TOutput; | |
| const data = parsed.result?.data; | |
| if (!parsed.result || data === undefined) { | |
| throw new HostServiceCallError( | |
| `missing result.data in relay response: ${rawBody.slice(0, 200)}`, | |
| response.status, | |
| rawBody, | |
| ); | |
| } | |
| return SuperJSON.deserialize( | |
| data as Parameters<typeof SuperJSON.deserialize>[0], | |
| ) as TOutput; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/mcp-v2/src/host-service-client.ts
Line: 77-80
Comment:
**Missing `result.data` guard before `SuperJSON.deserialize`**
If the relay returns a 2xx response with a body that lacks `result.data` (e.g., a tRPC error envelope or malformed payload), `parsed.result?.data` evaluates to `undefined`. Calling `SuperJSON.deserialize(undefined)` throws a cryptic `TypeError: Cannot destructure property 'json' of 'undefined'` instead of a descriptive `HostServiceCallError`.
The `relay-client.ts` file this mirrors includes an explicit guard for exactly this case (lines 86–92):
```ts
if (!parsed.result || parsed.result.data === undefined) {
throw new RelayDispatchError(
`missing result.data in relay response: ${rawBody.slice(0, 200)}`,
response.status,
rawBody,
);
}
```
```suggestion
const data = parsed.result?.data;
if (!parsed.result || data === undefined) {
throw new HostServiceCallError(
`missing result.data in relay response: ${rawBody.slice(0, 200)}`,
response.status,
rawBody,
);
}
return SuperJSON.deserialize(
data as Parameters<typeof SuperJSON.deserialize>[0],
) as TOutput;
```
How can I resolve this? If you propose a fix, please make it concise.| if (!workspace) { | ||
| throw new Error(`Workspace not found: ${input.id}`); | ||
| } |
There was a problem hiding this comment.
Idempotency removed without a fallback
The previous implementation was documented as "Idempotent — succeeds if the workspace is already gone." The new code throws a generic Error when the DB row is not found, which means a caller retrying a delete (e.g., after a transient failure) gets a hard error on the second attempt. The tool description was updated to remove the idempotency claim, but callers or agents that relied on that guarantee will now fail.
If intentional, consider returning a success/no-op response when the workspace is already absent (matching the old behavior and the host-service's own idempotent teardown semantics).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/mcp-v2/src/tools/workspaces/delete.ts
Line: 21-23
Comment:
**Idempotency removed without a fallback**
The previous implementation was documented as "Idempotent — succeeds if the workspace is already gone." The new code throws a generic `Error` when the DB row is not found, which means a caller retrying a delete (e.g., after a transient failure) gets a hard error on the second attempt. The tool description was updated to remove the idempotency claim, but callers or agents that relied on that guarantee will now fail.
If intentional, consider returning a success/no-op response when the workspace is already absent (matching the old behavior and the host-service's own idempotent teardown semantics).
How can I resolve this? If you propose a fix, please make it concise.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 `@packages/mcp-v2/src/auth.ts`:
- Around line 148-154: The current clientName logic doesn't treat empty or
whitespace-only payload.client_name as absent, causing clientLabel to be blank;
update the clientName determination in auth.ts (the variable clientName that
reads payload.client_name) to trim and validate non-empty strings (e.g., set
clientName to null if payload.client_name is not a string or
payload.client_name.trim() is empty) and keep the existing clientLabel selection
(clientLabel: clientName ?? azp) so azp is used as the fallback when client_name
is empty/whitespace.
In `@packages/mcp-v2/src/host-service-client.ts`:
- Around line 41-54: The fetch block must normalize transport/abort errors and
verify the service success envelope before calling deserialize: wrap the await
fetch(...) in try/catch so that network errors and AbortError are caught and
rethrown as HostServiceCallError (include original error), and after fetch
completes check response.ok and content-type includes "application/json", parse
the JSON and validate the expected success envelope shape (e.g., presence of
expected fields) before calling deserialize; if validation fails throw
HostServiceCallError so deserialize is never invoked on undefined; apply the
same changes to the analogous fetch block around lines 65-80.
🪄 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: 09380f1a-131f-48a1-add2-63b3cff821ba
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/api/src/app/api/v2/agent/[transport]/route.tspackages/auth/src/server.tspackages/mcp-v2/package.jsonpackages/mcp-v2/src/auth.tspackages/mcp-v2/src/host-service-client.tspackages/mcp-v2/src/in-memory.tspackages/mcp-v2/src/tools/workspaces/create.tspackages/mcp-v2/src/tools/workspaces/delete.ts
| const clientName = | ||
| typeof payload.client_name === "string" ? payload.client_name : null; | ||
| const azp = typeof payload.azp === "string" ? payload.azp : null; | ||
| return { | ||
| userId: payload.sub, | ||
| organizationId: payload.organizationId, | ||
| clientLabel: azp ? mapOAuthClientId(azp) : null, | ||
| clientLabel: clientName ?? azp, |
There was a problem hiding this comment.
Handle empty client_name before fallback selection.
If client_name is "", clientLabel becomes blank instead of falling back to azp. Treat empty/whitespace-only names as absent.
Suggested fix
- const clientName =
- typeof payload.client_name === "string" ? payload.client_name : null;
- const azp = typeof payload.azp === "string" ? payload.azp : null;
+ const clientName =
+ typeof payload.client_name === "string" &&
+ payload.client_name.trim().length > 0
+ ? payload.client_name
+ : null;
+ const azp =
+ typeof payload.azp === "string" && payload.azp.trim().length > 0
+ ? payload.azp
+ : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mcp-v2/src/auth.ts` around lines 148 - 154, The current clientName
logic doesn't treat empty or whitespace-only payload.client_name as absent,
causing clientLabel to be blank; update the clientName determination in auth.ts
(the variable clientName that reads payload.client_name) to trim and validate
non-empty strings (e.g., set clientName to null if payload.client_name is not a
string or payload.client_name.trim() is empty) and keep the existing clientLabel
selection (clientLabel: clientName ?? azp) so azp is used as the fallback when
client_name is empty/whitespace.
| let response: Response; | ||
| try { | ||
| response = await fetch(url, { | ||
| method: "POST", | ||
| headers: { | ||
| "content-type": "application/json", | ||
| authorization: `Bearer ${options.jwt}`, | ||
| }, | ||
| body: JSON.stringify(encoded), | ||
| signal: controller.signal, | ||
| }); | ||
| } finally { | ||
| clearTimeout(timer); | ||
| } |
There was a problem hiding this comment.
Normalize transport failures and validate success envelope before deserialize.
Right now, aborted/network failures bypass HostServiceCallError, and malformed “successful” responses can deserialize undefined without throwing. This can hide relay/host failures from tool callers.
Suggested hardening
type TrpcEnvelope = { result?: { data?: unknown } };
let parsed: TrpcEnvelope;
try {
parsed = JSON.parse(rawBody) as TrpcEnvelope;
} catch {
throw new HostServiceCallError(
`invalid JSON from relay: ${rawBody.slice(0, 200)}`,
response.status,
rawBody,
);
}
-const data = parsed.result?.data;
-return SuperJSON.deserialize(
- data as Parameters<typeof SuperJSON.deserialize>[0],
-) as TOutput;
+if (!parsed.result || !("data" in parsed.result)) {
+ throw new HostServiceCallError(
+ `invalid relay response envelope: ${rawBody.slice(0, 200)}`,
+ response.status,
+ rawBody,
+ );
+}
+
+return SuperJSON.deserialize(
+ parsed.result.data as Parameters<typeof SuperJSON.deserialize>[0],
+) as TOutput; let response: Response;
try {
response = await fetch(url, {
method: "POST",
@@
signal: controller.signal,
});
+} catch (error) {
+ const reason =
+ controller.signal.aborted
+ ? `relay timeout after ${options.timeoutMs ?? 25_000}ms`
+ : `relay request failed: ${error instanceof Error ? error.message : String(error)}`;
+ throw new HostServiceCallError(reason, 0, String(error));
} finally {
clearTimeout(timer);
}Also applies to: 65-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mcp-v2/src/host-service-client.ts` around lines 41 - 54, The fetch
block must normalize transport/abort errors and verify the service success
envelope before calling deserialize: wrap the await fetch(...) in try/catch so
that network errors and AbortError are caught and rethrown as
HostServiceCallError (include original error), and after fetch completes check
response.ok and content-type includes "application/json", parse the JSON and
validate the expected success envelope shape (e.g., presence of expected fields)
before calling deserialize; if validation fails throw HostServiceCallError so
deserialize is never invoked on undefined; apply the same changes to the
analogous fetch block around lines 65-80.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…escriptions
Tool descriptions no longer pre-document relay 503 — that's a runtime
condition, not a calling concern, and bloats every tool/list response.
Relay failures now surface as terse single-sentence errors:
- "Host {id} is not online"
- "You are not authenticated"
- "You don't have access to host {id}"
- "Host {id} not found"
Two bot-flagged issues from PR #3918 review: - host-service-client: a 2xx relay response with no result.data was silently deserializing undefined. Now throws HostServiceCallError so the failure surfaces. - workspaces_delete: returning alreadyGone:true when the cloud row is missing instead of throwing, matching the documented idempotency contract from the v1 tool.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/mcp-v2/src/auth.ts (1)
148-151:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore OAuth
azpfallback and normalize blankclient_name.Line 148–151 currently drops fallback behavior and can emit blank labels for whitespace-only names. That diverges from the stated
client_name ?? azpintent and degrades PostHog attribution.Suggested fix
- const clientLabel = - typeof payload.client_name === "string" && payload.client_name - ? payload.client_name - : null; + const clientName = + typeof payload.client_name === "string" && + payload.client_name.trim().length > 0 + ? payload.client_name + : null; + const azp = + typeof payload.azp === "string" && payload.azp.trim().length > 0 + ? payload.azp + : null; + const clientLabel = clientName ?? azp;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mcp-v2/src/auth.ts` around lines 148 - 151, The clientLabel assignment currently allows whitespace-only client_name and drops the azp fallback; change it to treat blank/whitespace-only payload.client_name as empty by trimming, and if that yields empty use payload.azp as the fallback (or null if neither present). Update the clientLabel logic (the variable computed from payload.client_name) to check typeof payload.client_name === "string" && payload.client_name.trim() !== "" ? use client_name : (typeof payload.azp === "string" && payload.azp ? payload.azp : null).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/mcp-v2/src/auth.ts`:
- Around line 148-151: The clientLabel assignment currently allows
whitespace-only client_name and drops the azp fallback; change it to treat
blank/whitespace-only payload.client_name as empty by trimming, and if that
yields empty use payload.azp as the fallback (or null if neither present).
Update the clientLabel logic (the variable computed from payload.client_name) to
check typeof payload.client_name === "string" && payload.client_name.trim() !==
"" ? use client_name : (typeof payload.azp === "string" && payload.azp ?
payload.azp : null).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 336b43de-39ce-4ab3-a878-5a9ada734c02
📒 Files selected for processing (1)
packages/mcp-v2/src/auth.ts
Summary
Two follow-up fixes from prod smoke testing of v2 MCP.
1.
workspaces_create/workspaces_deletenow actually do work on the hostThe MCP tools were wired to
caller.v2Workspace.create/.delete— the cloud tRPC procedures, which only write the DB row. The host-service was never told to materialize (or remove) the git worktree on disk. The desktop app then "noticed" the row via Electric SQL and acted on it asynchronously, with no signal back to the caller.That meant when the relay was down (which it was during today's smoke test),
workspaces_createreturned 200 OK and a row id while the actual worktree silently never existed.Switched to the same path the CLI uses:
POST ${RELAY_URL}/hosts/{routingKey}/trpc/{procedure}via a minimalhostServiceMutationtransport helper. The host-service now does the filesystem work and writes the cloud row before the call returns. Same code path that powersautomations_run— surfaces relay 503 honestly when the host isn't connected.Why a new helper instead of importing host-service's tRPC
AppRoutertypes directly: pulling the host-service source into the cloud TypeScript compile breaks because of host-only env strictness (better-sqlite3, node-pty, etc). Inline input/output types per call, mirroring the existingrelay-client.tspattern.2. PostHog
client_labelnow shows human-readable names for OAuth callersmcp_tool_calledevents for OAuth-authenticated requests were tagged with raw DCRclient_id(e.g.yGixmCSVdmnmMulxajcjhDtrsxntdmtq) instead of friendly labels. Fix:oauthProvider.customAccessTokenClaimspasses throughclient_namefrom RFC 7591 client metadata. mcp-v2 readspayload.client_namefirst, falls back toazpif absent. Purely additive, zero per-request DB cost. Applies only to OAuth access tokens (not JWT-plugin tokens used by CLI/relay/electric-proxy).Test plan
workspaces_createagainst an online host produces an actual worktree on disk (not just a DB row)workspaces_createagainst a host with no relay connection returns 503 instead of silent successmcp_tool_calledevent in PostHog to confirmclient_labelreflects the OAuth client'sclient_name(or rawazpif the client didn't set one)Summary by CodeRabbit
New Features
Improvements
Chores