Repository navigation
Trim MCP E2E coverage - #160
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughConsolidates and simplifies MCP server end-to-end tests: removes many small, brittle tests, updates unauthenticated GET /mcp expectation to 401 with a Bearer-style WWW-Authenticate header, reshapes authenticated assertions, and adds a single generated-UI end-to-end flow using bearer tokens and execute/ui_save_app/search/open_generated_ui. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant MCPServer
participant AppSessionService
participant ExecuteEndpoint
TestRunner->>MCPServer: GET /mcp (unauthenticated)
MCPServer-->>TestRunner: 401 + WWW-Authenticate: Bearer resource_metadata="<origin>/.well-known/oauth-protected-resource"
TestRunner->>MCPServer: Authenticated request -> list tools
MCPServer-->>TestRunner: tools include execute, open_generated_ui, search
TestRunner->>MCPServer: POST open_generated_ui
MCPServer->>AppSessionService: create session
AppSessionService-->>MCPServer: { appSession.token, endpoints.execute }
MCPServer-->>TestRunner: return appSession.token + execute endpoint
TestRunner->>ExecuteEndpoint: POST codemode.value_set (Authorization: Bearer <token>)
ExecuteEndpoint-->>TestRunner: { ok: true }
TestRunner->>ExecuteEndpoint: POST codemode.value_get (Authorization: Bearer <token>)
ExecuteEndpoint-->>TestRunner: { name: "example", value: "value" }
TestRunner->>MCPServer: POST codemode.ui_save_app (persist app)
MCPServer-->>TestRunner: { app_id }
TestRunner->>MCPServer: POST search (find saved app)
MCPServer-->>TestRunner: { results including saved app_id }
TestRunner->>MCPServer: POST open_generated_ui (reopen saved app)
MCPServer-->>TestRunner: { hostedUrl: "<origin>/ui/<savedAppId>" }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
April 13, 2026 05:39
Contributor
|
🔎 Preview deployed: https://kody-pr-160.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts (1)
19-22: Tighten thisWWW-Authenticateassertion.This only proves the header contains
resource_metadata=. A malformed challenge like a missingBearerscheme or the wrong metadata URL would still pass, and this is now the only unauthenticated MCP probe left in the suite. I’d pin at least the scheme and theserver.origin-derived URL here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts` around lines 19 - 22, The current assertion only checks for 'resource_metadata='; tighten it by asserting the WWW-Authenticate header begins with the Bearer scheme and that the resource_metadata value includes the expected server origin: read the header via response.headers.get('WWW-Authenticate'), assert it startsWith('Bearer ') (or matches /^Bearer\s+/), and assert it contains the server.origin-derived URL (e.g. contains server.origin or an encoded form of it) so both the scheme and the metadata URL are validated (use the same server.origin symbol used in the test to build the expected substring).
🤖 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/worker/src/mcp/mcp-server.mcp-e2e.test.ts`:
- Around line 126-159: The test currently performs codemode.value_set and
value_get in the same POST, so it only proves intra-execution visibility; change
it to two separate app-session requests: send one fetch to executeEndpoint
(using executeToken, same headers) whose body runs only codemode.value_set and
assert that executeResponse.ok and executePayload.ok are true, then send a
second fetch to executeEndpoint that runs only codemode.value_get and assert
that its executeResponse.ok/executePayload.ok are true and that
executePayload.result?.result matches { name: 'example', value: 'value' };
reference the existing variables executeEndpoint, executeToken,
codemode.value_set, codemode.value_get, executeResponse and executePayload when
making the split and asserting both responses.
---
Nitpick comments:
In `@packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts`:
- Around line 19-22: The current assertion only checks for 'resource_metadata=';
tighten it by asserting the WWW-Authenticate header begins with the Bearer
scheme and that the resource_metadata value includes the expected server origin:
read the header via response.headers.get('WWW-Authenticate'), assert it
startsWith('Bearer ') (or matches /^Bearer\s+/), and assert it contains the
server.origin-derived URL (e.g. contains server.origin or an encoded form of it)
so both the scheme and the metadata URL are validated (use the same
server.origin symbol used in the test to build the expected substring).
🪄 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: ce301754-a116-4f7d-8bcc-a549f00daf47
📒 Files selected for processing (1)
packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/worker/src/mcp/mcp-server.mcp-e2e.test.ts`:
- Around line 235-253: The test currently only verifies URL fields for the
reopened saved app after calling mcpClient.client.callTool with name
'open_generated_ui' (savedOpenResult / savedOpenStructured) but does not assert
that a usable app session is returned; update the test to assert the reopened
appSession fields (e.g., presence of appSession.id, token, or whatever the
session shape is returned in savedOpenStructured or savedOpenResult) and then
perform a simple smoke call against the saved app's execute endpoint (e.g., POST
to /ui/{savedAppId}/execute or use the client's execute helper) to confirm the
reopened UI can actually execute actions. Ensure you reference and validate the
exact session properties returned by 'open_generated_ui' (appSession or similar)
and check the /execute response for a successful status or expected result.
🪄 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: ac8a9ae0-ea83-4b6a-bb1c-b63514146d0e
📒 Files selected for processing (1)
packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts
Comment on lines
+235
to
253
| const savedOpenResult = await mcpClient.client.callTool({ | ||
| name: 'open_generated_ui', | ||
| arguments: { | ||
| app_id: savedAppId, | ||
| }, | ||
| }) | ||
| const savedOpenStructured = (savedAppOpenResult as CallToolResult) | ||
| const savedOpenStructured = (savedOpenResult as CallToolResult) | ||
| .structuredContent as | ||
| | { | ||
| renderSource?: string | ||
| appId?: string | null | ||
| hostedUrl?: string | null | ||
| renderSource?: string | ||
| } | ||
| | undefined | ||
| expect(savedOpenStructured?.renderSource).toBe('saved_app') | ||
| expect(savedOpenStructured?.appId).toBe(savedAppId) | ||
| expect(savedOpenStructured?.hostedUrl).toContain(`/ui/${savedAppId}`) | ||
| }) | ||
|
|
||
| test('mcp endpoint requires OAuth bearer auth', async () => { | ||
| await using database = await createTestDatabase() | ||
| await using server = await startDevServer(database.persistDir) | ||
| const response = await fetch(new URL('/mcp', server.origin), { | ||
| headers: { | ||
| Accept: 'application/json, text/event-stream', | ||
| }, | ||
| }) | ||
| expect(response.status).toBe(401) | ||
| expect(response.headers.get('WWW-Authenticate') ?? '').toContain( | ||
| 'resource_metadata=', | ||
| ) | ||
| }) | ||
|
|
||
| test('generated UI execute supports session storage context', async () => { | ||
| await using database = await createTestDatabase() | ||
| await using server = await startDevServer(database.persistDir) | ||
| await using mcpClient = await createMcpClient(server.origin, database.user) | ||
|
|
||
| const openResult = await mcpClient.client.callTool({ | ||
| name: 'open_generated_ui', | ||
| arguments: { | ||
| code: '<main><h1>Storage Context</h1></main>', | ||
| }, | ||
| }) | ||
| const openStructured = (openResult as CallToolResult).structuredContent as | ||
| | { | ||
| appSession?: { | ||
| token?: string | ||
| endpoints?: { execute?: string } | ||
| } | null | ||
| } | ||
| | undefined | ||
| const executeEndpoint = openStructured?.appSession?.endpoints?.execute | ||
| const executeToken = openStructured?.appSession?.token | ||
| expect(typeof executeEndpoint).toBe('string') | ||
| expect(typeof executeToken).toBe('string') | ||
|
|
||
| const response = await fetch(executeEndpoint!, { | ||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Bearer ${executeToken}`, | ||
| 'Content-Type': 'application/json', | ||
| Accept: 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| code: `async () => { | ||
| await codemode.value_set({ | ||
| name: 'example', | ||
| value: 'value', | ||
| scope: 'session', | ||
| }) | ||
| const result = await codemode.value_get({ | ||
| name: 'example', | ||
| scope: 'session', | ||
| }) | ||
| return { result } | ||
| }`, | ||
| }), | ||
| }) | ||
| expect(response.ok).toBe(true) | ||
| const payload = (await response.json()) as { | ||
| ok?: boolean | ||
| result?: { result?: { name?: string; value?: string } } | ||
| } | ||
| expect(payload.ok).toBe(true) | ||
| expect(payload.result?.result?.name).toBe('example') | ||
| expect(payload.result?.result?.value).toBe('value') | ||
| }) | ||
|
|
||
| test('mcp server resolves host approval errors into structured guidance', async () => { | ||
| await using database = await createTestDatabase() | ||
| await using server = await startDevServer(database.persistDir) | ||
| await using mcpClient = await createMcpClient(server.origin, database.user) | ||
| const appCookieHeader = await loginToApp(server.origin, database.user) | ||
|
|
||
| const secretSaveResponse = await fetch( | ||
| new URL('/account/secrets.json', server.origin), | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| Cookie: appCookieHeader, | ||
| 'Content-Type': 'application/json', | ||
| Accept: 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| action: 'save', | ||
| name: 'hostApprovalToken', | ||
| value: 'secret', | ||
| scope: 'user', | ||
| description: 'Host approval', | ||
| allowedHosts: [], | ||
| allowedCapabilities: ['fetch'], | ||
| }), | ||
| }, | ||
| ) | ||
| expect(secretSaveResponse.ok, await secretSaveResponse.text()).toBe(true) | ||
|
|
||
| const result = await mcpClient.client.callTool({ | ||
| name: 'execute', | ||
| arguments: { | ||
| code: `async () => { | ||
| await fetch('https://example.com', { | ||
| headers: { | ||
| Authorization: 'Bearer {{secret:hostApprovalToken|scope=user}}', | ||
| }, | ||
| }) | ||
| return { ok: true } | ||
| }`, | ||
| }, | ||
| }) | ||
|
|
||
| const structuredResult = (result as CallToolResult).structuredContent as | ||
| | { errorDetails?: Record<string, unknown> } | ||
| | undefined | ||
| expect((result as CallToolResult).isError).toBe(true) | ||
| expect(structuredResult?.errorDetails?.kind).toBe( | ||
| 'host_approval_required_batch', | ||
| ) | ||
| }) | ||
|
|
||
| test('mcp server exposes direct refreshAccessToken helper', async () => { | ||
| await using database = await createTestDatabase() | ||
| await using server = await startDevServer(database.persistDir) | ||
| await using mcpClient = await createMcpClient(server.origin, database.user) | ||
| const appCookieHeader = await loginToApp(server.origin, database.user) | ||
|
|
||
| const secretSaveResponse = await fetch( | ||
| new URL('/account/secrets.json', server.origin), | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| Cookie: appCookieHeader, | ||
| 'Content-Type': 'application/json', | ||
| Accept: 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| action: 'save', | ||
| name: 'restrictedRefreshToken', | ||
| value: 'secret', | ||
| scope: 'user', | ||
| description: 'Restricted for helper', | ||
| allowedHosts: [], | ||
| allowedCapabilities: [], | ||
| }), | ||
| }, | ||
| ) | ||
| expect(secretSaveResponse.ok, await secretSaveResponse.text()).toBe(true) | ||
|
|
||
| const setupResult = await mcpClient.client.callTool({ | ||
| name: 'execute', | ||
| arguments: { | ||
| code: `async () => { | ||
| await codemode.value_set({ | ||
| name: 'spotify-client-id', | ||
| value: 'spotify-client-id-value', | ||
| scope: 'user', | ||
| description: 'Spotify OAuth client id', | ||
| }) | ||
| await codemode.connector_save({ | ||
| name: 'spotify', | ||
| tokenUrl: 'https://accounts.spotify.com/api/token', | ||
| apiBaseUrl: 'https://api.spotify.com/v1', | ||
| flow: 'pkce', | ||
| clientIdValueName: 'spotify-client-id', | ||
| clientSecretSecretName: null, | ||
| accessTokenSecretName: 'spotifyAccessToken', | ||
| refreshTokenSecretName: 'restrictedRefreshToken', | ||
| requiredHosts: ['accounts.spotify.com', 'api.spotify.com'], | ||
| }) | ||
| return { ok: true } | ||
| }`, | ||
| }, | ||
| }) | ||
| if ((setupResult as CallToolResult).isError) { | ||
| throw new Error( | ||
| `Helper setup execute failed: ${getTextContent((setupResult as CallToolResult).content)}`, | ||
| ) | ||
| } | ||
|
|
||
| const result = await mcpClient.client.callTool({ | ||
| name: 'execute', | ||
| arguments: { | ||
| code: `async () => { | ||
| return await refreshAccessToken('spotify') | ||
| }`, | ||
| }, | ||
| }) | ||
|
|
||
| const structuredResult = (result as CallToolResult).structuredContent as | ||
| | { errorDetails?: Record<string, unknown> } | ||
| | undefined | ||
| expect((result as CallToolResult).isError).toBe(true) | ||
| expect(structuredResult?.errorDetails?.kind).toBe( | ||
| 'host_approval_required_batch', | ||
| expect(savedOpenStructured?.hostedUrl).toBe( | ||
| `${server.origin}/ui/${savedAppId}`, | ||
| ) |
There was a problem hiding this comment.
Reopened saved apps still need their own session check.
This branch now only proves that a saved app can be found and reopened by URL. If open_generated_ui stopped returning a usable appSession for app_id opens, this test would still pass even though the reopened UI could no longer execute anything. Please assert the reopened session fields here and ideally smoke the saved app's /execute endpoint too.
💡 Suggested tightening
const savedOpenStructured = (savedOpenResult as CallToolResult)
.structuredContent as
| {
renderSource?: string
appId?: string | null
hostedUrl?: string | null
+ appSession?: {
+ token?: string
+ endpoints?: { execute?: string }
+ } | null
}
| undefined
expect(savedOpenStructured?.renderSource).toBe('saved_app')
expect(savedOpenStructured?.appId).toBe(savedAppId)
expect(savedOpenStructured?.hostedUrl).toBe(
`${server.origin}/ui/${savedAppId}`,
)
+
+const reopenedExecuteEndpoint = savedOpenStructured?.appSession?.endpoints?.execute
+const reopenedExecuteToken = savedOpenStructured?.appSession?.token
+expect(typeof reopenedExecuteEndpoint).toBe('string')
+expect(typeof reopenedExecuteToken).toBe('string')
+
+const reopenedExecuteResponse = await fetch(reopenedExecuteEndpoint!, {
+ method: 'POST',
+ headers: {
+ Authorization: `Bearer ${reopenedExecuteToken}`,
+ 'Content-Type': 'application/json',
+ Accept: 'application/json',
+ },
+ body: JSON.stringify({
+ code: `async () => ({ ok: true })`,
+ }),
+})
+expect(reopenedExecuteResponse.ok).toBe(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts` around lines 235 - 253,
The test currently only verifies URL fields for the reopened saved app after
calling mcpClient.client.callTool with name 'open_generated_ui' (savedOpenResult
/ savedOpenStructured) but does not assert that a usable app session is
returned; update the test to assert the reopened appSession fields (e.g.,
presence of appSession.id, token, or whatever the session shape is returned in
savedOpenStructured or savedOpenResult) and then perform a simple smoke call
against the saved app's execute endpoint (e.g., POST to /ui/{savedAppId}/execute
or use the client's execute helper) to confirm the reopened UI can actually
execute actions. Ensure you reference and validate the exact session properties
returned by 'open_generated_ui' (appSession or similar) and check the /execute
response for a successful status or expected result.
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
2 participants
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
Testing
npm run test:mcpnode tools/prepare-e2e-env.ts && npm run build:mcp-apps && npx vitest run --project mcp-e2eSummary by CodeRabbit