fix(cli): pagespace mcp never dies or hangs before the MCP handshake + ChatGPT/Codex docs - #2327
fix(cli): pagespace mcp never dies or hangs before the MCP handshake + ChatGPT/Codex docs#23272witstudios wants to merge 3 commits into
Conversation
An MCP client that spawns 'pagespace mcp' (ChatGPT desktop, Codex, Claude Desktop, stdio bridges) can only observe a pre-transport exit or hang as a server that never came up — rendered as hours-long timeouts. Three startup blockers caused exactly that, all confirmed by live stdio probes against the published CLI: - no explicit credential: exited 1 before connecting the transport - stored oauth credential + unreachable host: OAuth discovery/refresh ran before initialize with no fetch timeout — hung forever, and enforceAuth then purged the stored credential on the transient failure - keychain read at startup could block on a GUI access prompt The mcp route now serves unconditionally and fails per-call instead (matching the old pagespace-mcp package's tolerance every migrated config was built against), while keeping the Phase 8 task 4 invariant strictly stronger than before: - routes gain lazyAuth; run.ts skips resolveCredentialSource entirely for mcp with nothing explicit named (zero store reads, zero network — the ambient login credential cannot even be read) and wires a FailingAuthProvider whose message surfaces on every tools/call - with an explicit credential, resolution (keychain read bounded at 5s) and discovery/refresh (both fetches now abort at 10s) defer to the first tool call via createLazyAuthProvider; failed resolutions retry on the next call and never purge the stored credential - commands/mcp.ts serves degraded through a stub sdk when no credential is named — ctx.sdk is structurally unreachable without one - tool-convert passes the AuthenticationError remediation text through instead of the fixed 'Run "pagespace login"' hint (the one remediation mcp deliberately refuses) - RunDependencies.createMcpTransport seam so run-level tests drive the mcp route over InMemoryTransport, never the test process's stdio Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZ4Dp6SazFhnSpDHmk15cd
…ing npx -p flag, startup timeout ChatGPT desktop and Codex share ~/.codex/config.toml (TOML), a shape no PageSpace doc showed — hand-translating the JSON mcpServers examples is exactly where the -p flag gets dropped, and npx then dies with 'could not determine executable to run', which Codex renders as Tools: (none) and ChatGPT as a connector timeout (reproduced live). Documents the verified working block, why -p is required (two bins, neither named after the package), startup_timeout_sec for cold npx installs, the GUI-PATH caveat for the global-install form, and that Codex's 'Auth: Unsupported' label is normal for stdio servers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZ4Dp6SazFhnSpDHmk15cd
📝 WalkthroughWalkthroughChangesThe CLI now starts MCP without explicit credentials, defers authentication until tool use, and returns actionable per-call errors. Discovery and refresh requests have abortable timeouts. Tests use in-memory MCP transports. Documentation covers ChatGPT, Codex, and Secure MCP Tunnel setup. MCP authentication and serving
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MCPTransport
participant MCPRoute
participant AuthProvider
participant PageSpaceSDK
MCPClient->>MCPTransport: Connect
MCPTransport->>MCPRoute: Start mcp route
MCPRoute-->>MCPClient: Tool registry and serving status
MCPClient->>MCPRoute: Invoke tool
MCPRoute->>AuthProvider: Resolve credentials
AuthProvider->>PageSpaceSDK: Request access token when configured
PageSpaceSDK-->>AuthProvider: Token or authentication error
AuthProvider-->>MCPRoute: Credential result
MCPRoute-->>MCPClient: Tool result or actionable authentication error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0d3d0ffb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (timedOut) { | ||
| throw new DiscoveryError(`Timed out reaching ${url} after ${timeoutMs}ms`); |
There was a problem hiding this comment.
When an explicitly named stored OAuth credential encounters a transient discovery timeout, this throws a plain DiscoveryError. OAuthTokenProvider passes it to classifyRefreshFailure, which recognizes only SDK network/timeout/rate-limit/server errors as retryable (packages/sdk/src/auth/decide.ts:51-60), so it marks the provider permanently unauthenticated (packages/sdk/src/auth/oauth.ts:138-147). Consequently, even after the host recovers, every subsequent MCP tool call immediately fails until the server is restarted, contrary to the intended next-call retry behavior; emit the SDK TimeoutError here (and a retryable network error for the adjacent fetch-failure path).
Useful? React with 👍 / 👎.
…stdio server The reported ChatGPT downtime traced to OpenAI's Secure MCP Tunnel: tunnel-client spawns the stdio MCP server via --mcp-command, so the two failure modes documented for Codex apply identically and both render as ChatGPT-side timeouts — a missing -p in the npx invocation, and credential env vars absent from the environment tunnel-client run executes in (the child inherits that env, not ChatGPT's). Documents the working --mcp-command, the env rule, and the doctor --explain check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZ4Dp6SazFhnSpDHmk15cd
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/cli/src/auth/discover.ts (1)
55-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe abort timeout bounds only the fetch, not the response body read, in both authentication requests. Both files clear the abort timer in the
finallyof the fetchtryblock. Header retrieval is bounded; the body read that follows is not. A host that sends headers and then stalls the body still hangs the firstpagespace mcptool call — the failure mode this change targets.
packages/cli/src/auth/discover.ts#L55-L70: moveclearTimeout(timer)into an outerfinallythat also coversawait response.json()on Line 76, and map a timeout abort raised during the body read to theTimed out reaching ${url} after ${timeoutMs}msmessage.packages/cli/src/auth/silent-refresh.ts#L69-L92: moveclearTimeout(timer)into an outerfinallythat also coversawait response.text()on Line 94, and map a timeout abort raised during the body read to the sameTimeoutErrorwithoperation: 'auth.refresh'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/auth/discover.ts` around lines 55 - 70, Extend the timeout coverage through the response body reads in both sites: in packages/cli/src/auth/discover.ts lines 55-70, wrap response.json() in the same timed operation, move clearTimeout(timer) to an outer finally, and map timeout aborts to the existing DiscoveryError message; in packages/cli/src/auth/silent-refresh.ts lines 69-92, do the equivalent for response.text(), preserving the TimeoutError with operation: 'auth.refresh'.packages/cli/src/__tests__/run.test.ts (1)
620-641: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the MCP handshake timer-independent under fake timers.
vi.useFakeTimers()starts beforerunMcpcallsclient.connect(clientTransport), and MCP SDK requests use a timer-backed request timeout. Enable fake timers only afterrunMcpreturns soconnect/initializecannot wait on fake timers that tests may not advance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/__tests__/run.test.ts` around lines 620 - 641, Update the test around runMcp so fake timers are enabled only after await runMcp(deps) completes, allowing client.connect/clientTransport initialization to use real timers; then enable fake timers before advancing the 5,000 ms bounded credential-store timeout, preserving the existing assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/marketing/src/app/docs/integrations/mcp/page.tsx`:
- Around line 124-133: Remove the self-hosted PAGESPACE_API_URL guidance from
the OpenAI Secure MCP Tunnel section in
apps/marketing/src/app/docs/integrations/mcp/page.tsx (lines 124-133) and the
corresponding migration guidance in
packages/cli/docs/migrating-from-pagespace-mcp.md (lines 146-154). Keep the
hosted PAGESPACE_TOKEN environment-variable instructions and remaining tunnel
setup guidance unchanged.
In `@packages/cli/src/run.ts`:
- Around line 320-339: Update createLazyAuthProvider so the credential-store
resolution uses one shared in-flight promise, reusing it when a prior
resolveCredentialSource call is still pending instead of starting another read
after resolveWithTimeout rejects. Preserve retry behavior after the underlying
read settles, while ensuring concurrent or subsequent tool calls join the
outstanding resolution.
---
Nitpick comments:
In `@packages/cli/src/__tests__/run.test.ts`:
- Around line 620-641: Update the test around runMcp so fake timers are enabled
only after await runMcp(deps) completes, allowing client.connect/clientTransport
initialization to use real timers; then enable fake timers before advancing the
5,000 ms bounded credential-store timeout, preserving the existing assertions.
In `@packages/cli/src/auth/discover.ts`:
- Around line 55-70: Extend the timeout coverage through the response body reads
in both sites: in packages/cli/src/auth/discover.ts lines 55-70, wrap
response.json() in the same timed operation, move clearTimeout(timer) to an
outer finally, and map timeout aborts to the existing DiscoveryError message; in
packages/cli/src/auth/silent-refresh.ts lines 69-92, do the equivalent for
response.text(), preserving the TimeoutError with operation: 'auth.refresh'.
🪄 Autofix
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 Plus
Run ID: 5382a754-b217-497f-b6de-22d50391190c
📒 Files selected for processing (19)
apps/marketing/src/app/docs/integrations/mcp/page.tsxpackages/cli/docs/migrating-from-pagespace-mcp.mdpackages/cli/src/__tests__/pagespace-mcp-bin.test.tspackages/cli/src/__tests__/run.test.tspackages/cli/src/auth/__tests__/discover.test.tspackages/cli/src/auth/__tests__/lazy-provider.test.tspackages/cli/src/auth/__tests__/silent-refresh.test.tspackages/cli/src/auth/discover.tspackages/cli/src/auth/lazy-provider.tspackages/cli/src/auth/silent-refresh.tspackages/cli/src/commands/__tests__/mcp.test.tspackages/cli/src/commands/mcp.tspackages/cli/src/mcp/__tests__/serve.test.tspackages/cli/src/mcp/__tests__/tool-convert.test.tspackages/cli/src/mcp/tool-convert.tspackages/cli/src/pagespace-mcp-bin.tspackages/cli/src/router/router.tspackages/cli/src/router/routes.tspackages/cli/src/run.ts
| ### OpenAI Secure MCP Tunnel | ||
|
|
||
| Using [OpenAI's Secure MCP Tunnel](https://developers.openai.com/api/docs/guides/secure-mcp-tunnels) to reach a private server from ChatGPT? \`tunnel-client\` spawns the stdio server itself: | ||
|
|
||
| \`\`\`bash | ||
| tunnel-client init --profile pagespace --tunnel-id <your-tunnel-id> \\ | ||
| --mcp-command "npx -y -p @pagespace/cli pagespace-mcp" | ||
| \`\`\` | ||
|
|
||
| The same two rules apply — \`-p\` is required in the \`--mcp-command\`, and the credential env vars (\`PAGESPACE_TOKEN\`, plus \`PAGESPACE_API_URL\` for self-hosted) must be exported **in the environment where \`tunnel-client run\` executes**, since the spawned server inherits that process's env. Both mistakes surface in ChatGPT as tool calls that time out; \`tunnel-client doctor --profile pagespace --explain\` shows whether the server actually came up. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove unsupported self-hosted deployment guidance.
PageSpace is hosted only. Do not instruct users to configure PAGESPACE_API_URL for self-hosted instances.
apps/marketing/src/app/docs/integrations/mcp/page.tsx#L124-L133: remove the self-hostedPAGESPACE_API_URLguidance.packages/cli/docs/migrating-from-pagespace-mcp.md#L146-L154: remove the self-hostedPAGESPACE_API_URLguidance.
Based on learnings, “PageSpace is exclusively offered as a hosted service (pagespace.com) and is no longer self-hosted.”
📍 Affects 2 files
apps/marketing/src/app/docs/integrations/mcp/page.tsx#L124-L133(this comment)packages/cli/docs/migrating-from-pagespace-mcp.md#L146-L154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/marketing/src/app/docs/integrations/mcp/page.tsx` around lines 124 -
133, Remove the self-hosted PAGESPACE_API_URL guidance from the OpenAI Secure
MCP Tunnel section in apps/marketing/src/app/docs/integrations/mcp/page.tsx
(lines 124-133) and the corresponding migration guidance in
packages/cli/docs/migrating-from-pagespace-mcp.md (lines 146-154). Keep the
hosted PAGESPACE_TOKEN environment-variable instructions and remaining tunnel
setup guidance unchanged.
Source: Learnings
| async function resolveWithTimeout<T>(promise: Promise<T>, timeoutMs: number): Promise<T> { | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| try { | ||
| return await Promise.race([ | ||
| promise, | ||
| new Promise<never>((_, reject) => { | ||
| timer = setTimeout(() => { | ||
| reject( | ||
| new AuthenticationError( | ||
| `Reading the stored credential timed out after ${timeoutMs}ms — the OS keychain may be locked or ` + | ||
| 'waiting on an access prompt this process cannot show. Pass --token (or set PAGESPACE_TOKEN) in ' + | ||
| 'the MCP config to avoid the credential store entirely.', | ||
| ), | ||
| ); | ||
| }, timeoutMs); | ||
| }), | ||
| ]); | ||
| } finally { | ||
| clearTimeout(timer); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A timed-out credential-store read is abandoned, and each retry starts another one.
Promise.race rejects, but the underlying resolveCredentialSource promise stays pending forever when the keychain read never returns. createLazyAuthProvider clears its memo on rejection, so every later tool call calls credentialStore.get again. On a machine with a blocked keychain prompt, outstanding native reads accumulate for the life of the MCP server — one per tool call.
Keep the retry behavior, but do not start a second read while one is still outstanding.
♻️ Proposed guard for the outstanding read
+let outstandingCredentialRead: Promise<unknown> | null = null;
+
async function resolveWithTimeout<T>(promise: Promise<T>, timeoutMs: number): Promise<T> {An alternative is to reuse one shared resolution promise inside the createLazyAuthProvider callback at Lines 208-221, so a still-blocked read is joined instead of duplicated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/run.ts` around lines 320 - 339, Update
createLazyAuthProvider so the credential-store resolution uses one shared
in-flight promise, reusing it when a prior resolveCredentialSource call is still
pending instead of starting another read after resolveWithTimeout rejects.
Preserve retry behavior after the underlying read settles, while ensuring
concurrent or subsequent tool calls join the outstanding resolution.
1 participant
Problem
A user reported hours of ChatGPT MCP timeouts with the new CLI while the old `pagespace-mcp` npm package kept working. Live stdio probes against the published CLI confirmed three startup failure modes, all of which an MCP client (ChatGPT desktop, Codex, Claude Desktop, stdio bridges) can only render as a server that never came up:
Separately, the field failure that actually broke a live config: `npx -y @pagespace/cli pagespace-mcp` without `-p` dies with `could not determine executable to run` (two bins, neither named after the package) — Codex shows Tools: (none). No doc showed the ChatGPT/Codex TOML config shape.
Fix
The Phase 8 task 4 invariant (never ride the ambient login credential) is preserved structurally — the no-credential path can no longer even read the store, and the zero-side-effect assertions in run.test.ts got stronger.
Verification
Sibling change (separate repo, not in this PR): `pagespace-mcp` 5.2.8 corrects its deprecation notice, which pointed users at `pagespace login` (refused by mcp) and the renamed `pagespace tokens create`.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QZ4Dp6SazFhnSpDHmk15cd
Summary by CodeRabbit
npxconfiguration, environment variables, timeouts, PATH settings, and credential inheritance.