Request all advertised OAuth scopes; keep sync verdicts visible

.changeset/oauth-discovered-scope-budget.md

@@ -0,0 +1,5 @@
+---
+"@executor-js/sdk": patch
+---
+
+Request every scope a resource advertises during OAuth scope discovery, bounded by an 8 KiB scope-string budget instead of a 100-scope count. Resources with many fine-grained scopes previously received a token missing the ones it needed. Health checks without a probe no longer replace a tool-sync failure verdict with "healthy".

packages/core/sdk/src/executor.ts

@@ -3605,7 +3605,15 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
                           ? { tools_synced_at: Date.now() }
                           : {
                               tools_synced_at: Date.now(),
-                              last_health: health ?? toolSyncHealth(reason),
+                              // A plugin-supplied verdict (e.g. the MCP server
+                              // rejecting the token during discovery) is still
+                              // sync-stamped: mark it so credential-only health
+                              // checks cannot bury it under "healthy", and a
+                              // later successful sync clears it.
+                              last_health:
+                                health === undefined
+                                  ? toolSyncHealth(reason)
+                                  : { ...health, reason: health.reason ?? "tool_sync_failed" },
                               updated_at: new Date(),
                             },
                     })
@@ -5138,7 +5146,11 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
     ): Effect.Effect<void> =>
       findConnectionRow(ref).pipe(
         Effect.flatMap((fresh) =>
-          fresh === null || oauthReauthRequiredFromProviderState(fresh.provider_state) !== null
+          fresh === null ||
+          oauthReauthRequiredFromProviderState(fresh.provider_state) !== null ||
+          // A credential verdict cannot refute a failed tool sync; only a
+          // successful sync clears that record (see `isToolSyncHealth`).
+          isToolSyncHealth(Option.getOrNull(decodeLastHealth(fresh.last_health)))
             ? Effect.void
             : persistHealthResult(ref, fresh, result),
         ),
@@ -5241,6 +5253,17 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
                 // failure is the one real signal this path can produce, and it
                 // must not hide inside a green span.
                 oauthCredentialHealthWithoutProbe(connectionRow).pipe(
+                  // A resolvable token says nothing about whether the
+                  // upstream accepts it. When tool sync has already recorded
+                  // that it does not (a rejected discovery handshake, an
+                  // unreachable server), that verdict stands until a sync
+                  // succeeds — serving "healthy" here would hide a connection
+                  // that has no tools behind a green badge.
+                  Effect.map((result) =>
+                    result.status === "healthy" && previous !== null && isToolSyncHealth(previous)
+                      ? previous
+                      : result,
+                  ),
                   Effect.tap((result) => persistProbeHealthResult(ref, result)),
                   Effect.map((result) => ({
                     source: "credential_only" as const,

packages/core/sdk/src/health-check.ts

@@ -145,6 +145,7 @@ export type HealthCheckResult = typeof HealthCheckResult.Type;
 export const toolSyncHealthDetailPrefix = "Tool sync failing";
 
 export const isToolSyncHealth = (result: HealthCheckResult | null | undefined): boolean =>
+  result?.reason === "tool_sync_failed" ||
   result?.detail?.startsWith(toolSyncHealthDetailPrefix) === true;
 
 // ---------------------------------------------------------------------------

packages/core/sdk/src/oauth-flow.test.ts

@@ -2033,6 +2033,73 @@ describe("oauth token refresh in resolveConnectionValue", () => {
       ),
   );
 
+  it.effect(
+    "checkHealth without a probe serves a sync-stamped verdict instead of burying it under healthy",
+    () =>
+      Effect.scoped(
+        Effect.gen(function* () {
+          const server = yield* serveOAuthTestServer({ scopes: ["read"] });
+          const { executor, config } = yield* makeTestWorkspaceHarness({ plugins });
+          yield* executor.acme.seed();
+
+          yield* executor.oauth.createClient({
+            owner: "org",
+            slug: CLIENT,
+            authorizationUrl: server.authorizationEndpoint,
+            tokenUrl: server.tokenEndpoint,
+            grant: "authorization_code",
+            clientId: "test-client",
+            clientSecret: "test-secret",
+            resource: server.mcpResourceUrl,
+          });
+
+          const started = yield* executor.oauth.start({
+            owner: "org",
+            client: CLIENT,
+            clientOwner: "org",
+            name: ConnectionName.make("main"),
+            integration: INTEG,
+            template: TEMPLATE,
+          });
+          expect(started.status).toBe("redirect");
+          if (started.status !== "redirect") return;
+          const callback = yield* server.completeAuthorizationCodeFlow({
+            authorizationUrl: started.authorizationUrl,
+          });
+          yield* executor.oauth.complete({ state: started.state, code: callback.code });
+
+          // Tool sync found the upstream rejecting the freshly minted token
+          // (e.g. an MCP discovery handshake answering 401) and stamped it.
+          // The token itself still resolves, so a credential-only check would
+          // otherwise report healthy and hide a connection that has no tools.
+          const stamped = {
+            status: "expired",
+            checkedAt: Date.now(),
+            detail: "MCP OAuth reauthorization required",
+            reason: "tool_sync_failed",
+          };
+          yield* Effect.promise(() =>
+            config.db.updateMany("connection", {
+              where: (b) => b("name", "=", "main"),
+              set: { last_health: stamped },
+            }),
+          );
+
+          const result = yield* executor.connections.checkHealth({
+            owner: "org",
+            integration: INTEG,
+            name: ConnectionName.make("main"),
+          });
+          expect(result).toMatchObject(stamped);
+
+          const row = yield* Effect.promise(() =>
+            config.db.findFirst("connection", { where: (b) => b("name", "=", "main") }),
+          );
+          expect(row?.last_health).toMatchObject(stamped);
+        }),
+      ),
+  );
+
   it.effect("records missing authorization-code scopes without blocking the connection", () =>
     Effect.scoped(
       Effect.gen(function* () {

packages/core/sdk/src/oauth-scope-union.test.ts

@@ -713,13 +713,42 @@ describe("oauth.start integration-driven scopes", () => {
       ),
   );
 
-  it.effect("(j) caps server-advertised resource scopes so the authorize URL stays bounded", () =>
+  it.effect("(j) requests every advertised scope of a large but realistic resource list", () =>
     Effect.scoped(
       Effect.gen(function* () {
-        // A hostile/buggy server advertises far more scopes than any real
-        // template. Discovery caps the request at 100 so the authorize URL
-        // cannot be blown up.
-        const manyScopes = Array.from({ length: 200 }, (_, i) => `scope:${i}`);
+        // A fine-grained resource can legitimately advertise well over a
+        // hundred scopes (PostHog lists 150). Dropping any of them mints a
+        // token the resource rejects, so the whole list must be requested.
+        const manyScopes = Array.from(
+          { length: 150 },
+          (_, i) => `resource_${i}:${i % 2 === 0 ? "read" : "write"}`,
+        );
+        const server = yield* serveMetadataServer({ prm: { scopesSupported: manyScopes } });
+        const executor = yield* setupMcpScopeClient(server);
+
+        const started = yield* executor.oauth.start({
+          owner: "org",
+          client: CLIENT,
+          clientOwner: "org",
+          name: ConnectionName.make("main"),
+          integration: INTEG,
+          template: TEMPLATE,
+        });
+        expect(started.status).toBe("redirect");
+        if (started.status !== "redirect") return;
+
+        expect(scopesFromAuthorizeUrl(started.authorizationUrl)).toEqual(manyScopes);
+      }),
+    ),
+  );
+
+  it.effect("(j2) caps server-advertised resource scopes so the authorize URL stays bounded", () =>
+    Effect.scoped(
+      Effect.gen(function* () {
+        // A hostile/buggy server advertises an absurd list. Discovery keeps
+        // the longest leading prefix whose joined `scope` value fits the
+        // 8 KiB budget so the authorize URL cannot be blown up.
+        const manyScopes = Array.from({ length: 2000 }, (_, i) => `scope:${i}`);
         const server = yield* serveMetadataServer({ prm: { scopesSupported: manyScopes } });
         const executor = yield* setupMcpScopeClient(server);
 
@@ -735,8 +764,10 @@ describe("oauth.start integration-driven scopes", () => {
         if (started.status !== "redirect") return;
 
         const requested = scopesFromAuthorizeUrl(started.authorizationUrl);
-        expect(requested.length).toBe(100);
-        expect(requested).toEqual(manyScopes.slice(0, 100));
+        expect(requested.length).toBeLessThan(manyScopes.length);
+        expect(requested).toEqual(manyScopes.slice(0, requested.length));
+        expect(requested.join(" ").length).toBeLessThanOrEqual(8192);
+        expect([...requested, manyScopes[requested.length]].join(" ").length).toBeGreaterThan(8192);
       }),
     ),
   );

packages/core/sdk/src/oauth-service.ts

@@ -782,9 +782,26 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => {
   // Caps on server-controlled discovery input — a hostile or buggy server must
   // not be able to hang `oauth.start` or overflow the authorize URL.
   const MAX_DISCOVERY_AUTH_SERVERS = 3; // AS-failover lists are tiny in practice
-  const MAX_DISCOVERED_SCOPES = 100; // far beyond any realistic authorization template
-  const capScopes = (scopes: readonly string[]): readonly string[] =>
-    dedupeScopes(scopes).slice(0, MAX_DISCOVERED_SCOPES);
+  // The cap is on the encoded `scope` parameter's length, not the scope
+  // count: the URL is what overflows, and a real resource can legitimately
+  // advertise well over a hundred fine-grained scopes (PostHog lists 150).
+  // Dropping any advertised scope silently mints a token the resource then
+  // rejects, so the budget is generous — 8 KiB leaves room for the rest of the
+  // authorize URL under the common 8-16 KiB request-line limits — and only an
+  // absurd list is truncated.
+  const MAX_DISCOVERED_SCOPE_CHARS = 8192;
+  const capScopes = (scopes: readonly string[]): readonly string[] => {
+    const unique = dedupeScopes(scopes);
+    let length = 0;
+    let count = 0;
+    for (const scope of unique) {
+      const next = length + scope.length + (count > 0 ? 1 : 0);
+      if (next > MAX_DISCOVERED_SCOPE_CHARS) break;
+      length = next;
+      count += 1;
+    }
+    return unique.slice(0, count);
+  };
 
   // Bound a whole discovery sequence (PRM + up to MAX_DISCOVERY_AUTH_SERVERS AS
   // fetches, each with its own request timeout). 30s is larger than a single