Skip to content

chore(mcp): remove temporary HTTP diagnostics#5795

Merged
waleedlatif1 merged 1 commit into
stagingfrom
chore/remove-mcp-http-diagnostics
Jul 21, 2026
Merged

chore(mcp): remove temporary HTTP diagnostics#5795
waleedlatif1 merged 1 commit into
stagingfrom
chore/remove-mcp-http-diagnostics

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Removes the temporary McpHttpDiag instrumentation added in diag(mcp): comprehensive HTTP logging for OAuth + streamable-HTTP transport #5782. It did its job: it confirmed the OAuth fetch fix (fix(mcp): bound and tear down one-shot OAuth fetches so auth can't hang #5789) makes the flow reach and complete the token exchange (200, tokens saved, "Server Authorized"), and it isolated a separate, pre-existing transport-initialize stall — Gauge returns 200 application/json + Mcp-Session-Id then sends no body for 30s, reproducible only from the staging egress IP (a fresh IP + the real SDK + h2 + pinning + even the diagnostic's body tee() all connect in ~600ms).
  • Reverts client.ts and oauth/auth.ts to their exact pre-diagnostic form and deletes http-diagnostics.ts. Also removes the tee() from the transport hot path so the streamable-HTTP connection runs clean while the transport stall is investigated on its own.

What changed

Type of Change

  • Chore / cleanup

Testing

  • Full lib/mcp suite green (371 tests); tsc clean; biome clean.
  • Verified client.ts/client.test.ts are byte-identical to the pre-diagnostic originals; auth.ts differs only in biome line-wrapping.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

Reverts the temporary McpHttpDiag instrumentation from #5782 now that it has
served its purpose: it confirmed the OAuth fetch fix (#5789) makes the flow reach
and complete the token exchange, and it isolated a separate, pre-existing
transport-initialize stall (Gauge returns 200 + session then no body for 30s,
reproducible only from the staging egress — not from a fresh IP, and not with the
SDK/h2/pinning/tee in isolation).

- Delete apps/sim/lib/mcp/http-diagnostics.ts.
- Restore client.ts transport fetch to `...(pinned ? { fetch: pinned.fetch } : {})`.
- Restore oauth/auth.ts fetchFn to the plain SSRF-guarded default.
- Drop the diagnostic-specific test assertion.

Removes the body tee() from the transport hot path so the streamable-HTTP
connection runs clean while the transport stall is investigated separately.
@vercel

vercel Bot commented Jul 21, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 21, 2026 3:42am

Request Review

@cursor

cursor Bot commented Jul 21, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Cleanup that reverts temporary logging/instrumentation on MCP HTTP paths; no new auth or SSRF behavior beyond restoring the prior fetch wiring.

Overview
Removes the temporary McpHttpDiag layer added for OAuth/streamable-HTTP debugging by deleting http-diagnostics.ts and all withMcpHttpDiagnostics usage.

McpClient again passes a custom fetch only when IP pinning is active (...(pinned ? { fetch: pinned.fetch } : {})), so unpinned transports use the SDK default and no longer run the diagnostic tee() on the initialize response path. mcpAuthGuarded defaults OAuth fetchFn back to createSsrfGuardedMcpFetch() only, without the OAuth-phase wrapper.

The OAuth transport test expectation drops the diagnostic-only fetch: expect.any(Function) assertion.

Reviewed by Cursor Bugbot for commit be2038d. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR removes temporary MCP HTTP diagnostics and restores the earlier fetch paths.

  • Deletes the diagnostic fetch wrapper and logging module.
  • Uses the pinned fetch only for pinned MCP transports.
  • Keeps the SSRF-guarded default for OAuth requests.
  • Removes the diagnostic-specific transport test assertion.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The deleted module has no remaining references.
  • Existing transport pinning and OAuth SSRF protection remain in place.

Important Files Changed

Filename Overview
apps/sim/lib/mcp/client.ts Removes diagnostic wrapping while retaining the pinned fetch for resolved MCP server addresses.
apps/sim/lib/mcp/oauth/auth.ts Removes diagnostic wrapping while preserving the caller override and SSRF-guarded default.
apps/sim/lib/mcp/http-diagnostics.ts Deletes the temporary diagnostic module after all consumers were removed.
apps/sim/lib/mcp/client.test.ts Removes the assertion that diagnostics always provide an explicit transport fetch.

Reviews (1): Last reviewed commit: "chore(mcp): remove temporary HTTP diagno..." | Re-trigger Greptile

@waleedlatif1
waleedlatif1 merged commit 1b2ee26 into staging Jul 21, 2026
15 checks passed
@waleedlatif1
waleedlatif1 deleted the chore/remove-mcp-http-diagnostics branch July 21, 2026 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant