Merge pull request #731 from RhysSullivan/codex/fix-mcp-oauth-openapi-parsing

[codex] Fix MCP OAuth and OpenAPI JSON parsing

Rhys Sullivan committed 6 months ago

Commit d35cb4c2 parents f8fc3b4 4e608926 files · +75 −27Browse files

Unified · Split

6 files

packages/plugins/mcp/src/api/group.ts +1 −0
packages/plugins/mcp/src/react/AddMcpSource.tsx +21 −12
packages/plugins/mcp/src/sdk/plugin.ts +6 −3
packages/plugins/openapi/src/sdk/parse.test.ts +14 −0
packages/plugins/openapi/src/sdk/parse.ts +20 −8
packages/react/src/plugins/oauth-sign-in.tsx +13 −4

packages/plugins/mcp/src/api/group.ts+1−0

@@ -88,6 +88,7 @@ const ProbeEndpointPayload = Schema.Struct({ · 87 unmodified lines
88	const ProbeEndpointResponse = Schema.Struct({	88	const ProbeEndpointResponse = Schema.Struct({
89	connected: Schema.Boolean,	89	connected: Schema.Boolean,
90	requiresOAuth: Schema.Boolean,	90	requiresOAuth: Schema.Boolean,
		91	supportsDynamicRegistration: Schema.Boolean,
91	name: Schema.String,	92	name: Schema.String,
92	namespace: Schema.String,	93	namespace: Schema.String,
93	toolCount: Schema.NullOr(Schema.Number),	94	toolCount: Schema.NullOr(Schema.Number),

packages/plugins/mcp/src/react/AddMcpSource.tsx+21−12

 type ProbeResult = {
   connected: boolean;
   requiresOAuth: boolean;
+  supportsDynamicRegistration: boolean;
   name: string;
   namespace: string;
   toolCount: number | null;
   const isAdding = state.step === "adding";
   const isOAuthBusy =
     state.step === "oauth-starting" || state.step === "oauth-waiting" || oauth.busy;
-  const canUseNone = probe?.requiresOAuth !== true;
+  const canUseNone = probe?.requiresOAuth !== true || probe.supportsDynamicRegistration === false;
   const remoteHeadersComplete = remoteHeaders.every(
     (header) => header.name.trim() && header.value.trim(),
   );
                 <FieldLabel>Authentication</FieldLabel>
                 <FilterTabs<RemoteAuthMode>
                   tabs={
-                    probe.requiresOAuth
+                    probe.requiresOAuth && probe.supportsDynamicRegistration
                       ? [{ value: "oauth2", label: "OAuth" }]
                       : [
                           { value: "none", label: "None" },
                     label="Connect via OAuth"
                     help="Start the provider OAuth flow."
                   >
-                    {!tokens && state.step === "probed" && (
+                    {!tokens &&
-                      <Button
+                      state.step === "probed" &&
-                        type="button"
+                      (probe.supportsDynamicRegistration ? (
-                        onClick={handleOAuth}
+                        <Button
-                        variant="outline"
+                          type="button"
-                        className="w-full"
+                          onClick={handleOAuth}
-                      >
+                          variant="outline"
-                        Sign in
+                          className="w-full"
-                      </Button>
+                        >
-                    )}
+                          Sign in
+                        </Button>
+                      ) : (
+                        <div className="rounded-md border border-border bg-muted/30 px-3 py-2 text-xs text-muted-foreground">
+                          This server requires OAuth, but its authorization server does not support
+                          dynamic client registration. Use request headers with a bearer token, or
+                          save the source and connect a supported OAuth connection later.
+                        </div>
+                      ))}
                     {!tokens && state.step === "oauth-starting" && (
                       <div className="flex min-h-9 items-center gap-2 rounded-md border border-border bg-muted/30 px-3 py-2">

packages/plugins/mcp/src/sdk/plugin.ts+6−3

 export interface McpProbeResult {
   readonly connected: boolean;
   readonly requiresOAuth: boolean;
+  readonly supportsDynamicRegistration: boolean;
   readonly name: string;
   readonly namespace: string;
   readonly toolCount: number | null;
             return {
               connected: true,
               requiresOAuth: false,
+              supportsDynamicRegistration: false,
               name: result.manifest.server?.name ?? name,
               namespace,
               toolCount: result.manifest.tools.length,
               queryParams: probeQueryParams,
             })
             .pipe(
-              Effect.map(() => true),
+              Effect.map((oauth) => ({ ok: true as const, oauth })),
-              Effect.catch(() => Effect.succeed(false)),
+              Effect.catch(() => Effect.succeed({ ok: false as const, oauth: null })),
               Effect.withSpan("mcp.plugin.probe_oauth"),
             );
-          if (probeResult) {
+          if (probeResult.ok) {
             return {
               connected: false,
               requiresOAuth: true,
+              supportsDynamicRegistration: probeResult.oauth.supportsDynamicRegistration,
               name,
               namespace,
               toolCount: null,

packages/plugins/openapi/src/sdk/parse.test.ts+14−0

@@ -33,6 +33,20 @@ paths: {} · 32 unmodified lines
33	}),	33	}),
34	);	34	);
35		35
		36	it.effect("falls back to YAML for flow-style YAML documents", () =>
		37	Effect.gen(function* () {
		38	const doc = yield* parse(`
		39	{
		40	openapi: 3.0.0,
		41	info: { title: Test, version: 1.0.0 },
		42	paths: {}
		43	}
		44	`);
		45
		46	expect(doc.openapi).toBe("3.0.0");
		47	}),
		48	);
		49
36	it.effect("returns a stable parse error for empty documents", () =>	50	it.effect("returns a stable parse error for empty documents", () =>
37	Effect.gen(function* () {	51	Effect.gen(function* () {
38	const error = yield* parse("").pipe(Effect.flip);	52	const error = yield* parse("").pipe(Effect.flip);

packages/plugins/openapi/src/sdk/parse.ts+20−8

 import type { OpenAPI, OpenAPIV3, OpenAPIV3_1 } from "openapi-types";
-import { Duration, Effect } from "effect";
+import { Duration, Effect, Schema } from "effect";
 import { HttpClient, HttpClientRequest } from "effect/unstable/http";
 import YAML from "yaml";
       });
     }
-    const parsed = yield* Effect.try({
+    const parsed = yield* parseJsonLike(trimmed).pipe(
-      try: () => YAML.parse(trimmed) as unknown,
+      Effect.mapError(
-      catch: () =>
+        () =>
-        new OpenApiParseError({
+          new OpenApiParseError({
-          message: "Failed to parse OpenAPI document",
+            message: "Failed to parse OpenAPI document",
-        }),
+          }),
-    });
+      ),
+    );
     if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
       return yield* new OpenApiParseError({
     return parsed as OpenAPI.Document;
   });
+const parseJsonText = Schema.decodeUnknownEffect(Schema.fromJsonString(Schema.Unknown));
+const parseJsonLike = (text: string): Effect.Effect<unknown, unknown> => {
+  const parseYaml = Effect.try({
+    try: () => YAML.parse(text) as unknown,
+    catch: () => "YamlParseFailed" as const,
+  });
+  if (!text.startsWith("{") && !text.startsWith("[")) return parseYaml;
+  return parseJsonText(text).pipe(Effect.catch(() => parseYaml));
+};

packages/react/src/plugins/oauth-sign-in.tsx+13−4

 import * as Exit from "effect/Exit";
 import { cancelOAuth, startOAuth } from "../api/atoms";
-import { messageFromUnknown, useReportHandledError } from "../api/error-reporting";
+import { messageFromExit, messageFromUnknown, useReportHandledError } from "../api/error-reporting";
 import { openOAuthPopup, reserveOAuthPopup, type OAuthPopupResult } from "../api/oauth-popup";
 import { Button } from "../components/button";
 import {
 class OAuthAuthorizationStartError extends Data.TaggedError("OAuthAuthorizationStartError")<{
   readonly cause: unknown;
+  readonly message: string;
 }> {}
 export type StartOAuthAuthorizationInput<TPayload extends OAuthCompletionPayload> = {
       const startExit = await Effect.runPromiseExit(
         Effect.tryPromise({
           try: input.run,
-          catch: (cause) => new OAuthAuthorizationStartError({ cause }),
+          catch: (cause) =>
+            new OAuthAuthorizationStartError({
+              cause,
+              message: messageFromUnknown(cause, startErrorMessage ?? "Failed to start sign-in"),
+            }),
         }),
       );
       if (Exit.isFailure(startExit)) {
-        const message = startErrorMessage ?? "Failed to start sign-in";
+        const message = messageFromExit(startExit, startErrorMessage ?? "Failed to start sign-in");
         reportHandledError(startExit.cause, {
           surface: "oauth",
           action: "start",
           }).then((exit) =>
             Exit.isSuccess(exit)
               ? exit.value
-              : Effect.runPromise(Effect.fail(startErrorMessage ?? "Failed to start sign-in")),
+              : Effect.runPromise(
+                  Effect.fail({
+                    message: messageFromExit(exit, startErrorMessage ?? "Failed to start sign-in"),
+                  }),
+                ),
           ),
       });
     },